Re: [PATCH v2 13/19] midx: teach `midx_preferred_pack()` about incremental MIDXs
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Aug 1, 2024, 20:05 UTC
- Message-ID
- <ZqvqkHph6YYyEHy2@nand.local>
- In-Reply-To
- <20240801102517.GM1159276@coredump.intra.peff.net>
On Thu, Aug 01, 2024 at 06:25:17AM -0400, Jeff King wrote:
Show 29 quoted lines
> > diff --git a/midx.c b/midx.c
> > index 0fa8febb9d..d2dbea41e4 100644
> > --- a/midx.c
> > +++ b/midx.c
> > @@ -500,13 +500,16 @@ int midx_contains_pack(struct multi_pack_index *m, const char *idx_or_pack_name)
> > int midx_preferred_pack(struct multi_pack_index *m, uint32_t *pack_int_id)
> > {
> > if (m->preferred_pack_idx == -1) {
> > + uint32_t midx_pos;
> > if (load_midx_revindex(m) < 0) {
> > m->preferred_pack_idx = -2;
> > return -1;
> > }
> >
> > - m->preferred_pack_idx =
> > - nth_midxed_pack_int_id(m, pack_pos_to_midx(m, 0));
> > + midx_pos = pack_pos_to_midx(m, m->num_objects_in_base);
> > +
> > + m->preferred_pack_idx = nth_midxed_pack_int_id(m, midx_pos);
> > +
>
> OK, so rather than looking for the pack of object 0, we're looking for
> the first one in _this_ layer, since the position is global within the
> midx. That makes some sense, but is pack_pos_to_midx() ready for that?
> It looks like it just looks at m->revindex_data. Are we going to be
> generating a revindex for the whole chain? I'd think that each layer
> would have its own revindex (and any trickiness would happen at the
> generation stage, making sure we don't insert objects that are already
> mentioned in earlier layers).pack_pos_to_midx() is kind of ready, and kind of not.
The way that the pseudo-pack order is constructed within the midx-write.c code, we will write reverse indexes (within each MIDX layer itself as a separate chunk) that contain data for each object within that layer in the expected reverse index format.
But we don't bother writing any reverse indexes for MIDXs which are incremental at this point in the multi-series plan, since we just bail if the BITMAP flag is set (saying that it is unsupported at this point).
Arguably we could have just left this hunk / patch out of the series as a whole. It's this kind of stuff that's really at the boundary between adjacent "phases" that I think is awkward no matter which way you slice it.
> -Peff
Thanks, Taylor