{"thread":{"id":"48006","subject":"[PATCH 0/2] Make cvs tests pass with '-x' tracing and /bin/sh","startedAt":"2018-03-08T12:38:58Z","lastAt":"2018-03-13T00:05:19Z","messageCount":14,"participants":["SZEDER Gábor","Eric Sunshine","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"341285","messageId":"20180308123844.15163-1-szeder.dev@gmail.com","threadId":"48006","inReplyTo":null,"subject":"[PATCH 0/2] Make cvs tests pass with '-x' tracing and /bin/sh","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-03-08T12:38:42Z","receivedAt":"2018-03-08T12:38:58Z","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> \n> I installed 'cvs' and whatnot to run t94* and t96* tests, and sure\n> enough, 5 tests in 2 test scripts fail with '-x' tracing and /bin/sh.\n> I think I will be able to get around to send v2 during the weekend.\n\nWell, I wasn't able to get around to it, and in the meantime\n'sg/test-x' got merged into 'next'.  So here are the updates for those\ntwo test scripts.\n\nThe commit message of the first patch is perhaps unnecessary long, but\nit's mostly about explaining why the affected test was working\nreasonably well despite the rather weird 'test_cmp this that >>out\n2>&1' thing.\n\nSZEDER Gábor (2):\n  t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'\n  t9402-git-cvsserver-refs: don't check the stderr of a subshell\n\n t/t9400-git-cvsserver-server.sh | 15 +++++++++------\n t/t9402-git-cvsserver-refs.sh   |  8 ++++----\n 2 files changed, 13 insertions(+), 10 deletions(-)\n\n-- \n2.16.2.603.g180c1428f0\n\n"},{"id":"341286","messageId":"20180308123844.15163-3-szeder.dev@gmail.com","threadId":"48006","inReplyTo":"20180308123844.15163-1-szeder.dev@gmail.com","subject":"[PATCH 2/2] t9402-git-cvsserver-refs: don't check the stderr of a subshell","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-03-08T12:38:44Z","receivedAt":"2018-03-08T12:38:59Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Four 'cvs diff' related tests in 't9402-git-cvsserver-refs.sh' fail\nwhen the test script is run with '-x' tracing (and using a shell other\nthan a Bash version supporting BASH_XTRACEFD).  The reason for those\nfailures is that the tests check the emptiness of a subshell's stderr,\nwhich includes the trace of commands executed in that subshell as\nwell, throwing off the emptiness check.\n\nSave the stdout and stderr of the invoked 'cvs' command instead of the\nwhole subshell, so the latter remains free from tracing output.  (Note\nthat changing how stdout is saved is only done for the sake of\nconsistency, it's not necessary for correctness.)\n\nAfter this change t9402 passes with '-x', even when running with\n/bin/sh.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/t9402-git-cvsserver-refs.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t9402-git-cvsserver-refs.sh b/t/t9402-git-cvsserver-refs.sh\nindex 6d2d3c8739..cf31ace667 100755\n--- a/t/t9402-git-cvsserver-refs.sh\n+++ b/t/t9402-git-cvsserver-refs.sh\n@@ -455,20 +455,20 @@ test_expect_success 'cvs up -r $(git rev-parse v1)' '\n '\n \n test_expect_success 'cvs diff -r v1 -u' '\n-\t( cd cvswork && cvs -f diff -r v1 -u ) >cvsDiff.out 2>cvs.log &&\n+\t( cd cvswork && cvs -f diff -r v1 -u >../cvsDiff.out 2>../cvs.log ) &&\n \ttest_must_be_empty cvsDiff.out &&\n \ttest_must_be_empty cvs.log\n '\n \n test_expect_success 'cvs diff -N -r v2 -u' '\n-\t( cd cvswork && ! cvs -f diff -N -r v2 -u ) >cvsDiff.out 2>cvs.log &&\n+\t( cd cvswork && ! cvs -f diff -N -r v2 -u >../cvsDiff.out 2>../cvs.log ) &&\n \ttest_must_be_empty cvs.log &&\n \ttest -s cvsDiff.out &&\n \tcheck_diff cvsDiff.out v2 v1 >check_diff.out 2>&1\n '\n \n test_expect_success 'cvs diff -N -r v2 -r v1.2' '\n-\t( cd cvswork && ! cvs -f diff -N -r v2 -r v1.2 -u ) >cvsDiff.out 2>cvs.log &&\n+\t( cd cvswork && ! cvs -f diff -N -r v2 -r v1.2 -u >../cvsDiff.out 2>../cvs.log ) &&\n \ttest_must_be_empty cvs.log &&\n \ttest -s cvsDiff.out &&\n \tcheck_diff cvsDiff.out v2 v1.2 >check_diff.out 2>&1\n@@ -487,7 +487,7 @@ test_expect_success 'apply early [cvswork3] diff to b3' '\n '\n \n test_expect_success 'check [cvswork3] diff' '\n-\t( cd cvswork3 && ! cvs -f diff -N -u ) >\"$WORKDIR/cvsDiff.out\" 2>cvs.log &&\n+\t( cd cvswork3 && ! cvs -f diff -N -u >\"$WORKDIR/cvsDiff.out\" 2>../cvs.log ) &&\n \ttest_must_be_empty cvs.log &&\n \ttest -s cvsDiff.out &&\n \ttest $(grep Index: cvsDiff.out | wc -l) = 3 &&\n-- \n2.16.2.603.g180c1428f0\n\n"},{"id":"341287","messageId":"20180308123844.15163-2-szeder.dev@gmail.com","threadId":"48006","inReplyTo":"20180308123844.15163-1-szeder.dev@gmail.com","subject":"[PATCH 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-03-08T12:38:43Z","receivedAt":"2018-03-08T12:39:02Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The test 'cvs update (-p)' redirects and checks 'test_cmp's stdout and\neven its stderr.  The commit introducing this test in 6e8937a084\n(cvsserver: Add test for update -p, 2008-03-27) doesn't discuss why,\nin fact its log message only consists of that subject line.  Anyway,\nweird as it is, it kind of made sense due to the way that test was\nstructured:\n\nAfter a bit of preparation, this test updates four files via CVS and\nchecks their contents using 'test_cmp', but it does so in a for loop\niterating over the names of those four files.  Now, the exit status of\na for loop is the exit status of the last command executed in the\nloop, meaning that the test can't simply rely on the exit code of\n'test_cmp' in the loop's body.  Instead, the test works it around by\nrelying on the stdout of 'test_cmp' being silent on success and\nshowing the diff on failure, as it appends the stdout of all four\n'test_cmp' invocations to a single file and checks that file's\nemptiness after the loop (with 'test -z \"$(cat ...)\"', no less; there\nwas no 'test_must_be_empty' back then).  Furthermore, the test\nredirects the stderr of those 'test_cmp' invocations to this file,\ntoo: while 'test_cmp' itself doesn't output anything to stderr, the\ninvoked 'diff' or 'cmp' commands do send their error messages there,\ne.g. if they can't open a file because its name was misspelled.\n\nThis also makes this test fail when the test script is run with '-x'\ntracing (and using a shell other than a Bash version supporting\nBASH_XTRACEFD), because 'test_cmp's stderr contains the trace of the\n'diff' command executed inside the helper function, throwing off the\nsubsequent emptiness check.\n\nUnroll that for loop, so we can check the files' contents the usual\nway and rely on 'test_cmp's exit code failing the && chain.  Extract\nupdating a file via CVS and verifying its contents using 'test_cmp'\ninto a helper function requiring the file's name as parameter to avoid\nmuch of the repetition resulting from unrolling the loop.\n\nAfter this change t9400 passes with '-x', even when running with\n/bin/sh.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/t9400-git-cvsserver-server.sh | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh\nindex c30660d606..1f67d2822f 100755\n--- a/t/t9400-git-cvsserver-server.sh\n+++ b/t/t9400-git-cvsserver-server.sh\n@@ -437,6 +437,11 @@ test_expect_success 'cvs update (merge no-op)' \\\n     GIT_CONFIG=\"$git_config\" cvs -Q update &&\n     test_cmp merge ../merge'\n \n+check_cvs_update_p () {\n+    GIT_CONFIG=\"$git_config\" cvs update -p \"$1\" >\"$1\".out &&\n+    test_cmp \"$1\".out ../\"$1\"\n+}\n+\n cd \"$WORKDIR\"\n test_expect_success 'cvs update (-p)' '\n     touch really-empty &&\n@@ -447,12 +452,10 @@ test_expect_success 'cvs update (-p)' '\n     git push gitcvs.git >/dev/null &&\n     cd cvswork &&\n     GIT_CONFIG=\"$git_config\" cvs update &&\n-    rm -f failures &&\n-    for i in merge no-lf empty really-empty; do\n-        GIT_CONFIG=\"$git_config\" cvs update -p \"$i\" >$i.out\n-\ttest_cmp $i.out ../$i >>failures 2>&1\n-    done &&\n-    test -z \"$(cat failures)\"\n+    check_cvs_update_p merge &&\n+    check_cvs_update_p no-lf &&\n+    check_cvs_update_p empty &&\n+    check_cvs_update_p really-empty\n '\n \n cd \"$WORKDIR\"\n-- \n2.16.2.603.g180c1428f0\n\n"},{"id":"341307","messageId":"CAPig+cTtV2unsixsMCWytdJMiYYytgdvbavfENhBrwvXq_79Bw@mail.gmail.com","threadId":"48006","inReplyTo":"20180308123844.15163-2-szeder.dev@gmail.com","subject":"Re: [PATCH 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-08T18:51:17Z","receivedAt":"2018-03-08T18:51:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 8, 2018 at 7:38 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> The test 'cvs update (-p)' redirects and checks 'test_cmp's stdout and\n> even its stderr.  The commit introducing this test in 6e8937a084\n> (cvsserver: Add test for update -p, 2008-03-27) doesn't discuss why,\n> in fact its log message only consists of that subject line.  Anyway,\n> weird as it is, it kind of made sense due to the way that test was\n> structured:\n\n[excellent explanation snipped]\n\n> Unroll that for loop, so we can check the files' contents the usual\n> way and rely on 'test_cmp's exit code failing the && chain.  Extract\n> updating a file via CVS and verifying its contents using 'test_cmp'\n> into a helper function requiring the file's name as parameter to avoid\n> much of the repetition resulting from unrolling the loop.\n\nAn alternative approach used elsewhere in the test suite[1] would be\nsimply to 'exit' if test_cmp fails:\n\n    for i in merge no-lf empty really-empty; do\n        GIT_CONFIG=\"$git_config\" cvs update -p \"$i\" >$i.out\n        test_cmp $i.out ../$i || exit 1\n    done &&\n\n(And, like the existing patch, this removes the need for capturing\ntest_cmp's output into a \"failures\" file.)\n\n[1]: For example, the \"setup\" test of t2204-add-ignored.sh.\n\n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> ---\n> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh\n> @@ -447,12 +452,10 @@ test_expect_success 'cvs update (-p)' '\n>      git push gitcvs.git >/dev/null &&\n>      cd cvswork &&\n>      GIT_CONFIG=\"$git_config\" cvs update &&\n> -    rm -f failures &&\n> -    for i in merge no-lf empty really-empty; do\n> -        GIT_CONFIG=\"$git_config\" cvs update -p \"$i\" >$i.out\n> -       test_cmp $i.out ../$i >>failures 2>&1\n> -    done &&\n> -    test -z \"$(cat failures)\"\n> +    check_cvs_update_p merge &&\n> +    check_cvs_update_p no-lf &&\n> +    check_cvs_update_p empty &&\n> +    check_cvs_update_p really-empty\n>  '\n"},{"id":"341316","messageId":"CAM0VKjmKcFVOLA-fFnSm=tVapa5qgUoU3Sa7RjSrqoHp5qXZiw@mail.gmail.com","threadId":"48006","inReplyTo":"CAPig+cTtV2unsixsMCWytdJMiYYytgdvbavfENhBrwvXq_79Bw@mail.gmail.com","subject":"Re: [PATCH 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-03-08T21:49:50Z","receivedAt":"2018-03-08T21:49:56Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Mar 8, 2018 at 7:51 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Thu, Mar 8, 2018 at 7:38 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:\n\n>> Unroll that for loop, so we can check the files' contents the usual\n>> way and rely on 'test_cmp's exit code failing the && chain.  Extract\n>> updating a file via CVS and verifying its contents using 'test_cmp'\n>> into a helper function requiring the file's name as parameter to avoid\n>> much of the repetition resulting from unrolling the loop.\n>\n> An alternative approach used elsewhere in the test suite[1] would be\n> simply to 'exit' if test_cmp fails:\n>\n>     for i in merge no-lf empty really-empty; do\n>         GIT_CONFIG=\"$git_config\" cvs update -p \"$i\" >$i.out\n>         test_cmp $i.out ../$i || exit 1\n>     done &&\n>\n> (And, like the existing patch, this removes the need for capturing\n> test_cmp's output into a \"failures\" file.)\n>\n> [1]: For example, the \"setup\" test of t2204-add-ignored.sh.\n\nBut 't/README' sayeth:\n\n    Don't:\n\n     - exit() within a <script> part.\n\nAnd it's right: 'exit' terminates the shell process it's invoked in,\ni.e. the whole test script (well, unless it's invoked in a subshell)\nwithout executing the remaining tests and the usual housekeeping and\nreporting.\n\nConsider the following test script:\n\n  -- >8 --\n\n#!/bin/sh\n\ntest_description='exit 1?'\n\n. ./test-lib.sh\n\ntest_expect_success 'exit 1' '\n        exit 1\n'\n\ntest_expect_success 'second test' '\n        true\n'\n\ntest_done\n\n  -- >8 --\n\nIt's output:\n\n  $ ./t9999-exit.sh\n  FATAL: Unexpected exit with code 1\n  $ sed -e 's/exit 1/false/' -i t9999-exit.sh\n  $ ./t9999-exit.sh not ok 1 - false\n  #\n  #             false\n  #\n  ok 2 - second test\n  # failed 1 among 2 test(s)\n  1..2\n"},{"id":"341317","messageId":"CAPig+cRSDbFZ_C9opu3pr=m7HwFkeuoxUD_8Yqbd4XxX-W0cHg@mail.gmail.com","threadId":"48006","inReplyTo":"CAM0VKjmKcFVOLA-fFnSm=tVapa5qgUoU3Sa7RjSrqoHp5qXZiw@mail.gmail.com","subject":"Re: [PATCH 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-08T22:01:34Z","receivedAt":"2018-03-08T22:01:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 8, 2018 at 4:49 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> On Thu, Mar 8, 2018 at 7:51 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> An alternative approach used elsewhere in the test suite[1] would be\n>> simply to 'exit' if test_cmp fails:\n>>\n>>     for i in merge no-lf empty really-empty; do\n>>         GIT_CONFIG=\"$git_config\" cvs update -p \"$i\" >$i.out\n>>         test_cmp $i.out ../$i || exit 1\n>>     done &&\n>\n> And it's right: 'exit' terminates the shell process it's invoked in,\n> i.e. the whole test script (well, unless it's invoked in a subshell)\n> without executing the remaining tests and the usual housekeeping and\n> reporting.\n>\n> Consider the following test script:\n>\n>   $ ./t9999-exit.sh\n>   FATAL: Unexpected exit with code 1\n\nSorry for the confusion. I meant \"return 1\" as used elsewhere in the\ntest suite[1].\n\n--- >8 ---\n#!/bin/sh\ntest_description='return 1?'\n. ./test-lib.sh\n\ntest_expect_success 'return 1' '\n        return 1\n'\n\ntest_expect_success 'second test' '\n        true\n'\n\ntest_done\n--- >8 ---\n\n$ ./t9977.sh\nnot ok 1 - return 1\n#\n#         return 1\n#\nok 2 - second test\n# failed 1 among 2 test(s)\n1..2\n$\n\n[1]: For example, the \"setup\" test of t4151-am-abort.sh.\n"},{"id":"341320","messageId":"CAPig+cRA36YHu+kf3t8=zEOq6OHvgKbsHaV_mNQEKoGNpunHcA@mail.gmail.com","threadId":"48006","inReplyTo":"CAPig+cRSDbFZ_C9opu3pr=m7HwFkeuoxUD_8Yqbd4XxX-W0cHg@mail.gmail.com","subject":"Re: [PATCH 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-08T22:07:35Z","receivedAt":"2018-03-08T22:07:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 8, 2018 at 5:01 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> Sorry for the confusion. I meant \"return 1\" as used elsewhere in the\n> test suite[1].\n> [1]: For example, the \"setup\" test of t4151-am-abort.sh.\n\nAdditional context: e6821d09e4 (t: fix some trivial cases of ignored\nexit codes in loops, 2015-03-25)\n"},{"id":"341321","messageId":"CAM0VKjkreUoefAmmNm-OqbCO01vaceb2ehNcVj9jVF_wNoH7XA@mail.gmail.com","threadId":"48006","inReplyTo":"CAPig+cRSDbFZ_C9opu3pr=m7HwFkeuoxUD_8Yqbd4XxX-W0cHg@mail.gmail.com","subject":"Re: [PATCH 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-03-08T22:38:35Z","receivedAt":"2018-03-08T22:38:41Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Mar 8, 2018 at 11:01 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Thu, Mar 8, 2018 at 4:49 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:\n>> On Thu, Mar 8, 2018 at 7:51 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>> An alternative approach used elsewhere in the test suite[1] would be\n>>> simply to 'exit' if test_cmp fails:\n>>>\n>>>     for i in merge no-lf empty really-empty; do\n>>>         GIT_CONFIG=\"$git_config\" cvs update -p \"$i\" >$i.out\n>>>         test_cmp $i.out ../$i || exit 1\n>>>     done &&\n\n> Sorry for the confusion. I meant \"return 1\" as used elsewhere in the\n> test suite[1].\n\nOh, I see, that's indeed better.  Thanks.\n"},{"id":"341322","messageId":"20180308224458.5730-1-szeder.dev@gmail.com","threadId":"48006","inReplyTo":"CAPig+cRSDbFZ_C9opu3pr=m7HwFkeuoxUD_8Yqbd4XxX-W0cHg@mail.gmail.com","subject":"[PATCH v1.1 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-03-08T22:44:58Z","receivedAt":"2018-03-08T22:45:13Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The test 'cvs update (-p)' redirects and checks 'test_cmp's stdout and\neven its stderr.  The commit introducing this test in 6e8937a084\n(cvsserver: Add test for update -p, 2008-03-27) doesn't discuss why,\nin fact its log message only consists of that subject line.  Anyway,\nweird as it is, it kind of made sense due to the way that test was\nstructured:\n\nAfter a bit of preparation, this test updates four files via CVS and\nchecks their contents using 'test_cmp', but it does so in a for loop\niterating over the names of those four files.  Now, the exit status of\na for loop is the exit status of the last command executed in the\nloop, meaning that the test can't simply rely on the exit code of\n'test_cmp' in the loop's body.  Instead, the test works it around by\nrelying on the stdout of 'test_cmp' being silent on success and\nshowing the diff on failure, as it appends the stdout of all four\n'test_cmp' invocations to a single file and checks that file's\nemptiness after the loop (with 'test -z \"$(cat ...)\"', no less; there\nwas no 'test_must_be_empty' back then).  Furthermore, the test\nredirects the stderr of those 'test_cmp' invocations to this file,\ntoo: while 'test_cmp' itself doesn't output anything to stderr, the\ninvoked 'diff' or 'cmp' commands do send their error messages there,\ne.g. if they can't open a file because its name was misspelled.\n\nThis also makes this test fail when the test script is run with '-x'\ntracing (and using a shell other than a Bash version supporting\nBASH_XTRACEFD), because 'test_cmp's stderr contains the trace of the\n'diff' command executed inside the helper function, throwing off the\nsubsequent emptiness check.\n\nStop relying on 'test_cmp's output and instead run 'test_cmp a b ||\nreturn 1' in the for loop in order to make 'test_cmp's error code fail\nthe test.  Furthermore, add the missing && after the cvs command to\ncreate a && chain in the loop's body.\n\nAfter this change t9400 passes with '-x', even when running with\n/bin/sh.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n\nChanges:\nUse Eric's suggestion and run 'test_cmp a b || return 1' in the for\nloop to fail the test.\n\n t/t9400-git-cvsserver-server.sh | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh\nindex c30660d606..5ff3789199 100755\n--- a/t/t9400-git-cvsserver-server.sh\n+++ b/t/t9400-git-cvsserver-server.sh\n@@ -449,10 +449,9 @@ test_expect_success 'cvs update (-p)' '\n     GIT_CONFIG=\"$git_config\" cvs update &&\n     rm -f failures &&\n     for i in merge no-lf empty really-empty; do\n-        GIT_CONFIG=\"$git_config\" cvs update -p \"$i\" >$i.out\n-\ttest_cmp $i.out ../$i >>failures 2>&1\n-    done &&\n-    test -z \"$(cat failures)\"\n+\tGIT_CONFIG=\"$git_config\" cvs update -p \"$i\" >$i.out &&\n+\ttest_cmp $i.out ../$i || return 1\n+    done\n '\n \n cd \"$WORKDIR\"\n-- \n2.16.2.603.g180c1428f0\n\n"},{"id":"341326","messageId":"xmqqzi3i6t2v.fsf@gitster-ct.c.googlers.com","threadId":"48006","inReplyTo":"20180308224458.5730-1-szeder.dev@gmail.com","subject":"Re: [PATCH v1.1 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-08T23:33:28Z","receivedAt":"2018-03-08T23:33:37Z","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> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh\n> index c30660d606..5ff3789199 100755\n> --- a/t/t9400-git-cvsserver-server.sh\n> +++ b/t/t9400-git-cvsserver-server.sh\n> @@ -449,10 +449,9 @@ test_expect_success 'cvs update (-p)' '\n>      GIT_CONFIG=\"$git_config\" cvs update &&\n>      rm -f failures &&\n>      for i in merge no-lf empty really-empty; do\n> -        GIT_CONFIG=\"$git_config\" cvs update -p \"$i\" >$i.out\n> -\ttest_cmp $i.out ../$i >>failures 2>&1\n> -    done &&\n> -    test -z \"$(cat failures)\"\n> +\tGIT_CONFIG=\"$git_config\" cvs update -p \"$i\" >$i.out &&\n> +\ttest_cmp $i.out ../$i || return 1\n> +    done\n>  '\n\nThis makes \"rm -f failures &&\" unnecessary, no?\n"},{"id":"341327","messageId":"CAM0VKjkkFR-zVQMMqp0pJn489bV1uqEHZkB8Mo789J0K+qO3vA@mail.gmail.com","threadId":"48006","inReplyTo":"xmqqzi3i6t2v.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1.1 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-03-08T23:41:48Z","receivedAt":"2018-03-08T23:41:54Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Mar 9, 2018 at 12:33 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> SZEDER Gábor <szeder.dev@gmail.com> writes:\n>\n>> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh\n>> index c30660d606..5ff3789199 100755\n>> --- a/t/t9400-git-cvsserver-server.sh\n>> +++ b/t/t9400-git-cvsserver-server.sh\n>> @@ -449,10 +449,9 @@ test_expect_success 'cvs update (-p)' '\n>>      GIT_CONFIG=\"$git_config\" cvs update &&\n>>      rm -f failures &&\n>>      for i in merge no-lf empty really-empty; do\n>> -        GIT_CONFIG=\"$git_config\" cvs update -p \"$i\" >$i.out\n>> -     test_cmp $i.out ../$i >>failures 2>&1\n>> -    done &&\n>> -    test -z \"$(cat failures)\"\n>> +     GIT_CONFIG=\"$git_config\" cvs update -p \"$i\" >$i.out &&\n>> +     test_cmp $i.out ../$i || return 1\n>> +    done\n>>  '\n>\n> This makes \"rm -f failures &&\" unnecessary, no?\n\nIndeed, it does.\n"},{"id":"341328","messageId":"xmqqvae66ske.fsf@gitster-ct.c.googlers.com","threadId":"48006","inReplyTo":"CAM0VKjkkFR-zVQMMqp0pJn489bV1uqEHZkB8Mo789J0K+qO3vA@mail.gmail.com","subject":"Re: [PATCH v1.1 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-08T23:44:33Z","receivedAt":"2018-03-08T23:44:41Z","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, Mar 9, 2018 at 12:33 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> SZEDER Gábor <szeder.dev@gmail.com> writes:\n>>\n>>> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh\n>>> index c30660d606..5ff3789199 100755\n>>> --- a/t/t9400-git-cvsserver-server.sh\n>>> +++ b/t/t9400-git-cvsserver-server.sh\n>>> @@ -449,10 +449,9 @@ test_expect_success 'cvs update (-p)' '\n>>>      GIT_CONFIG=\"$git_config\" cvs update &&\n>>>      rm -f failures &&\n>>>      for i in merge no-lf empty really-empty; do\n>>> -        GIT_CONFIG=\"$git_config\" cvs update -p \"$i\" >$i.out\n>>> -     test_cmp $i.out ../$i >>failures 2>&1\n>>> -    done &&\n>>> -    test -z \"$(cat failures)\"\n>>> +     GIT_CONFIG=\"$git_config\" cvs update -p \"$i\" >$i.out &&\n>>> +     test_cmp $i.out ../$i || return 1\n>>> +    done\n>>>  '\n>>\n>> This makes \"rm -f failures &&\" unnecessary, no?\n>\n> Indeed, it does.\n\nOK, no need to resend, as I'll amend it locally before queuing\n(unless there is something else that needs updating, that is).\n\nThanks.\n"},{"id":"341335","messageId":"CAM0VKjnfNBGd4BPXGbpGSTH_YW8dfX=19Lr-DCcS=B5BHp_nCg@mail.gmail.com","threadId":"48006","inReplyTo":"xmqqvae66ske.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1.1 1/2] t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-03-09T11:20:20Z","receivedAt":"2018-03-09T11:20:27Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Mar 9, 2018 at 12:44 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n>>> This makes \"rm -f failures &&\" unnecessary, no?\n>>\n>> Indeed, it does.\n>\n> OK, no need to resend, as I'll amend it locally before queuing\n> (unless there is something else that needs updating, that is).\n\nThanks.\n\nThat 'rm' was unnecessary to begin with, because none of the previous\ntests wrote and left behind a file 'failures', ever.  Though one could\nargue that the original author was careful and added that 'rm failures'\nas a protection against future changes to previous tests...  Dunno, we\nare talking about a test that checked the stderr of 'test_cmp', so\nanything is possible :)\n"},{"id":"341516","messageId":"20180313000512.GH1968@sigill.intra.peff.net","threadId":"48006","inReplyTo":"20180308123844.15163-1-szeder.dev@gmail.com","subject":"Re: [PATCH 0/2] Make cvs tests pass with '-x' tracing and /bin/sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-13T00:05:12Z","receivedAt":"2018-03-13T00:05:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 08, 2018 at 01:38:42PM +0100, SZEDER Gábor wrote:\n\n> > I installed 'cvs' and whatnot to run t94* and t96* tests, and sure\n> > enough, 5 tests in 2 test scripts fail with '-x' tracing and /bin/sh.\n> > I think I will be able to get around to send v2 during the weekend.\n> \n> Well, I wasn't able to get around to it, and in the meantime\n> 'sg/test-x' got merged into 'next'.  So here are the updates for those\n> two test scripts.\n> \n> The commit message of the first patch is perhaps unnecessary long, but\n> it's mostly about explaining why the affected test was working\n> reasonably well despite the rather weird 'test_cmp this that >>out\n> 2>&1' thing.\n\nYou know I would never complain about a long commit message. :)\n\nBoth patches look OK to me. My only comment on the first one was \"you\nshould just use 'return'\", but I see Eric beat me to it.\n\nI do think the postimage of the second one is a little less readable.\nThat's not a big deal to me, but I'm pretty sure I would intentionally\nwrite it the \"original\" way if I found myself in a similar situation.\nWhich makes me wonder whether we'll end up accidentally re-introducing\nthese kinds of \"-x\" failures. But as I said before, I'm willing to wait\nand see.\n\n-Peff\n"}]}