Re: [PATCH v3 11/13] pack-bitmap.c: keep track of each layer's type bitmaps
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Mar 1, 2025, 00:26 UTC
- Message-ID
- <Z8JUPlXSZ7tEKWET@nand.local>
- In-Reply-To
- <Z8GJfjhGAbXnA1LW@pks.im>
On Fri, Feb 28, 2025 at 11:01:34AM +0100, Patrick Steinhardt wrote:
Show 26 quoted lines
> On Tue, Nov 19, 2024 at 05:07:50PM -0500, Taylor Blau wrote:
> > @@ -586,7 +604,29 @@ static int load_reverse_index(struct repository *r, struct bitmap_index *bitmap_
> > return load_pack_revindex(r, bitmap_git->pack);
> > }
> >
> > -static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git)
> > +static void load_all_type_bitmaps(struct bitmap_index *bitmap_git)
> > +{
> > + struct bitmap_index *curr = bitmap_git;
> > + size_t i = bitmap_git->base_nr - 1;
> > +
> > + ALLOC_ARRAY(bitmap_git->commits_all, bitmap_git->base_nr);
> > + ALLOC_ARRAY(bitmap_git->trees_all, bitmap_git->base_nr);
> > + ALLOC_ARRAY(bitmap_git->blobs_all, bitmap_git->base_nr);
> > + ALLOC_ARRAY(bitmap_git->tags_all, bitmap_git->base_nr);
> > +
> > + while (curr) {
> > + bitmap_git->commits_all[i] = curr->commits;
> > + bitmap_git->trees_all[i] = curr->trees;
> > + bitmap_git->blobs_all[i] = curr->blobs;
> > + bitmap_git->tags_all[i] = curr->tags;
> > +
> > + curr = curr->base;
> > + i -= 1;
>
> Do we want to `BUG()` in case `i == 0` before this statement?Good idea, but I think we only want to do this when curr is non-NULL after assigning it to curr->base, so something like:
--- 8< ---
diff --git a/pack-bitmap.c b/pack-bitmap.c index 48da3f3b5b..7cea838f58 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -616,6 +616,9 @@ static void load_all_type_bitmaps(struct bitmap_index *bitmap_git) bitmap_git->tags_all[i] = curr->tags; curr = curr->base; + if (curr && !i) + BUG("unexpected number of bitmap layers, expected %lu", + bitmap_git->base_nr); i -= 1; } } --- >8 --- (though note that this is on top of an adjustment I made earlier to make base_nr 0-indexed as it always should have been, though I don't think it is relevant to the portion of the change shown here). Thanks, Taylor