test-lib: some shells do not let $? propagate into an eval

In 3bf7886 (test-lib: Let tests specify commands to be run at end of
test, 2010-05-02), the git test harness learned to run cleanup
commands unconditionally at the end of a test.  During each test,
the intended cleanup actions are collected in the test_cleanup variable
and evaluated.  That variable looks something like this:

	eval_ret=$?; clean_something && (exit "$eval_ret")
	eval_ret=$?; clean_something_else && (exit "$eval_ret")
	eval_ret=$?; final_cleanup && (exit "$eval_ret")
	eval_ret=$?

All cleanup actions are run unconditionally but if one of them fails
it is properly reported through $eval_ret.

On FreeBSD, unfortunately, $? is set at the beginning of an ‘eval’
to 0 instead of the exit status of the previous command.  This results
in tests using test_expect_code appearing to fail and all others
appearing to pass, unless their cleanup fails.  Avoid the problem by
setting eval_ret before the ‘eval’ begins.

Thanks to Jeff King for the explanation.

Cc: Jeff King <peff@peff.net>
Cc: Johannes Sixt <j6t@kdbg.org>
Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

2 files changed, 25 insertions, 3 deletions

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index f4ca4fc85c..3ec9cbef2c 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -73,6 +73,27 @@ then
 	exit 1
 fi
 
+clean=no
+test_expect_success 'tests clean up after themselves' '
+    test_when_finished clean=yes
+'
+
+cleaner=no
+test_expect_code 1 'tests clean up even after a failure' '
+    test_when_finished cleaner=yes &&
+    (exit 1)
+'
+
+if test $clean$cleaner != yesyes
+then
+	say "bug in test framework: cleanup commands do not work reliably"
+	exit 1
+fi
+
+test_expect_code 2 'failure to clean up causes the test to fail' '
+    test_when_finished "(exit 2)"
+'
+
 ################################################################
 # Basics of the basics
 
diff --git a/t/test-lib.sh b/t/test-lib.sh
index acce3d06a8..7422bba47e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -366,8 +366,9 @@ test_debug () {
 }
 
 test_run_ () {
-	test_cleanup='eval_ret=$?'
+	test_cleanup=:
 	eval >&3 2>&4 "$1"
+	eval_ret=$?
 	eval >&3 2>&4 "$test_cleanup"
 	return 0
 }
@@ -567,8 +568,8 @@ test_cmp() {
 # the test to pass.
 
 test_when_finished () {
-	test_cleanup="eval_ret=\$?; { $*
-		} && (exit \"\$eval_ret\"); $test_cleanup"
+	test_cleanup="{ $*
+		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_cleanup"
 }
 
 # Most tests can use the created repository, but some may need to create more.