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

[Taylor Blau <me@ttaylorr.com>]

On Tue, Apr 14, 2026 at 12:48:49PM -0700, Junio C Hamano wrote:
> 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.

It has been a while since I have thought about pseudo-merges since I
returned to the topic with this series, so bear with me just the same
;-).

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

I agree. I vaguely remember discussing this on the list with Ævar years
ago, but clearly we should register our cleanup handler before we do the
thing that needs to be cleaned up in case it fails part of the way
through.

> So we bulk-created 64 commits, repacked into one, and then
> [...]
> wrote the tip-commit in "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).

That's right.

> > +		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?

In this particular instance, nothing should happen, and this test
isn't directly testing the pseudo-merge selection at all here. We don't
expect any pseudo-merges to be generated here, only for us to limit the
selection of which commits get bitmaps.

> 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)?

Yes.

> 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?

That's right, and to the point of your original question, I think a
better name is warranted here, perhaps: "test-tool bitmap write limits
bitmap selection" or something.

To the question of what it's testing, it's testing only its basic
functionality of altering the selection of which commits receive
bitmaps. In that sense I view it as a smoke test of that basic
functionality more than anything else.

How about:

--- 8< ---
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 9489e59fa55..ea94735fd66 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -649,8 +649,8 @@ test_expect_success 'truncated bitmap fails gracefully (lookup table)' '
 '

 test_expect_success 'test-tool bitmap write' '
-	git init bitmap-write-helper &&
 	test_when_finished "rm -fr bitmap-write-helper" &&
+	git init bitmap-write-helper &&
 	(
 		cd bitmap-write-helper &&

@@ -659,12 +659,12 @@ test_expect_success 'test-tool bitmap write' '

 		pack="$(ls .git/objects/pack/pack-*.pack)" &&

-		git rev-parse HEAD >commits &&
-		test-tool bitmap write "$(basename $pack)" <commits &&
+		git rev-parse HEAD >in &&
+		test-tool bitmap write "$(basename $pack)" <in &&

-		test-tool bitmap list-commits | sort >actual &&
-		sort commits >expect &&
-		test_cmp expect actual &&
+		test-tool bitmap list-commits >bitmaps.raw &&
+		sort bitmaps.raw >bitmaps &&
+		test_cmp in bitmaps &&

 		git rev-list --count --objects --use-bitmap-index HEAD >actual &&
 		git rev-list --count --objects HEAD >expect &&
--- >8 ---

Thanks,
Taylor