Make cvs tests pass with '-x' tracing and /bin/sh

Well, I wasn't able to get around to it, and in the meantime 'sg/test-x' got merged into 'next'. So here are the updates for those two test scripts.

The commit message of the first patch is perhaps unnecessary long, but it's mostly about explaining why the affected test was working reasonably well despite the rather weird 'test_cmp this that >>out 2>&1' thing.

SZEDER Gábor (2):
t9400-git-cvsserver-server: don't rely on the output of 'test_cmp'
t9402-git-cvsserver-refs: don't check the stderr of a subshell

 t/t9400-git-cvsserver-server.sh | 15 +++++++++------
 t/t9402-git-cvsserver-refs.sh   |  8 ++++----
 2 files changed, 13 insertions(+), 10 deletions(-)

2.16.2.603.g180c1428f0

The test 'cvs update (-p)' redirects and checks 'test_cmp's stdout and even its stderr. The commit introducing this test in 6e8937a084 (cvsserver: Add test for update -p, 2008-03-27) doesn't discuss why, in fact its log message only consists of that subject line. Anyway, weird as it is, it kind of made sense due to the way that test was structured:

After a bit of preparation, this test updates four files via CVS and checks their contents using 'test_cmp', but it does so in a for loop iterating over the names of those four files. Now, the exit status of a for loop is the exit status of the last command executed in the loop, meaning that the test can't simply rely on the exit code of 'test_cmp' in the loop's body. Instead, the test works it around by relying on the stdout of 'test_cmp' being silent on success and showing the diff on failure, as it appends the stdout of all four 'test_cmp' invocations to a single file and checks that file's emptiness after the loop (with 'test -z "$(cat ...)"', no less; there was no 'test_must_be_empty' back then). Furthermore, the test redirects the stderr of those 'test_cmp' invocations to this file,
too: while 'test_cmp' itself doesn't output anything to stderr, the invoked 'diff' or 'cmp' commands do send their error messages there, e.g. if they can't open a file because its name was misspelled.

This also makes this test fail when the test script is run with '-x' tracing (and using a shell other than a Bash version supporting BASH_XTRACEFD), because 'test_cmp's stderr contains the trace of the 'diff' command executed inside the helper function, throwing off the subsequent emptiness check.

Unroll that for loop, so we can check the files' contents the usual way and rely on 'test_cmp's exit code failing the && chain. Extract updating a file via CVS and verifying its contents using 'test_cmp' into a helper function requiring the file's name as parameter to avoid much of the repetition resulting from unrolling the loop.

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

---
 t/t9400-git-cvsserver-server.sh | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

index c30660d606..1f67d2822f 100755
@@ -437,6 +437,11 @@ test_expect_success 'cvs update (merge no-op)' \
     GIT_CONFIG="$git_config" cvs -Q update &&
     test_cmp merge ../merge'
 
+check_cvs_update_p () {
+    GIT_CONFIG="$git_config" cvs update -p "$1" >"$1".out &&
+    test_cmp "$1".out ../"$1"
+}
+
 cd "$WORKDIR"
 test_expect_success 'cvs update (-p)' '
     touch really-empty &&

@@ -447,12 +452,10 @@ test_expect_success 'cvs update (-p)' '
     git push gitcvs.git >/dev/null &&
     cd cvswork &&
     GIT_CONFIG="$git_config" cvs update &&
-    rm -f failures &&
-    for i in merge no-lf empty really-empty; do
-        GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out
-	test_cmp $i.out ../$i >>failures 2>&1
-    done &&
-    test -z "$(cat failures)"
+    check_cvs_update_p merge &&
+    check_cvs_update_p no-lf &&
+    check_cvs_update_p empty &&
+    check_cvs_update_p really-empty
 '
 
 cd "$WORKDIR"

2.16.2.603.g180c1428f0

[excellent explanation snipped]

An alternative approach used elsewhere in the test suite[1] would be simply to 'exit' if test_cmp fails:

    for i in merge no-lf empty really-empty; do
        GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out
        test_cmp $i.out ../$i || exit 1
    done &&

(And, like the existing patch, this removes the need for capturing test_cmp's output into a "failures" file.)

[1]: For example, the "setup" test of t2204-add-ignored.sh.

But 't/README' sayeth:

Don't:

- exit() within a <script> part.

And it's right: 'exit' terminates the shell process it's invoked in, i.e. the whole test script (well, unless it's invoked in a subshell) without executing the remaining tests and the usual housekeeping and reporting.

Consider the following test script:

-- >8 --

#!/bin/sh

test_description='exit 1?'

. ./test-lib.sh

test_expect_success 'exit 1' '
exit 1
'

test_expect_success 'second test' '
true
'

test_done

-- >8 --

It's output:

  $ ./t9999-exit.sh
  FATAL: Unexpected exit with code 1
  $ sed -e 's/exit 1/false/' -i t9999-exit.sh
  $ ./t9999-exit.sh not ok 1 - false
  #
  #             false
  #
  ok 2 - second test
  # failed 1 among 2 test(s)
  1..2

Sorry for the confusion. I meant "return 1" as used elsewhere in the test suite[1].

--- >8 ---
#!/bin/sh
test_description='return 1?'
. ./test-lib.sh

test_expect_success 'return 1' '
return 1
'

test_expect_success 'second test' '
true
'

test_done
--- >8 ---

$ ./t9977.sh
not ok 1 - return 1
#
#         return 1
#
ok 2 - second test
# failed 1 among 2 test(s)
1..2
$

[1]: For example, the "setup" test of t4151-am-abort.sh.

Additional context: e6821d09e4 (t: fix some trivial cases of ignored exit codes in loops, 2015-03-25)

Oh, I see, that's indeed better. Thanks.

The test 'cvs update (-p)' redirects and checks 'test_cmp's stdout and even its stderr. The commit introducing this test in 6e8937a084 (cvsserver: Add test for update -p, 2008-03-27) doesn't discuss why, in fact its log message only consists of that subject line. Anyway, weird as it is, it kind of made sense due to the way that test was structured:

After a bit of preparation, this test updates four files via CVS and checks their contents using 'test_cmp', but it does so in a for loop iterating over the names of those four files. Now, the exit status of a for loop is the exit status of the last command executed in the loop, meaning that the test can't simply rely on the exit code of 'test_cmp' in the loop's body. Instead, the test works it around by relying on the stdout of 'test_cmp' being silent on success and showing the diff on failure, as it appends the stdout of all four 'test_cmp' invocations to a single file and checks that file's emptiness after the loop (with 'test -z "$(cat ...)"', no less; there was no 'test_must_be_empty' back then). Furthermore, the test redirects the stderr of those 'test_cmp' invocations to this file,
too: while 'test_cmp' itself doesn't output anything to stderr, the invoked 'diff' or 'cmp' commands do send their error messages there, e.g. if they can't open a file because its name was misspelled.

This also makes this test fail when the test script is run with '-x' tracing (and using a shell other than a Bash version supporting BASH_XTRACEFD), because 'test_cmp's stderr contains the trace of the 'diff' command executed inside the helper function, throwing off the subsequent emptiness check.

Stop relying on 'test_cmp's output and instead run 'test_cmp a b || return 1' in the for loop in order to make 'test_cmp's error code fail the test. Furthermore, add the missing && after the cvs command to create a && chain in the loop's body.

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

---

Changes:
Use Eric's suggestion and run 'test_cmp a b || return 1' in the for loop to fail the test.

 t/t9400-git-cvsserver-server.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

index c30660d606..5ff3789199 100755
@@ -449,10 +449,9 @@ test_expect_success 'cvs update (-p)' '
     GIT_CONFIG="$git_config" cvs update &&
     rm -f failures &&
     for i in merge no-lf empty really-empty; do
-        GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out
-	test_cmp $i.out ../$i >>failures 2>&1
-    done &&
-    test -z "$(cat failures)"
+	GIT_CONFIG="$git_config" cvs update -p "$i" >$i.out &&
+	test_cmp $i.out ../$i || return 1
+    done
 '
 
 cd "$WORKDIR"

2.16.2.603.g180c1428f0

Indeed, it does.

OK, no need to resend, as I'll amend it locally before queuing (unless there is something else that needs updating, that is).

Thanks.

Thanks.

That 'rm' was unnecessary to begin with, because none of the previous tests wrote and left behind a file 'failures', ever. Though one could argue that the original author was careful and added that 'rm failures' as a protection against future changes to previous tests... Dunno, we are talking about a test that checked the stderr of 'test_cmp', so anything is possible :)

Four 'cvs diff' related tests in 't9402-git-cvsserver-refs.sh' fail when the test script is run with '-x' tracing (and using a shell other than a Bash version supporting BASH_XTRACEFD). The reason for those failures is that the tests check the emptiness of a subshell's stderr, which includes the trace of commands executed in that subshell as well, throwing off the emptiness check.

Save the stdout and stderr of the invoked 'cvs' command instead of the whole subshell, so the latter remains free from tracing output. (Note that changing how stdout is saved is only done for the sake of consistency, it's not necessary for correctness.)

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

---
 t/t9402-git-cvsserver-refs.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 6d2d3c8739..cf31ace667 100755
@@ -455,20 +455,20 @@ test_expect_success 'cvs up -r $(git rev-parse v1)' '
 '
 
 test_expect_success 'cvs diff -r v1 -u' '
-	( cd cvswork && cvs -f diff -r v1 -u ) >cvsDiff.out 2>cvs.log &&
+	( cd cvswork && cvs -f diff -r v1 -u >../cvsDiff.out 2>../cvs.log ) &&
 	test_must_be_empty cvsDiff.out &&
 	test_must_be_empty cvs.log
 '
 
 test_expect_success 'cvs diff -N -r v2 -u' '
-	( cd cvswork && ! cvs -f diff -N -r v2 -u ) >cvsDiff.out 2>cvs.log &&
+	( cd cvswork && ! cvs -f diff -N -r v2 -u >../cvsDiff.out 2>../cvs.log ) &&
 	test_must_be_empty cvs.log &&
 	test -s cvsDiff.out &&
 	check_diff cvsDiff.out v2 v1 >check_diff.out 2>&1
 '
 
 test_expect_success 'cvs diff -N -r v2 -r v1.2' '
-	( cd cvswork && ! cvs -f diff -N -r v2 -r v1.2 -u ) >cvsDiff.out 2>cvs.log &&
+	( cd cvswork && ! cvs -f diff -N -r v2 -r v1.2 -u >../cvsDiff.out 2>../cvs.log ) &&
 	test_must_be_empty cvs.log &&
 	test -s cvsDiff.out &&
 	check_diff cvsDiff.out v2 v1.2 >check_diff.out 2>&1

@@ -487,7 +487,7 @@ test_expect_success 'apply early [cvswork3] diff to b3' '
 '
 
 test_expect_success 'check [cvswork3] diff' '
-	( cd cvswork3 && ! cvs -f diff -N -u ) >"$WORKDIR/cvsDiff.out" 2>cvs.log &&
+	( cd cvswork3 && ! cvs -f diff -N -u >"$WORKDIR/cvsDiff.out" 2>../cvs.log ) &&
 	test_must_be_empty cvs.log &&
 	test -s cvsDiff.out &&
 	test $(grep Index: cvsDiff.out | wc -l) = 3 &&

2.16.2.603.g180c1428f0

You know I would never complain about a long commit message. :)

Both patches look OK to me. My only comment on the first one was "you should just use 'return'", but I see Eric beat me to it.

I do think the postimage of the second one is a little less readable. That's not a big deal to me, but I'm pretty sure I would intentionally write it the "original" way if I found myself in a similar situation. Which makes me wonder whether we'll end up accidentally re-introducing these kinds of "-x" failures. But as I said before, I'm willing to wait and see.

-Peff