{"thread":{"id":"47915","subject":"[PATCH 00/11] Make the test suite pass with '-x' and /bin/sh","startedAt":"2018-02-23T23:40:08Z","lastAt":"2018-03-05T21:18:41Z","messageCount":26,"participants":["SZEDER Gábor","Eric Sunshine","Junio C Hamano","Stefan Beller","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"340099","messageId":"20180223233951.11154-1-szeder.dev@gmail.com","threadId":"47915","inReplyTo":null,"subject":"[PATCH 00/11] Make the test suite pass with '-x' and /bin/sh","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-23T23:39:40Z","receivedAt":"2018-02-23T23:40:08Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"This patch series makes '-x' tracing of tests work reliably even when\nrunning them with /bin/sh, and setting TEST_SHELL_PATH=bash will be\nunnecessary.\n\n  make GIT_TEST_OPTS='-x --verbose-log' test\n\npasses on my setup and on all Travis CI build jobs (though neither me\nnor Travis CI run all tests, e.g. CVS).\n\n\nThe first patch is the most important: with a couple of well-placed file\ndescriptor redirections it ensures that the stderr of the test helper\nfunctions running git commands only contain the stderr of the tested\ncommand, thereby resolving over 90% of the failures resulting from\nrunning the test suite with '-x' and /bin/sh.\n\nMost of the following patches resolve the remaining failures, one test\nscript at a time, in most cases by limiting the scope of stderr\nredirections from functions and subshells to the tested git commands.\nExcept the second and ninth patches, which, arguably, could be\nconsidered as cheating...  I admit, my enthusiasm suddenly run out when\nI saw t1510 :)\n\nThe last two patches are just finishing touches with a bit of\ndocumentation updates and enabling '-x' tracing in Travis CI build jobs.\n\n\nThere is currently nothing in 'pu' that would require additional fixes\nto make this patch series work.\n\n\nSZEDER Gábor (11):\n  t: prevent '-x' tracing from interfering with test helpers' stderr\n  t: add means to disable '-x' tracing for individual test scripts\n  t1507-rev-parse-upstream: don't check the stderr of a shell function\n  t3030-merge-recursive: don't check the stderr of a subshell\n  t5500-fetch-pack: don't check the stderr of a subshell\n  t5526: use $TRASH_DIRECTORY to specify the path of GIT_TRACE log file\n  t5570-git-daemon: don't check the stderr of a subshell\n  t9903-bash-prompt: don't check the stderr of __git_ps1()\n  t1510-repo-setup: mark as untraceable with '-x'\n  t/README: add a note about don't saving stderr of compound commands\n  travis-ci: run tests with '-x' tracing\n\n ci/lib-travisci.sh            |  2 +-\n t/README                      | 23 +++++++++++++++++++---\n t/lib-terminal.sh             |  4 ++--\n t/t1507-rev-parse-upstream.sh | 14 +++++++-------\n t/t1510-repo-setup.sh         |  4 ++++\n t/t3030-merge-recursive.sh    | 36 +++++++++++++++++++----------------\n t/t5500-fetch-pack.sh         | 12 ++++++------\n t/t5526-fetch-submodules.sh   |  2 +-\n t/t5570-git-daemon.sh         |  2 +-\n t/t9903-bash-prompt.sh        | 14 ++------------\n t/test-lib-functions.sh       | 24 +++++++++++------------\n t/test-lib.sh                 | 19 +++++++++++++++++-\n 12 files changed, 94 insertions(+), 62 deletions(-)\n\n-- \n2.16.2.400.g911b7cc0da\n\n"},{"id":"340100","messageId":"20180223233951.11154-3-szeder.dev@gmail.com","threadId":"47915","inReplyTo":"20180223233951.11154-1-szeder.dev@gmail.com","subject":"[PATCH 02/11] t: add means to disable '-x' tracing for individual test scripts","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-23T23:39:42Z","receivedAt":"2018-02-23T23:40:16Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The previous patch resolved most of the test failures caused by\nrunning our test suite with '-x' tracing and /bin/sh, and the\nfollowing patches in this series will resolve almost all of the\nremaining failures.  Unfortunately, not yet all.\n\nAdd means to disable '-x' tracing for individual test scripts by\nsetting the $test_untraceable variable to a non-empty value in the\ntest script before sourcing 'test-lib.sh'.  However, since '-x'\ntracing is not an issue with recent Bash versions supporting\nBASH_XTRACEFD, i.e. v4.1 and later, don't disable tracing when the\ntest script is run with such a Bash version even when\n$test_untraceable is set.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/README      |  3 +++\n t/test-lib.sh | 19 ++++++++++++++++++-\n 2 files changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/t/README b/t/README\nindex b3f7b449c3..c430e9c52c 100644\n--- a/t/README\n+++ b/t/README\n@@ -87,6 +87,9 @@ appropriately before running \"make\".\n \tthemselves. Implies `--verbose`. Note that in non-bash shells,\n \tthis can cause failures in some tests which redirect and test\n \tthe output of shell functions. Use with caution.\n+\tIgnored in test scripts that set the variable 'test_untraceable'\n+\tto a non-empty value, unless it's run with a Bash version\n+\tsupporting BASH_XTRACEFD, i.e. v4.1 or later.\n \n -d::\n --debug::\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 33f6ce26f6..732213ef1b 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -263,7 +263,24 @@ do\n \t\tGIT_TEST_CHAIN_LINT=0\n \t\tshift ;;\n \t-x)\n-\t\ttrace=t\n+\t\t# Some test scripts can't be reliably traced  with '-x',\n+\t\t# unless the test is run with a Bash version supporting\n+\t\t# BASH_XTRACEFD (introduced in Bash v4.1).  Check whether\n+\t\t# this test is marked as such, and ignore '-x' if it\n+\t\t# isn't executed with a suitable Bash version.\n+\t\tif test -z \"$test_untraceable\" || {\n+\t\t     test -n \"$BASH_VERSION\" && {\n+\t\t       test ${BASH_VERSINFO[0]} -gt 4 || {\n+\t\t\t test ${BASH_VERSINFO[0]} -eq 4 &&\n+\t\t\t test ${BASH_VERSINFO[1]} -ge 1\n+\t\t       }\n+\t\t     }\n+\t\t   }\n+\t\tthen\n+\t\t\ttrace=t\n+\t\telse\n+\t\t\techo >&2 \"warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD\"\n+\t\tfi\n \t\tshift ;;\n \t--verbose-log)\n \t\tverbose_log=t\n-- \n2.16.2.400.g911b7cc0da\n\n"},{"id":"340101","messageId":"20180223233951.11154-2-szeder.dev@gmail.com","threadId":"47915","inReplyTo":"20180223233951.11154-1-szeder.dev@gmail.com","subject":"[PATCH 01/11] t: prevent '-x' tracing from interfering with test helpers' stderr","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-23T23:39:41Z","receivedAt":"2018-02-23T23:40:19Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Running a test script with '-x' turns on 'set -x' tracing, the output\nof which is normally sent to stderr.  This causes a lot of\ntest failures, because many tests redirect and verify the stderr\nof shell functions, most frequently that of 'test_must_fail'.\nThese issues were worked around somewhat in d88785e424 (test-lib: set\nBASH_XTRACEFD automatically, 2016-05-11), so at least we could\nreliably run tests with '-x' tracing under a Bash version supporting\nBASH_XTRACEFD, i.e. v4.1 and later.\n\nThis patch makes it safe to redirect and verify the stderr of those\ntest helper functions which are meant to run the tested command given\nas argument, even when running tests with '-x' and /bin/sh.  This is\nachieved through a couple of file descriptor redirections:\n\n  - Duplicate stderr of the tested command executed in the test helper\n    function from the function's fd 7 (see next point), to ensure that\n    the tested command's error messages go to a different fd than the\n    '-x' trace of the commands executed in the function.\n\n  - Duplicate the test helper function's fd 7 from the function's\n    original stderr, meaning that, after taking a detour through fd 7,\n    the error messages of the tested command do end up on the\n    function's original stderr.\n\n  - Duplicate stderr of the test helper function from fd 4, i.e. the\n    fd connected to the test script's original stderr and the fd used\n    for BASH_XTRACEFD.  This ensures that the '-x' trace of the\n    commands executed in the function\n\n      - doesn't go to the function's original stderr, so it won't mess\n\twith callers who want to save and verify the tested command's\n\tstderr.\n\n      - does go to the same fd independently from the shell running\n        the test script, be it /bin/sh, an older Bash without\n        BASH_XTRACEFD, or a more recent Bash already supporting\n        BASH_XTRACEFD.\n\n  - Specify the latter two redirections above in the test helper\n    function's definition, so they are performed every time the\n    function is invoked, without the need to modify the callsites of\n    the function.\n\nPerform these redirections in those test helper functions which can be\nexpected to have their stderr redirected, i.e. in the functions\n'test_must_fail', 'test_might_fail', 'test_expect_code', 'test_env',\n'nongit', 'test_terminal' and 'perl'.  Note that 'test_might_fail',\n'test_env', and 'nongit' are not involved in any test failures when\nrunning tests with '-x' and /bin/sh.\n\nThe other test helper functions are left unchanged, because they\neither don't run commands specified as their arguments, or redirecting\ntheir stderr wouldn't make sense, or both.\n\nWith this change the number of failures when running the test suite\nwith '-x' tracing and /bin/sh goes down from 340 failed tests in 43\ntest scripts to 22 failed tests in 6 scripts (or 23 in 7, if the\nsystem (OSX) uses an older Bash version without BASH_XTRACEFD to run\n't9903-bash-prompt.sh').\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/lib-terminal.sh       |  4 ++--\n t/test-lib-functions.sh | 24 ++++++++++++------------\n 2 files changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/t/lib-terminal.sh b/t/lib-terminal.sh\nindex cd220e378e..b3acb4c6f8 100644\n--- a/t/lib-terminal.sh\n+++ b/t/lib-terminal.sh\n@@ -9,8 +9,8 @@ test_terminal () {\n \t\techo >&4 \"test_terminal: need to declare TTY prerequisite\"\n \t\treturn 127\n \tfi\n-\tperl \"$TEST_DIRECTORY\"/test-terminal.perl \"$@\"\n-}\n+\tperl \"$TEST_DIRECTORY\"/test-terminal.perl \"$@\" 2>&9\n+} 9>&2 2>&4\n \n test_lazy_prereq TTY '\n \ttest_have_prereq PERL &&\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 67b5994afb..37eb34044a 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -621,7 +621,7 @@ test_must_fail () {\n \t\t_test_ok=\n \t\t;;\n \tesac\n-\t\"$@\"\n+\t\"$@\" 2>&7\n \texit_code=$?\n \tif test $exit_code -eq 0 && ! list_contains \"$_test_ok\" success\n \tthen\n@@ -644,7 +644,7 @@ test_must_fail () {\n \t\treturn 1\n \tfi\n \treturn 0\n-}\n+} 7>&2 2>&4\n \n # Similar to test_must_fail, but tolerates success, too.  This is\n # meant to be used in contexts like:\n@@ -658,8 +658,8 @@ test_must_fail () {\n # because we want to notice if it fails due to segv.\n \n test_might_fail () {\n-\ttest_must_fail ok=success \"$@\"\n-}\n+\ttest_must_fail ok=success \"$@\" 2>&7\n+} 7>&2 2>&4\n \n # Similar to test_must_fail and test_might_fail, but check that a\n # given command exited with a given exit code. Meant to be used as:\n@@ -671,7 +671,7 @@ test_might_fail () {\n test_expect_code () {\n \twant_code=$1\n \tshift\n-\t\"$@\"\n+\t\"$@\" 2>&7\n \texit_code=$?\n \tif test $exit_code = $want_code\n \tthen\n@@ -680,7 +680,7 @@ test_expect_code () {\n \n \techo >&2 \"test_expect_code: command exited with $exit_code, we wanted $want_code $*\"\n \treturn 1\n-}\n+} 7>&2 2>&4\n \n # test_cmp is a helper function to compare actual and expected output.\n # You can use it like:\n@@ -882,8 +882,8 @@ test_write_lines () {\n }\n \n perl () {\n-\tcommand \"$PERL_PATH\" \"$@\"\n-}\n+\tcommand \"$PERL_PATH\" \"$@\" 2>&7\n+} 7>&2 2>&4\n \n # Is the value one of the various ways to spell a boolean true/false?\n test_normalize_bool () {\n@@ -1023,13 +1023,13 @@ test_env () {\n \t\t\t\tshift\n \t\t\t\t;;\n \t\t\t*)\n-\t\t\t\t\"$@\"\n+\t\t\t\t\"$@\" 2>&7\n \t\t\t\texit\n \t\t\t\t;;\n \t\t\tesac\n \t\tdone\n \t)\n-}\n+} 7>&2 2>&4\n \n # Returns true if the numeric exit code in \"$2\" represents the expected signal\n # in \"$1\". Signals should be given numerically.\n@@ -1071,9 +1071,9 @@ nongit () {\n \t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n \t\texport GIT_CEILING_DIRECTORIES &&\n \t\tcd non-repo &&\n-\t\t\"$@\"\n+\t\t\"$@\" 2>&7\n \t)\n-}\n+} 7>&2 2>&4\n \n # convert stdin to pktline representation; note that empty input becomes an\n # empty packet, not a flush packet (for that you can just print 0000 yourself).\n-- \n2.16.2.400.g911b7cc0da\n\n"},{"id":"340102","messageId":"20180223233951.11154-4-szeder.dev@gmail.com","threadId":"47915","inReplyTo":"20180223233951.11154-1-szeder.dev@gmail.com","subject":"[PATCH 03/11] t1507-rev-parse-upstream: don't check the stderr of a shell function","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-23T23:39:43Z","receivedAt":"2018-02-23T23:40:21Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Three tests in 't1507-rev-parse-upstream.sh' fail when the test script\nis run with '-x' tracing (and using a shell other than a Bash version\nsupporting BASH_XTRACEFD).  The reason for those failures is that the\ntests check the stderr of the function 'error_message', which includes\nthe trace of commands executed in that function as well, throwing off\nthe comparison with the expected output.\n\nSave stderr of 'git rev-parse' only instead of the whole function, so\nit remains free from tracing output.\n\nAfter this change t1507 passes with '-x', even when running with\n/bin/sh.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/t1507-rev-parse-upstream.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex b23c4e3fab..2ce68cc277 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -42,7 +42,7 @@ commit_subject () {\n \n error_message () {\n \t(cd clone &&\n-\t test_must_fail git rev-parse --verify \"$@\")\n+\t test_must_fail git rev-parse --verify \"$@\" 2>../error)\n }\n \n test_expect_success '@{upstream} resolves to correct full name' '\n@@ -159,8 +159,8 @@ test_expect_success 'branch@{u} error message when no upstream' '\n \tcat >expect <<-EOF &&\n \tfatal: no upstream configured for branch ${sq}non-tracking${sq}\n \tEOF\n-\terror_message non-tracking@{u} 2>actual &&\n-\ttest_i18ncmp expect actual\n+\terror_message non-tracking@{u} &&\n+\ttest_i18ncmp expect error\n '\n \n test_expect_success '@{u} error message when no upstream' '\n@@ -175,8 +175,8 @@ test_expect_success 'branch@{u} error message with misspelt branch' '\n \tcat >expect <<-EOF &&\n \tfatal: no such branch: ${sq}no-such-branch${sq}\n \tEOF\n-\terror_message no-such-branch@{u} 2>actual &&\n-\ttest_i18ncmp expect actual\n+\terror_message no-such-branch@{u} &&\n+\ttest_i18ncmp expect error\n '\n \n test_expect_success '@{u} error message when not on a branch' '\n@@ -192,8 +192,8 @@ test_expect_success 'branch@{u} error message if upstream branch not fetched' '\n \tcat >expect <<-EOF &&\n \tfatal: upstream branch ${sq}refs/heads/side${sq} not stored as a remote-tracking branch\n \tEOF\n-\terror_message bad-upstream@{u} 2>actual &&\n-\ttest_i18ncmp expect actual\n+\terror_message bad-upstream@{u} &&\n+\ttest_i18ncmp expect error\n '\n \n test_expect_success 'pull works when tracking a local branch' '\n-- \n2.16.2.400.g911b7cc0da\n\n"},{"id":"340103","messageId":"20180223233951.11154-6-szeder.dev@gmail.com","threadId":"47915","inReplyTo":"20180223233951.11154-1-szeder.dev@gmail.com","subject":"[PATCH 05/11] t5500-fetch-pack: don't check the stderr of a subshell","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-23T23:39:45Z","receivedAt":"2018-02-23T23:40:24Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Three \"missing reference\" tests in 't5500-fetch-pack.sh' fail when the\ntest script is run with '-x' tracing (and using a shell other than a\nBash version supporting BASH_XTRACEFD).  The reason for those failures\nis that the tests check a subshell's stderr, which includes the trace\nof executing commands in that subshell as well, throwing off the\ncomparison with the expected output.\n\nSave the stderr of 'git fetch-pack' only instead of the whole\nsubshell, so it remains free from tracing output.\n\nAfter this change t5500 passes with '-x', even when running with\n/bin/sh.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/t5500-fetch-pack.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex ec9ba9bf6e..0680dec808 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -482,24 +482,24 @@ test_expect_success 'set up tests of missing reference' '\n test_expect_success 'test lonely missing ref' '\n \t(\n \t\tcd client &&\n-\t\ttest_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy\n-\t) >/dev/null 2>error-m &&\n+\t\ttest_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy 2>../error-m\n+\t) &&\n \ttest_i18ncmp expect-error error-m\n '\n \n test_expect_success 'test missing ref after existing' '\n \t(\n \t\tcd client &&\n-\t\ttest_must_fail git fetch-pack --no-progress .. refs/heads/A refs/heads/xyzzy\n-\t) >/dev/null 2>error-em &&\n+\t\ttest_must_fail git fetch-pack --no-progress .. refs/heads/A refs/heads/xyzzy 2>../error-em\n+\t) &&\n \ttest_i18ncmp expect-error error-em\n '\n \n test_expect_success 'test missing ref before existing' '\n \t(\n \t\tcd client &&\n-\t\ttest_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy refs/heads/A\n-\t) >/dev/null 2>error-me &&\n+\t\ttest_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy refs/heads/A 2>../error-me\n+\t) &&\n \ttest_i18ncmp expect-error error-me\n '\n \n-- \n2.16.2.400.g911b7cc0da\n\n"},{"id":"340104","messageId":"20180223233951.11154-8-szeder.dev@gmail.com","threadId":"47915","inReplyTo":"20180223233951.11154-1-szeder.dev@gmail.com","subject":"[PATCH 07/11] t5570-git-daemon: don't check the stderr of a subshell","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-23T23:39:47Z","receivedAt":"2018-02-23T23:40:26Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The test 'no-op fetch without \"-v\" is quiet' in 't5570-git-daemon.sh'\nfails when the test script is run with '-x' tracing (and using a shell\nother than a Bash version supporting BASH_XTRACEFD).  The reason for\nthe failure is that the test checks the emptiness of a subshell's\nstderr, which includes the trace of commands executed in that subshell\nas well, throwing off the emptiness check.\n\nSave the stderr of 'git fetch' only instead of the whole subshell's, so\nit remains free from tracing output.\n\nAfter this change t5570 passes with '-x', even when running with\n/bin/sh.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/t5570-git-daemon.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex 755b05a8ae..0d4c52016b 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -50,7 +50,7 @@ test_expect_success 'no-op fetch -v stderr is as expected' '\n '\n \n test_expect_success 'no-op fetch without \"-v\" is quiet' '\n-\t(cd clone && git fetch) 2>stderr &&\n+\t(cd clone && git fetch 2>../stderr) &&\n \t! test -s stderr\n '\n \n-- \n2.16.2.400.g911b7cc0da\n\n"},{"id":"340105","messageId":"20180223233951.11154-12-szeder.dev@gmail.com","threadId":"47915","inReplyTo":"20180223233951.11154-1-szeder.dev@gmail.com","subject":"[PATCH 11/11] travis-ci: run tests with '-x' tracing","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-23T23:39:51Z","receivedAt":"2018-02-23T23:40:29Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Now that the test suite runs successfully with '-x' tracing even with\n/bin/sh, enable it on Travis CI in order to\n\n  - get more information about test failures, and\n\n  - catch constructs breaking '-x' with /bin/sh sneaking into our test\n    suite.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n ci/lib-travisci.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh\nindex 1efee55ef7..109ef280da 100755\n--- a/ci/lib-travisci.sh\n+++ b/ci/lib-travisci.sh\n@@ -97,7 +97,7 @@ fi\n export DEVELOPER=1\n export DEFAULT_TEST_TARGET=prove\n export GIT_PROVE_OPTS=\"--timer --jobs 3 --state=failed,slow,save\"\n-export GIT_TEST_OPTS=\"--verbose-log\"\n+export GIT_TEST_OPTS=\"--verbose-log -x\"\n export GIT_TEST_CLONE_2GB=YesPlease\n \n case \"$jobname\" in\n-- \n2.16.2.400.g911b7cc0da\n\n"},{"id":"340106","messageId":"20180223233951.11154-5-szeder.dev@gmail.com","threadId":"47915","inReplyTo":"20180223233951.11154-1-szeder.dev@gmail.com","subject":"[PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-23T23:39:44Z","receivedAt":"2018-02-23T23:40:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The two test checking 'git mmerge-recursive' in an empty worktree in\n't3030-merge-recursive.sh' fail when the test script is run with '-x'\ntracing (and using a shell other than a Bash version supporting\nBASH_XTRACEFD).  The reason for those failures is that the tests check\nthe emptiness of a subshell's stderr, which includes the trace of\ncommands executed in that subshell as well, throwing off the emptiness\ncheck.\n\nNote that both subshells execute four git commands each, meaning that\nchecking the emptiness of the whole subshell implicitly ensures that\nnot only 'git merge-recursive' but none of the other three commands\noutputs anything to their stderr.  Note also that if one of those\ncommands were to output anything on its stderr, then the current\ncombined check would not tell us which one of those four commands the\nunexpected output came from.\n\nSave the stderr of those four commands only instead of the whole\nsubshell, so it remains free from tracing output, and save and check\nthem individually, so they will show us from which command the\nunexpected output came from.\n\nAfter this change t3030 passes with '-x', even when running with\n/bin/sh.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/t3030-merge-recursive.sh | 36 ++++++++++++++++++++----------------\n 1 file changed, 20 insertions(+), 16 deletions(-)\n\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex cdc38fe5d1..cbeea1cf94 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -525,20 +525,22 @@ test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '\n \t\tGIT_INDEX_FILE=\"$PWD/ours-has-rename-index\" &&\n \t\texport GIT_INDEX_FILE &&\n \t\tmkdir \"$GIT_WORK_TREE\" &&\n-\t\tgit read-tree -i -m $c7 &&\n-\t\tgit update-index --ignore-missing --refresh &&\n-\t\tgit merge-recursive $c0 -- $c7 $c3 &&\n-\t\tgit ls-files -s >actual-files\n-\t) 2>actual-err &&\n-\t>expected-err &&\n+\t\tgit read-tree -i -m $c7 2>actual-err &&\n+\t\ttest_must_be_empty expected-err &&\n+\t\tgit update-index --ignore-missing --refresh 2>actual-err &&\n+\t\ttest_must_be_empty expected-err &&\n+\t\tgit merge-recursive $c0 -- $c7 $c3 2>actual-err &&\n+\t\ttest_must_be_empty expected-err &&\n+\t\tgit ls-files -s >actual-files 2>actual-err &&\n+\t\ttest_must_be_empty expected-err\n+\t) &&\n \tcat >expected-files <<-EOF &&\n \t100644 $o3 0\tb/c\n \t100644 $o0 0\tc\n \t100644 $o0 0\td/e\n \t100644 $o0 0\te\n \tEOF\n-\ttest_cmp expected-files actual-files &&\n-\ttest_cmp expected-err actual-err\n+\ttest_cmp expected-files actual-files\n '\n \n test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '\n@@ -548,20 +550,22 @@ test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '\n \t\tGIT_INDEX_FILE=\"$PWD/theirs-has-rename-index\" &&\n \t\texport GIT_INDEX_FILE &&\n \t\tmkdir \"$GIT_WORK_TREE\" &&\n-\t\tgit read-tree -i -m $c3 &&\n-\t\tgit update-index --ignore-missing --refresh &&\n-\t\tgit merge-recursive $c0 -- $c3 $c7 &&\n-\t\tgit ls-files -s >actual-files\n-\t) 2>actual-err &&\n-\t>expected-err &&\n+\t\tgit read-tree -i -m $c3 2>actual-err &&\n+\t\ttest_must_be_empty expected-err &&\n+\t\tgit update-index --ignore-missing --refresh 2>>actual-err &&\n+\t\ttest_must_be_empty expected-err &&\n+\t\tgit merge-recursive $c0 -- $c3 $c7 2>>actual-err &&\n+\t\ttest_must_be_empty expected-err &&\n+\t\tgit ls-files -s >actual-files 2>>actual-err &&\n+\t\ttest_must_be_empty expected-err\n+\t) &&\n \tcat >expected-files <<-EOF &&\n \t100644 $o3 0\tb/c\n \t100644 $o0 0\tc\n \t100644 $o0 0\td/e\n \t100644 $o0 0\te\n \tEOF\n-\ttest_cmp expected-files actual-files &&\n-\ttest_cmp expected-err actual-err\n+\ttest_cmp expected-files actual-files\n '\n \n test_expect_success 'merge removes empty directories' '\n-- \n2.16.2.400.g911b7cc0da\n\n"},{"id":"340107","messageId":"20180223233951.11154-11-szeder.dev@gmail.com","threadId":"47915","inReplyTo":"20180223233951.11154-1-szeder.dev@gmail.com","subject":"[PATCH 10/11] t/README: add a note about don't saving stderr of compound commands","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-23T23:39:50Z","receivedAt":"2018-02-23T23:40:33Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Explain in 't/README' why it is a bad idea to redirect and verify the\nstderr of compound commands, in the hope that future contributions\nwill follow this advice and the test suite will keep working with '-x'\ntracing and /bin/sh.\n\nWhile at it, since we can now run the test suite with '-x' without\nneeding a Bash version supporting BASH_XTRACEFD, remove the now\noutdated caution note about non-Bash shells from the description of\nthe '-x' option.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/README | 20 +++++++++++++++++---\n 1 file changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex c430e9c52c..615682263e 100644\n--- a/t/README\n+++ b/t/README\n@@ -84,9 +84,7 @@ appropriately before running \"make\".\n \n -x::\n \tTurn on shell tracing (i.e., `set -x`) during the tests\n-\tthemselves. Implies `--verbose`. Note that in non-bash shells,\n-\tthis can cause failures in some tests which redirect and test\n-\tthe output of shell functions. Use with caution.\n+\tthemselves. Implies `--verbose`.\n \tIgnored in test scripts that set the variable 'test_untraceable'\n \tto a non-empty value, unless it's run with a Bash version\n \tsupporting BASH_XTRACEFD, i.e. v4.1 or later.\n@@ -455,6 +453,22 @@ Don't:\n    causing the next test to start in an unexpected directory.  Do so\n    inside a subshell if necessary.\n \n+ - save and verify the standard error of compound commands, i.e. group\n+   commands, subshells, and shell functions (except test helper\n+   functions like 'test_must_fail') like this:\n+\n+     ( cd dir && git cmd ) 2>error &&\n+     test_cmp expect error\n+\n+   When running the test with '-x' tracing, then the trace of commands\n+   executed in the compound command will be included in standard error\n+   as well, quite possibly throwing off the subsequent checks examining\n+   the output.  Instead, save only the relevant git command's standard\n+   error:\n+\n+     ( cd dir && git cmd 2>../error ) &&\n+     test_cmp expect error\n+\n  - Break the TAP output\n \n    The raw output from your test may be interpreted by a TAP harness. TAP\n-- \n2.16.2.400.g911b7cc0da\n\n"},{"id":"340108","messageId":"20180223233951.11154-9-szeder.dev@gmail.com","threadId":"47915","inReplyTo":"20180223233951.11154-1-szeder.dev@gmail.com","subject":"[PATCH 08/11] t9903-bash-prompt: don't check the stderr of __git_ps1()","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-23T23:39:48Z","receivedAt":"2018-02-23T23:40:40Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"A test in 't9903-bash-prompt.sh' fails when the test script is run\nwith '-x' tracing and a Bash version not yet supporting BASH_XTRACEFD,\nnotably the default Bash version shipped in OSX.  The reason for the\nfailure is that the test checks the emptiness of __git_ps1()'s stderr,\nwhich includes the trace of all commands executed within __git_ps1()\nas well, throwing off the emptiness check.\n\nHaving only a single test checking the empty stderr doesn't bring us\nmuch when none of the other tests do so, so remove this test for now.\n\nAfter this change t9903 passes with '-x', even when running with a\nBash version not yet supporing BASH_XTRACEFD.\n\nIn the future we might want to consider checking the emptiness of\n__git_ps1()'s stderr in each and every test, in which case we'd have\nto mark this test script as 'test_untraceable', but that's a different\ntopic.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/t9903-bash-prompt.sh | 14 ++------------\n 1 file changed, 2 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh\nindex 97c9b32c2e..8f5c811dd7 100755\n--- a/t/t9903-bash-prompt.sh\n+++ b/t/t9903-bash-prompt.sh\n@@ -735,22 +735,12 @@ test_expect_success 'prompt - hide if pwd ignored - env var set, config unset, p\n \ttest_cmp expected \"$actual\"\n '\n \n-test_expect_success 'prompt - hide if pwd ignored - inside gitdir (stdout)' '\n+test_expect_success 'prompt - hide if pwd ignored - inside gitdir' '\n \tprintf \" (GIT_DIR!)\" >expected &&\n \t(\n \t\tGIT_PS1_HIDE_IF_PWD_IGNORED=y &&\n \t\tcd .git &&\n-\t\t__git_ps1 >\"$actual\" 2>/dev/null\n-\t) &&\n-\ttest_cmp expected \"$actual\"\n-'\n-\n-test_expect_success 'prompt - hide if pwd ignored - inside gitdir (stderr)' '\n-\tprintf \"\" >expected &&\n-\t(\n-\t\tGIT_PS1_HIDE_IF_PWD_IGNORED=y &&\n-\t\tcd .git &&\n-\t\t__git_ps1 >/dev/null 2>\"$actual\"\n+\t\t__git_ps1 >\"$actual\"\n \t) &&\n \ttest_cmp expected \"$actual\"\n '\n-- \n2.16.2.400.g911b7cc0da\n\n"},{"id":"340109","messageId":"20180223233951.11154-7-szeder.dev@gmail.com","threadId":"47915","inReplyTo":"20180223233951.11154-1-szeder.dev@gmail.com","subject":"[PATCH 06/11] t5526: use $TRASH_DIRECTORY to specify the path of GIT_TRACE log file","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-23T23:39:46Z","receivedAt":"2018-02-23T23:40:41Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The test 'fetch --recurse-submodules -j2 has the same output\nbehaviour' in 't5526-fetch-submodules.sh' fails when the test script\nis run with '-x' tracing (and using a shell other than a Bash version\nsupporting BASH_XTRACEFD).  The reason of that failure is the\nfollowing command:\n\n  GIT_TRACE=$(pwd)/../trace.out git fetch <...> 2>../actual.err\n\nbecause the trace of executing 'pwd' in the command substitution ends\nup in 'actual.err' as well, throwing off the subsequent\n'test_i18ncmp'.\n\nUse $TRASH_DIRECTORY to specify the path of the GIT_TRACE log file\ninstead of $(pwd), so the command's stderr remains free from tracing\noutput.\n\nAfter this change t5526 passes with '-x', even when running with\n/bin/sh.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/t5526-fetch-submodules.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex a552ad4ead..ce44d8aa46 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -85,7 +85,7 @@ test_expect_success \"fetch --recurse-submodules -j2 has the same output behaviou\n \tadd_upstream_commit &&\n \t(\n \t\tcd downstream &&\n-\t\tGIT_TRACE=$(pwd)/../trace.out git fetch --recurse-submodules -j2 2>../actual.err\n+\t\tGIT_TRACE=\"$TRASH_DIRECTORY/trace.out\" git fetch --recurse-submodules -j2 2>../actual.err\n \t) &&\n \ttest_must_be_empty actual.out &&\n \ttest_i18ncmp expect.err actual.err &&\n-- \n2.16.2.400.g911b7cc0da\n\n"},{"id":"340110","messageId":"20180223233951.11154-10-szeder.dev@gmail.com","threadId":"47915","inReplyTo":"20180223233951.11154-1-szeder.dev@gmail.com","subject":"[PATCH 09/11] t1510-repo-setup: mark as untraceable with '-x'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-23T23:39:49Z","receivedAt":"2018-02-23T23:40:44Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'t1510-repo-setup.sh' checks the stderr of nested function calls way\ntoo many times, resulting in several failures when using '-x' tracing,\nunless it's executed with a Bash version supporting BASH_XTRACEFD.\n\nMaybe someday we will clear up this test script, but until then mark\nit as 'test_untraceable'.\n\nAfter this change\n\n  make GIT_TEST_OPTS='-x --verbose-log' test\n\nfinally fully passes without setting TEST_SHELL_PATH to Bash.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/t1510-repo-setup.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh\nindex 13ae12dfa7..e6854b828e 100755\n--- a/t/t1510-repo-setup.sh\n+++ b/t/t1510-repo-setup.sh\n@@ -39,6 +39,10 @@ A few rules for repo setup:\n 11. When user's cwd is outside worktree, cwd remains unchanged,\n     prefix is NULL.\n \"\n+\n+# This test heavily relies on the standard error of nested function calls.\n+test_untraceable=UnfortunatelyYes\n+\n . ./test-lib.sh\n \n here=$(pwd)\n-- \n2.16.2.400.g911b7cc0da\n\n"},{"id":"340159","messageId":"CAPig+cTSGNE_qzP74Bn+JnhvRsVt1-HzAiEf=1w_Rk6rUCrsWg@mail.gmail.com","threadId":"47915","inReplyTo":"20180223233951.11154-5-szeder.dev@gmail.com","subject":"Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-24T08:04:20Z","receivedAt":"2018-02-24T08:04:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 23, 2018 at 6:39 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> The two test checking 'git mmerge-recursive' in an empty worktree in\n\ns/mmerge/merge/, I guess.\n\n> 't3030-merge-recursive.sh' fail when the test script is run with '-x'\n> tracing (and using a shell other than a Bash version supporting\n> BASH_XTRACEFD).  The reason for those failures is that the tests check\n> the emptiness of a subshell's stderr, which includes the trace of\n> commands executed in that subshell as well, throwing off the emptiness\n> check.\n>\n> Note that both subshells execute four git commands each, meaning that\n> checking the emptiness of the whole subshell implicitly ensures that\n> not only 'git merge-recursive' but none of the other three commands\n> outputs anything to their stderr.  Note also that if one of those\n> commands were to output anything on its stderr, then the current\n> combined check would not tell us which one of those four commands the\n> unexpected output came from.\n>\n> Save the stderr of those four commands only instead of the whole\n> subshell, so it remains free from tracing output, and save and check\n> them individually, so they will show us from which command the\n> unexpected output came from.\n>\n> After this change t3030 passes with '-x', even when running with\n> /bin/sh.\n>\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n"},{"id":"340161","messageId":"CAM0VKjkKV2NsHA-Zpv-jnXaULTGnKy=HQ+MPRebTo17wHdFENg@mail.gmail.com","threadId":"47915","inReplyTo":"20180223233951.11154-2-szeder.dev@gmail.com","subject":"Re: [PATCH 01/11] t: prevent '-x' tracing from interfering with test helpers' stderr","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-24T12:19:08Z","receivedAt":"2018-02-24T12:19:15Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sat, Feb 24, 2018 at 12:39 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:\n\n>   - Duplicate stderr of the tested command executed in the test helper\n>     function from the function's fd 7 (see next point), to ensure that\n>     the tested command's error messages go to a different fd than the\n>     '-x' trace of the commands executed in the function.\n>\n>   - Duplicate the test helper function's fd 7 from the function's\n>     original stderr, meaning that, after taking a detour through fd 7,\n>     the error messages of the tested command do end up on the\n>     function's original stderr.\n\n> diff --git a/t/lib-terminal.sh b/t/lib-terminal.sh\n> index cd220e378e..b3acb4c6f8 100644\n> --- a/t/lib-terminal.sh\n> +++ b/t/lib-terminal.sh\n> @@ -9,8 +9,8 @@ test_terminal () {\n>                 echo >&4 \"test_terminal: need to declare TTY prerequisite\"\n>                 return 127\n>         fi\n> -       perl \"$TEST_DIRECTORY\"/test-terminal.perl \"$@\"\n> -}\n> +       perl \"$TEST_DIRECTORY\"/test-terminal.perl \"$@\" 2>&9\n> +} 9>&2 2>&4\n\nOops, these should duplicate from/to fd 7, not fd 9.\n"},{"id":"340208","messageId":"20180225134015.26964-1-szeder.dev@gmail.com","threadId":"47915","inReplyTo":"20180223233951.11154-2-szeder.dev@gmail.com","subject":"[PATCH v1.1] t: prevent '-x' tracing from interfering with test helpers' stderr","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-25T13:40:15Z","receivedAt":"2018-02-25T13:40:34Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Running a test script with '-x' turns on 'set -x' tracing, the output\nof which is normally sent to stderr.  This causes a lot of\ntest failures, because many tests redirect and verify the stderr\nof shell functions, most frequently that of 'test_must_fail'.\nThese issues were worked around somewhat in d88785e424 (test-lib: set\nBASH_XTRACEFD automatically, 2016-05-11), so at least we could\nreliably run tests with '-x' tracing under a Bash version supporting\nBASH_XTRACEFD, i.e. v4.1 and later.\n\nFuthermore, redirecting the stderr of test helper functions like\n'test_must_fail' or 'test_expect_code' is the cause of a different\nissue as well.  If these functions detect something unexpected, they\nwill write their error messages intended to the user to thier stderr.\nHowever, if their stderr is redirected in order to save and verify the\nstderr of the tested git command invoked in the function, then the\nfunction's error messages will be redirected as well.  Consequently,\nthose messages won't reach the user, making the test's verbose output\nless useful.\n\nThis patch makes it safe to redirect and verify the stderr of those\ntest helper functions which are meant to run the tested command given\nas argument, even when running tests with '-x' and /bin/sh.  This is\nachieved through a couple of file descriptor redirections:\n\n  - Duplicate stderr of the tested command executed in the test helper\n    function from the function's fd 7 (see next point), to ensure that\n    the tested command's error messages go to a different fd than the\n    '-x' trace of the commands executed in the function or the\n    function's error messages.\n\n  - Duplicate the test helper function's fd 7 from the function's\n    original stderr, meaning that, after taking a detour through fd 7,\n    the error messages of the tested command do end up on the\n    function's original stderr.\n\n  - Duplicate stderr of the test helper function from fd 4, i.e. the\n    fd connected to the test script's original stderr and the fd used\n    for BASH_XTRACEFD.  This ensures that the '-x' trace of the\n    commands executed in the function\n\n      - doesn't go to the function's original stderr, so it won't mess\n\twith callers who want to save and verify the tested command's\n\tstderr.\n\n      - does go to the same fd independently from the shell running\n        the test script, be it /bin/sh, an older Bash without\n        BASH_XTRACEFD, or a more recent Bash already supporting\n        BASH_XTRACEFD.\n\n    Furthermore, this also makes sure that the function's error\n    messages go to this fd 4, meaning that the user will be able to\n    see them even if the function's stderr is redirected in the test.\n\n  - Specify the latter two redirections above in the test helper\n    function's definition, so they are performed every time the\n    function is invoked, without the need to modify the callsites of\n    the function.\n\nPerform these redirections in those test helper functions which can be\nexpected to have their stderr redirected, i.e. in the functions\n'test_must_fail', 'test_might_fail', 'test_expect_code', 'test_env',\n'nongit', 'test_terminal' and 'perl'.  Note that 'test_might_fail',\n'test_env', and 'nongit' are not involved in any test failures when\nrunning tests with '-x' and /bin/sh.\n\nThe other test helper functions are left unchanged, because they\neither don't run commands specified as their arguments, or redirecting\ntheir stderr wouldn't make sense, or both.\n\nWith this change the number of failures when running the test suite\nwith '-x' tracing and /bin/sh goes down from 340 failed tests in 43\ntest scripts to 22 failed tests in 6 scripts (or 23 in 7, if the\nsystem (OSX) uses an older Bash version without BASH_XTRACEFD to run\n't9903-bash-prompt.sh').\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n\nChanges:\n\n  - Duplicate from/to fd 7 instead of fd 9 in 'test_terminal'.\n\n  - Talk about the issue that redirecting stderr of test helper\n    functions affect their error messages as well, and how this patch\n    resolves that issue as well.\n\n t/lib-terminal.sh       |  4 ++--\n t/test-lib-functions.sh | 24 ++++++++++++------------\n 2 files changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/t/lib-terminal.sh b/t/lib-terminal.sh\nindex cd220e378e..e3809dcead 100644\n--- a/t/lib-terminal.sh\n+++ b/t/lib-terminal.sh\n@@ -9,8 +9,8 @@ test_terminal () {\n \t\techo >&4 \"test_terminal: need to declare TTY prerequisite\"\n \t\treturn 127\n \tfi\n-\tperl \"$TEST_DIRECTORY\"/test-terminal.perl \"$@\"\n-}\n+\tperl \"$TEST_DIRECTORY\"/test-terminal.perl \"$@\" 2>&7\n+} 7>&2 2>&4\n \n test_lazy_prereq TTY '\n \ttest_have_prereq PERL &&\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 67b5994afb..37eb34044a 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -621,7 +621,7 @@ test_must_fail () {\n \t\t_test_ok=\n \t\t;;\n \tesac\n-\t\"$@\"\n+\t\"$@\" 2>&7\n \texit_code=$?\n \tif test $exit_code -eq 0 && ! list_contains \"$_test_ok\" success\n \tthen\n@@ -644,7 +644,7 @@ test_must_fail () {\n \t\treturn 1\n \tfi\n \treturn 0\n-}\n+} 7>&2 2>&4\n \n # Similar to test_must_fail, but tolerates success, too.  This is\n # meant to be used in contexts like:\n@@ -658,8 +658,8 @@ test_must_fail () {\n # because we want to notice if it fails due to segv.\n \n test_might_fail () {\n-\ttest_must_fail ok=success \"$@\"\n-}\n+\ttest_must_fail ok=success \"$@\" 2>&7\n+} 7>&2 2>&4\n \n # Similar to test_must_fail and test_might_fail, but check that a\n # given command exited with a given exit code. Meant to be used as:\n@@ -671,7 +671,7 @@ test_might_fail () {\n test_expect_code () {\n \twant_code=$1\n \tshift\n-\t\"$@\"\n+\t\"$@\" 2>&7\n \texit_code=$?\n \tif test $exit_code = $want_code\n \tthen\n@@ -680,7 +680,7 @@ test_expect_code () {\n \n \techo >&2 \"test_expect_code: command exited with $exit_code, we wanted $want_code $*\"\n \treturn 1\n-}\n+} 7>&2 2>&4\n \n # test_cmp is a helper function to compare actual and expected output.\n # You can use it like:\n@@ -882,8 +882,8 @@ test_write_lines () {\n }\n \n perl () {\n-\tcommand \"$PERL_PATH\" \"$@\"\n-}\n+\tcommand \"$PERL_PATH\" \"$@\" 2>&7\n+} 7>&2 2>&4\n \n # Is the value one of the various ways to spell a boolean true/false?\n test_normalize_bool () {\n@@ -1023,13 +1023,13 @@ test_env () {\n \t\t\t\tshift\n \t\t\t\t;;\n \t\t\t*)\n-\t\t\t\t\"$@\"\n+\t\t\t\t\"$@\" 2>&7\n \t\t\t\texit\n \t\t\t\t;;\n \t\t\tesac\n \t\tdone\n \t)\n-}\n+} 7>&2 2>&4\n \n # Returns true if the numeric exit code in \"$2\" represents the expected signal\n # in \"$1\". Signals should be given numerically.\n@@ -1071,9 +1071,9 @@ nongit () {\n \t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n \t\texport GIT_CEILING_DIRECTORIES &&\n \t\tcd non-repo &&\n-\t\t\"$@\"\n+\t\t\"$@\" 2>&7\n \t)\n-}\n+} 7>&2 2>&4\n \n # convert stdin to pktline representation; note that empty input becomes an\n # empty packet, not a flush packet (for that you can just print 0000 yourself).\n-- \n2.16.2.409.g7f85f628cc\n\n"},{"id":"340444","messageId":"xmqqtvu2p2kx.fsf@gitster-ct.c.googlers.com","threadId":"47915","inReplyTo":"20180223233951.11154-5-szeder.dev@gmail.com","subject":"Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-27T21:03:42Z","receivedAt":"2018-02-27T21:03:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> The two test checking 'git mmerge-recursive' in an empty worktree in\n> ...\n>  \t\tGIT_INDEX_FILE=\"$PWD/ours-has-rename-index\" &&\n>  \t\texport GIT_INDEX_FILE &&\n>  \t\tmkdir \"$GIT_WORK_TREE\" &&\n> -\t\tgit read-tree -i -m $c7 &&\n> -\t\tgit update-index --ignore-missing --refresh &&\n> -\t\tgit merge-recursive $c0 -- $c7 $c3 &&\n> -\t\tgit ls-files -s >actual-files\n> -\t) 2>actual-err &&\n> -\t>expected-err &&\n> +\t\tgit read-tree -i -m $c7 2>actual-err &&\n> +\t\ttest_must_be_empty expected-err &&\n> +\t\tgit update-index --ignore-missing --refresh 2>actual-err &&\n> +\t\ttest_must_be_empty expected-err &&\n> +\t\tgit merge-recursive $c0 -- $c7 $c3 2>actual-err &&\n> +\t\ttest_must_be_empty expected-err &&\n> +\t\tgit ls-files -s >actual-files 2>actual-err &&\n> +\t\ttest_must_be_empty expected-err\n\nWhere do the contents of all of these expected-err files come from?\nShould all of the test_must_be_empty checks be checking actual-err\ninstead?\n\n"},{"id":"340446","messageId":"xmqqpo4qp290.fsf@gitster-ct.c.googlers.com","threadId":"47915","inReplyTo":"xmqqtvu2p2kx.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-27T21:10:51Z","receivedAt":"2018-02-27T21:10:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> SZEDER Gábor <szeder.dev@gmail.com> writes:\n>\n>> The two test checking 'git mmerge-recursive' in an empty worktree in\n>> ...\n>>  \t\tGIT_INDEX_FILE=\"$PWD/ours-has-rename-index\" &&\n>>  \t\texport GIT_INDEX_FILE &&\n>>  \t\tmkdir \"$GIT_WORK_TREE\" &&\n>> -\t\tgit read-tree -i -m $c7 &&\n>> -\t\tgit update-index --ignore-missing --refresh &&\n>> -\t\tgit merge-recursive $c0 -- $c7 $c3 &&\n>> -\t\tgit ls-files -s >actual-files\n>> -\t) 2>actual-err &&\n>> -\t>expected-err &&\n>> +\t\tgit read-tree -i -m $c7 2>actual-err &&\n>> +\t\ttest_must_be_empty expected-err &&\n>> +\t\tgit update-index --ignore-missing --refresh 2>actual-err &&\n>> +\t\ttest_must_be_empty expected-err &&\n>> +\t\tgit merge-recursive $c0 -- $c7 $c3 2>actual-err &&\n>> +\t\ttest_must_be_empty expected-err &&\n>> +\t\tgit ls-files -s >actual-files 2>actual-err &&\n>> +\t\ttest_must_be_empty expected-err\n>\n> Where do the contents of all of these expected-err files come from?\n> Should all of the test_must_be_empty checks be checking actual-err\n> instead?\n\n\nAnd the reason why your pre-submission testing did not catch may be\nbecause test_must_be_empty is broken?  I wonder if this is a good\nway forward to catch a possible bug like this.\n\nOf course, if somebody was using the helepr for \"must be either\nmissing or empty\", this change will break it, but I somehow doubt\nit.  A program that creates/opens and writes an error message only\nwhen an error is detected is certainly possible, and could be tested\nwith the current test_must_be_empty this way:\n\n\trm -f actual-err &&\n\tgit frotz --error-to=actual-err &&\n\ttest_must_be_empty actual-err\n\nbut then the last step in such a test like the above is more natural\nto check if actual-err _exists_ in the first place anyway, so...\n\n t/test-lib-functions.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 37eb34044a..6cfbee60e4 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -772,7 +772,11 @@ verbose () {\n # otherwise.\n \n test_must_be_empty () {\n-\tif test -s \"$1\"\n+\tif ! test -f \"$1\"\n+\tthen\n+\t\techo \"'$1' is missing\"\n+\t\treturn 1\n+\telif test -s \"$1\"\n \tthen\n \t\techo \"'$1' is not empty, it contains:\"\n \t\tcat \"$1\"\n"},{"id":"340447","messageId":"xmqqlgfep1x7.fsf@gitster-ct.c.googlers.com","threadId":"47915","inReplyTo":"20180223233951.11154-5-szeder.dev@gmail.com","subject":"Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-27T21:17:56Z","receivedAt":"2018-02-27T21:18:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> +\t\tgit read-tree -i -m $c3 2>actual-err &&\n> +\t\ttest_must_be_empty expected-err &&\n> +\t\tgit update-index --ignore-missing --refresh 2>>actual-err &&\n> +\t\ttest_must_be_empty expected-err &&\n> +\t\tgit merge-recursive $c0 -- $c3 $c7 2>>actual-err &&\n> +\t\ttest_must_be_empty expected-err &&\n> +\t\tgit ls-files -s >actual-files 2>>actual-err &&\n> +\t\ttest_must_be_empty expected-err\n\nAlso, with the error output of individual steps tested like this\n(assuming that test-must-be-empty checks are to be done on\nthe actual-err file, not ecpected-err that nobody creates), I do not\nsee a point in appending to the file.  So perhaps squash this in?\n\n t/t3030-merge-recursive.sh | 22 +++++++++++-----------\n 1 file changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex cbeea1cf94..3563e77b37 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -526,13 +526,13 @@ test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '\n \t\texport GIT_INDEX_FILE &&\n \t\tmkdir \"$GIT_WORK_TREE\" &&\n \t\tgit read-tree -i -m $c7 2>actual-err &&\n-\t\ttest_must_be_empty expected-err &&\n+\t\ttest_must_be_empty actual-err &&\n \t\tgit update-index --ignore-missing --refresh 2>actual-err &&\n-\t\ttest_must_be_empty expected-err &&\n+\t\ttest_must_be_empty actual-err &&\n \t\tgit merge-recursive $c0 -- $c7 $c3 2>actual-err &&\n-\t\ttest_must_be_empty expected-err &&\n+\t\ttest_must_be_empty actual-err &&\n \t\tgit ls-files -s >actual-files 2>actual-err &&\n-\t\ttest_must_be_empty expected-err\n+\t\ttest_must_be_empty actual-err\n \t) &&\n \tcat >expected-files <<-EOF &&\n \t100644 $o3 0\tb/c\n@@ -551,13 +551,13 @@ test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '\n \t\texport GIT_INDEX_FILE &&\n \t\tmkdir \"$GIT_WORK_TREE\" &&\n \t\tgit read-tree -i -m $c3 2>actual-err &&\n-\t\ttest_must_be_empty expected-err &&\n-\t\tgit update-index --ignore-missing --refresh 2>>actual-err &&\n-\t\ttest_must_be_empty expected-err &&\n-\t\tgit merge-recursive $c0 -- $c3 $c7 2>>actual-err &&\n-\t\ttest_must_be_empty expected-err &&\n-\t\tgit ls-files -s >actual-files 2>>actual-err &&\n-\t\ttest_must_be_empty expected-err\n+\t\ttest_must_be_empty actual-err &&\n+\t\tgit update-index --ignore-missing --refresh 2>actual-err &&\n+\t\ttest_must_be_empty actual-err &&\n+\t\tgit merge-recursive $c0 -- $c3 $c7 2>actual-err &&\n+\t\ttest_must_be_empty actual-err &&\n+\t\tgit ls-files -s >actual-files 2>actual-err &&\n+\t\ttest_must_be_empty actual-err\n \t) &&\n \tcat >expected-files <<-EOF &&\n \t100644 $o3 0\tb/c\n"},{"id":"340450","messageId":"xmqqh8q2p1ha.fsf_-_@gitster-ct.c.googlers.com","threadId":"47915","inReplyTo":"xmqqpo4qp290.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] test_must_be_empty: make sure the file exists, not just empty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-27T21:27:29Z","receivedAt":"2018-02-27T21:27:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The helper function test_must_be_empty is meant to make sure the\ngiven file is empty, but its implementation is:\n\n\tif test -s \"$1\"\n\tthen\n\t\t... not empty, we detected a failure ...\n\tfi\n\nSurely, the file having non-zero size is a sign that the condition\n\"the file must be empty\" is violated, but it misses the case where\nthe file does not even exist.  It is an accident waiting to happen\nwith a buggy test like this:\n\n\tgit frotz 2>error-message &&\n\ttest_must_be_empty errro-message\n\nthat won't get caught until you deliberately break 'git frotz' and\nnotice why the test does not fail.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/test-lib-functions.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 37eb34044a..6cfbee60e4 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -772,7 +772,11 @@ verbose () {\n # otherwise.\n \n test_must_be_empty () {\n-\tif test -s \"$1\"\n+\tif ! test -f \"$1\"\n+\tthen\n+\t\techo \"'$1' is missing\"\n+\t\treturn 1\n+\telif test -s \"$1\"\n \tthen\n \t\techo \"'$1' is not empty, it contains:\"\n \t\tcat \"$1\"\n-- \n2.16.2-325-g2fc74f41c5\n\n"},{"id":"340451","messageId":"CAM0VKjnVWZ8Ysp5YyfcStgcBuME0q=usT5MKiAcWyxDwAKk+Fg@mail.gmail.com","threadId":"47915","inReplyTo":"xmqqpo4qp290.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-27T21:28:45Z","receivedAt":"2018-02-27T21:28:51Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Feb 27, 2018 at 10:10 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> SZEDER Gábor <szeder.dev@gmail.com> writes:\n>>\n>>> The two test checking 'git mmerge-recursive' in an empty worktree in\n>>> ...\n>>>              GIT_INDEX_FILE=\"$PWD/ours-has-rename-index\" &&\n>>>              export GIT_INDEX_FILE &&\n>>>              mkdir \"$GIT_WORK_TREE\" &&\n>>> -            git read-tree -i -m $c7 &&\n>>> -            git update-index --ignore-missing --refresh &&\n>>> -            git merge-recursive $c0 -- $c7 $c3 &&\n>>> -            git ls-files -s >actual-files\n>>> -    ) 2>actual-err &&\n>>> -    >expected-err &&\n>>> +            git read-tree -i -m $c7 2>actual-err &&\n>>> +            test_must_be_empty expected-err &&\n>>> +            git update-index --ignore-missing --refresh 2>actual-err &&\n>>> +            test_must_be_empty expected-err &&\n>>> +            git merge-recursive $c0 -- $c7 $c3 2>actual-err &&\n>>> +            test_must_be_empty expected-err &&\n>>> +            git ls-files -s >actual-files 2>actual-err &&\n>>> +            test_must_be_empty expected-err\n>>\n>> Where do the contents of all of these expected-err files come from?\n>> Should all of the test_must_be_empty checks be checking actual-err\n>> instead?\n\nUgh, I messed that up.\n\n> And the reason why your pre-submission testing did not catch may be\n> because test_must_be_empty is broken?  I wonder if this is a good\n> way forward to catch a possible bug like this.\n\nYeah.  'test -s file' means \"exists and has a size greater than zero\",\nso the missing file doesn't trigger the error code path.\n\n> Of course, if somebody was using the helepr for \"must be either\n> missing or empty\", this change will break it, but I somehow doubt\n> it.\n\nFWIW, I just run the test suite with this change added, and there were\nno failures.  I think it's a good change.\n\n>  A program that creates/opens and writes an error message only\n> when an error is detected is certainly possible, and could be tested\n> with the current test_must_be_empty this way:\n>\n>         rm -f actual-err &&\n>         git frotz --error-to=actual-err &&\n>         test_must_be_empty actual-err\n>\n> but then the last step in such a test like the above is more natural\n> to check if actual-err _exists_ in the first place anyway, so...\n>\n>  t/test-lib-functions.sh | 6 +++++-\n>  1 file changed, 5 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 37eb34044a..6cfbee60e4 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -772,7 +772,11 @@ verbose () {\n>  # otherwise.\n>\n>  test_must_be_empty () {\n> -       if test -s \"$1\"\n> +       if ! test -f \"$1\"\n> +       then\n> +               echo \"'$1' is missing\"\n> +               return 1\n> +       elif test -s \"$1\"\n>         then\n>                 echo \"'$1' is not empty, it contains:\"\n>                 cat \"$1\"\n"},{"id":"340460","messageId":"CAGZ79kYs8n2NF03S_ieFpVo14zqZMAcTn2x-7+Bm1-2Mwr0=Ng@mail.gmail.com","threadId":"47915","inReplyTo":"xmqqh8q2p1ha.fsf_-_@gitster-ct.c.googlers.com","subject":"Re: [PATCH] test_must_be_empty: make sure the file exists, not just empty","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-02-27T21:42:38Z","receivedAt":"2018-02-27T21:42:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Feb 27, 2018 at 1:27 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> The helper function test_must_be_empty is meant to make sure the\n> given file is empty, but its implementation is:\n>\n>         if test -s \"$1\"\n>         then\n>                 ... not empty, we detected a failure ...\n>         fi\n>\n> Surely, the file having non-zero size is a sign that the condition\n> \"the file must be empty\" is violated, but it misses the case where\n> the file does not even exist.  It is an accident waiting to happen\n> with a buggy test like this:\n>\n>         git frotz 2>error-message &&\n>         test_must_be_empty errro-message\n>\n> that won't get caught until you deliberately break 'git frotz' and\n> notice why the test does not fail.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nReviewed-by: Stefan Beller <sbeller@google.com>\n"},{"id":"340469","messageId":"20180227220821.GC11187@sigill.intra.peff.net","threadId":"47915","inReplyTo":"xmqqh8q2p1ha.fsf_-_@gitster-ct.c.googlers.com","subject":"Re: [PATCH] test_must_be_empty: make sure the file exists, not just empty","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-27T22:08:21Z","receivedAt":"2018-02-27T22:08:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 27, 2018 at 01:27:29PM -0800, Junio C Hamano wrote:\n\n> The helper function test_must_be_empty is meant to make sure the\n> given file is empty, but its implementation is:\n> \n> \tif test -s \"$1\"\n> \tthen\n> \t\t... not empty, we detected a failure ...\n> \tfi\n> \n> Surely, the file having non-zero size is a sign that the condition\n> \"the file must be empty\" is violated, but it misses the case where\n> the file does not even exist.  It is an accident waiting to happen\n> with a buggy test like this:\n> \n> \tgit frotz 2>error-message &&\n> \ttest_must_be_empty errro-message\n> \n> that won't get caught until you deliberately break 'git frotz' and\n> notice why the test does not fail.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nThis seems like a huge and obvious improvement to me. I'm amazed it\nhasn't come up before (and that this doesn't reveal any existing typos\nlike the one you showed).\n\n-Peff\n"},{"id":"340500","messageId":"CAM0VKjmxT=KE4OR5KtkWx0Tk=ddBjWt6HQn=BGgXaxm4NTmA0Q@mail.gmail.com","threadId":"47915","inReplyTo":"xmqqlgfep1x7.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 04/11] t3030-merge-recursive: don't check the stderr of a subshell","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-28T00:44:10Z","receivedAt":"2018-02-28T00:44:16Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Feb 27, 2018 at 10:17 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> SZEDER Gábor <szeder.dev@gmail.com> writes:\n>\n>> +             git read-tree -i -m $c3 2>actual-err &&\n>> +             test_must_be_empty expected-err &&\n>> +             git update-index --ignore-missing --refresh 2>>actual-err &&\n>> +             test_must_be_empty expected-err &&\n>> +             git merge-recursive $c0 -- $c3 $c7 2>>actual-err &&\n>> +             test_must_be_empty expected-err &&\n>> +             git ls-files -s >actual-files 2>>actual-err &&\n>> +             test_must_be_empty expected-err\n>\n> Also, with the error output of individual steps tested like this\n> (assuming that test-must-be-empty checks are to be done on\n> the actual-err file, not ecpected-err that nobody creates), I do not\n> see a point in appending to the file.  So perhaps squash this in?\n\nAgreed again.\n\n\n>  t/t3030-merge-recursive.sh | 22 +++++++++++-----------\n>  1 file changed, 11 insertions(+), 11 deletions(-)\n>\n> diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\n> index cbeea1cf94..3563e77b37 100755\n> --- a/t/t3030-merge-recursive.sh\n> +++ b/t/t3030-merge-recursive.sh\n> @@ -526,13 +526,13 @@ test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '\n>                 export GIT_INDEX_FILE &&\n>                 mkdir \"$GIT_WORK_TREE\" &&\n>                 git read-tree -i -m $c7 2>actual-err &&\n> -               test_must_be_empty expected-err &&\n> +               test_must_be_empty actual-err &&\n>                 git update-index --ignore-missing --refresh 2>actual-err &&\n> -               test_must_be_empty expected-err &&\n> +               test_must_be_empty actual-err &&\n>                 git merge-recursive $c0 -- $c7 $c3 2>actual-err &&\n> -               test_must_be_empty expected-err &&\n> +               test_must_be_empty actual-err &&\n>                 git ls-files -s >actual-files 2>actual-err &&\n> -               test_must_be_empty expected-err\n> +               test_must_be_empty actual-err\n>         ) &&\n>         cat >expected-files <<-EOF &&\n>         100644 $o3 0    b/c\n> @@ -551,13 +551,13 @@ test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '\n>                 export GIT_INDEX_FILE &&\n>                 mkdir \"$GIT_WORK_TREE\" &&\n>                 git read-tree -i -m $c3 2>actual-err &&\n> -               test_must_be_empty expected-err &&\n> -               git update-index --ignore-missing --refresh 2>>actual-err &&\n> -               test_must_be_empty expected-err &&\n> -               git merge-recursive $c0 -- $c3 $c7 2>>actual-err &&\n> -               test_must_be_empty expected-err &&\n> -               git ls-files -s >actual-files 2>>actual-err &&\n> -               test_must_be_empty expected-err\n> +               test_must_be_empty actual-err &&\n> +               git update-index --ignore-missing --refresh 2>actual-err &&\n> +               test_must_be_empty actual-err &&\n> +               git merge-recursive $c0 -- $c3 $c7 2>actual-err &&\n> +               test_must_be_empty actual-err &&\n> +               git ls-files -s >actual-files 2>actual-err &&\n> +               test_must_be_empty actual-err\n>         ) &&\n>         cat >expected-files <<-EOF &&\n>         100644 $o3 0    b/c\n"},{"id":"340809","messageId":"CAM0VKj=vvJubs0AdcrQAzDka6pe2nWtpJ_cwSEwWmhncjrkh3g@mail.gmail.com","threadId":"47915","inReplyTo":"20180223233951.11154-1-szeder.dev@gmail.com","subject":"Re: [PATCH 00/11] Make the test suite pass with '-x' and /bin/sh","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-03-02T15:32:13Z","receivedAt":"2018-03-02T15:32:23Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sat, Feb 24, 2018 at 12:39 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> This patch series makes '-x' tracing of tests work reliably even when\n> running them with /bin/sh, and setting TEST_SHELL_PATH=bash will be\n> unnecessary.\n>\n>   make GIT_TEST_OPTS='-x --verbose-log' test\n>\n> passes on my setup and on all Travis CI build jobs (though neither me\n> nor Travis CI run all tests, e.g. CVS).\n\nI installed 'cvs' and whatnot to run t94* and t96* tests, and sure\nenough, 5 tests in 2 test scripts fail with '-x' tracing and /bin/sh.\nI think I will be able to get around to send v2 during the weekend.\n"},{"id":"340877","messageId":"20180303071259.GB17312@sigill.intra.peff.net","threadId":"47915","inReplyTo":"20180223233951.11154-1-szeder.dev@gmail.com","subject":"Re: [PATCH 00/11] Make the test suite pass with '-x' and /bin/sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-03T07:12:59Z","receivedAt":"2018-03-03T07:13:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 24, 2018 at 12:39:40AM +0100, SZEDER Gábor wrote:\n\n> The first patch is the most important: with a couple of well-placed file\n> descriptor redirections it ensures that the stderr of the test helper\n> functions running git commands only contain the stderr of the tested\n> command, thereby resolving over 90% of the failures resulting from\n> running the test suite with '-x' and /bin/sh.\n\nI dunno. It seems like this requires a lot of caveats for people using\nsubshells and shell functions, and I suspect it's going to be an\non-going maintenance burden.\n\nThat said, I'm not opposed if you want to do the work to try to get the\nwhole test-suite clean, and we can see how it goes from there. It\nshouldn't be hurting anything, I don't think, aside from some\nmysterious-looking redirects (but your commit messages seem to explain\nit, so anybody can dig).\n\nDoes it make descriptor 7 magical, and something that scripts should\navoid touching? That would mean we have 2 magical descriptors now.\n\n-Peff\n"},{"id":"341065","messageId":"CAM0VKjmaL16M87SVt9xh3nrHBT49Gygr7=9RqvfxA6E9D6OzmA@mail.gmail.com","threadId":"47915","inReplyTo":"20180303071259.GB17312@sigill.intra.peff.net","subject":"Re: [PATCH 00/11] Make the test suite pass with '-x' and /bin/sh","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-03-05T21:18:34Z","receivedAt":"2018-03-05T21:18:41Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sat, Mar 3, 2018 at 8:12 AM, Jeff King <peff@peff.net> wrote:\n> On Sat, Feb 24, 2018 at 12:39:40AM +0100, SZEDER Gábor wrote:\n>\n>> The first patch is the most important: with a couple of well-placed file\n>> descriptor redirections it ensures that the stderr of the test helper\n>> functions running git commands only contain the stderr of the tested\n>> command, thereby resolving over 90% of the failures resulting from\n>> running the test suite with '-x' and /bin/sh.\n>\n> I dunno. It seems like this requires a lot of caveats for people using\n> subshells and shell functions, and I suspect it's going to be an\n> on-going maintenance burden.\n\nAfter finally figuring out the redirections in the first patch, I was\nquite surprised by how few failing tests remained.  We only gathered 28\nsuch tests over all these years; if it continues at this rate, that\nprobably won't be that much of a burden.  And the second patch provides\nan escape hatch, should it ever be needed.\n\nThe current situation, however, is a burden much more frequently,\nbecause the idiosyncrasies of TEST_SHELL_PATH and/or '--verbose-log' pop\nup whenever trying to run any test script with '-x' that has such a test\nin it.\n\nI think this is the right tradeoff.\n\n> That said, I'm not opposed if you want to do the work to try to get the\n> whole test-suite clean, and we can see how it goes from there. It\n> shouldn't be hurting anything, I don't think, aside from some\n> mysterious-looking redirects (but your commit messages seem to explain\n> it, so anybody can dig).\n>\n> Does it make descriptor 7 magical, and something that scripts should\n> avoid touching? That would mean we have 2 magical descriptors now.\n\nTests can still use fd 7 as long as they don't intend to attach it\ndirectly to that particular git command that is run inside one of these\ntest helper functions.\n\nI settled on fd 7 because that fd is already used as stderr for the\n'test_pause' and 'debug' helper functions and it isn't used in any of\nour tests.\n"}]}