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 <me@xxxxxxxxxxxx> >> > --- >> > 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/YUOds4kcHRgMk5nC@nand.local/ 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/patch-v6-01.22-ebe89f65354-20210907T104559Z-avarab@xxxxxxxxx/; 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/8735vrvg39.fsf@xxxxxxxxxxxxxxxxxxx/). 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 &&
