On Sat, Jan 23 2021, Junio C Hamano wrote: > Junio C Hamano <gitster@xxxxxxxxx> 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. Yes, not adding !MINGW here is a stupid oversight on my part, I can re-roll with that added, which seems to be like it'll work & be better. I.e. we'll actually test for SIGPIPE ...(read on).... > 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. I don't get it. I understand why we'd do any of: 1. Keep my patch with !MINGW added. I.e. the intent of your 0693f9ddad (Make sure lockfiles are unlocked when dying on SIGPIPE, 2008-12-18) which added the test is to explicitly stress SIGPIPE, but we never actually checked it explicitly... 2. Just remove/rewrite that part of the test. We have 7559a1be8a (unblock and unignore SIGPIPE, 2014-09-18) (the other test whose pattern I copied) now. That along with 12e0437f23 (common-main: call restore_sigpipe_to_default(), 2016-07-01) means we do this everywhere, so why test "git rm" in particular in this one place but not other git commands? 3. Remove the overly specific PIPE test added in 7559a1be8a in favor of this "git rm" test. After all if we want to test the SIGPIPE pattern but sometimes we get SIGPIPE, sometimes we don't (MINGW), but we don't really care because we assume on some platforms it's being tested. But not why we'd keep the test as-is now that we've dug up this old code and found that since it got added we have a reliable way to test for actually-sigpipe. Just to maintain the coverage on MINGW? Wouldn't it be better to have two tests then, one without the prereq to run everywhere, and another identical one with the "trap" on !MINGW? I don't really care and can re-roll in whatever way you prefer, I just don't understand what I'd put as a reason in the commit message(s), depending on which route we go...
