Re: [PATCH v2 11/11] tests: add a "set -o pipefail" for a patched bash

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

-test_expect_success !MINGW 'a constipated git dies with SIGPIPE' '
+test_expect_success !MINGW,!BASH_SET_O_PIPEFAIL 'a constipated git dies with SIGPIPE' '
 	OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
 	test_match_signal 13 "$OUT"
 '
 
-test_expect_success !MINGW 'a constipated git dies with SIGPIPE even if parent ignores it' '
+test_expect_success !MINGW,!BASH_SET_O_PIPEFAIL 'a constipated git dies with SIGPIPE even if parent ignores it' '
 	OUT=$( ((trap "" PIPE; large_git; echo $? 1>&3) | :) 3>&1 ) &&
 	test_match_signal 13 "$OUT"
 '

The above have already !MINGW

but this one does not.  Yet, we've been using test_match_signal on 13
without issues, it appears.

And somehow with the lazy prereq on SET_O_PIPEFAIL, this part starts
to break, like so:

  https://github.com/git/git/runs/1752687552?check_suite_focus=true#step:7:37042

The output captured in OUT is 0 as we can see on #37032 in the test
log.

I am tempted to eject 11/11 and probably 10/11 out of the topic, as
the earlier patches before them look more or less uncontroversial
cleanups, and 11/11 seems to be more trouble than it is worth at
this moment.

It's not like this would allow us to loosen the rule that we
shouldn't put a "git" invocation of the git subcommand being tested
on the upstream side of a pipe---not everybody is running bash, and
it is unrealistic to tell our developers "if you want to make sure
your tests are good, you must install and use this patched bash".

Thanks.

On Sat, Jan 23 2021, Junio C Hamano wrote:

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

quoted

-test_expect_success !MINGW 'a constipated git dies with SIGPIPE' '
+test_expect_success !MINGW,!BASH_SET_O_PIPEFAIL 'a constipated git dies with SIGPIPE' '
 	OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
 	test_match_signal 13 "$OUT"
 '
 
-test_expect_success !MINGW 'a constipated git dies with SIGPIPE even if parent ignores it' '
+test_expect_success !MINGW,!BASH_SET_O_PIPEFAIL 'a constipated git dies with SIGPIPE even if parent ignores it' '
 	OUT=$( ((trap "" PIPE; large_git; echo $? 1>&3) | :) 3>&1 ) &&
 	test_match_signal 13 "$OUT"
 '

The above have already !MINGW

quoted

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 4f7e62d05c..7b5d92add5 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh

@@ -251,7 +251,10 @@ test_expect_success 'choking "git rm" should not let it die with cruft' '
 		i=$(( $i + 1 ))
 	done | git update-index --index-info &&
 	OUT=$( ((trap "" PIPE; git rm -n "some-file-*"; echo $? 1>&3) | :) 3>&1 ) &&
-	test_match_signal 13 "$OUT" &&
+	if ! test_have_prereq BASH_SET_O_PIPEFAIL
+	then
+		test_match_signal 13 "$OUT"
+	fi &&
 	test_path_is_missing .git/index.lock
 '

but this one does not.  Yet, we've been using test_match_signal on 13
without issues, it appears.

And somehow with the lazy prereq on SET_O_PIPEFAIL, this part starts
to break, like so:

  https://github.com/git/git/runs/1752687552?check_suite_focus=true#step:7:37042

The output captured in OUT is 0 as we can see on #37032 in the test
log.

I am tempted to eject 11/11 and probably 10/11 out of the topic, as
the earlier patches before them look more or less uncontroversial
cleanups, and 11/11 seems to be more trouble than it is worth at
this moment.

I think that makes sense, once I fix the breakage on 07/11 you noted
downthread.

It's not like this would allow us to loosen the rule that we
shouldn't put a "git" invocation of the git subcommand being tested
on the upstream side of a pipe[...]

FWIW it seems from my off-list discussion with the bash maintainer that
no version of my patch is likely to make it into bash. He views it as a
feature that "pipefail" treats all non-zero exit codes equally.

But as it pertains to our test suite I mainly wrote this to check if we
had any failures that didn't make sense once SIGPIPE was ignored. I
think smoking out any potential historical cases (and finding we didn't
have any that mattered) was probably an effort worth it in itself. Then
we just have to continue not putting git on the LHS on a pipe for new
tests.

[...]---not everybody is running bash, and it is unrealistic to tell
our developers "if you want to make sure your tests are good, you must
install and use this patched bash".

I think if others think this was worth keeping and bash never accepted
the patches we could rather easily get most of the benefit from it by
having a CI job that ran with such a patched bash. To test with "set -o
pipefail" it's enough that it's done semi-regularly by someone if you
want to smoke out bugs in the tests, not everyone has to do it all the
time.