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

From: Junio C Hamano <[email-masked]>
Date: Fri, 22 Jan 2021 23:37:04 -0800

Junio C Hamano <[email-masked]> writes:

> ...
> The above have already !MINGW
>
>> 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.

Nah, it seems that t3600-rm's "match signal 13" is already broken
without O_PIPEFAIL patch on Windows.  For example:

https://github.com/git/git/runs/1753231308?check_suite_focus=true#step:7:36912

This was introduced by c15ffae5 (rm tests: actually test for SIGPIPE
in SIGPIPE test, 2021-01-16) in the same series.

I am not sure "actually testing for SIGPIPE" is more important than
"make sure 'git rm' choked should not die with cruft", so without
thinking too deeply about the issue, my gut reaction is that
reverting is better than using !MINGW as other tests.  That is, no
matter how "git rm" gets killed, it should not leave .git/index.lock
behind, and the original already tests that.  The patch tried to
make sure it dies with signal #13 (and fails the test on Windows)
before it even looks at the leftover index.lock file, which feels a
bit backwards.