Re: [PATCH v4 02/13] pack-revindex: prepare for incremental MIDX bitmaps

[Taylor Blau <me@ttaylorr.com>]

On Mon, Mar 17, 2025 at 09:27:26PM -0400, Jeff King wrote:
> On Fri, Mar 14, 2025 at 04:18:24PM -0400, Taylor Blau wrote:
>
> > diff --git a/pack-bitmap.c b/pack-bitmap.c
> > index 6406953d32..c26d85b5db 100644
> > --- a/pack-bitmap.c
> > +++ b/pack-bitmap.c
> > @@ -170,6 +170,15 @@ static struct ewah_bitmap *read_bitmap_1(struct bitmap_index *index)
> >  	return read_bitmap(index->map, index->map_size, &index->map_pos);
> >  }
> >
> > +static uint32_t bitmap_non_extended_bits(struct bitmap_index *index)
> > +{
> > +	if (index->midx) {
> > +		struct multi_pack_index *m = index->midx;
> > +		return m->num_objects + m->num_objects_in_base;
> > +	}
> > +	return index->pack->num_objects;
> > +}
>
> I understand why we need to account for the objects in the base to
> offset our total size.
>
> Similar to Patrick's comments on v3, I wondered about why we couldn't
> just modify bitmap_num_objects() here, and why some callers would be
> left with the other.
>
> I guess sometimes we still need to consider a single layer. We can't
> quite just access m->num_objects there, because we still need the midx
> vs pack abstraction layer. I just thought there'd be more discussion
> here, but it looks the same as v3.

Right; some callers care about the number of objects in *their* layer,
like computing the size of some bitmap extensions, bounds-checking
pseudo-merge commit lookups, or generating positions for objects in the
extended index.

I'm happy to include that discussion somewhere in the commit message or
as a comment nearby bitmap_non_extended_bits(), but I'm not sure which
is better. If you have thoughts, LMK.

> I wonder if it is worth renaming bitmap_num_objects() to indicate that
> it is a single layer (and make sure other callers are examined). I
> dunno.
>
> I also suspect from previous forays into bitmap indexing that it will be
> easy to mix up positions in various units (local to the layer vs in the
> global pseudo-pack ordering, for example). In theory we could use types
> to help us with this, but they're kind of weak in C (unless we wrap all
> of the ints in structs). Maybe not worth it.

Perhaps. I do like the idea of using types to help with all of this, but
like you I suspect they may be more trouble than they're worth.

Thanks,
Taylor