Re: [PATCH v2 4/5] repack: use tempfiles for signal cleanup

From: Jeff King <peff@peff.net>
Date: Oct 22, 2022, 20:35 UTC
Message-ID: <Y1RUI8ny2mexxwKm@coredump.intra.peff.net>
In-Reply-To: <Y1M3oie5dPrRaOni@coredump.intra.peff.net>

On Fri, Oct 21, 2022 at 08:21:54PM -0400, Jeff King wrote:

Show 7 quoted lines

> +test_expect_success 'clean up .tmp-* packs on error' '
> +	test_must_fail git \
> +		-c repack.cruftwindow=bogus \
> +		repack -ad --cruft &&
> +	find $objdir/pack -name '.tmp-*' >tmpfiles &&
> +	test_must_be_empty tmpfiles
> +'

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.

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?

-Peff