Re: [PATCH] builtin/repack.c: avoid making cruft packs preferred

From: Jeff King <peff@peff.net>
Date: Oct 3, 2023, 20:16 UTC
Message-ID: <20231003201617.GE1562@coredump.intra.peff.net>
In-Reply-To: <19d9aae08eab05c6b5dda4c2090236b1c3f62998.1696349955.git.me@ttaylorr.com>

On Tue, Oct 03, 2023 at 12:27:51PM -0400, Taylor Blau wrote:

Show 13 quoted lines

> Note that this behavior is usually just a performance regression. But
> it's possible it could be a correctness issue.
> 
> Suppose an object was duplicated among the cruft and non-cruft pack. The
> MIDX will pick the one from the pack with the lowest mtime, which will
> always be the cruft one. But if the non-cruft pack happens to sort
> earlier in lexical order, we'll treat that one as preferred, but not all
> duplicates will be resolved in favor of that pack.
> 
> So if we happened to have an object which appears in both packs
> (e.g., due to a cruft object being freshened, causing it to appear
> loose, and then repacking it via the `--geometric` repack) it's possible
> the duplicate would be picked from the non-preferred pack.

I'm not sure I understand how that is a correctness issue. The contents of the object are the same in either pack. Or do you mean that the pack-reuse code in pack-objects.c may get confused and try to use the wrong pack/offset when sending the object out? I would think it would always be coming from the preferred pack for that code (so the outcome is just that we fail to do the pack-reuse optimization very well, but we don't generate a wrong answer).

Other than that, the explanation and patch make perfect sense to me, and the patch looks good.

-Peff