Re: [PATCH v2 4/5] repack: use tempfiles for signal cleanup
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 23, 2022, 00:14 UTC
- Message-ID
- <xmqqtu3vflau.fsf@gitster.g>
- In-Reply-To
- <Y1RUI8ny2mexxwKm@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 7 quoted lines
> Ugh, I think this test is racy. I saw a failure via SIGPIPE on OS X in > CI, and running it locally with --stress confirmed. I think the problem > is in our method to trigger pack-objects to fail for the cruft pack. We > pass a bogus command line argument, so pack-objects exits more or less > immediately. But the parent git-repack process is trying to write to its > stdin (to name packs to keep/exclude). So that write may succeed or fail > based on how quickly the child dies.
Yeah, I saw flaky failures myself during my integration tests.
Show 18 quoted lines
> Some options: > > - find a different way to convince repack to die. The most likely is > probably corrupting the cruft objects in some way such that > pack-objects dies, but not until it starts doing real work. > > - mark the test_must_fail with ok=sigpipe. In most case this is a > band-aid, but here we still test what we want. The .tmp cleanup > should kick in from a die() or from a signal death, so either is > sufficient for our purposes. > > - teach git-repack to ignore sigpipe. We don't actually check the > writes to pack-objects (since they're done by fprintf), but we'd > notice its failing exit code. And arguably this is improving the > user experience. Saying "pack-objects died with an error" is more > useful than a sigpipe. > > Thoughts?
I agree that for this particular one the second one is a reasonable thing to do in the context of the test. The third one may actually be a better fix, exactly for the reason you state here.
Thanks.