threads / patch / 47915

patch, 11 partsMake the test suite pass with '-x' and /bin/sh

Subject: [PATCH 00/11] Make the test suite pass with '-x' and /bin/sh

## tl;dr

26 messages between Feb 23, 2018 and Mar 5, 2018. Diffs are folded; open one to read it.

replies: 25people: 5as markdown or json

SZEDER Gábor· Feb 23, 2018, 23:39 UTC · lore

This patch series makes '-x' tracing of tests work reliably even when running them with /bin/sh, and setting TEST_SHELL_PATH=bash will be unnecessary.

  make GIT_TEST_OPTS='-x --verbose-log' test

passes on my setup and on all Travis CI build jobs (though neither me nor Travis CI run all tests, e.g. CVS).

The first patch is the most important: with a couple of well-placed file descriptor redirections it ensures that the stderr of the test helper functions running git commands only contain the stderr of the tested command, thereby resolving over 90% of the failures resulting from running the test suite with '-x' and /bin/sh.

Most of the following patches resolve the remaining failures, one test script at a time, in most cases by limiting the scope of stderr redirections from functions and subshells to the tested git commands. Except the second and ninth patches, which, arguably, could be considered as cheating... I admit, my enthusiasm suddenly run out when I saw t1510 :)

The last two patches are just finishing touches with a bit of documentation updates and enabling '-x' tracing in Travis CI build jobs.

There is currently nothing in 'pu' that would require additional fixes to make this patch series work.

SZEDER Gábor (11):
  t: prevent '-x' tracing from interfering with test helpers' stderr
  t: add means to disable '-x' tracing for individual test scripts
  t1507-rev-parse-upstream: don't check the stderr of a shell function
  t3030-merge-recursive: don't check the stderr of a subshell
  t5500-fetch-pack: don't check the stderr of a subshell
  t5526: use $TRASH_DIRECTORY to specify the path of GIT_TRACE log file
  t5570-git-daemon: don't check the stderr of a subshell
  t9903-bash-prompt: don't check the stderr of __git_ps1()
  t1510-repo-setup: mark as untraceable with '-x'
  t/README: add a note about don't saving stderr of compound commands
  travis-ci: run tests with '-x' tracing
 ci/lib-travisci.sh            |  2 +-
 t/README                      | 23 +++++++++++++++++++---
 t/lib-terminal.sh             |  4 ++--
 t/t1507-rev-parse-upstream.sh | 14 +++++++-------
 t/t1510-repo-setup.sh         |  4 ++++
 t/t3030-merge-recursive.sh    | 36 +++++++++++++++++++----------------
 t/t5500-fetch-pack.sh         | 12 ++++++------
 t/t5526-fetch-submodules.sh   |  2 +-
 t/t5570-git-daemon.sh         |  2 +-
 t/t9903-bash-prompt.sh        | 14 ++------------
 t/test-lib-functions.sh       | 24 +++++++++++------------
 t/test-lib.sh                 | 19 +++++++++++++++++-
 12 files changed, 94 insertions(+), 62 deletions(-)
-- 
2.16.2.400.g911b7cc0da
SZEDER Gábor· Feb 23, 2018, 23:39 UTC · re: SZEDER Gábor · lore

[PATCH 02/11] t: add means to disable '-x' tracing for individual test scripts

The previous patch resolved most of the test failures caused by running our test suite with '-x' tracing and /bin/sh, and the following patches in this series will resolve almost all of the remaining failures. Unfortunately, not yet all.

Add means to disable '-x' tracing for individual test scripts by setting the $test_untraceable variable to a non-empty value in the test script before sourcing 'test-lib.sh'. However, since '-x' tracing is not an issue with recent Bash versions supporting BASH_XTRACEFD, i.e. v4.1 and later, don't disable tracing when the test script is run with such a Bash version even when $test_untraceable is set.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/README      |  3 +++
 t/test-lib.sh | 19 ++++++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)
Show changes to 2 files +21 −1

t/README, t/test-lib.sh

diff --git a/t/README b/t/README
index b3f7b449c3..c430e9c52c 100644
--- a/t/README
+++ b/t/README
@@ -87,6 +87,9 @@ appropriately before running "make".
 	themselves. Implies `--verbose`. Note that in non-bash shells,
 	this can cause failures in some tests which redirect and test
 	the output of shell functions. Use with caution.
+	Ignored in test scripts that set the variable 'test_untraceable'
+	to a non-empty value, unless it's run with a Bash version
+	supporting BASH_XTRACEFD, i.e. v4.1 or later.
 
 -d::
 --debug::
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 33f6ce26f6..732213ef1b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -263,7 +263,24 @@ do
 		GIT_TEST_CHAIN_LINT=0
 		shift ;;
 	-x)
-		trace=t
+		# Some test scripts can't be reliably traced  with '-x',
+		# unless the test is run with a Bash version supporting
+		# BASH_XTRACEFD (introduced in Bash v4.1).  Check whether
+		# this test is marked as such, and ignore '-x' if it
+		# isn't executed with a suitable Bash version.
+		if test -z "$test_untraceable" || {
+		     test -n "$BASH_VERSION" && {
+		       test ${BASH_VERSINFO[0]} -gt 4 || {
+			 test ${BASH_VERSINFO[0]} -eq 4 &&
+			 test ${BASH_VERSINFO[1]} -ge 1
+		       }
+		     }
+		   }
+		then
+			trace=t
+		else
+			echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
+		fi
 		shift ;;
 	--verbose-log)
 		verbose_log=t
-- 
2.16.2.400.g911b7cc0da
SZEDER Gábor· Feb 23, 2018, 23:39 UTC · re: SZEDER Gábor · lore

[PATCH 01/11] t: prevent '-x' tracing from interfering with test helpers' stderr

Running a test script with '-x' turns on 'set -x' tracing, the output of which is normally sent to stderr. This causes a lot of test failures, because many tests redirect and verify the stderr of shell functions, most frequently that of 'test_must_fail'. These issues were worked around somewhat in d88785e424 (test-lib: set BASH_XTRACEFD automatically, 2016-05-11), so at least we could reliably run tests with '-x' tracing under a Bash version supporting BASH_XTRACEFD, i.e. v4.1 and later.

This patch makes it safe to redirect and verify the stderr of those test helper functions which are meant to run the tested command given as argument, even when running tests with '-x' and /bin/sh. This is achieved through a couple of file descriptor redirections:

  - Duplicate stderr of the tested command executed in the test helper
    function from the function's fd 7 (see next point), to ensure that
    the tested command's error messages go to a different fd than the
    '-x' trace of the commands executed in the function.
  - Duplicate the test helper function's fd 7 from the function's
    original stderr, meaning that, after taking a detour through fd 7,
    the error messages of the tested command do end up on the
    function's original stderr.
  - Duplicate stderr of the test helper function from fd 4, i.e. the
    fd connected to the test script's original stderr and the fd used
    for BASH_XTRACEFD.  This ensures that the '-x' trace of the
    commands executed in the function
      - doesn't go to the function's original stderr, so it won't mess
	with callers who want to save and verify the tested command's
	stderr.
      - does go to the same fd independently from the shell running
        the test script, be it /bin/sh, an older Bash without
        BASH_XTRACEFD, or a more recent Bash already supporting
        BASH_XTRACEFD.
  - Specify the latter two redirections above in the test helper
    function's definition, so they are performed every time the
    function is invoked, without the need to modify the callsites of
    the function.

Perform these redirections in those test helper functions which can be expected to have their stderr redirected, i.e. in the functions 'test_must_fail', 'test_might_fail', 'test_expect_code', 'test_env', 'nongit', 'test_terminal' and 'perl'. Note that 'test_might_fail', 'test_env', and 'nongit' are not involved in any test failures when running tests with '-x' and /bin/sh.

The other test helper functions are left unchanged, because they either don't run commands specified as their arguments, or redirecting their stderr wouldn't make sense, or both.

With this change the number of failures when running the test suite with '-x' tracing and /bin/sh goes down from 340 failed tests in 43 test scripts to 22 failed tests in 6 scripts (or 23 in 7, if the system (OSX) uses an older Bash version without BASH_XTRACEFD to run 't9903-bash-prompt.sh').

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/lib-terminal.sh       |  4 ++--
 t/test-lib-functions.sh | 24 ++++++++++++------------
 2 files changed, 14 insertions(+), 14 deletions(-)
Show changes to 2 files +14 −14

t/lib-terminal.sh, t/test-lib-functions.sh

diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh
index cd220e378e..b3acb4c6f8 100644
--- a/t/lib-terminal.sh
+++ b/t/lib-terminal.sh
@@ -9,8 +9,8 @@ test_terminal () {
 		echo >&4 "test_terminal: need to declare TTY prerequisite"
 		return 127
 	fi
-	perl "$TEST_DIRECTORY"/test-terminal.perl "$@"
-}
+	perl "$TEST_DIRECTORY"/test-terminal.perl "$@" 2>&9
+} 9>&2 2>&4
 
 test_lazy_prereq TTY '
 	test_have_prereq PERL &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 67b5994afb..37eb34044a 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -621,7 +621,7 @@ test_must_fail () {
 		_test_ok=
 		;;
 	esac
-	"$@"
+	"$@" 2>&7
 	exit_code=$?
 	if test $exit_code -eq 0 && ! list_contains "$_test_ok" success
 	then
@@ -644,7 +644,7 @@ test_must_fail () {
 		return 1
 	fi
 	return 0
-}
+} 7>&2 2>&4
 
 # Similar to test_must_fail, but tolerates success, too.  This is
 # meant to be used in contexts like:
@@ -658,8 +658,8 @@ test_must_fail () {
 # because we want to notice if it fails due to segv.
 
 test_might_fail () {
-	test_must_fail ok=success "$@"
-}
+	test_must_fail ok=success "$@" 2>&7
+} 7>&2 2>&4
 
 # Similar to test_must_fail and test_might_fail, but check that a
 # given command exited with a given exit code. Meant to be used as:
@@ -671,7 +671,7 @@ test_might_fail () {
 test_expect_code () {
 	want_code=$1
 	shift
-	"$@"
+	"$@" 2>&7
 	exit_code=$?
 	if test $exit_code = $want_code
 	then
@@ -680,7 +680,7 @@ test_expect_code () {
 
 	echo >&2 "test_expect_code: command exited with $exit_code, we wanted $want_code $*"
 	return 1
-}
+} 7>&2 2>&4
 
 # test_cmp is a helper function to compare actual and expected output.
 # You can use it like:
@@ -882,8 +882,8 @@ test_write_lines () {
 }
 
 perl () {
-	command "$PERL_PATH" "$@"
-}
+	command "$PERL_PATH" "$@" 2>&7
+} 7>&2 2>&4
 
 # Is the value one of the various ways to spell a boolean true/false?
 test_normalize_bool () {
@@ -1023,13 +1023,13 @@ test_env () {
 				shift
 				;;
 			*)
-				"$@"
+				"$@" 2>&7
 				exit
 				;;
 			esac
 		done
 	)
-}
+} 7>&2 2>&4
 
 # Returns true if the numeric exit code in "$2" represents the expected signal
 # in "$1". Signals should be given numerically.
@@ -1071,9 +1071,9 @@ nongit () {
 		GIT_CEILING_DIRECTORIES=$(pwd) &&
 		export GIT_CEILING_DIRECTORIES &&
 		cd non-repo &&
-		"$@"
+		"$@" 2>&7
 	)
-}
+} 7>&2 2>&4
 
 # convert stdin to pktline representation; note that empty input becomes an
 # empty packet, not a flush packet (for that you can just print 0000 yourself).
-- 
2.16.2.400.g911b7cc0da
SZEDER Gábor· Feb 24, 2018, 12:19 UTC · re: SZEDER Gábor · lore

Re: [PATCH 01/11] t: prevent '-x' tracing from interfering with test helpers' stderr

On Sat, Feb 24, 2018 at 12:39 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
Show 9 quoted lines
>   - Duplicate stderr of the tested command executed in the test helper
>     function from the function's fd 7 (see next point), to ensure that
>     the tested command's error messages go to a different fd than the
>     '-x' trace of the commands executed in the function.
>
>   - Duplicate the test helper function's fd 7 from the function's
>     original stderr, meaning that, after taking a detour through fd 7,
>     the error messages of the tested command do end up on the
>     function's original stderr.
Show 12 quoted lines
> diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh
> index cd220e378e..b3acb4c6f8 100644
> --- a/t/lib-terminal.sh
> +++ b/t/lib-terminal.sh
> @@ -9,8 +9,8 @@ test_terminal () {
>                 echo >&4 "test_terminal: need to declare TTY prerequisite"
>                 return 127
>         fi
> -       perl "$TEST_DIRECTORY"/test-terminal.perl "$@"
> -}
> +       perl "$TEST_DIRECTORY"/test-terminal.perl "$@" 2>&9
> +} 9>&2 2>&4
Oops, these should duplicate from/to fd 7, not fd 9.
SZEDER Gábor· Feb 25, 2018, 13:40 UTC · re: SZEDER Gábor · lore

[PATCH v1.1] t: prevent '-x' tracing from interfering with test helpers' stderr

Running a test script with '-x' turns on 'set -x' tracing, the output of which is normally sent to stderr. This causes a lot of test failures, because many tests redirect and verify the stderr of shell functions, most frequently that of 'test_must_fail'. These issues were worked around somewhat in d88785e424 (test-lib: set BASH_XTRACEFD automatically, 2016-05-11), so at least we could reliably run tests with '-x' tracing under a Bash version supporting BASH_XTRACEFD, i.e. v4.1 and later.

Futhermore, redirecting the stderr of test helper functions like 'test_must_fail' or 'test_expect_code' is the cause of a different issue as well. If these functions detect something unexpected, they will write their error messages intended to the user to thier stderr. However, if their stderr is redirected in order to save and verify the stderr of the tested git command invoked in the function, then the function's error messages will be redirected as well. Consequently, those messages won't reach the user, making the test's verbose output less useful.

This patch makes it safe to redirect and verify the stderr of those test helper functions which are meant to run the tested command given as argument, even when running tests with '-x' and /bin/sh. This is achieved through a couple of file descriptor redirections:

  - Duplicate stderr of the tested command executed in the test helper
    function from the function's fd 7 (see next point), to ensure that
    the tested command's error messages go to a different fd than the
    '-x' trace of the commands executed in the function or the
    function's error messages.
  - Duplicate the test helper function's fd 7 from the function's
    original stderr, meaning that, after taking a detour through fd 7,
    the error messages of the tested command do end up on the
    function's original stderr.
  - Duplicate stderr of the test helper function from fd 4, i.e. the
    fd connected to the test script's original stderr and the fd used
    for BASH_XTRACEFD.  This ensures that the '-x' trace of the
    commands executed in the function
      - doesn't go to the function's original stderr, so it won't mess
	with callers who want to save and verify the tested command's
	stderr.
      - does go to the same fd independently from the shell running
        the test script, be it /bin/sh, an older Bash without
        BASH_XTRACEFD, or a more recent Bash already supporting
        BASH_XTRACEFD.
    Furthermore, this also makes sure that the function's error
    messages go to this fd 4, meaning that the user will be able to
    see them even if the function's stderr is redirected in the test.
  - Specify the latter two redirections above in the test helper
    function's definition, so they are performed every time the
    function is invoked, without the need to modify the callsites of
    the function.

Perform these redirections in those test helper functions which can be expected to have their stderr redirected, i.e. in the functions 'test_must_fail', 'test_might_fail', 'test_expect_code', 'test_env', 'nongit', 'test_terminal' and 'perl'. Note that 'test_might_fail', 'test_env', and 'nongit' are not involved in any test failures when running tests with '-x' and /bin/sh.

The other test helper functions are left unchanged, because they either don't run commands specified as their arguments, or redirecting their stderr wouldn't make sense, or both.

With this change the number of failures when running the test suite with '-x' tracing and /bin/sh goes down from 340 failed tests in 43 test scripts to 22 failed tests in 6 scripts (or 23 in 7, if the system (OSX) uses an older Bash version without BASH_XTRACEFD to run 't9903-bash-prompt.sh').

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
Changes:
  - Duplicate from/to fd 7 instead of fd 9 in 'test_terminal'.
  - Talk about the issue that redirecting stderr of test helper
    functions affect their error messages as well, and how this patch
    resolves that issue as well.
 t/lib-terminal.sh       |  4 ++--
 t/test-lib-functions.sh | 24 ++++++++++++------------
 2 files changed, 14 insertions(+), 14 deletions(-)
Show changes to 2 files +14 −14

t/lib-terminal.sh, t/test-lib-functions.sh

diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh
index cd220e378e..e3809dcead 100644
--- a/t/lib-terminal.sh
+++ b/t/lib-terminal.sh
@@ -9,8 +9,8 @@ test_terminal () {
 		echo >&4 "test_terminal: need to declare TTY prerequisite"
 		return 127
 	fi
-	perl "$TEST_DIRECTORY"/test-terminal.perl "$@"
-}
+	perl "$TEST_DIRECTORY"/test-terminal.perl "$@" 2>&7
+} 7>&2 2>&4
 
 test_lazy_prereq TTY '
 	test_have_prereq PERL &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 67b5994afb..37eb34044a 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -621,7 +621,7 @@ test_must_fail () {
 		_test_ok=
 		;;
 	esac
-	"$@"
+	"$@" 2>&7
 	exit_code=$?
 	if test $exit_code -eq 0 && ! list_contains "$_test_ok" success
 	then
@@ -644,7 +644,7 @@ test_must_fail () {
 		return 1
 	fi
 	return 0
-}
+} 7>&2 2>&4
 
 # Similar to test_must_fail, but tolerates success, too.  This is
 # meant to be used in contexts like:
@@ -658,8 +658,8 @@ test_must_fail () {
 # because we want to notice if it fails due to segv.
 
 test_might_fail () {
-	test_must_fail ok=success "$@"
-}
+	test_must_fail ok=success "$@" 2>&7
+} 7>&2 2>&4
 
 # Similar to test_must_fail and test_might_fail, but check that a
 # given command exited with a given exit code. Meant to be used as:
@@ -671,7 +671,7 @@ test_might_fail () {
 test_expect_code () {
 	want_code=$1
 	shift
-	"$@"
+	"$@" 2>&7
 	exit_code=$?
 	if test $exit_code = $want_code
 	then
@@ -680,7 +680,7 @@ test_expect_code () {
 
 	echo >&2 "test_expect_code: command exited with $exit_code, we wanted $want_code $*"
 	return 1
-}
+} 7>&2 2>&4
 
 # test_cmp is a helper function to compare actual and expected output.
 # You can use it like:
@@ -882,8 +882,8 @@ test_write_lines () {
 }
 
 perl () {
-	command "$PERL_PATH" "$@"
-}
+	command "$PERL_PATH" "$@" 2>&7
+} 7>&2 2>&4
 
 # Is the value one of the various ways to spell a boolean true/false?
 test_normalize_bool () {
@@ -1023,13 +1023,13 @@ test_env () {
 				shift
 				;;
 			*)
-				"$@"
+				"$@" 2>&7
 				exit
 				;;
 			esac
 		done
 	)
-}
+} 7>&2 2>&4
 
 # Returns true if the numeric exit code in "$2" represents the expected signal
 # in "$1". Signals should be given numerically.
@@ -1071,9 +1071,9 @@ nongit () {
 		GIT_CEILING_DIRECTORIES=$(pwd) &&
 		export GIT_CEILING_DIRECTORIES &&
 		cd non-repo &&
-		"$@"
+		"$@" 2>&7
 	)
-}
+} 7>&2 2>&4
 
 # convert stdin to pktline representation; note that empty input becomes an
 # empty packet, not a flush packet (for that you can just print 0000 yourself).
-- 
2.16.2.409.g7f85f628cc
SZEDER Gábor· Feb 23, 2018, 23:39 UTC · re: SZEDER Gábor · lore

[PATCH 03/11] t1507-rev-parse-upstream: don't check the stderr of a shell function

Three tests in 't1507-rev-parse-upstream.sh' fail when the test script is run with '-x' tracing (and using a shell other than a Bash version supporting BASH_XTRACEFD). The reason for those failures is that the tests check the stderr of the function 'error_message', which includes the trace of commands executed in that function as well, throwing off the comparison with the expected output.

Save stderr of 'git rev-parse' only instead of the whole function, so it remains free from tracing output.

After this change t1507 passes with '-x', even when running with /bin/sh.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t1507-rev-parse-upstream.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
Show changes to t/t1507-rev-parse-upstream.sh +7 −7
diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index b23c4e3fab..2ce68cc277 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -42,7 +42,7 @@ commit_subject () {
 
 error_message () {
 	(cd clone &&
-	 test_must_fail git rev-parse --verify "$@")
+	 test_must_fail git rev-parse --verify "$@" 2>../error)
 }
 
 test_expect_success '@{upstream} resolves to correct full name' '
@@ -159,8 +159,8 @@ test_expect_success 'branch@{u} error message when no upstream' '
 	cat >expect <<-EOF &&
 	fatal: no upstream configured for branch ${sq}non-tracking${sq}
 	EOF
-	error_message non-tracking@{u} 2>actual &&
-	test_i18ncmp expect actual
+	error_message non-tracking@{u} &&
+	test_i18ncmp expect error
 '
 
 test_expect_success '@{u} error message when no upstream' '
@@ -175,8 +175,8 @@ test_expect_success 'branch@{u} error message with misspelt branch' '
 	cat >expect <<-EOF &&
 	fatal: no such branch: ${sq}no-such-branch${sq}
 	EOF
-	error_message no-such-branch@{u} 2>actual &&
-	test_i18ncmp expect actual
+	error_message no-such-branch@{u} &&
+	test_i18ncmp expect error
 '
 
 test_expect_success '@{u} error message when not on a branch' '
@@ -192,8 +192,8 @@ test_expect_success 'branch@{u} error message if upstream branch not fetched' '
 	cat >expect <<-EOF &&
 	fatal: upstream branch ${sq}refs/heads/side${sq} not stored as a remote-tracking branch
 	EOF
-	error_message bad-upstream@{u} 2>actual &&
-	test_i18ncmp expect actual
+	error_message bad-upstream@{u} &&
+	test_i18ncmp expect error
 '
 
 test_expect_success 'pull works when tracking a local branch' '
-- 
2.16.2.400.g911b7cc0da
SZEDER Gábor· Feb 23, 2018, 23:39 UTC · re: SZEDER Gábor · lore

[PATCH 05/11] t5500-fetch-pack: don't check the stderr of a subshell

Three "missing reference" tests in 't5500-fetch-pack.sh' fail when the test script is run with '-x' tracing (and using a shell other than a Bash version supporting BASH_XTRACEFD). The reason for those failures is that the tests check a subshell's stderr, which includes the trace of executing commands in that subshell as well, throwing off the comparison with the expected output.

Save the stderr of 'git fetch-pack' only instead of the whole subshell, so it remains free from tracing output.

After this change t5500 passes with '-x', even when running with /bin/sh.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5500-fetch-pack.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Show changes to t/t5500-fetch-pack.sh +6 −6
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index ec9ba9bf6e..0680dec808 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -482,24 +482,24 @@ test_expect_success 'set up tests of missing reference' '
 test_expect_success 'test lonely missing ref' '
 	(
 		cd client &&
-		test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy
-	) >/dev/null 2>error-m &&
+		test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy 2>../error-m
+	) &&
 	test_i18ncmp expect-error error-m
 '
 
 test_expect_success 'test missing ref after existing' '
 	(
 		cd client &&
-		test_must_fail git fetch-pack --no-progress .. refs/heads/A refs/heads/xyzzy
-	) >/dev/null 2>error-em &&
+		test_must_fail git fetch-pack --no-progress .. refs/heads/A refs/heads/xyzzy 2>../error-em
+	) &&
 	test_i18ncmp expect-error error-em
 '
 
 test_expect_success 'test missing ref before existing' '
 	(
 		cd client &&
-		test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy refs/heads/A
-	) >/dev/null 2>error-me &&
+		test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy refs/heads/A 2>../error-me
+	) &&
 	test_i18ncmp expect-error error-me
 '
 
-- 
2.16.2.400.g911b7cc0da
SZEDER Gábor· Feb 23, 2018, 23:39 UTC · re: SZEDER Gábor · lore

[PATCH 07/11] t5570-git-daemon: don't check the stderr of a subshell

The test 'no-op fetch without "-v" is quiet' in 't5570-git-daemon.sh' fails when the test script is run with '-x' tracing (and using a shell other than a Bash version supporting BASH_XTRACEFD). The reason for the failure is that the test checks the emptiness of a subshell's stderr, which includes the trace of commands executed in that subshell as well, throwing off the emptiness check.

Save the stderr of 'git fetch' only instead of the whole subshell's, so it remains free from tracing output.

After this change t5570 passes with '-x', even when running with /bin/sh.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5570-git-daemon.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t5570-git-daemon.sh +1 −1
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 755b05a8ae..0d4c52016b 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -50,7 +50,7 @@ test_expect_success 'no-op fetch -v stderr is as expected' '
 '
 
 test_expect_success 'no-op fetch without "-v" is quiet' '
-	(cd clone && git fetch) 2>stderr &&
+	(cd clone && git fetch 2>../stderr) &&
 	! test -s stderr
 '
 
-- 
2.16.2.400.g911b7cc0da
SZEDER Gábor· Feb 23, 2018, 23:39 UTC · re: SZEDER Gábor · lore

[PATCH 11/11] travis-ci: run tests with '-x' tracing

Now that the test suite runs successfully with '-x' tracing even with /bin/sh, enable it on Travis CI in order to

  - get more information about test failures, and
  - catch constructs breaking '-x' with /bin/sh sneaking into our test
    suite.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/lib-travisci.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to ci/lib-travisci.sh +1 −1
diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
index 1efee55ef7..109ef280da 100755
--- a/ci/lib-travisci.sh
+++ b/ci/lib-travisci.sh
@@ -97,7 +97,7 @@ fi
 export DEVELOPER=1
 export DEFAULT_TEST_TARGET=prove
 export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
-export GIT_TEST_OPTS="--verbose-log"
+export GIT_TEST_OPTS="--verbose-log -x"
 export GIT_TEST_CLONE_2GB=YesPlease
 
 case "$jobname" in
-- 
2.16.2.400.g911b7cc0da
SZEDER Gábor· Feb 23, 2018, 23:39 UTC · re: SZEDER Gábor · lore

[PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell

The two test checking 'git mmerge-recursive' in an empty worktree in 't3030-merge-recursive.sh' fail when the test script is run with '-x' tracing (and using a shell other than a Bash version supporting BASH_XTRACEFD). The reason for those failures is that the tests check the emptiness of a subshell's stderr, which includes the trace of commands executed in that subshell as well, throwing off the emptiness check.

Note that both subshells execute four git commands each, meaning that checking the emptiness of the whole subshell implicitly ensures that not only 'git merge-recursive' but none of the other three commands outputs anything to their stderr. Note also that if one of those commands were to output anything on its stderr, then the current combined check would not tell us which one of those four commands the unexpected output came from.

Save the stderr of those four commands only instead of the whole subshell, so it remains free from tracing output, and save and check them individually, so they will show us from which command the unexpected output came from.

After this change t3030 passes with '-x', even when running with /bin/sh.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t3030-merge-recursive.sh | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)
Show changes to t/t3030-merge-recursive.sh +20 −16
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index cdc38fe5d1..cbeea1cf94 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -525,20 +525,22 @@ test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '
 		GIT_INDEX_FILE="$PWD/ours-has-rename-index" &&
 		export GIT_INDEX_FILE &&
 		mkdir "$GIT_WORK_TREE" &&
-		git read-tree -i -m $c7 &&
-		git update-index --ignore-missing --refresh &&
-		git merge-recursive $c0 -- $c7 $c3 &&
-		git ls-files -s >actual-files
-	) 2>actual-err &&
-	>expected-err &&
+		git read-tree -i -m $c7 2>actual-err &&
+		test_must_be_empty expected-err &&
+		git update-index --ignore-missing --refresh 2>actual-err &&
+		test_must_be_empty expected-err &&
+		git merge-recursive $c0 -- $c7 $c3 2>actual-err &&
+		test_must_be_empty expected-err &&
+		git ls-files -s >actual-files 2>actual-err &&
+		test_must_be_empty expected-err
+	) &&
 	cat >expected-files <<-EOF &&
 	100644 $o3 0	b/c
 	100644 $o0 0	c
 	100644 $o0 0	d/e
 	100644 $o0 0	e
 	EOF
-	test_cmp expected-files actual-files &&
-	test_cmp expected-err actual-err
+	test_cmp expected-files actual-files
 '
 
 test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '
@@ -548,20 +550,22 @@ test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '
 		GIT_INDEX_FILE="$PWD/theirs-has-rename-index" &&
 		export GIT_INDEX_FILE &&
 		mkdir "$GIT_WORK_TREE" &&
-		git read-tree -i -m $c3 &&
-		git update-index --ignore-missing --refresh &&
-		git merge-recursive $c0 -- $c3 $c7 &&
-		git ls-files -s >actual-files
-	) 2>actual-err &&
-	>expected-err &&
+		git read-tree -i -m $c3 2>actual-err &&
+		test_must_be_empty expected-err &&
+		git update-index --ignore-missing --refresh 2>>actual-err &&
+		test_must_be_empty expected-err &&
+		git merge-recursive $c0 -- $c3 $c7 2>>actual-err &&
+		test_must_be_empty expected-err &&
+		git ls-files -s >actual-files 2>>actual-err &&
+		test_must_be_empty expected-err
+	) &&
 	cat >expected-files <<-EOF &&
 	100644 $o3 0	b/c
 	100644 $o0 0	c
 	100644 $o0 0	d/e
 	100644 $o0 0	e
 	EOF
-	test_cmp expected-files actual-files &&
-	test_cmp expected-err actual-err
+	test_cmp expected-files actual-files
 '
 
 test_expect_success 'merge removes empty directories' '
-- 
2.16.2.400.g911b7cc0da
Eric Sunshine· Feb 24, 2018, 08:04 UTC · re: SZEDER Gábor · lore

Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell

On Fri, Feb 23, 2018 at 6:39 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> The two test checking 'git mmerge-recursive' in an empty worktree in
s/mmerge/merge/, I guess.
Show 24 quoted lines
> 't3030-merge-recursive.sh' fail when the test script is run with '-x'
> tracing (and using a shell other than a Bash version supporting
> BASH_XTRACEFD).  The reason for those failures is that the tests check
> the emptiness of a subshell's stderr, which includes the trace of
> commands executed in that subshell as well, throwing off the emptiness
> check.
>
> Note that both subshells execute four git commands each, meaning that
> checking the emptiness of the whole subshell implicitly ensures that
> not only 'git merge-recursive' but none of the other three commands
> outputs anything to their stderr.  Note also that if one of those
> commands were to output anything on its stderr, then the current
> combined check would not tell us which one of those four commands the
> unexpected output came from.
>
> Save the stderr of those four commands only instead of the whole
> subshell, so it remains free from tracing output, and save and check
> them individually, so they will show us from which command the
> unexpected output came from.
>
> After this change t3030 passes with '-x', even when running with
> /bin/sh.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Junio C Hamano· Feb 27, 2018, 21:03 UTC · re: SZEDER Gábor · lore

Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell

SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 19 quoted lines
> The two test checking 'git mmerge-recursive' in an empty worktree in
> ...
>  		GIT_INDEX_FILE="$PWD/ours-has-rename-index" &&
>  		export GIT_INDEX_FILE &&
>  		mkdir "$GIT_WORK_TREE" &&
> -		git read-tree -i -m $c7 &&
> -		git update-index --ignore-missing --refresh &&
> -		git merge-recursive $c0 -- $c7 $c3 &&
> -		git ls-files -s >actual-files
> -	) 2>actual-err &&
> -	>expected-err &&
> +		git read-tree -i -m $c7 2>actual-err &&
> +		test_must_be_empty expected-err &&
> +		git update-index --ignore-missing --refresh 2>actual-err &&
> +		test_must_be_empty expected-err &&
> +		git merge-recursive $c0 -- $c7 $c3 2>actual-err &&
> +		test_must_be_empty expected-err &&
> +		git ls-files -s >actual-files 2>actual-err &&
> +		test_must_be_empty expected-err

Where do the contents of all of these expected-err files come from? Should all of the test_must_be_empty checks be checking actual-err instead?

Junio C Hamano· Feb 27, 2018, 21:10 UTC · re: Junio C Hamano · lore

Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell

Junio C Hamano <gitster@pobox.com> writes:
Show 25 quoted lines
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> The two test checking 'git mmerge-recursive' in an empty worktree in
>> ...
>>  		GIT_INDEX_FILE="$PWD/ours-has-rename-index" &&
>>  		export GIT_INDEX_FILE &&
>>  		mkdir "$GIT_WORK_TREE" &&
>> -		git read-tree -i -m $c7 &&
>> -		git update-index --ignore-missing --refresh &&
>> -		git merge-recursive $c0 -- $c7 $c3 &&
>> -		git ls-files -s >actual-files
>> -	) 2>actual-err &&
>> -	>expected-err &&
>> +		git read-tree -i -m $c7 2>actual-err &&
>> +		test_must_be_empty expected-err &&
>> +		git update-index --ignore-missing --refresh 2>actual-err &&
>> +		test_must_be_empty expected-err &&
>> +		git merge-recursive $c0 -- $c7 $c3 2>actual-err &&
>> +		test_must_be_empty expected-err &&
>> +		git ls-files -s >actual-files 2>actual-err &&
>> +		test_must_be_empty expected-err
>
> Where do the contents of all of these expected-err files come from?
> Should all of the test_must_be_empty checks be checking actual-err
> instead?

And the reason why your pre-submission testing did not catch may be because test_must_be_empty is broken? I wonder if this is a good way forward to catch a possible bug like this.

Of course, if somebody was using the helepr for "must be either missing or empty", this change will break it, but I somehow doubt it. A program that creates/opens and writes an error message only when an error is detected is certainly possible, and could be tested with the current test_must_be_empty this way:

	rm -f actual-err &&
	git frotz --error-to=actual-err &&
	test_must_be_empty actual-err

but then the last step in such a test like the above is more natural to check if actual-err _exists_ in the first place anyway, so...

 t/test-lib-functions.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
Show changes to t/test-lib-functions.sh +5 −1
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 37eb34044a..6cfbee60e4 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -772,7 +772,11 @@ verbose () {
 # otherwise.
 
 test_must_be_empty () {
-	if test -s "$1"
+	if ! test -f "$1"
+	then
+		echo "'$1' is missing"
+		return 1
+	elif test -s "$1"
 	then
 		echo "'$1' is not empty, it contains:"
 		cat "$1"
Junio C Hamano· Feb 27, 2018, 21:27 UTC · re: Junio C Hamano · lore

[PATCH] test_must_be_empty: make sure the file exists, not just empty

The helper function test_must_be_empty is meant to make sure the given file is empty, but its implementation is:

	if test -s "$1"
	then
		... not empty, we detected a failure ...
	fi

Surely, the file having non-zero size is a sign that the condition "the file must be empty" is violated, but it misses the case where the file does not even exist. It is an accident waiting to happen with a buggy test like this:

	git frotz 2>error-message &&
	test_must_be_empty errro-message

that won't get caught until you deliberately break 'git frotz' and notice why the test does not fail.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/test-lib-functions.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
Show changes to t/test-lib-functions.sh +5 −1
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 37eb34044a..6cfbee60e4 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -772,7 +772,11 @@ verbose () {
 # otherwise.
 
 test_must_be_empty () {
-	if test -s "$1"
+	if ! test -f "$1"
+	then
+		echo "'$1' is missing"
+		return 1
+	elif test -s "$1"
 	then
 		echo "'$1' is not empty, it contains:"
 		cat "$1"
-- 
2.16.2-325-g2fc74f41c5
Stefan Beller· Feb 27, 2018, 21:42 UTC · re: Junio C Hamano · lore

Re: [PATCH] test_must_be_empty: make sure the file exists, not just empty

On Tue, Feb 27, 2018 at 1:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
> The helper function test_must_be_empty is meant to make sure the
> given file is empty, but its implementation is:
>
>         if test -s "$1"
>         then
>                 ... not empty, we detected a failure ...
>         fi
>
> Surely, the file having non-zero size is a sign that the condition
> "the file must be empty" is violated, but it misses the case where
> the file does not even exist.  It is an accident waiting to happen
> with a buggy test like this:
>
>         git frotz 2>error-message &&
>         test_must_be_empty errro-message
>
> that won't get caught until you deliberately break 'git frotz' and
> notice why the test does not fail.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Reviewed-by: Stefan Beller <sbeller@google.com>
Jeff King· Feb 27, 2018, 22:08 UTC · re: Junio C Hamano · lore

Re: [PATCH] test_must_be_empty: make sure the file exists, not just empty

On Tue, Feb 27, 2018 at 01:27:29PM -0800, Junio C Hamano wrote:
Show 20 quoted lines
> The helper function test_must_be_empty is meant to make sure the
> given file is empty, but its implementation is:
> 
> 	if test -s "$1"
> 	then
> 		... not empty, we detected a failure ...
> 	fi
> 
> Surely, the file having non-zero size is a sign that the condition
> "the file must be empty" is violated, but it misses the case where
> the file does not even exist.  It is an accident waiting to happen
> with a buggy test like this:
> 
> 	git frotz 2>error-message &&
> 	test_must_be_empty errro-message
> 
> that won't get caught until you deliberately break 'git frotz' and
> notice why the test does not fail.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

This seems like a huge and obvious improvement to me. I'm amazed it hasn't come up before (and that this doesn't reveal any existing typos like the one you showed).

-Peff
SZEDER Gábor· Feb 27, 2018, 21:28 UTC · re: Junio C Hamano · lore

Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell

On Tue, Feb 27, 2018 at 10:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 27 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> SZEDER Gábor <szeder.dev@gmail.com> writes:
>>
>>> The two test checking 'git mmerge-recursive' in an empty worktree in
>>> ...
>>>              GIT_INDEX_FILE="$PWD/ours-has-rename-index" &&
>>>              export GIT_INDEX_FILE &&
>>>              mkdir "$GIT_WORK_TREE" &&
>>> -            git read-tree -i -m $c7 &&
>>> -            git update-index --ignore-missing --refresh &&
>>> -            git merge-recursive $c0 -- $c7 $c3 &&
>>> -            git ls-files -s >actual-files
>>> -    ) 2>actual-err &&
>>> -    >expected-err &&
>>> +            git read-tree -i -m $c7 2>actual-err &&
>>> +            test_must_be_empty expected-err &&
>>> +            git update-index --ignore-missing --refresh 2>actual-err &&
>>> +            test_must_be_empty expected-err &&
>>> +            git merge-recursive $c0 -- $c7 $c3 2>actual-err &&
>>> +            test_must_be_empty expected-err &&
>>> +            git ls-files -s >actual-files 2>actual-err &&
>>> +            test_must_be_empty expected-err
>>
>> Where do the contents of all of these expected-err files come from?
>> Should all of the test_must_be_empty checks be checking actual-err
>> instead?
Ugh, I messed that up.
> And the reason why your pre-submission testing did not catch may be
> because test_must_be_empty is broken?  I wonder if this is a good
> way forward to catch a possible bug like this.

Yeah. 'test -s file' means "exists and has a size greater than zero", so the missing file doesn't trigger the error code path.

> Of course, if somebody was using the helepr for "must be either
> missing or empty", this change will break it, but I somehow doubt
> it.

FWIW, I just run the test suite with this change added, and there were no failures. I think it's a good change.

Show 31 quoted lines
>  A program that creates/opens and writes an error message only
> when an error is detected is certainly possible, and could be tested
> with the current test_must_be_empty this way:
>
>         rm -f actual-err &&
>         git frotz --error-to=actual-err &&
>         test_must_be_empty actual-err
>
> but then the last step in such a test like the above is more natural
> to check if actual-err _exists_ in the first place anyway, so...
>
>  t/test-lib-functions.sh | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 37eb34044a..6cfbee60e4 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -772,7 +772,11 @@ verbose () {
>  # otherwise.
>
>  test_must_be_empty () {
> -       if test -s "$1"
> +       if ! test -f "$1"
> +       then
> +               echo "'$1' is missing"
> +               return 1
> +       elif test -s "$1"
>         then
>                 echo "'$1' is not empty, it contains:"
>                 cat "$1"
Junio C Hamano· Feb 27, 2018, 21:17 UTC · re: SZEDER Gábor · lore

Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell

SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 8 quoted lines
> +		git read-tree -i -m $c3 2>actual-err &&
> +		test_must_be_empty expected-err &&
> +		git update-index --ignore-missing --refresh 2>>actual-err &&
> +		test_must_be_empty expected-err &&
> +		git merge-recursive $c0 -- $c3 $c7 2>>actual-err &&
> +		test_must_be_empty expected-err &&
> +		git ls-files -s >actual-files 2>>actual-err &&
> +		test_must_be_empty expected-err

Also, with the error output of individual steps tested like this (assuming that test-must-be-empty checks are to be done on the actual-err file, not ecpected-err that nobody creates), I do not see a point in appending to the file. So perhaps squash this in?

 t/t3030-merge-recursive.sh | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)
Show changes to t/t3030-merge-recursive.sh +11 −11
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index cbeea1cf94..3563e77b37 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -526,13 +526,13 @@ test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '
 		export GIT_INDEX_FILE &&
 		mkdir "$GIT_WORK_TREE" &&
 		git read-tree -i -m $c7 2>actual-err &&
-		test_must_be_empty expected-err &&
+		test_must_be_empty actual-err &&
 		git update-index --ignore-missing --refresh 2>actual-err &&
-		test_must_be_empty expected-err &&
+		test_must_be_empty actual-err &&
 		git merge-recursive $c0 -- $c7 $c3 2>actual-err &&
-		test_must_be_empty expected-err &&
+		test_must_be_empty actual-err &&
 		git ls-files -s >actual-files 2>actual-err &&
-		test_must_be_empty expected-err
+		test_must_be_empty actual-err
 	) &&
 	cat >expected-files <<-EOF &&
 	100644 $o3 0	b/c
@@ -551,13 +551,13 @@ test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '
 		export GIT_INDEX_FILE &&
 		mkdir "$GIT_WORK_TREE" &&
 		git read-tree -i -m $c3 2>actual-err &&
-		test_must_be_empty expected-err &&
-		git update-index --ignore-missing --refresh 2>>actual-err &&
-		test_must_be_empty expected-err &&
-		git merge-recursive $c0 -- $c3 $c7 2>>actual-err &&
-		test_must_be_empty expected-err &&
-		git ls-files -s >actual-files 2>>actual-err &&
-		test_must_be_empty expected-err
+		test_must_be_empty actual-err &&
+		git update-index --ignore-missing --refresh 2>actual-err &&
+		test_must_be_empty actual-err &&
+		git merge-recursive $c0 -- $c3 $c7 2>actual-err &&
+		test_must_be_empty actual-err &&
+		git ls-files -s >actual-files 2>actual-err &&
+		test_must_be_empty actual-err
 	) &&
 	cat >expected-files <<-EOF &&
 	100644 $o3 0	b/c
SZEDER Gábor· Feb 28, 2018, 00:44 UTC · re: Junio C Hamano · lore

Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell

On Tue, Feb 27, 2018 at 10:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> +             git read-tree -i -m $c3 2>actual-err &&
>> +             test_must_be_empty expected-err &&
>> +             git update-index --ignore-missing --refresh 2>>actual-err &&
>> +             test_must_be_empty expected-err &&
>> +             git merge-recursive $c0 -- $c3 $c7 2>>actual-err &&
>> +             test_must_be_empty expected-err &&
>> +             git ls-files -s >actual-files 2>>actual-err &&
>> +             test_must_be_empty expected-err
>
> Also, with the error output of individual steps tested like this
> (assuming that test-must-be-empty checks are to be done on
> the actual-err file, not ecpected-err that nobody creates), I do not
> see a point in appending to the file.  So perhaps squash this in?
Agreed again.
Show 46 quoted lines
>  t/t3030-merge-recursive.sh | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
> index cbeea1cf94..3563e77b37 100755
> --- a/t/t3030-merge-recursive.sh
> +++ b/t/t3030-merge-recursive.sh
> @@ -526,13 +526,13 @@ test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '
>                 export GIT_INDEX_FILE &&
>                 mkdir "$GIT_WORK_TREE" &&
>                 git read-tree -i -m $c7 2>actual-err &&
> -               test_must_be_empty expected-err &&
> +               test_must_be_empty actual-err &&
>                 git update-index --ignore-missing --refresh 2>actual-err &&
> -               test_must_be_empty expected-err &&
> +               test_must_be_empty actual-err &&
>                 git merge-recursive $c0 -- $c7 $c3 2>actual-err &&
> -               test_must_be_empty expected-err &&
> +               test_must_be_empty actual-err &&
>                 git ls-files -s >actual-files 2>actual-err &&
> -               test_must_be_empty expected-err
> +               test_must_be_empty actual-err
>         ) &&
>         cat >expected-files <<-EOF &&
>         100644 $o3 0    b/c
> @@ -551,13 +551,13 @@ test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '
>                 export GIT_INDEX_FILE &&
>                 mkdir "$GIT_WORK_TREE" &&
>                 git read-tree -i -m $c3 2>actual-err &&
> -               test_must_be_empty expected-err &&
> -               git update-index --ignore-missing --refresh 2>>actual-err &&
> -               test_must_be_empty expected-err &&
> -               git merge-recursive $c0 -- $c3 $c7 2>>actual-err &&
> -               test_must_be_empty expected-err &&
> -               git ls-files -s >actual-files 2>>actual-err &&
> -               test_must_be_empty expected-err
> +               test_must_be_empty actual-err &&
> +               git update-index --ignore-missing --refresh 2>actual-err &&
> +               test_must_be_empty actual-err &&
> +               git merge-recursive $c0 -- $c3 $c7 2>actual-err &&
> +               test_must_be_empty actual-err &&
> +               git ls-files -s >actual-files 2>actual-err &&
> +               test_must_be_empty actual-err
>         ) &&
>         cat >expected-files <<-EOF &&
>         100644 $o3 0    b/c
SZEDER Gábor· Feb 23, 2018, 23:39 UTC · re: SZEDER Gábor · lore

[PATCH 10/11] t/README: add a note about don't saving stderr of compound commands

Explain in 't/README' why it is a bad idea to redirect and verify the stderr of compound commands, in the hope that future contributions will follow this advice and the test suite will keep working with '-x' tracing and /bin/sh.

While at it, since we can now run the test suite with '-x' without needing a Bash version supporting BASH_XTRACEFD, remove the now outdated caution note about non-Bash shells from the description of the '-x' option.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/README | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)
Show changes to t/README +17 −3
diff --git a/t/README b/t/README
index c430e9c52c..615682263e 100644
--- a/t/README
+++ b/t/README
@@ -84,9 +84,7 @@ appropriately before running "make".
 
 -x::
 	Turn on shell tracing (i.e., `set -x`) during the tests
-	themselves. Implies `--verbose`. Note that in non-bash shells,
-	this can cause failures in some tests which redirect and test
-	the output of shell functions. Use with caution.
+	themselves. Implies `--verbose`.
 	Ignored in test scripts that set the variable 'test_untraceable'
 	to a non-empty value, unless it's run with a Bash version
 	supporting BASH_XTRACEFD, i.e. v4.1 or later.
@@ -455,6 +453,22 @@ Don't:
    causing the next test to start in an unexpected directory.  Do so
    inside a subshell if necessary.
 
+ - save and verify the standard error of compound commands, i.e. group
+   commands, subshells, and shell functions (except test helper
+   functions like 'test_must_fail') like this:
+
+     ( cd dir && git cmd ) 2>error &&
+     test_cmp expect error
+
+   When running the test with '-x' tracing, then the trace of commands
+   executed in the compound command will be included in standard error
+   as well, quite possibly throwing off the subsequent checks examining
+   the output.  Instead, save only the relevant git command's standard
+   error:
+
+     ( cd dir && git cmd 2>../error ) &&
+     test_cmp expect error
+
  - Break the TAP output
 
    The raw output from your test may be interpreted by a TAP harness. TAP
-- 
2.16.2.400.g911b7cc0da
SZEDER Gábor· Feb 23, 2018, 23:39 UTC · re: SZEDER Gábor · lore

[PATCH 08/11] t9903-bash-prompt: don't check the stderr of __git_ps1()

A test in 't9903-bash-prompt.sh' fails when the test script is run with '-x' tracing and a Bash version not yet supporting BASH_XTRACEFD, notably the default Bash version shipped in OSX. The reason for the failure is that the test checks the emptiness of __git_ps1()'s stderr, which includes the trace of all commands executed within __git_ps1() as well, throwing off the emptiness check.

Having only a single test checking the empty stderr doesn't bring us much when none of the other tests do so, so remove this test for now.

After this change t9903 passes with '-x', even when running with a Bash version not yet supporing BASH_XTRACEFD.

In the future we might want to consider checking the emptiness of __git_ps1()'s stderr in each and every test, in which case we'd have to mark this test script as 'test_untraceable', but that's a different topic.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t9903-bash-prompt.sh | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)
Show changes to t/t9903-bash-prompt.sh +2 −12
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 97c9b32c2e..8f5c811dd7 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -735,22 +735,12 @@ test_expect_success 'prompt - hide if pwd ignored - env var set, config unset, p
 	test_cmp expected "$actual"
 '
 
-test_expect_success 'prompt - hide if pwd ignored - inside gitdir (stdout)' '
+test_expect_success 'prompt - hide if pwd ignored - inside gitdir' '
 	printf " (GIT_DIR!)" >expected &&
 	(
 		GIT_PS1_HIDE_IF_PWD_IGNORED=y &&
 		cd .git &&
-		__git_ps1 >"$actual" 2>/dev/null
-	) &&
-	test_cmp expected "$actual"
-'
-
-test_expect_success 'prompt - hide if pwd ignored - inside gitdir (stderr)' '
-	printf "" >expected &&
-	(
-		GIT_PS1_HIDE_IF_PWD_IGNORED=y &&
-		cd .git &&
-		__git_ps1 >/dev/null 2>"$actual"
+		__git_ps1 >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
-- 
2.16.2.400.g911b7cc0da
SZEDER Gábor· Feb 23, 2018, 23:39 UTC · re: SZEDER Gábor · lore

[PATCH 06/11] t5526: use $TRASH_DIRECTORY to specify the path of GIT_TRACE log file

The test 'fetch --recurse-submodules -j2 has the same output behaviour' in 't5526-fetch-submodules.sh' fails when the test script is run with '-x' tracing (and using a shell other than a Bash version supporting BASH_XTRACEFD). The reason of that failure is the following command:

  GIT_TRACE=$(pwd)/../trace.out git fetch <...> 2>../actual.err

because the trace of executing 'pwd' in the command substitution ends up in 'actual.err' as well, throwing off the subsequent 'test_i18ncmp'.

Use $TRASH_DIRECTORY to specify the path of the GIT_TRACE log file instead of $(pwd), so the command's stderr remains free from tracing output.

After this change t5526 passes with '-x', even when running with /bin/sh.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5526-fetch-submodules.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t5526-fetch-submodules.sh +1 −1
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index a552ad4ead..ce44d8aa46 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -85,7 +85,7 @@ test_expect_success "fetch --recurse-submodules -j2 has the same output behaviou
 	add_upstream_commit &&
 	(
 		cd downstream &&
-		GIT_TRACE=$(pwd)/../trace.out git fetch --recurse-submodules -j2 2>../actual.err
+		GIT_TRACE="$TRASH_DIRECTORY/trace.out" git fetch --recurse-submodules -j2 2>../actual.err
 	) &&
 	test_must_be_empty actual.out &&
 	test_i18ncmp expect.err actual.err &&
-- 
2.16.2.400.g911b7cc0da
SZEDER Gábor· Feb 23, 2018, 23:39 UTC · re: SZEDER Gábor · lore

[PATCH 09/11] t1510-repo-setup: mark as untraceable with '-x'

't1510-repo-setup.sh' checks the stderr of nested function calls way too many times, resulting in several failures when using '-x' tracing, unless it's executed with a Bash version supporting BASH_XTRACEFD.

Maybe someday we will clear up this test script, but until then mark it as 'test_untraceable'.

After this change
  make GIT_TEST_OPTS='-x --verbose-log' test
finally fully passes without setting TEST_SHELL_PATH to Bash.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t1510-repo-setup.sh | 4 ++++
 1 file changed, 4 insertions(+)
Show changes to t/t1510-repo-setup.sh +4 −0
diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
index 13ae12dfa7..e6854b828e 100755
--- a/t/t1510-repo-setup.sh
+++ b/t/t1510-repo-setup.sh
@@ -39,6 +39,10 @@ A few rules for repo setup:
 11. When user's cwd is outside worktree, cwd remains unchanged,
     prefix is NULL.
 "
+
+# This test heavily relies on the standard error of nested function calls.
+test_untraceable=UnfortunatelyYes
+
 . ./test-lib.sh
 
 here=$(pwd)
-- 
2.16.2.400.g911b7cc0da
SZEDER Gábor· Mar 2, 2018, 15:32 UTC · re: SZEDER Gábor · lore

Re: [PATCH 00/11] Make the test suite pass with '-x' and /bin/sh

On Sat, Feb 24, 2018 at 12:39 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
Show 8 quoted lines
> This patch series makes '-x' tracing of tests work reliably even when
> running them with /bin/sh, and setting TEST_SHELL_PATH=bash will be
> unnecessary.
>
>   make GIT_TEST_OPTS='-x --verbose-log' test
>
> passes on my setup and on all Travis CI build jobs (though neither me
> nor Travis CI run all tests, e.g. CVS).

I installed 'cvs' and whatnot to run t94* and t96* tests, and sure enough, 5 tests in 2 test scripts fail with '-x' tracing and /bin/sh. I think I will be able to get around to send v2 during the weekend.

Jeff King· Mar 3, 2018, 07:12 UTC · re: SZEDER Gábor · lore

Re: [PATCH 00/11] Make the test suite pass with '-x' and /bin/sh

On Sat, Feb 24, 2018 at 12:39:40AM +0100, SZEDER Gábor wrote:
Show 5 quoted lines
> The first patch is the most important: with a couple of well-placed file
> descriptor redirections it ensures that the stderr of the test helper
> functions running git commands only contain the stderr of the tested
> command, thereby resolving over 90% of the failures resulting from
> running the test suite with '-x' and /bin/sh.

I dunno. It seems like this requires a lot of caveats for people using subshells and shell functions, and I suspect it's going to be an on-going maintenance burden.

That said, I'm not opposed if you want to do the work to try to get the whole test-suite clean, and we can see how it goes from there. It shouldn't be hurting anything, I don't think, aside from some mysterious-looking redirects (but your commit messages seem to explain it, so anybody can dig).

Does it make descriptor 7 magical, and something that scripts should avoid touching? That would mean we have 2 magical descriptors now.

-Peff
SZEDER Gábor· Mar 5, 2018, 21:18 UTC · re: Jeff King · lore

Re: [PATCH 00/11] Make the test suite pass with '-x' and /bin/sh

On Sat, Mar 3, 2018 at 8:12 AM, Jeff King <peff@peff.net> wrote:
Show 11 quoted lines
> On Sat, Feb 24, 2018 at 12:39:40AM +0100, SZEDER Gábor wrote:
>
>> The first patch is the most important: with a couple of well-placed file
>> descriptor redirections it ensures that the stderr of the test helper
>> functions running git commands only contain the stderr of the tested
>> command, thereby resolving over 90% of the failures resulting from
>> running the test suite with '-x' and /bin/sh.
>
> I dunno. It seems like this requires a lot of caveats for people using
> subshells and shell functions, and I suspect it's going to be an
> on-going maintenance burden.

After finally figuring out the redirections in the first patch, I was quite surprised by how few failing tests remained. We only gathered 28 such tests over all these years; if it continues at this rate, that probably won't be that much of a burden. And the second patch provides an escape hatch, should it ever be needed.

The current situation, however, is a burden much more frequently, because the idiosyncrasies of TEST_SHELL_PATH and/or '--verbose-log' pop up whenever trying to run any test script with '-x' that has such a test in it.

I think this is the right tradeoff.
Show 8 quoted lines
> That said, I'm not opposed if you want to do the work to try to get the
> whole test-suite clean, and we can see how it goes from there. It
> shouldn't be hurting anything, I don't think, aside from some
> mysterious-looking redirects (but your commit messages seem to explain
> it, so anybody can dig).
>
> Does it make descriptor 7 magical, and something that scripts should
> avoid touching? That would mean we have 2 magical descriptors now.

Tests can still use fd 7 as long as they don't intend to attach it directly to that particular git command that is run inside one of these test helper functions.

I settled on fd 7 because that fd is already used as stderr for the 'test_pause' and 'debug' helper functions and it isn't used in any of our tests.

← back to recent threads