Re: [PATCH] index-pack: spawn threads atomically

From: Jeff King <peff@peff.net>
Date: Jan 11, 2024, 06:53 UTC
Message-ID: <20240111065338.GA48154@coredump.intra.peff.net>
In-Reply-To: <ZZ7VEVXSg1T8ZkIK@nand.local>

On Wed, Jan 10, 2024 at 12:34:09PM -0500, Taylor Blau wrote:

Show 14 quoted lines

> > If do go with "--threads=1", I suspect several tests in that file need
> > it.
> 
> Yeah, there are a couple of others. I think the ones that need modifying
> are at the intersection of "expected to fail" and "in a test which is
> expected to pass leak-free":
> 
>     $ grep -l 'TEST_PASSES_SANITIZE_LEAK=true' t????-*.sh |
>       xargs grep -l 'test_must_fail git index-pack'
>     t5302-pack-index.sh
>     t5308-pack-detect-duplicates.sh
>     t5309-pack-delta-cycles.sh
>     t5313-pack-bounds-checks.sh
>     t5325-reverse-index.sh

I think that is more than we need. It's only a problem when we hit a die() inside a thread, which happens only during delta resolution. So your patch 2, for example, touches a test which triggers the --max-input-size check. But we would find that out on the initial unthreaded pass over the pack.

The one in patch 3 seems at first glance like it might be a problem (it's another duplicate-object case, like the one of them in 5309). But it isn't a problem because the duplicate object isn't a delta, so we notice the problem in write_idx_file() from the main thread (which I verified by running it under gdb and setting a breakpoint at die()).

I suspect patch 4 is the same, but didn't run gdb on each case. And patch 5 is about a corrupt reverse index, so almost certainly the main thread. So I suspect that patch 1 is the only one that matters here (and probably all of those are needed, because it is all about broken deltas).

All that said, I am on the fence between the two approaches. If Junio prefers the atomic-spawn direction, I'm fine with that, and there's not much point in polishing the --threads=1 approach further.

-Peff