On Fri, Feb 28, 2025 at 06:49:03PM -0500, Taylor Blau wrote:
> On Fri, Feb 28, 2025 at 11:01:16AM +0100, Patrick Steinhardt wrote:
> > On Tue, Nov 19, 2024 at 05:07:26PM -0500, Taylor Blau wrote:
> > > Prepare the pack-bitmap machinery to work with incremental MIDXs by
> > > adding a new "base" field to keep track of the bitmap index associated
> > > with the previous MIDX layer.
> > >
> > > The changes in this commit are mostly boilerplate to open the correct
> > > bitmap(s), add them to the chain bitmap layers along the "base" pointer,
> >
> > s/bitmap layers/of &/
> >
> > > diff --git a/pack-bitmap.c b/pack-bitmap.c
> > > index bba9c6a905a..41675a69f68 100644
> > > --- a/pack-bitmap.c
> > > +++ b/pack-bitmap.c
> > > @@ -54,6 +54,13 @@ struct bitmap_index {
> > > struct packed_git *pack;
> > > struct multi_pack_index *midx;
> > >
> > > + /*
> > > + * If using a multi-pack index chain, 'base' points to the
> > > + * bitmap index corresponding to this bitmap's midx->base_midx.
> > > + */
> > > + struct bitmap_index *base;
> > > + uint32_t base_nr;
> > > +
> >
> > It would be nice to point out that `base_nr` is not 0-indexed, but
> > 1-indexed, which is rather uncommon. Is there any particular reason why
> > you made it 1-indexed?
>
> Hah, I have no idea! If I remember correctly, it's because it makes it
> (slightly) more convenient to do:
>
> ewah_or_iterator_init(it, bitmap_git->commits_all,
> bitmap_git->base_nr);
>
> , instead of incrementing 'base_nr' by 1 to determine the number of
> sub-iterators to allocate.
>
> So I think there are a couple of options here. Short of doing nothing,
> we could:
>
> 1. Rename 'base_nr' to 'layers_nr' which would make it clearer that the
> count includes the current layer, thus making it 1-indexed.
>
> 2. Leave 'base_nr' named as-is, but make it 0-indexed, and have callers add
> 1 when they need to know the number of layers.
>
> I prefer the explicitness of (2), which is how I adjusted things
> locally. But if you prefer (1) or some yet-unknown (3), I'm happy to
> adjust it further!
Yup, I also favor (2) here as it is the least surprising option to me.
Patrick
