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

From: Jeff King <peff@peff.net>
Date: Oct 11, 2024, 08:16 UTC
Message-ID: <20241011081615.GE18010@coredump.intra.peff.net>
In-Reply-To: <94e5c96f6859479e0206d6d775eacf54b3639ee5.1728505840.git.me@ttaylorr.com>

On Wed, Oct 09, 2024 at 04:31:21PM -0400, Taylor Blau wrote:

Show 17 quoted lines

> +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:

Show 9 quoted lines

> +	} 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