On Wed, Oct 09, 2024 at 04:31:21PM -0400, Taylor Blau wrote:
> +static uint32_t bitmap_to_pack_pos(struct packed_git *reuse_packfile,
> + size_t pos)
> +{
> + if (bitmap_is_midx(bitmap_git)) {
> + /*
> + * When doing multi-pack reuse on a
> + * non-preferred pack, translate bit positions
> + * from the MIDX pseudo-pack order back to their
> + * pack-relative positions before attempting
> + * reuse.
> + */
> + struct multi_pack_index *m = bitmap_midx(bitmap_git);
> + uint32_t midx_pos, pack_pos;
> + off_t pack_ofs;
> +
> + if (!m)
> + BUG("non-zero bitmap position without MIDX");
The text of this BUG() seems weird: we haven't asserted a non-zero
bitmap position. We're really only checking that bitmap_is_midx() and
bitmap_midx() agree that there is a midx. I was going to suggest that
the former could be implemented with a NULL-check on the latter, but
really, that is already how it works (except that it accesses
bitmap_index->midx directly).
So yes, it truly would be a surprising BUG() to see them disagree. :)
I do not mind keeping the BUG() there if you want to be extra careful,
but I just found the message text confusing.
Ah...hmm. This is all being copied from the earlier function. So I think
the culprit may be patch 6, which swaps:
if (reuse_packfile->bitmap_pos)
for:
if (bitmap_is_midx(bitmap_git))
which is what makes the BUG() text confusing. But then, what about this:
> + } else {
> + /*
> + * Can use bit positions directly, even for MIDX
> + * bitmaps. See comment in try_partial_reuse()
> + * for why.
> + */
> + return pos;
> + }
> +}
This "even for MIDX" is not really accurate, as we know this else block
is for the non-midx case. Are we losing the optimization that the first
pack in the midx can be treated the same as the single-pack case (we
know that its pack positions and the start of the midx bit positions are
identical, which is what the comment it mentions explains)?
-Peff
