| Message ID | f88569c819292a824c78cdffd4e1fbc329f07f8e.1721250704.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | midx: incremental multi-pack indexes, part one | expand |
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
On Thu, Aug 01, 2024 at 05:35:50AM -0400, Jeff King wrote: > OK, I'm adequately prepared for more global/local confusion. :) Good, since there is definitely more where that came from ;-). > 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. I meandered a lot about different approaches before I arrived at what became midx_for_pack() and midx_for_object(). So I think declaring a new local_pack_int_id was a relic from when perhaps the function returned void and expected to have a uint32_t* to write the translated pack ID to. Later translations make use of the: pack_int_id = midx_for_pack(&m, pack_int_id); pattern when they do not care about the global pack ID, and this one should as well. I'll update the patch to do that. Thanks, Taylor
diff --git a/midx.c b/midx.c index 39d358da20..da5e0bb940 100644 --- a/midx.c +++ b/midx.c @@ -259,20 +259,37 @@ static uint32_t midx_for_object(struct multi_pack_index **_m, uint32_t pos) return pos - m->num_objects_in_base; } -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; - if (pack_int_id >= m->num_packs) + 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); - if (m->packs[pack_int_id]) + *_m = m; + + return pack_int_id - m->num_packs_in_base; +} + +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); + + 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]); p = add_packed_git(pack_name.buf, pack_name.len, m->local); strbuf_release(&pack_name); @@ -281,7 +298,7 @@ int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t return 1; p->multi_pack_index = 1; - m->packs[pack_int_id] = p; + m->packs[local_pack_int_id] = p; install_packed_git(r, p); list_add_tail(&p->mru, &r->objects->packed_git_mru);
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. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- midx.c | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-)