On Wed, Jan 10, 2024 at 12:34:09PM -0500, Taylor Blau wrote: > > 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
