On Fri, Oct 11, 2024 at 04:16:15AM -0400, Jeff King wrote:
> 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)?
Great catch.
We indeed lost that optimization when converting "if
(reuse_packfile->bitmap_pos)" to "if (bitmap_is_midx(bitmap_git))".
Let's restore that by keeping the conditional unchanged, which:
- makes the BUG() make sense as written, and
- preserves the optimization where the first pack in a MIDX can be
treated the same as if it came from a single-pack bitmap, and
bypass the bit position translations.
Thanks for spotting.
Thanks,
Taylor
