Subject: Re: [PATCH 4/4] t5326: test propagating hashcache values

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Fri, 17 Sep 2021 10:56:09 +0200

On Tue, Sep 07 2021, Taylor Blau wrote:

> On Wed, Sep 08, 2021 at 03:46:29AM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Tue, Sep 07 2021, Taylor Blau wrote:
>>
>> > Now that we both can propagate values from the hashcache, and respect
>> > the configuration to enable the hashcache at all, test that both of
>> > these function correctly by hardening their behavior with a test.
>> >
>> > Signed-off-by: Taylor Blau <[email-masked]>
>> > ---
>> >  t/t5326-multi-pack-bitmaps.sh | 32 ++++++++++++++++++++++++++++++++
>> >  1 file changed, 32 insertions(+)
>> >
>> > diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
>> > index 4ad7c2c969..24148ca35b 100755
>> > --- a/t/t5326-multi-pack-bitmaps.sh
>> > +++ b/t/t5326-multi-pack-bitmaps.sh
>> > @@ -283,4 +283,36 @@ test_expect_success 'pack.preferBitmapTips' '
>> >  	)
>> >  '
>> >
>> > +test_expect_success 'hash-cache values are propagated from pack bitmaps' '
>> > +	rm -fr repo &&
>> > +	git init repo &&
>> > +	test_when_finished "rm -fr repo" &&
>>
>> It seems the need for this "rm -fr repo" dance instead of just relying
>> on test_when_finished "rm -rf repo" is because of a 3x tests in a
>> function in tb/multi-pack-bitmaps that should probably be combined into
>> one, i.e. they share the same logical "repo" setup.
>
> Yeah. There's definitely room for clean-up there if we want to have each
> of the tests operate on the same repo. I have always found sharing a
> repo between tests difficult to reason about, since we have to assume
> that any --run parameters could be passed in.
>
> So I have tended to err on the side of creating and throwing away a new
> repo per test, but I understand that's somewhat atypical for Git's
> tests.

Just in an effort to make myself clear here, because between your note
in https://lore.kernel.org/git/[email-masked]/ and
re-reading my mail here I can barely understand what I meant here :)

What I mean is that the whole "rm -rf" dance at the start of tests is
fallout from an earlier call you made in c51f5a6437c (t5326: test
multi-pack bitmap behavior, 2021-08-31) to split up three tests that
really are the same logical test in terms of setup/teardown.

If they were to be re-combined as shown in the diff-on-top at the end
here none of the tests need a "rm -rf repo" at the beginning, because
they can all rely on a starting pattern of:

    test_when_finished "rm -rf repo" &&
    git init repo &&
    ( cd repo ... )

Or, you can also just not re-use the "repo" name, which is what I did in
the fsck tests at
https://lore.kernel.org/git/[email-masked]/;
then you don't even need test_when_finished "rm -rf[ ...]".

None of this requires fixing here or a re-roll, unless you happen to
think this diff is the best thing since sliced bread (assume my
Signed-off-by).

But just as a general musing on patterns to use in tests, I see why you
used that 1x test as 3x, because you want "test_expect_success" to give
you the label on for each "step".

I think that would be worth fixing in test-lib.sh, there's no inherent
reason we couldn't support "checkpoints" or "subtests" within
"test_expect_success" (the latter being a part of the TAP protocol).

But until then IMNSHO this sort of thing is an anti-pattern,
i.e. needing to write things like:

    test_expect_success '[...]' 'A'
    test_expect_success '[...]' 'B'

Where a part of what B needs to do is to clean up the mess left behind
A, or relies on its setup.

That's just added verbosity and context when reading the code. Ideally
you'd just need to read the "setup" at the start, and then individual
tests, with this pattern you need to read the preceding test(s) to see
where the crap they're cleaning came from, and if it's even needed etc.

(For the "setup" part I've suggested a test_expect_setup, see
https://lore.kernel.org/git/[email-masked]/).

The "needs the setup" part of this has the negative side-effect of
breaking the semantics of the --run option. As noted in the E-Mail
linked in the last paragraph it doesn't work well in general because of
things like this, but let's steer in the right direction.

I.e. with this change you can run:

    ./t5326-multi-pack-bitmaps.sh --run=111-113

Which is great, usually you need at least 1,<nums you want> ,r
1-10,<nums you want> to get the setup. But because you split them this
breaks:

    ./t5326-multi-pack-bitmaps.sh --run=112-113

I.e. we'll run only the 2nd and 3rd test in a sequence that needs the
1st for setup.

For context: My preference for this isn't just an asthetic or
theoretical thing. It's really nice to be hacking some code, find that
I've broken the bitmap code somehow in your test #112, and be able to
just run (since running the whole thing takes 13s on my box, and I'd
like to test in a tight loop):

    ./t5326-multi-pack-bitmaps.sh --run=112

But that breaks as noted above, so I need read earlier tests (I was
probably looking at test #112 already at this point) and see how their
setup works etc.

Anyway, as with so many things in git.git being able to just assume
--run works like this isn't something you can rely on in the general
case, but just as a matter of where we should be headed...

>> > +	(
>> > +		cd repo &&
>> > +
>> > +		git config pack.writeBitmapHashCache true &&
>>
>> s/git config/test_config/, surely.
>
> No, since this is in a subshell (and we don't care about unsetting the
> value of a config in a repo that we're going to throw away, anyways).
>
> (Side-note: since this has a /bin/sh shebang, we can't detect that we're
> in a subshell and so test_config actually _would_ work here. But
> switching this test to run with /bin/bash where we can detect whether or
> not we're in a subshell would cause this test to fail with test_config).

Yes, you're 100% right here. This was purely a misreading on my part, I
managed to somehow not see/take into account the subshell. Using
test_config makes no sense here.

The diff-on-top for discussion mentioned above:

diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
index 24148ca35b3..a6bb0abb387 100755
--- a/t/t5326-multi-pack-bitmaps.sh
+++ b/t/t5326-multi-pack-bitmaps.sh
@@ -133,8 +133,8 @@ bitmap_reuse_tests() {
 	from=$1
 	to=$2
 
-	test_expect_success "setup pack reuse tests ($from -> $to)" '
-		rm -fr repo &&
+	test_expect_success "setup/build/verify ($from -> $to) pack reuse bitmaps" '
+		test_when_finished "rm -rf repo" &&
 		git init repo &&
 		(
 			cd repo &&
@@ -148,13 +148,9 @@ bitmap_reuse_tests() {
 				git multi-pack-index write --bitmap
 			else
 				git repack -Adb
-			fi
-		)
-	'
+			fi &&
 
-	test_expect_success "build bitmap from existing ($from -> $to)" '
-		(
-			cd repo &&
+			# Build
 			test_commit_bulk --id=further 16 &&
 			git tag new-tip &&
 
@@ -164,13 +160,9 @@ bitmap_reuse_tests() {
 				git multi-pack-index write --bitmap
 			else
 				git repack -Adb
-			fi
-		)
-	'
+			fi &&
 
-	test_expect_success "verify resulting bitmaps ($from -> $to)" '
-		(
-			cd repo &&
+			# Verify
 			git for-each-ref &&
 			git rev-list --test-bitmap refs/tags/old-tip &&
 			git rev-list --test-bitmap refs/tags/new-tip
@@ -183,9 +175,8 @@ bitmap_reuse_tests 'MIDX' 'pack'
 bitmap_reuse_tests 'MIDX' 'MIDX'
 
 test_expect_success 'missing object closure fails gracefully' '
-	rm -fr repo &&
-	git init repo &&
 	test_when_finished "rm -fr repo" &&
+	git init repo &&
 	(
 		cd repo &&
 
@@ -217,9 +208,8 @@ test_expect_success 'setup partial bitmaps' '
 basic_bitmap_tests HEAD~
 
 test_expect_success 'removing a MIDX clears stale bitmaps' '
-	rm -fr repo &&
-	git init repo &&
 	test_when_finished "rm -fr repo" &&
+	git init repo &&
 	(
 		cd repo &&
 		test_commit base &&
@@ -245,8 +235,8 @@ test_expect_success 'removing a MIDX clears stale bitmaps' '
 '
 
 test_expect_success 'pack.preferBitmapTips' '
-	git init repo &&
 	test_when_finished "rm -fr repo" &&
+	git init repo &&
 	(
 		cd repo &&
 
@@ -284,9 +274,8 @@ test_expect_success 'pack.preferBitmapTips' '
 '
 
 test_expect_success 'hash-cache values are propagated from pack bitmaps' '
-	rm -fr repo &&
-	git init repo &&
 	test_when_finished "rm -fr repo" &&
+	git init repo &&
 	(
 		cd repo &&