Re: [PATCH v3 03/13] pack-bitmap.c: open and store incremental bitmap layers

[Patrick Steinhardt <ps@pks.im>]

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