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

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

[SZEDER Gábor]

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

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

[SZEDER Gábor]

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(-)

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

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

[SZEDER Gábor]

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(-)

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

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

[SZEDER Gábor]

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(-)

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

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

[SZEDER Gábor]

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(-)

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

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

[SZEDER Gábor]

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(-)

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

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

[SZEDER Gábor]

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(-)

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

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

[SZEDER Gábor]

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(-)

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

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

[SZEDER Gábor]

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(-)

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

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

[SZEDER Gábor]

'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(+)

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

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

[SZEDER Gábor]

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(-)

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

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

[SZEDER Gábor]

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(-)

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

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

[Eric Sunshine]

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.

> '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>

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

[SZEDER Gábor]

On Sat, Feb 24, 2018 at 12:39 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:

>   - 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.

> 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.

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

[SZEDER Gábor]

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(-)

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

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

[Junio C Hamano]

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?

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

[Junio C Hamano]

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?


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(-)

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"

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

[Junio C Hamano]

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?

 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

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

[Junio C Hamano]

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(-)

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

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

[SZEDER Gábor]

On Tue, Feb 27, 2018 at 10:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

>  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"

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

[Stefan Beller]

On Tue, Feb 27, 2018 at 1:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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>

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

[Jeff King]

On Tue, Feb 27, 2018 at 01:27:29PM -0800, Junio C Hamano wrote:

> 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

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

[SZEDER Gábor]

On Tue, Feb 27, 2018 at 10:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.


>  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

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

[SZEDER Gábor]

On Sat, Feb 24, 2018 at 12:39 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> 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.

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

[Jeff King]

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.

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

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

[SZEDER Gábor]

On Sat, Mar 3, 2018 at 8:12 AM, Jeff King <peff@peff.net> wrote:
> 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.

> 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.