Re: [PATCH 1/8] t/helper: add 'test-tool bitmap write' subcommand

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <me@ttaylorr.com> writes:

> In f16eb1c091 (pseudo-merge: fix disk reads from find_pseudo_merge(),
> 2026-03-31), we noted that `apply_pseudo_merges_for_commit()` is never
> triggered by the existing test suite, and that this bears further
> investigation.
>
> This patch is the first one to begin that investigation. The following
> patches will expose and fix a variety of bugs in the implementation of
> pseudo-merge bitmaps.
>
> In order to do so, however, many of these tests require very precise
> selection of which commits receive bitmaps and which do not. To date,
> there isn't a standard approach to easily facilitate this. Address this
> by introducing a `test-tool bitmap write` subcommand that writes a
> bitmap for a given packfile, reading the set of commits which should
> receive individual bitmaps from stdin like so:
>
>     test-tool bitmap write <pack-basename> </path/to/commits.list
>
> , where "<pack-basename>" is the filename for a specific packfile (e.g.,
> "pack-abc123.pack"), and "/path/to/commits.list" is a list of commit
> OIDs which will receive bitmaps.
>
> The helper respects `bitmapPseudoMerge.*` configuration for creating
> pseudo-merge bitmaps alongside the regular commit bitmaps.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  t/helper/test-bitmap.c  | 110 +++++++++++++++++++++++++++++++++++++++-
>  t/t5310-pack-bitmaps.sh |  24 +++++++++
>  2 files changed, 133 insertions(+), 1 deletion(-)

I haven't been paying attention at all to pseudo-merge stuff, so my
comment may be too trivial and/or misses the point, but please bear
with me.

> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index f693cb56691..9489e59fa55 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -648,4 +648,28 @@ test_expect_success 'truncated bitmap fails gracefully (lookup table)' '
>  	test_grep corrupted.bitmap.index stderr
>  '
>  
> +test_expect_success 'test-tool bitmap write' '

It is very unclear what aspect of "test-tool bitmap write" is being
tested to me.  Let me think aloud to see if I can convey my
puzzlement.

> +	git init bitmap-write-helper &&
> +	test_when_finished "rm -fr bitmap-write-helper" &&

A tangent but the above two lines may want to be swapped.  "rm -fr"
does not fail when bitmap-write-helper directory does not yet exist,
so "prepare to clear anytime it fails from now on and then create"
would be a safer order than "create, and prepare to clear anytime it
fails from now on".

> +	(
> +		cd bitmap-write-helper &&
> +
> +		test_commit_bulk 64 &&
> +		git repack -ad &&
> +
> +		pack="$(ls .git/objects/pack/pack-*.pack)" &&

So we bulk-created 64 commits, repacked into one, and then

> +		git rev-parse HEAD >commits &&

wrote the tip-commit in "commits".

> +		test-tool bitmap write "$(basename $pack)" <commits &&

And told the new tool to write a bitmap file, with only that HEAD
commit and nothing else to get bitmap.

> +		test-tool bitmap list-commits | sort >actual &&
> +		sort commits >expect &&
> +		test_cmp expect actual &&

And compare the list of bitmapped commits with the singleton HEAD
(it is puzzling to sort a single element list, though).

> +		git rev-list --count --objects --use-bitmap-index HEAD >actual &&
> +		git rev-list --count --objects HEAD >expect &&
> +		test_cmp expect actual
> +	)
> +'
> +
>  test_done

If we look at the implementation of bitmap_write() below, the object
name for HEAD is fed to bitmap_write_push_commit() with pseudo bit
off.  After reading the list (which has only one element), we call
select_pseudo_merges().  What do we expect to happen in this call?
Since no bitmap_write_push_commit() call is made with pseudo bit on,
we will return immediately without doing anything?

After that bitmap_write_build() is called, and as this is expected
to add bitmaps to the commits fed to bitmap_write_push_commit(), we
are expecting to see bitmap given to HEAD (and nothing else)?

I said it is unclear what is being tested.  Putting it another way,
what could go wrong to cause this test to fail?  We give commit A,
B, and C to "bitmap write", and it somehow chooses other commits to
also give bitmap, which will be reported by "bitmap list-commits"
and we detect that as a failure?

Thanks.