git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Taylor Blau <me@ttaylorr.com>
Date
Sep 17, 2021, 17:32 UTC
Message-ID
<YUTRHaHDjCaxmQyY@nand.local>
In-Reply-To
<87y27veeyj.fsf@evledraar.gmail.com>
On Fri, Sep 17, 2021 at 10:56:09AM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 17 quoted lines
> >> 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@gmail.com/;
> 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" &&
 	(
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 21 of 55 in “pack-bitmap: permute existing namehash values”
  1. 0/4 pack-bitmap: permute existing namehash valuesTaylor Blau, Sep 7, 2021
  2. 1/4 t/helper/test-bitmap.c: add 'dump-hashes' modeTaylor Blau, Sep 7, 2021
  3. Ævar Arnfjörð BjarmasonSep 8, 2021
  4. Taylor BlauSep 8, 2021
  5. 2/4 pack-bitmap.c: propagate namehash values from existing bitmapsTaylor Blau, Sep 7, 2021
  6. 3/4 midx.c: respect 'pack.writeBitmapHashcache' when writing bitmapsTaylor Blau, Sep 7, 2021
  7. Ævar Arnfjörð BjarmasonSep 8, 2021
  8. Taylor BlauSep 8, 2021
  9. Ævar Arnfjörð BjarmasonSep 9, 2021
  10. Ævar Arnfjörð BjarmasonSep 9, 2021
  11. Taylor BlauSep 9, 2021
  12. Ævar Arnfjörð BjarmasonSep 9, 2021
  13. Taylor BlauSep 9, 2021
  14. Taylor BlauSep 9, 2021
  15. Junio C HamanoSep 13, 2021
  16. Taylor BlauSep 14, 2021
  17. 4/4 t5326: test propagating hashcache valuesTaylor Blau, Sep 7, 2021
  18. Ævar Arnfjörð BjarmasonSep 8, 2021
  19. Taylor BlauSep 8, 2021
  20. Ævar Arnfjörð BjarmasonSep 17, 2021
  21. Taylor BlauSep 17, 2021
  22. Ævar Arnfjörð BjarmasonSep 17, 2021
  23. Junio C HamanoSep 13, 2021
  24. Taylor BlauSep 14, 2021
  25. Junio C HamanoSep 14, 2021
  26. Taylor BlauSep 14, 2021
  27. Taylor BlauSep 14, 2021
  28. Jeff KingSep 14, 2021
  29. Taylor BlauSep 14, 2021
  30. Jeff KingSep 14, 2021
  31. Junio C HamanoSep 14, 2021
  32. 0/7 pack-bitmap: permute existing namehash valuesTaylor Blau, Sep 14, 2021
  33. 1/7 t/helper/test-bitmap.c: add 'dump-hashes' modeTaylor Blau, Sep 14, 2021
  34. 2/7 pack-bitmap.c: propagate namehash values from existing bitmapsTaylor Blau, Sep 14, 2021
  35. 3/7 midx.c: respect 'pack.writeBitmapHashcache' when writing bitmapsTaylor Blau, Sep 14, 2021
  36. 4/7 p5326: create missing 'perf-tag' tagTaylor Blau, Sep 14, 2021
  37. Jeff KingSep 16, 2021
  38. Taylor BlauSep 17, 2021
  39. 5/7 p5326: don't set core.multiPackIndex unnecessarilyTaylor Blau, Sep 14, 2021
  40. Jeff KingSep 16, 2021
  41. 6/7 p5326: generate pack bitmaps before writing the MIDX bitmapTaylor Blau, Sep 14, 2021
  42. Jeff KingSep 16, 2021
  43. Taylor BlauSep 17, 2021
  44. 7/7 t5326: test propagating hashcache valuesTaylor Blau, Sep 14, 2021
  45. Jeff KingSep 16, 2021
  46. Jeff KingSep 16, 2021
  47. 0/7 pack-bitmap: permute existing namehash valuesTaylor Blau, Sep 17, 2021
  48. 1/7 t/helper/test-bitmap.c: add 'dump-hashes' modeTaylor Blau, Sep 17, 2021
  49. 2/7 pack-bitmap.c: propagate namehash values from existing bitmapsTaylor Blau, Sep 17, 2021
  50. 3/7 midx.c: respect 'pack.writeBitmapHashcache' when writing bitmapsTaylor Blau, Sep 17, 2021
  51. 4/7 p5326: create missing 'perf-tag' tagTaylor Blau, Sep 17, 2021
  52. 5/7 p5326: don't set core.multiPackIndex unnecessarilyTaylor Blau, Sep 17, 2021
  53. 6/7 p5326: generate pack bitmaps before writing the MIDX bitmapTaylor Blau, Sep 17, 2021
  54. 7/7 t5326: test propagating hashcache valuesTaylor Blau, Sep 17, 2021
  55. Jeff KingSep 17, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.