On Fri, Sep 17, 2021 at 10:56:09AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> 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/ 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@xxxxxxxxx/; > 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" && (
