On Fri, Feb 28, 2025 at 11:01:26AM +0100, Patrick Steinhardt wrote:
> On Tue, Nov 19, 2024 at 05:07:38PM -0500, Taylor Blau wrote:
> > diff --git a/pack-bitmap.c b/pack-bitmap.c
> > index 1dddb242434..02864a0e1f7 100644
> > --- a/pack-bitmap.c
> > +++ b/pack-bitmap.c
> > @@ -2564,13 +2584,57 @@ static void test_show_commit(struct commit *commit, void *data)
> > display_progress(tdata->prg, ++tdata->seen);
> > }
> >
> > +static uint32_t bitmap_total_entry_count(struct bitmap_index *bitmap_git)
> > +{
> > + uint32_t total = 0;
> > + do {
> > + total = st_add(total, bitmap_git->entry_count);
> > + bitmap_git = bitmap_git->base;
> > + } while (bitmap_git);
> > +
> > + return total;
> > +}
> > +
> > +static void prepare_bitmap_test_data(struct bitmap_test_data *tdata,
> > + struct bitmap_index *bitmap_git)
>
> Nit: according to our style guide this should be called
> `bitmap_test_data_prepare()`.
Ah, I didn't know about that part of the style guide. It looks like it
comes from your 10f0723c8d (Documentation: document idiomatic function
names, 2024-07-30), which is semi-recent ;-).
> > +{
> > + memset(tdata, 0, sizeof(struct bitmap_test_data));
> > +
> > + tdata->bitmap_git = bitmap_git;
> > + tdata->base = bitmap_new();
> > + tdata->commits = ewah_to_bitmap(bitmap_git->commits);
> > + tdata->trees = ewah_to_bitmap(bitmap_git->trees);
> > + tdata->blobs = ewah_to_bitmap(bitmap_git->blobs);
> > + tdata->tags = ewah_to_bitmap(bitmap_git->tags);
> > +
> > + if (bitmap_git->base) {
> > + CALLOC_ARRAY(tdata->base_tdata, 1);
> > + prepare_bitmap_test_data(tdata->base_tdata, bitmap_git->base);
> > + }
> > +}
> > +
> > +static void free_bitmap_test_data(struct bitmap_test_data *tdata)
>
> Same nit here, this should be called `bitmap_test_data_free()`. In fact,
> it should be called `bitmap_test_data_release()`, because we don't free
> `tadata` itself.
Noted, and adjusted!
> > @@ -2579,17 +2643,26 @@ void test_bitmap_walk(struct rev_info *revs)
> > if (revs->pending.nr != 1)
> > die(_("you must specify exactly one commit to test"));
> >
> > - fprintf_ln(stderr, "Bitmap v%d test (%d entries%s)",
> > + fprintf_ln(stderr, "Bitmap v%d test (%d entries%s, %d total)",
> > bitmap_git->version,
> > bitmap_git->entry_count,
> > - bitmap_git->table_lookup ? "" : " loaded");
> > + bitmap_git->table_lookup ? "" : " loaded",
> > + bitmap_total_entry_count(bitmap_git));
> >
> > root = revs->pending.objects[0].item;
> > - bm = bitmap_for_commit(bitmap_git, (struct commit *)root);
> > + bm = find_bitmap_for_commit(bitmap_git, (struct commit *)root, &found);
> >
> > if (bm) {
> > fprintf_ln(stderr, "Found bitmap for '%s'. %d bits / %08x checksum",
> > - oid_to_hex(&root->oid), (int)bm->bit_size, ewah_checksum(bm));
> > + oid_to_hex(&root->oid),
> > + (int)bm->bit_size, ewah_checksum(bm));
> > +
> > + if (bitmap_is_midx(found))
> > + fprintf_ln(stderr, "Located via MIDX '%s'.",
> > + hash_to_hex(get_midx_checksum(found->midx)));
> > + else
> > + fprintf_ln(stderr, "Located via pack '%s'.",
> > + hash_to_hex(found->pack->hash));
> >
> > result = ewah_to_bitmap(bm);
> > }
>
> I'm a bit surprised that this doesn't result in any changes required in
> our tests.
That's intentional, the aim of this commit is really just to get us
ready for the eventuality of having incremental MIDXs. In the meantime,
we don't disrupt the behavior of --test-bitmap for single-layer MIDX
bitmaps.
Thanks,
Taylor
