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