{"thread":{"id":"47795","subject":"[PATCH 0/3] Teach 'test_must_fail' to save the command's stderr to a file","startedAt":"2018-02-09T02:43:04Z","lastAt":"2018-02-23T23:26:49Z","messageCount":13,"participants":["SZEDER Gábor","Eric Sunshine","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"338797","messageId":"20180209024235.3431-1-szeder.dev@gmail.com","threadId":"47795","inReplyTo":null,"subject":"[PATCH 0/3] Teach 'test_must_fail' to save the command's stderr to a file","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-09T02:42:32Z","receivedAt":"2018-02-09T02:43:04Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"To check that a git command fails with the expected error message, we \nusually execute a command like this:\n        \n  test_must_fail git command --option 2>output.err\n\t          \nAlas, this command doesn't limit the redirection to the git command,\nbut it redirects the standard error of the 'test_must_fail' helper\nfunction as well, causing various issues discussed in detail in the\nsecond patch.  Therefore that patch introduces the 'test_must_fail\nstderr=<file>' option to save the executed git command's standard\nerror to the given file.\n\nThe last patch converts one test script to use 'test_must_fail\nstderr=<file>' to demonstrate its benefits: thereafter that script\nwill succeed with '-x'.  There are plenty more places to convert:\n\n  $ git grep -E 'test_(must|might)_fail .* 2>' t/*.sh |wc -l\n  430\n  $ git grep --name-only -E 'test_(must|might)_fail .* 2>' t/*.sh |wc -l\n  135\n\n... and this doesn't even count commands spanning more lines, and\nthere are more in 'pu'.\n\nI didn't convert more test scripts, because it's boring ;) but more\nimportantly because it could give us 135+ GSoC micro projects.\n\nSZEDER Gábor (3):\n  t: document 'test_must_fail ok=<signal-name>'\n  t: teach 'test_must_fail' to save the command's stderr to a file\n  t1404: use 'test_must_fail stderr=<file>'\n\n t/README                     | 20 +++++++++++++++++--\n t/t1404-update-ref-errors.sh | 46 ++++++++++++++++++++++----------------------\n t/test-lib-functions.sh      | 45 +++++++++++++++++++++++++++++++++----------\n 3 files changed, 76 insertions(+), 35 deletions(-)\n\n-- \n2.16.1.180.g07550b0b1b\n\n"},{"id":"338798","messageId":"20180209024235.3431-2-szeder.dev@gmail.com","threadId":"47795","inReplyTo":"20180209024235.3431-1-szeder.dev@gmail.com","subject":"[PATCH 1/3] t: document 'test_must_fail ok=<signal-name>'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-09T02:42:33Z","receivedAt":"2018-02-09T02:43:10Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Since 'test_might_fail' is implemented as a thin wrapper around\n'test_must_fail', it also accepts the same options.  Mention this in\nthe docs as well.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/README                | 14 ++++++++++++--\n t/test-lib-functions.sh | 10 ++++++++++\n 2 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex b3f7b449c3..1a1361a806 100644\n--- a/t/README\n+++ b/t/README\n@@ -655,7 +655,7 @@ library for your script to use.\n \t\ttest_expect_code 1 git merge \"merge msg\" B master\n \t'\n \n- - test_must_fail <git-command>\n+ - test_must_fail [<options>] <git-command>\n \n    Run a git command and ensure it fails in a controlled way.  Use\n    this instead of \"! <git-command>\".  When git-command dies due to a\n@@ -663,11 +663,21 @@ library for your script to use.\n    treats it as just another expected failure, which would let such a\n    bug go unnoticed.\n \n- - test_might_fail <git-command>\n+   Accepts the following options:\n+\n+     ok=<signal-name>[,<...>]:\n+       Don't treat an exit caused by the given signal as error.\n+       Multiple signals can be specified as a comma separated list.\n+       Currently recognized signal names are: sigpipe, success.\n+       (Don't use 'success', use 'test_might_fail' instead.)\n+\n+ - test_might_fail [<options>] <git-command>\n \n    Similar to test_must_fail, but tolerate success, too.  Use this\n    instead of \"<git-command> || :\" to catch failures due to segv.\n \n+   Accepts the same options as test_must_fail.\n+\n  - test_cmp <expected> <actual>\n \n    Check whether the content of the <actual> file matches the\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 1701fe2a06..26b149ac1d 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -610,6 +610,14 @@ list_contains () {\n #\n # Writing this as \"! git checkout ../outerspace\" is wrong, because\n # the failure could be due to a segv.  We want a controlled failure.\n+#\n+# Accepts the following options:\n+#\n+#   ok=<signal-name>[,<...>]:\n+#     Don't treat an exit caused by the given signal as error.\n+#     Multiple signals can be specified as a comma separated list.\n+#     Currently recognized signal names are: sigpipe, success.\n+#     (Don't use 'success', use 'test_might_fail' instead.)\n \n test_must_fail () {\n \tcase \"$1\" in\n@@ -656,6 +664,8 @@ test_must_fail () {\n #\n # Writing \"git config --unset all.configuration || :\" would be wrong,\n # because we want to notice if it fails due to segv.\n+#\n+# Accepts the same options as test_must_fail.\n \n test_might_fail () {\n \ttest_must_fail ok=success \"$@\"\n-- \n2.16.1.180.g07550b0b1b\n\n"},{"id":"338799","messageId":"20180209024235.3431-3-szeder.dev@gmail.com","threadId":"47795","inReplyTo":"20180209024235.3431-1-szeder.dev@gmail.com","subject":"[PATCH 2/3] t: teach 'test_must_fail' to save the command's stderr to a file","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-09T02:42:34Z","receivedAt":"2018-02-09T02:43:12Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"To check that a git command fails and to inspect its error message we\nusually execute a command like this throughout our test suite:\n\n  test_must_fail git command --option 2>output.err\n\nNote that this command doesn't limit the redirection to the git\ncommand, but it redirects the standard error of the 'test_must_fail'\nhelper function as well.  This is wrong for several reasons:\n\n  - If that git command were to succeed or die for an unexpected\n    reason e.g. signal, then 'test_must_fail's helpful error message\n    would end up in the given file instead of on the terminal or in\n    't/test-results/$T.out', when the test is run with '-v' or\n    '--verbose-log', respectively.\n\n  - If the test is run with '-x', the trace of executed commands in\n    'test_must_fail' will go to stderr as well (except for more recent\n    Bash versions supporting $BASH_XTRACEFD), and then in turn will\n    end up in the given file.\n\n  - If the git command's error message is checked verbatim with\n    'test_cmp', then the '-x' trace will cause the test to fail.\n\n  - Though rather unlikely, if the git command's error message is\n    checked with 'test_i18ngrep', then its pattern might match\n    'test_must_fail's error message (or '-x' trace) instead of the\n    given git command's, potentially hiding a bug.\n\nTeach the 'test_must_fail' helper the 'stderr=<file>' option to save\nthe executed git command's standard error to that file, so we can\navoid all the bad side effects of redirecting the whole thing's\nstandard error.\n\nThe same issues apply to the 'test_might_fail' helper function as\nwell, but since it's implemented as a thin wrapper around\n'test_must_fail', no modifications were necessary and 'test_might_fail\nstderr=output.err git command' will just work.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n\nNotes:\n    I considered adding an analogous 'stdout=<file>' option as well, but in\n    the end haven't, because:\n    \n      - 'test_must_fail' doesn't print anything to its standard output, so a\n        plain '>out' redirection at the end of the line doesn't have any\n        undesirable side effects.\n    \n      - We would need four conditions to cover all possible combinations of\n        'stdout=' and 'stderr='.  Considering the above point, I don't think\n        it's worth it.\n\n t/README                |  6 ++++++\n t/test-lib-functions.sh | 35 +++++++++++++++++++++++++----------\n 2 files changed, 31 insertions(+), 10 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 1a1361a806..2528359e9d 100644\n--- a/t/README\n+++ b/t/README\n@@ -430,6 +430,10 @@ Don't:\n    use 'test_must_fail git cmd'.  This will signal a failure if git\n    dies in an unexpected way (e.g. segfault).\n \n+   Don't redirect 'test_must_fail's standard error like\n+   'test_must_fail git cmd 2>err'.  Instead, run 'test_must_fail\n+   stderr=err git cmd'.\n+\n    On the other hand, don't use test_must_fail for running regular\n    platform commands; just use '! cmd'.  We are not in the business\n    of verifying that the world given to us sanely works.\n@@ -670,6 +674,8 @@ library for your script to use.\n        Multiple signals can be specified as a comma separated list.\n        Currently recognized signal names are: sigpipe, success.\n        (Don't use 'success', use 'test_might_fail' instead.)\n+     stderr=<file>:\n+       Save <git-command>'s standard error to <file>.\n \n  - test_might_fail [<options>] <git-command>\n \ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 26b149ac1d..d2f561477c 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -618,18 +618,33 @@ list_contains () {\n #     Multiple signals can be specified as a comma separated list.\n #     Currently recognized signal names are: sigpipe, success.\n #     (Don't use 'success', use 'test_might_fail' instead.)\n+#   stderr=<file>:\n+#     Save the git command's standard error to <file>.\n \n test_must_fail () {\n-\tcase \"$1\" in\n-\tok=*)\n-\t\t_test_ok=${1#ok=}\n-\t\tshift\n-\t\t;;\n-\t*)\n-\t\t_test_ok=\n-\t\t;;\n-\tesac\n-\t\"$@\"\n+\t_test_ok= _test_stderr=\n+\twhile test $# -gt 0\n+\tdo\n+\t\tcase \"$1\" in\n+\t\tstderr=*)\n+\t\t\t_test_stderr=${1#stderr=}\n+\t\t\tshift\n+\t\t\t;;\n+\t\tok=*)\n+\t\t\t_test_ok=${1#ok=}\n+\t\t\tshift\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\t\t;;\n+\t\tesac\n+\tdone\n+\tif test -n \"$_test_stderr\"\n+\tthen\n+\t\t\"$@\" 2>\"$_test_stderr\"\n+\telse\n+\t\t\"$@\"\n+\tfi\n \texit_code=$?\n \tif test $exit_code -eq 0 && ! list_contains \"$_test_ok\" success\n \tthen\n-- \n2.16.1.180.g07550b0b1b\n\n"},{"id":"338800","messageId":"20180209024235.3431-4-szeder.dev@gmail.com","threadId":"47795","inReplyTo":"20180209024235.3431-1-szeder.dev@gmail.com","subject":"[PATCH 3/3] t1404: use 'test_must_fail stderr=<file>'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-09T02:42:35Z","receivedAt":"2018-02-09T02:43:16Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Instead of 'test_must_fail git cmd... 2>output.err', which redirects\nthe standard error of the 'test_must_fail' helper function as well,\ncausing various issues as discussed in the previous patch.\n\nWith this patch t1404 succeeds when run with '-x', even with shells\nnot supporting $BASH_XTRACEFD.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/t1404-update-ref-errors.sh | 46 ++++++++++++++++++++++----------------------\n 1 file changed, 23 insertions(+), 23 deletions(-)\n\ndiff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\nindex 3a887b5113..91250bc6a0 100755\n--- a/t/t1404-update-ref-errors.sh\n+++ b/t/t1404-update-ref-errors.sh\n@@ -26,7 +26,7 @@ test_update_rejected () {\n \t\tgit pack-refs --all\n \tfi &&\n \tprintf \"create $prefix/%s $C\\n\" $create >input &&\n-\ttest_must_fail git update-ref --stdin <input 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin <input &&\n \tgrep -F \"$error\" output.err &&\n \tgit for-each-ref $prefix >actual &&\n \ttest_cmp unchanged actual\n@@ -102,7 +102,7 @@ df_test() {\n \telse\n \t\tprintf \"%s\\n\" \"delete $delname\" \"create $addname $D\"\n \tfi >commands &&\n-\ttest_must_fail git update-ref --stdin <commands 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin <commands &&\n \ttest_cmp expected-err output.err &&\n \tprintf \"%s\\n\" \"$C $delref\" >expected-refs &&\n \tgit for-each-ref --format=\"%(objectname) %(refname)\" $prefix/r >actual-refs &&\n@@ -337,7 +337,7 @@ test_expect_success 'missing old value blocks update' '\n \tfatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q\n \tEOF\n \tprintf \"%s\\n\" \"update $prefix/foo $E $D\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err\n '\n \n@@ -348,7 +348,7 @@ test_expect_success 'incorrect old value blocks update' '\n \tfatal: cannot lock ref $Q$prefix/foo$Q: is at $C but expected $D\n \tEOF\n \tprintf \"%s\\n\" \"update $prefix/foo $E $D\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err\n '\n \n@@ -359,7 +359,7 @@ test_expect_success 'existing old value blocks create' '\n \tfatal: cannot lock ref $Q$prefix/foo$Q: reference already exists\n \tEOF\n \tprintf \"%s\\n\" \"create $prefix/foo $E\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err\n '\n \n@@ -370,7 +370,7 @@ test_expect_success 'incorrect old value blocks delete' '\n \tfatal: cannot lock ref $Q$prefix/foo$Q: is at $C but expected $D\n \tEOF\n \tprintf \"%s\\n\" \"delete $prefix/foo $D\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err\n '\n \n@@ -381,7 +381,7 @@ test_expect_success 'missing old value blocks indirect update' '\n \tfatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q\n \tEOF\n \tprintf \"%s\\n\" \"update $prefix/symref $E $D\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err\n '\n \n@@ -393,7 +393,7 @@ test_expect_success 'incorrect old value blocks indirect update' '\n \tfatal: cannot lock ref $Q$prefix/symref$Q: is at $C but expected $D\n \tEOF\n \tprintf \"%s\\n\" \"update $prefix/symref $E $D\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err\n '\n \n@@ -405,7 +405,7 @@ test_expect_success 'existing old value blocks indirect create' '\n \tfatal: cannot lock ref $Q$prefix/symref$Q: reference already exists\n \tEOF\n \tprintf \"%s\\n\" \"create $prefix/symref $E\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err\n '\n \n@@ -417,7 +417,7 @@ test_expect_success 'incorrect old value blocks indirect delete' '\n \tfatal: cannot lock ref $Q$prefix/symref$Q: is at $C but expected $D\n \tEOF\n \tprintf \"%s\\n\" \"delete $prefix/symref $D\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err\n '\n \n@@ -428,7 +428,7 @@ test_expect_success 'missing old value blocks indirect no-deref update' '\n \tfatal: cannot lock ref $Q$prefix/symref$Q: reference is missing but expected $D\n \tEOF\n \tprintf \"%s\\n\" \"option no-deref\" \"update $prefix/symref $E $D\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err\n '\n \n@@ -440,7 +440,7 @@ test_expect_success 'incorrect old value blocks indirect no-deref update' '\n \tfatal: cannot lock ref $Q$prefix/symref$Q: is at $C but expected $D\n \tEOF\n \tprintf \"%s\\n\" \"option no-deref\" \"update $prefix/symref $E $D\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err\n '\n \n@@ -452,7 +452,7 @@ test_expect_success 'existing old value blocks indirect no-deref create' '\n \tfatal: cannot lock ref $Q$prefix/symref$Q: reference already exists\n \tEOF\n \tprintf \"%s\\n\" \"option no-deref\" \"create $prefix/symref $E\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err\n '\n \n@@ -464,7 +464,7 @@ test_expect_success 'incorrect old value blocks indirect no-deref delete' '\n \tfatal: cannot lock ref $Q$prefix/symref$Q: is at $C but expected $D\n \tEOF\n \tprintf \"%s\\n\" \"option no-deref\" \"delete $prefix/symref $D\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err\n '\n \n@@ -477,13 +477,13 @@ test_expect_success 'non-empty directory blocks create' '\n \tfatal: cannot lock ref $Q$prefix/foo$Q: there is a non-empty directory $Q.git/$prefix/foo$Q blocking reference $Q$prefix/foo$Q\n \tEOF\n \tprintf \"%s\\n\" \"update $prefix/foo $C\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err &&\n \tcat >expected <<-EOF &&\n \tfatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q\n \tEOF\n \tprintf \"%s\\n\" \"update $prefix/foo $D $C\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err\n '\n \n@@ -496,13 +496,13 @@ test_expect_success 'broken reference blocks create' '\n \tfatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken\n \tEOF\n \tprintf \"%s\\n\" \"update $prefix/foo $C\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err &&\n \tcat >expected <<-EOF &&\n \tfatal: cannot lock ref $Q$prefix/foo$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken\n \tEOF\n \tprintf \"%s\\n\" \"update $prefix/foo $D $C\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err\n '\n \n@@ -516,13 +516,13 @@ test_expect_success 'non-empty directory blocks indirect create' '\n \tfatal: cannot lock ref $Q$prefix/symref$Q: there is a non-empty directory $Q.git/$prefix/foo$Q blocking reference $Q$prefix/foo$Q\n \tEOF\n \tprintf \"%s\\n\" \"update $prefix/symref $C\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err &&\n \tcat >expected <<-EOF &&\n \tfatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q\n \tEOF\n \tprintf \"%s\\n\" \"update $prefix/symref $D $C\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err\n '\n \n@@ -535,13 +535,13 @@ test_expect_success 'broken reference blocks indirect create' '\n \tfatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken\n \tEOF\n \tprintf \"%s\\n\" \"update $prefix/symref $C\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err &&\n \tcat >expected <<-EOF &&\n \tfatal: cannot lock ref $Q$prefix/symref$Q: unable to resolve reference $Q$prefix/foo$Q: reference broken\n \tEOF\n \tprintf \"%s\\n\" \"update $prefix/symref $D $C\" |\n-\ttest_must_fail git update-ref --stdin 2>output.err &&\n+\ttest_must_fail stderr=output.err git update-ref --stdin &&\n \ttest_cmp expected output.err\n '\n \n@@ -612,7 +612,7 @@ test_expect_success 'delete fails cleanly if packed-refs file is locked' '\n \t# Now try to delete it while the `packed-refs` lock is held:\n \t: >.git/packed-refs.lock &&\n \ttest_when_finished \"rm -f .git/packed-refs.lock\" &&\n-\ttest_must_fail git update-ref -d $prefix/foo >out 2>err &&\n+\ttest_must_fail stderr=err git update-ref -d $prefix/foo >out &&\n \tgit for-each-ref $prefix >actual &&\n \ttest_i18ngrep \"Unable to create $Q.*packed-refs.lock$Q: File exists\" err &&\n \ttest_cmp unchanged actual\n-- \n2.16.1.180.g07550b0b1b\n\n"},{"id":"338801","messageId":"CAPig+cRUqwOMuv60FsN7Xm1iqnkBBPRP8wW+JpCXTfi8vOHbew@mail.gmail.com","threadId":"47795","inReplyTo":"20180209024235.3431-3-szeder.dev@gmail.com","subject":"Re: [PATCH 2/3] t: teach 'test_must_fail' to save the command's stderr to a file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-09T03:11:11Z","receivedAt":"2018-02-09T03:11:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 8, 2018 at 9:42 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> To check that a git command fails and to inspect its error message we\n> usually execute a command like this throughout our test suite:\n>\n>   test_must_fail git command --option 2>output.err\n>\n> Note that this command doesn't limit the redirection to the git\n> command, but it redirects the standard error of the 'test_must_fail'\n> helper function as well.  This is wrong for several reasons:\n>\n>   [...snip rationale...]\n>\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> ---\n> diff --git a/t/README b/t/README\n> @@ -430,6 +430,10 @@ Don't:\n>     use 'test_must_fail git cmd'.  This will signal a failure if git\n>     dies in an unexpected way (e.g. segfault).\n>\n> +   Don't redirect 'test_must_fail's standard error like\n> +   'test_must_fail git cmd 2>err'.  Instead, run 'test_must_fail\n> +   stderr=err git cmd'.\n\nPerhaps not worth a re-roll, but it might be nice to cite a bit of the\nrationale from the commit message for the \"don't redirect\" rule since\nit's not necessarily obvious for readers of t/README why this\nlimitation exists (and it seems unlikely they would check this commit\nmessage). The rationale here doesn't necessarily need to go into\nexquisite detail of the commit message, but could be perhaps as simple\nas:\n\n    Don't redirect 'test_must_fail's standard error to a file (e.g.\n    'test_must_fail git cmd 2>err') since error output from commands\n    run internally by 'test_must_fail' may pollute file \"err\".\n    Instead, run 'test_must_fail stderr=err git cmd'.\n"},{"id":"338802","messageId":"CAPig+cTtC8WaqJg301WE+EN2RYzka-+pOoJZNZFzjqsHtx+M-Q@mail.gmail.com","threadId":"47795","inReplyTo":"20180209024235.3431-4-szeder.dev@gmail.com","subject":"Re: [PATCH 3/3] t1404: use 'test_must_fail stderr=<file>'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-09T03:16:01Z","receivedAt":"2018-02-09T03:16:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 8, 2018 at 9:42 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> Instead of 'test_must_fail git cmd... 2>output.err', which redirects\n> the standard error of the 'test_must_fail' helper function as well,\n> causing various issues as discussed in the previous patch.\n\nECANTPARSE: This either wants to say:\n\n    \"Instead of <foo>, do <bar>.\"\n\nor:\n\n    \"'test_must_fail ... 2>...', which redirects ...\n     causes various issues discussed...\"\n\nbut fails to say either.\n\n> With this patch t1404 succeeds when run with '-x', even with shells\n> not supporting $BASH_XTRACEFD.\n>\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n"},{"id":"338803","messageId":"CAM0VKjnnt0Nq6GQXWaq6Jq_tvOzJx-KsEBjs-7NS2bim0UH+Og@mail.gmail.com","threadId":"47795","inReplyTo":"CAPig+cTtC8WaqJg301WE+EN2RYzka-+pOoJZNZFzjqsHtx+M-Q@mail.gmail.com","subject":"Re: [PATCH 3/3] t1404: use 'test_must_fail stderr=<file>'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-09T03:33:22Z","receivedAt":"2018-02-09T03:33:28Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Feb 9, 2018 at 4:16 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Thu, Feb 8, 2018 at 9:42 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:\n>> Instead of 'test_must_fail git cmd... 2>output.err', which redirects\n>> the standard error of the 'test_must_fail' helper function as well,\n>> causing various issues as discussed in the previous patch.\n>\n> ECANTPARSE: This either wants to say:\n>\n>     \"Instead of <foo>, do <bar>.\"\n\nThe \"do <bar>\" part is in the subject line.\n\n> or:\n>\n>     \"'test_must_fail ... 2>...', which redirects ...\n>      causes various issues discussed...\"\n\nWell, at this point I got the ECANTPARSE :)\n\n> but fails to say either.\n>\n>> With this patch t1404 succeeds when run with '-x', even with shells\n>> not supporting $BASH_XTRACEFD.\n>>\n>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n"},{"id":"338880","messageId":"20180209142131.GA18701@sigill.intra.peff.net","threadId":"47795","inReplyTo":"20180209024235.3431-3-szeder.dev@gmail.com","subject":"Re: [PATCH 2/3] t: teach 'test_must_fail' to save the command's stderr to a file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-09T14:21:31Z","receivedAt":"2018-02-09T14:21:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 09, 2018 at 03:42:34AM +0100, SZEDER Gábor wrote:\n\n> To check that a git command fails and to inspect its error message we\n> usually execute a command like this throughout our test suite:\n> \n>   test_must_fail git command --option 2>output.err\n> \n> Note that this command doesn't limit the redirection to the git\n> command, but it redirects the standard error of the 'test_must_fail'\n> helper function as well.  This is wrong for several reasons:\n> \n>   - If that git command were to succeed or die for an unexpected\n>     reason e.g. signal, then 'test_must_fail's helpful error message\n>     would end up in the given file instead of on the terminal or in\n>     't/test-results/$T.out', when the test is run with '-v' or\n>     '--verbose-log', respectively.\n> \n>   - If the test is run with '-x', the trace of executed commands in\n>     'test_must_fail' will go to stderr as well (except for more recent\n>     Bash versions supporting $BASH_XTRACEFD), and then in turn will\n>     end up in the given file.\n\nI have to admit that I'm slightly negative on this approach, just\nbecause:\n\n  1. It requires every caller to do something special, rather than just\n     using normal redirection. And the feature isn't as powerful as\n     redirection. E.g., some callers do:\n\n       test_must_fail foo >output 2>&1\n\n     But:\n\n       test_must_fail stderr=output foo >output\n\n     is not quite right (stdout and stderr outputs may clobber each\n     other, because they have independent position pointers).\n\n  2. The \"-x\" problems aren't specific to test_must_fail at all. They're\n     a general issue with shell functions.\n\nI'm not entirely happy with saying \"if you want to use -x, please use\nbash\". But given that it actually solves the problems everywhere with no\nfurther effort, is it really that bad a solution?\n\nFor the error messages from test_must_fail, could we go in the same\ndirection, and send them to descriptor 4 rather than 2? We've already\nstaked out descriptor 4 as something magical that must be left alone\n(see 9be795fb). If we can rely on that, then it becomes a convenient way\nfor functions to make sure their output is going to the script's stderr.\n\n-Peff\n"},{"id":"338887","messageId":"xmqqh8qqxbtc.fsf@gitster-ct.c.googlers.com","threadId":"47795","inReplyTo":"CAM0VKjnnt0Nq6GQXWaq6Jq_tvOzJx-KsEBjs-7NS2bim0UH+Og@mail.gmail.com","subject":"Re: [PATCH 3/3] t1404: use 'test_must_fail stderr=<file>'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-09T18:22:39Z","receivedAt":"2018-02-09T18:22:48Z","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> On Fri, Feb 9, 2018 at 4:16 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Thu, Feb 8, 2018 at 9:42 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:\n>>> Instead of 'test_must_fail git cmd... 2>output.err', which redirects\n>>> the standard error of the 'test_must_fail' helper function as well,\n>>> causing various issues as discussed in the previous patch.\n>>\n>> ECANTPARSE: This either wants to say:\n>>\n>>     \"Instead of <foo>, do <bar>.\"\n>\n> The \"do <bar>\" part is in the subject line.\n\nPlease don't.  The title should not be a half-sentence that begins\nthe first paragraph.\n"},{"id":"338889","messageId":"xmqqd11exb6k.fsf@gitster-ct.c.googlers.com","threadId":"47795","inReplyTo":"20180209142131.GA18701@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] t: teach 'test_must_fail' to save the command's stderr to a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-09T18:36:19Z","receivedAt":"2018-02-09T18:36:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>   2. The \"-x\" problems aren't specific to test_must_fail at all. They're\n>      a general issue with shell functions.\n>\n> I'm not entirely happy with saying \"if you want to use -x, please use\n> bash\". But given that it actually solves the problems everywhere with no\n> further effort, is it really that bad a solution?\n>\n> For the error messages from test_must_fail, could we go in the same\n> direction, and send them to descriptor 4 rather than 2? We've already\n> staked out descriptor 4 as something magical that must be left alone\n> (see 9be795fb). If we can rely on that, then it becomes a convenient way\n> for functions to make sure their output is going to the script's stderr.\n\nThat sounds clever and rather attractive.  It isn't that much of an\nlayering violation for test-lib-functions.sh::test_must_fail to have\nsuch an intimate knowledge on how test_-lib.sh::test_eval_ sets up\nthe file descriptors, either.\n\n"},{"id":"338892","messageId":"20180209185710.GA23403@sigill.intra.peff.net","threadId":"47795","inReplyTo":"xmqqd11exb6k.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] t: teach 'test_must_fail' to save the command's stderr to a file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-09T18:57:10Z","receivedAt":"2018-02-09T18:57:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 09, 2018 at 10:36:19AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >   2. The \"-x\" problems aren't specific to test_must_fail at all. They're\n> >      a general issue with shell functions.\n> >\n> > I'm not entirely happy with saying \"if you want to use -x, please use\n> > bash\". But given that it actually solves the problems everywhere with no\n> > further effort, is it really that bad a solution?\n> >\n> > For the error messages from test_must_fail, could we go in the same\n> > direction, and send them to descriptor 4 rather than 2? We've already\n> > staked out descriptor 4 as something magical that must be left alone\n> > (see 9be795fb). If we can rely on that, then it becomes a convenient way\n> > for functions to make sure their output is going to the script's stderr.\n> \n> That sounds clever and rather attractive.  It isn't that much of an\n> layering violation for test-lib-functions.sh::test_must_fail to have\n> such an intimate knowledge on how test_-lib.sh::test_eval_ sets up\n> the file descriptors, either.\n\nHere's what it looks like as a patch.\n\n-- >8 --\nSubject: [PATCH] t: send verbose test-helper output to fd 4\n\nTest helper functions like test_must_fail may write messages\nto stderr when they see a problem. When the tests are run\nwith \"--verbose\", this ends up on the test script's stderr,\nand the user can read it.\n\nBut there's a problem. Some tests record stderr as part of\nthe test, like:\n\n  test_must_fail git foo 2>output &&\n  test_i18ngrep expected.message output\n\nIn this case any message from test_must_fail goes into\n\"output\", making the --verbose output less useful. It also\nmeans we might accidentally match it in the second line,\nthough in practice we tend to produce these messages only on\nerror. So we'd abort the test when the first command fails.\n\nLet's send this user-facing output directly to descriptor 4,\nwhich always points to the original stderr (or /dev/null in\nnon-verbose mode). It's already forbidden to redirect\ndescriptor 4, since we use it for BASH_XTRACEFD, as\nexplained in 9be795fbce (t5615: avoid re-using descriptor 4,\n2017-12-08).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI've considered a patch to teach \"make test-lint\" to complain if it sees\n\" 4>\" in any shell script, to try to enforce that rule automatically.\nBut maybe that's overkill (and also it felt funny to put it in\ncheck-non-portable-shell.pl, but we can always change the name ;) ).\n\n t/test-lib-functions.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 1701fe2a06..a156b81aa5 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -625,22 +625,22 @@ test_must_fail () {\n \texit_code=$?\n \tif test $exit_code -eq 0 && ! list_contains \"$_test_ok\" success\n \tthen\n-\t\techo >&2 \"test_must_fail: command succeeded: $*\"\n+\t\techo >&4 \"test_must_fail: command succeeded: $*\"\n \t\treturn 1\n \telif test_match_signal 13 $exit_code && list_contains \"$_test_ok\" sigpipe\n \tthen\n \t\treturn 0\n \telif test $exit_code -gt 129 && test $exit_code -le 192\n \tthen\n-\t\techo >&2 \"test_must_fail: died by signal $(($exit_code - 128)): $*\"\n+\t\techo >&4 \"test_must_fail: died by signal $(($exit_code - 128)): $*\"\n \t\treturn 1\n \telif test $exit_code -eq 127\n \tthen\n-\t\techo >&2 \"test_must_fail: command not found: $*\"\n+\t\techo >&4 \"test_must_fail: command not found: $*\"\n \t\treturn 1\n \telif test $exit_code -eq 126\n \tthen\n-\t\techo >&2 \"test_must_fail: valgrind error: $*\"\n+\t\techo >&4 \"test_must_fail: valgrind error: $*\"\n \t\treturn 1\n \tfi\n \treturn 0\n@@ -678,7 +678,7 @@ test_expect_code () {\n \t\treturn 0\n \tfi\n \n-\techo >&2 \"test_expect_code: command exited with $exit_code, we wanted $want_code $*\"\n+\techo >&4 \"test_expect_code: command exited with $exit_code, we wanted $want_code $*\"\n \treturn 1\n }\n \n@@ -710,7 +710,7 @@ test_cmp_bin() {\n # not output anything when they fail.\n verbose () {\n \t\"$@\" && return 0\n-\techo >&2 \"command failed: $(git rev-parse --sq-quote \"$@\")\"\n+\techo >&4 \"command failed: $(git rev-parse --sq-quote \"$@\")\"\n \treturn 1\n }\n \n-- \n2.16.1.464.gc4bae515b7\n\n"},{"id":"338894","messageId":"20180209190329.GA15005@sigill.intra.peff.net","threadId":"47795","inReplyTo":"20180209185710.GA23403@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] t: teach 'test_must_fail' to save the command's stderr to a file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-09T19:03:30Z","receivedAt":"2018-02-09T19:03:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 09, 2018 at 01:57:10PM -0500, Jeff King wrote:\n\n> Here's what it looks like as a patch.\n> \n> -- >8 --\n> Subject: [PATCH] t: send verbose test-helper output to fd 4\n\nThat applies on 'master'. If we go this route, we'd want this on\nsg/test-i18ngrep, which is in 'next' right now:\n\n-- >8 --\nSubject: [PATCH] test_i18n_grep: send error messages to fd 4\n\nThese were newly added in 63b1a175ee (t: make\n'test_i18ngrep' more informative on failure, 2018-02-08),\nand so missed the conversion in commit X.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n(The X above is whatever you queue the original at).\n\n t/test-lib-functions.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex cddddd2082..aabee13e5d 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -742,18 +742,18 @@ test_i18ngrep () {\n \t\tshift\n \t\t! grep \"$@\" && return 0\n \n-\t\techo >&2 \"error: '! grep $@' did find a match in:\"\n+\t\techo >&4 \"error: '! grep $@' did find a match in:\"\n \telse\n \t\tgrep \"$@\" && return 0\n \n-\t\techo >&2 \"error: 'grep $@' didn't find a match in:\"\n+\t\techo >&4 \"error: 'grep $@' didn't find a match in:\"\n \tfi\n \n \tif test -s \"$last_arg\"\n \tthen\n-\t\tcat >&2 \"$last_arg\"\n+\t\tcat >&4 \"$last_arg\"\n \telse\n-\t\techo >&2 \"<File '$last_arg' is empty>\"\n+\t\techo >&4 \"<File '$last_arg' is empty>\"\n \tfi\n \n \treturn 1\n-- \n2.16.1.464.gc4bae515b7\n\n"},{"id":"340098","messageId":"CAM0VKjnKg=e7ETeKFAmnEsU0thd7nLynyG7iAwUMycdZ91EpCA@mail.gmail.com","threadId":"47795","inReplyTo":"20180209142131.GA18701@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] t: teach 'test_must_fail' to save the command's stderr to a file","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-02-23T23:26:42Z","receivedAt":"2018-02-23T23:26:49Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Feb 9, 2018 at 3:21 PM, Jeff King <peff@peff.net> wrote:\n> On Fri, Feb 09, 2018 at 03:42:34AM +0100, SZEDER Gábor wrote:\n>\n>> To check that a git command fails and to inspect its error message we\n>> usually execute a command like this throughout our test suite:\n>>\n>>   test_must_fail git command --option 2>output.err\n>>\n>> Note that this command doesn't limit the redirection to the git\n>> command, but it redirects the standard error of the 'test_must_fail'\n>> helper function as well.  This is wrong for several reasons:\n>>\n>>   - If that git command were to succeed or die for an unexpected\n>>     reason e.g. signal, then 'test_must_fail's helpful error message\n>>     would end up in the given file instead of on the terminal or in\n>>     't/test-results/$T.out', when the test is run with '-v' or\n>>     '--verbose-log', respectively.\n>>\n>>   - If the test is run with '-x', the trace of executed commands in\n>>     'test_must_fail' will go to stderr as well (except for more recent\n>>     Bash versions supporting $BASH_XTRACEFD), and then in turn will\n>>     end up in the given file.\n>\n> I have to admit that I'm slightly negative on this approach\n\nWell, now I'm not just slightly negative, see the patch series I will\nsend out in a minute :)\n\n> just\n> because:\n>\n>   1. It requires every caller to do something special, rather than just\n>      using normal redirection. And the feature isn't as powerful as\n>      redirection. E.g., some callers do:\n>\n>        test_must_fail foo >output 2>&1\n\nYeah, they do, but most of the time they shouldn't.\n\nI've looked at uses of 'test_must_fail cmd... 2>&1', and also uses of\nother test helper functions with stderr duplicated from stdout (and a\ncouple of cases of \"plain\" 'git cmd ... 2>&1' as well; that's where the\nt6300-for-each-ref fix came from some days ago).\n\nI think all but one of those cases would benefit from handling stdout\nand stderr separately, and that one exception shouldn't use\n'test_must_fail' for a platform command in the first place[1].\n\n  - Many of these tests check error messages after they mingled stderr\n    and stdout like this:\n\n      test_must_fail git cmd >out 2>&1 &&\n      (test_i18n)grep \"relevant part of the error msg\" out\n\n    In these cases there is no need for 2>&1 as they don't care about\n    stdout at all.  They could just grep the failed command's stderr,\n    with the added benefit that they would ensure that the error message\n    indeed goes to stderr.\n\n  - A couple of these tests check the output more strictly:\n\n      test_must_fail git cmd >actual 2>&1 &&\n      echo \"error message\" >expect &&\n      test_(i18n)cmp expect actual\n\n    which also checks that nothing was printed on stdout, but does so\n    implicitly.  Considering all the sloppy uses of 2>&1, it's hard to\n    tell whether it was deliberate to check the empty stdout, or\n    accidental, because the writer of the test used 'test_cmp' instead\n    of 'grep' for its verbosity on failure, or simply mixed up the two.\n    Therefore, I think it's better when it's written more explicitly:\n\n      test_must_fail git cmd >out 2>err &&\n      echo \"exact error message\" >expect &&\n      test_cmp expect err &&\n      test_must_be_empty out\n\n    ... both when reading such a test and when looking at it's output on\n    failure.\n\n  - Then there are some cases that, for some reason, completely silence\n    a git command with:\n\n      test_must_fail git cmd >/dev/null 2>&1\n\n    The output of a command could turn out to be useful when debugging a\n    failing tests, so tests shouldn't do this, unless there is a very\n    good reason (e.g. the command produces a lot of output).\n\n\n  - Finally, a few tests check that something is surely missing from a\n    git command's output, both from stdout and stderr:\n\n      test_must_fail git cmd >out 2>&1 &&\n      ! grep \"should not be there\" out\n\n    Or that a command is completely silent:\n\n      test_must_fail git cmd --quiet >out 2>&1 &&\n      test_must_be_empty out\n\n    Checking the emptiness of stdout and stderr separately would give\n    more info on failure, as it would tell us right away where the\n    unexpected output is coming from (though arguably the presence of\n    words like \"error\" or \"fatal\" in the output of 'test_must_be_empty'\n    would be a dead giveaway).\n    BTW, we already have a couple of careful tests that do:\n\n      git cmd >out 2>err &&\n      test_must_be_empty out &&\n      test_must_be_empty err\n\n    (Which makes me think that extending 'test_must_be_empty' to check\n    the emptiness of multiple files at once would be worth it: fewer\n    lines, fewer characters to type, and it could report all non-empty\n    files, not just the first.)\n\n>      But:\n>\n>        test_must_fail stderr=output foo >output\n>\n>      is not quite right (stdout and stderr outputs may clobber each\n>      other, because they have independent position pointers).\n\nRelying on the combination of the unbuffered stderr and the\nblock-buffered stdout could be fragile to begin with, though in general\nit doesn't really cause troubles if the data on stdout is less than the\nbuffer size or if there's no output on stderr after the stdout buffer is\nflushed.  The stdout buffer size is pagesize on Linux (I don't know\noffhand about others), which is large enough compared to the typical\namount of output git commands produce in our test scripts, so we could\nget away with it.\n\n>   2. The \"-x\" problems aren't specific to test_must_fail at all. They're\n>      a general issue with shell functions.\n\nSure.  I tried to address 'test_must_fail' first, because, with about\n90% of the cases, it is by far the worst offender among all test helper\nfunctions with redirected stderr.  'test_expect_code' comes in second\nfar behind, and there are a mere handful of the combination of\n'test_terminal' and 'perl'.\n\nThe 'test_expect_code 129 ... 2>&1' in t0012-help.sh's row of '$builtin\ncan handle -h' tests is particularly interesting, because it hides the\ninconsistency that 76 commands send their help/usage text to stdout,\nwhile 45 to stderr.\n\n> I'm not entirely happy with saying \"if you want to use -x, please use\n> bash\". But given that it actually solves the problems everywhere with no\n> further effort, is it really that bad a solution?\n\nWell, not that bad a workaround, but in my opinion the real solution is\nto make '-x' tracing work reliably with /bin/sh.\n\nSince we strive for POSIX shell compatibility in our test scripts, I\ndutifully run the test suite with /bin/sh.  Switching shells for '-x' to\nget more info about what went wrong (and then switching back) is a\nhassle even when it happens to work, and rather annoying when it\ndoesn't.  I've lost count of the various ways of running tests with '-x'\nand Bash that I naively thought would Just Work, but didn't because of\nthe subtleties of GIT-BUILD-OPTIONS and/or re-exec in case of\n'--verbose-log'.  I find I still reached for various ad-hoc\ndebugging/tracing or 'test_pause' first, and kept '-x' only as last\nresort.\n\nIf we start running tests with '-x' and Bash on Travis CI, then it won't\nbe able to (semi-)automatically catch non-POSIX constructs entering the\ntest scripts.  And I do want to run all tests with '-x', to get the most\ninformation about rarely occurring transient errors.\n\n\n[1] - 't/lib-git-p4.sh' contains the following command:\n\n        test_must_fail kill $pid >/dev/null 2>&1\n"}]}