Re: [PATCH v2 04/19] midx: teach `prepare_midx_pack()` about incremental MIDXs

From: Jeff King <peff@peff.net>
Date: Aug 1, 2024, 09:35 UTC
Message-ID: <20240801093550.GD1159276@coredump.intra.peff.net>
In-Reply-To: <f88569c819292a824c78cdffd4e1fbc329f07f8e.1721250704.git.me@ttaylorr.com>

On Wed, Jul 17, 2024 at 05:12:07PM -0400, Taylor Blau wrote:

Show 16 quoted lines

> The function `prepare_midx_pack()` is part of the midx.h API and
> loads the pack identified by the MIDX-local 'pack_int_id'. This patch
> prepares that function to be aware of an incremental MIDX world.
> 
> To do this, introduce the second of the two general purpose helpers
> mentioned in the previous commit. This commit introduces
> `midx_for_pack()`, which is the pack-specific analog of
> `midx_for_object()`, and works in the same fashion.
> 
> Like `midx_for_object()`, this function chases down the '->base_midx'
> field until it finds the MIDX layer within the chain that contains the
> given pack.
> 
> Use this function within `prepare_midx_pack()` so that the `pack_int_id`
> it expects is now relative to the entire MIDX chain, and that it
> prepares the given pack in the appropriate MIDX.

OK, I'm adequately prepared for more global/local confusion. :)

Show 9 quoted lines

> -int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id)
> +static uint32_t midx_for_pack(struct multi_pack_index **_m,
> +			      uint32_t pack_int_id)
>  {
> -	struct strbuf pack_name = STRBUF_INIT;
> -	struct packed_git *p;
> +	struct multi_pack_index *m = *_m;
> +	while (m && pack_int_id < m->num_packs_in_base)
> +		m = m->base_midx;

OK, so we chase down the pack id as before...

Show 7 quoted lines

> +	if (!m)
> +		BUG("NULL multi-pack-index for pack ID: %"PRIu32, pack_int_id);
> +
> +	if (pack_int_id >= m->num_packs + m->num_packs_in_base)
>  		die(_("bad pack-int-id: %u (%u total packs)"),
> -		    pack_int_id, m->num_packs);
> +		    pack_int_id, m->num_packs + m->num_packs_in_base);

...with the same sanity checks...

> +	*_m = m;
> +
> +	return pack_int_id - m->num_packs_in_base;

...and the same global to local offset conversion. Looks good so far.

Show 6 quoted lines

> +int prepare_midx_pack(struct repository *r, struct multi_pack_index *m,
> +		      uint32_t pack_int_id)
> +{
> +	struct strbuf pack_name = STRBUF_INIT;
> +	struct packed_git *p;
> +	uint32_t local_pack_int_id = midx_for_pack(&m, pack_int_id);

This one uses a separate variable with the word "local" in it. Helpful. :)

Show 6 quoted lines

> +	if (m->packs[local_pack_int_id])
>  		return 0;
>  
>  	strbuf_addf(&pack_name, "%s/pack/%s", m->object_dir,
> -		    m->pack_names[pack_int_id]);
> +		    m->pack_names[local_pack_int_id]);

OK, and then this is just existing lazy-load of the pack struct. Good.

I guess if you just reused pack_int_id for the local id, the diff would be much smaller (this part would remain exactly the same). I dunno which is better, but it was a little curious that the two patches differed in approach. Probably not worth caring too much about, though.

-Peff