Re: [PATCH 2/2] pack-objects: only perform verbatim reuse on the preferred pack

From: Jeff King <peff@peff.net>
Date: Nov 14, 2024, 00:25 UTC
Message-ID: <20241114002504.GB1140565@coredump.intra.peff.net>
In-Reply-To: <2520abf24a8a194b3f7040e218f878dc88a740a0.1731518931.git.me@ttaylorr.com>

On Wed, Nov 13, 2024 at 12:32:58PM -0500, Taylor Blau wrote:

Show 5 quoted lines

> Instead, we can only safely perform the whole-word reuse optimization on
> the preferred pack, where we know with certainty that no gaps exist in
> that region of the bitmap. We can still reuse objects from non-preferred
> packs, but we have to inspect them individually in write_reused_pack()
> to ensure that any gaps that may exist are accounted for.

Yep. With the disclaimer that I'm biased because I helped a little with debugging, this change is obviously correct. Forgetting the bug you saw in the real world, we know this function cannot work as-is because of the potential for those gaps.

Show 9 quoted lines

> This allows us to simplify the implementation of
> write_reused_pack_verbatim() back to almost its pre-multi-pack reuse
> form, since we can now assume that the beginning of the pack appears at
> the beginning of the bitmap, meaning that we don't have to account for
> any bits up to the first word boundary (like we had to special case in
> ca0fd69e37).
> 
> The only significant changes from the pre-ca0fd69e37 implementation are:
> [...]

Thanks for this section. My first instinct was to go back and look at the diff to the pre-midx version of the function, and this nicely explains the hunks I see there.

So this patch looks good to me. I was able to follow your explanation in the commit message, but that may not count for much since I'm probably the only other person with deep knowledge of the verbatim-reuse code in the first place. ;)

I do think the explanation in the message of the first commit would be a lot simpler if it were simply combined into this patch. With them split you effectively have to explain the problem twice. I don't feel that strongly about changing it, though.

-Peff