Re: [PATCH 01/12] t: prepare `test_match_signal ()` calls for `set -e`

[Patrick Steinhardt <ps@pks.im>]

On Mon, Apr 13, 2026 at 09:26:13AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > We have a couple of calls to `test_match_signal ()` where we execute a
> > Git command and expect it to die with a specific signal. These calls
> > will essentially execute the process in a subshell via `foo; echo $?`,
> > but as we expect `foo` to fail this will cause the overall subshell to
> > fail once we `set -e`.
> >
> > Fix this issue by using `foo || echo $?` instead.
> 
> Hmph, if the 'foo' command that is expected to fail succeeds by a
> bug, we won't see 0 on the standard output anymore.  Comparing 13
> with 0 or 13 with an empty string "" would fail either way, so that
> may not give us a practical difference, but this somehow leaves a
> foul small in my nose.

Hm, true, it is a wee bit ugly.

> The technique used in <20260325062114.2067946-2-gitster@pobox.com>
> would give us something ugly like
> 
> -	OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
> +	OUT=$( ((st=0; large_git || st=$?; echo $st 1>&3) | :) 3>&1 ) &&
> 
> and it ensures that we give 0 when we succeed, but I am not sure if
> this is worth doing.
> 
> Or perhaps
> 
> -	OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
> +	OUT=$( ((large_git || echo $? 1>&3) | :) 3>&1 ) &&
> +	OUT=${OUT:-0} &&
> 
> I dunno.
> 
> If "large_git" somehow writes into file descriptor #3, then OUT will
> be contaminated with something other than $?, so the additional "if
> empty substitute with 0" would not work without doing something like
> 
> -	OUT=$( ((large_git; echo $? 1>&3) | :) 3>&1 ) &&
> +	OUT=$( ((large_git 3>/dev/null || echo $? 1>&3) | :) 3>&1 ) &&
> +	OUT=${OUT:-0} &&
> 
> I guess.  For that matter, if large_git spits out "13" to its file
> descriptor #3, I do not know what would have happened ;-)

How about the below patch? It's a mouthful, but the intent is quite
explicit and it's overall not too bad, if you ask me.

Patrick

diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh
index 74c2a27972..84319cf169 100755
--- a/t/t0005-signals.sh
+++ b/t/t0005-signals.sh
@@ -42,12 +42,12 @@ test_expect_success 'create blob' '
 '
 
 test_expect_success !MINGW 'a constipated git dies with SIGPIPE' '
-	OUT=$( ((large_git || echo $? 1>&3) | :) 3>&1 ) &&
+	OUT=$( ((large_git && echo 0 1>&3 || 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' '
-	OUT=$( ((trap "" PIPE && large_git || echo $? 1>&3) | :) 3>&1 ) &&
+	OUT=$( ((trap "" PIPE && large_git && echo 0 1>&3 || echo $? 1>&3) | :) 3>&1 ) &&
 	test_match_signal 13 "$OUT"
 '
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 8e68a00dcb..a371ea690e 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -260,7 +260,7 @@ test_expect_success 'choking "git rm" should not let it die with cruft (induce S
 
 test_expect_success !MINGW 'choking "git rm" should not let it die with cruft (induce and check SIGPIPE)' '
 	choke_git_rm_setup &&
-	OUT=$( ((trap "" PIPE && git rm -n "some-file-*" || echo $? 1>&3) | :) 3>&1 ) &&
+	OUT=$( ((trap "" PIPE && git rm -n "some-file-*" && echo 0 1>&3 || echo $? 1>&3) | :) 3>&1 ) &&
 	test_match_signal 13 "$OUT" &&
 	test_path_is_missing .git/index.lock
 '