Re: [PATCH 07/11] write_reused_pack_one(): translate bit positions directly

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 4, 2024, 20:36 UTC
Message-ID: <ZykwMW3PIpsPjbwz@nand.local>
In-Reply-To: <20241011081615.GE18010@coredump.intra.peff.net>

On Fri, Oct 11, 2024 at 04:16:15AM -0400, Jeff King wrote:

Show 26 quoted lines

> 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