On Wed, Jul 17, 2024 at 05:12:07PM -0400, Taylor Blau wrote:
> 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. :)
> -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...
> + 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.
> +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. :)
> + 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
