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

From: Taylor Blau <me@ttaylorr.com>
Date: Feb 28, 2025, 23:49 UTC
Message-ID: <Z8JLbxBQh7XUpplz@nand.local>
In-Reply-To: <Z8GJbOoY9Z3VMSEM@pks.im>

On Fri, Feb 28, 2025 at 11:01:16AM +0100, Patrick Steinhardt wrote:

Show 29 quoted lines

> 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!

Show 20 quoted lines

> > @@ -397,10 +409,17 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
> >  {
> >  	struct stat st;
> >  	char *bitmap_name = midx_bitmap_filename(midx);
> > -	int fd = git_open(bitmap_name);
> > +	int fd;
> >  	uint32_t i, preferred_pack;
> >  	struct packed_git *preferred;
> >
> > +	fd = git_open(bitmap_name);
> > +	if (fd < 0 && errno == ENOENT) {
> > +		FREE_AND_NULL(bitmap_name);
> > +		bitmap_name = midx_bitmap_filename(midx);
> > +		fd = git_open(bitmap_name);
> > +	}
> > +
>
> Wait, this looks weird to me. `bitmap_name` already contains the result
> of `midx_bitmap_filename()`, so you're essentially retrying the exact
> same operation as before?

Hmm. I have no idea, but you're exactly right. I dropped it from my local copy.

Thanks, Taylor