Re: [PATCH v3 07/13] pack-bitmap.c: teach `rev-list --test-bitmap` about incremental MIDXs

[Patrick Steinhardt <ps@pks.im>]

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()`.

> +{
> +	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.

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

Patrick