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

On Fri, Sep 17, 2021 at 10:56:09AM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

quoted

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/YUOds4kcHRgMk5nC@nand.local/ (local) and
re-reading my mail here I can barely understand what I meant here :)

Thanks for clarifying; if I could summarize I would say that this
discussion got started since a new test I added starts with:

    rm -fr repo &&
    git init repo &&
    test_when_finished "rm -fr repo"

where the first rm is designed to clean any state left behind from the
split-up tests added in c51f5a6437c.

I understand your suggestion, and I even think that it may be worth
doing, but I'm not sure that I buy that any such cleanup is related to
this series for any reason other than "you happened to add a new test in
this file which extended the pattern".

So let's pursue the cleanup, but outside of this series (and maybe once
the dust has settled more generally on the MIDX bitmaps topics to avoid
having the maintainer deal with any conflicts).

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/patch-v6-01.22-ebe89f65354-20210907T104559Z-avarab@gmail.com/ (local);
then you don't even need test_when_finished "rm -rf[ ...]".

TBH, I think that this is the most appealing thing to do. It's easy and
doesn't require us to think too hard about test_expect_setup or
sub-tests or anything like that where I'm not sure the complexity is
warranted.

I would probably do something like this:

--- >8 ---

diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
index ec4aa89f63..11845f67ae 100755
--- a/t/t5326-multi-pack-bitmaps.sh
+++ b/t/t5326-multi-pack-bitmaps.sh

@@ -132,12 +132,12 @@ test_expect_success 'clone with bitmaps enabled' '
 bitmap_reuse_tests() {
 	from=$1
 	to=$2
+	repo="bitmap-reuse-$from-$to"

 	test_expect_success "setup pack reuse tests ($from -> $to)" '
-		rm -fr repo &&
-		git init repo &&
+		git init $repo &&
 		(
-			cd repo &&
+			cd $repo &&
 			test_commit_bulk 16 &&
 			git tag old-tip &&

@@ -154,7 +154,7 @@ bitmap_reuse_tests() {

 	test_expect_success "build bitmap from existing ($from -> $to)" '
 		(
-			cd repo &&
+			cd $repo &&
 			test_commit_bulk --id=further 16 &&
 			git tag new-tip &&

@@ -170,7 +170,7 @@ bitmap_reuse_tests() {

 	test_expect_success "verify resulting bitmaps ($from -> $to)" '
 		(
-			cd repo &&
+			cd $repo &&
 			git for-each-ref &&
 			git rev-list --test-bitmap refs/tags/old-tip &&
 			git rev-list --test-bitmap refs/tags/new-tip

@@ -183,7 +183,6 @@ 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" &&
 	(

@@ -217,7 +216,6 @@ 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" &&
 	(

@@ -284,7 +282,6 @@ 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" &&
 	(