On Sun, Feb 21, 2021 at 08:25:12PM +0100, SZEDER Gábor wrote:

> In many test helper functions we verify that they were invoked with
> sensible parameters, and call BUG() to abort the test script when the
> parameters are buggy.  6a67c75948 (test-lib-functions: restrict
> test_must_fail usage, 2020-07-07) added such a parameter verification
> to 'test_must_fail', but it didn't report the error with BUG(), like
> we usually do.

OK. I do not care all that much between BUG() and not-BUG here, since we
are unlikely to have a test where test_must_fail returning 0 yields
success. I guess the most interesting outcome is that we would notice a
bug in a test_expect_failure block.

> The two tests checking that 'test_must_fail' recognizes invalid
> parameters need some updates:
> 
>   - BUG() calls 'exit 1' to abort the test script, but we don't want
>     that to happen while testing 'test_must_fail' itself, so in those
>     tests we must invoke that function in a subshell.
>   - These tests check that 'test_must_fail' failed with the
>     appropriate error message, but BUG() sends its error message to a
>     different file descriptor, so update the redirection accordingly.

This is a bit intimate with the magic 7 descriptor. I think it would be
cleaner to trigger the bug in a sub-test. We do have helpers for that,
like:

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index b9d5c6c404..b3fd740452 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -1315,13 +1315,25 @@ test_expect_success 'test_must_fail on a failing git command with env' '
 '
 
 test_expect_success 'test_must_fail rejects a non-git command' '
-	! ( test_must_fail grep ^$ notafile ) 7>err &&
-	grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err
+	cmd="grep ^$ notafile" &&
+	run_sub_test_lib_test_err bug-fail-nongit "fail nongit" <<-EOF &&
+	test_expect_success "non-git command" "test_must_fail $cmd"
+	EOF
+	check_sub_test_lib_test_err bug-fail-nongit <<-\EOF_OUT 3<<-EOF_ERR
+	EOF_OUT
+	> error: bug in the test script: test_must_fail: only ${SQ}git${SQ} is allowed: $cmd
+	EOF_ERR
 '
 
 test_expect_success 'test_must_fail rejects a non-git command with env' '
-	! ( test_must_fail env var1=a var2=b grep ^$ notafile ) 7>err &&
-	grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err
+	cmd="env var1=a var2=b grep ^$ notafile" &&
+	run_sub_test_lib_test_err bug-fail-env "fail nongit with env" <<-EOF &&
+	test_expect_success "non-git command with env" "test_must_fail $cmd"
+	EOF
+	check_sub_test_lib_test_err bug-fail-env <<-\EOF_OUT 3<<-EOF_ERR
+	EOF_OUT
+	> error: bug in the test script: test_must_fail: only ${SQ}git${SQ} is allowed: $cmd
+	EOF_ERR
 '
 
 test_done

This is modeled after other similar tests. I find the use of
check_sub_test_lib_test_err here a bit verbose, but I think we could
also easily do:

  grep "bug in the test.*only .git. is allowed" bug-fail-nongit/err

Note that there are some other cases which could likewise be converted
(the one for test_bool_env, which I noticed when grepping for "7>" when
investigating the first patch).

>  test_expect_success 'test_must_fail rejects a non-git command' '
> -	! test_must_fail grep ^$ notafile 2>err &&
> +	! ( test_must_fail grep ^$ notafile ) 7>err &&
>  	grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err
>  '

Holy double-quoting batman! I do think using $SQ or just "." (if using
grep) to match single-quotes makes things more readable. Obviously not
something you're introducing, but perhaps worth addressing as we touch
this test.

-Peff