diff mbox series

[v2,5/8] t5326: extract `test_rev_exists`

Message ID 73faab9f429221b7be88ea88ce59bc47afb57348.1639446906.git.me@ttaylorr.com (mailing list archive)
State Superseded
Headers show
Series midx: prevent bitmap corruption when permuting pack order | expand

Commit Message

Taylor Blau Dec. 14, 2021, 1:55 a.m. UTC 
To determine which source of data is used for the MIDX's reverse index
cache, introduce a helper which forces loading the reverse index, and
then looks for the special trace2 event introduced in a previous commit.

For now, this helper just looks for when the legacy MIDX .rev file was
loaded, but in a subsequent commit will become parameterized over the
the reverse index's source.

This function replaces checking for the existence of the .rev file. We
could write a similar helper to ensure that the .rev file is cleaned up
after repacking, but it will make subsequent tests more difficult to
write, and provides marginal value since we already check that the MIDX
.bitmap file is removed.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t5326-multi-pack-bitmaps.sh | 38 +++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 15 deletions(-)

Comments

Derrick Stolee Dec. 20, 2021, 6:33 p.m. UTC | #1 
On 12/13/2021 8:55 PM, Taylor Blau wrote:
> To determine which source of data is used for the MIDX's reverse index
> cache, introduce a helper which forces loading the reverse index, and
> then looks for the special trace2 event introduced in a previous commit.
> 
> For now, this helper just looks for when the legacy MIDX .rev file was
> loaded, but in a subsequent commit will become parameterized over the
> the reverse index's source.
> 
> This function replaces checking for the existence of the .rev file. We
> could write a similar helper to ensure that the .rev file is cleaned up
> after repacking, but it will make subsequent tests more difficult to
> write, and provides marginal value since we already check that the MIDX
> .bitmap file is removed.

...

> +test_rev_exists () {
> +	commit="$1"
> +
> +	test_expect_success 'reverse index exists' '
> +		GIT_TRACE2_EVENT_NESTING=10 \

Very recently, b8de3d6 (test-lib.sh: set GIT_TRACE2_EVENT_NESTING,
2021-11-29) made it to master and sets this to 100 across the test
suite, so you don't need this line.

> +		GIT_TRACE2_EVENT=$(pwd)/event.trace \
> +			git rev-list --test-bitmap "$commit" &&

This use of $commit has me worried. Do the tests become too flaky
to changes in how we choose commits for the bitmaps? Does that
require callers to be too aware of the refstate when creating the
bitmaps?

Perhaps just `git rev-list --use-bitmap-index [--objects] HEAD`
would suffice to generate the trace event?

> +		test_path_is_file $midx-$(midx_checksum $objdir).rev &&
> +		grep "\"category\":\"load_midx_revindex\",\"key\":\"source\",\"value\":\"rev\"" event.trace
> +	'
> +}
> +
>  setup_bitmap_history
>  
>  test_expect_success 'create single-pack midx with bitmaps' '
>  	git repack -ad &&
>  	git multi-pack-index write --bitmap &&
>  	test_path_is_file $midx &&
> -	test_path_is_file $midx-$(midx_checksum $objdir).bitmap &&
> -	test_path_is_file $midx-$(midx_checksum $objdir).rev
> +	test_path_is_file $midx-$(midx_checksum $objdir).bitmap
>  '
>  
> +test_rev_exists HEAD
> +

Perhaps this helper would be more appropriate as a helper method
within a test, rather than creating a test of its own? I think
it looks better to include it next to the setup lines, something
like

test_expect_success 'create single-pack midx with bitmaps' '
	git repack -ad &&
	git multi-pack-index write --bitmap &&
	test_path_is_file $midx &&
	test_path_is_file $midx-$(midx_checksum $objdir).bitmap &&
	test_rev_exists HEAD
'

This would lead to a failure within 'create single-pack midx
with bitmaps' which is easier to find in the file.

Thanks,
-Stolee
Taylor Blau Jan. 4, 2022, 3:33 p.m. UTC | #2 
On Mon, Dec 20, 2021 at 01:33:32PM -0500, Derrick Stolee wrote:
> On 12/13/2021 8:55 PM, Taylor Blau wrote:
> > To determine which source of data is used for the MIDX's reverse index
> > cache, introduce a helper which forces loading the reverse index, and
> > then looks for the special trace2 event introduced in a previous commit.
> >
> > For now, this helper just looks for when the legacy MIDX .rev file was
> > loaded, but in a subsequent commit will become parameterized over the
> > the reverse index's source.
> >
> > This function replaces checking for the existence of the .rev file. We
> > could write a similar helper to ensure that the .rev file is cleaned up
> > after repacking, but it will make subsequent tests more difficult to
> > write, and provides marginal value since we already check that the MIDX
> > .bitmap file is removed.
>
> ...
>
> > +test_rev_exists () {
> > +	commit="$1"
> > +
> > +	test_expect_success 'reverse index exists' '
> > +		GIT_TRACE2_EVENT_NESTING=10 \
>
> Very recently, b8de3d6 (test-lib.sh: set GIT_TRACE2_EVENT_NESTING,
> 2021-11-29) made it to master and sets this to 100 across the test
> suite, so you don't need this line.

Thanks; I had written it with this in place in case this topic was going
to reach master before the topic containing b8de3d6. But it didn't, and
you're right that setting GIT_TRACE2_EVENT_NESTING explicitly here is no
longer necessary.

> > +		GIT_TRACE2_EVENT=$(pwd)/event.trace \
> > +			git rev-list --test-bitmap "$commit" &&
>
> This use of $commit has me worried. Do the tests become too flaky
> to changes in how we choose commits for the bitmaps? Does that
> require callers to be too aware of the refstate when creating the
> bitmaps?
>
> Perhaps just `git rev-list --use-bitmap-index [--objects] HEAD`
> would suffice to generate the trace event?

It's necessary for the group of tests which exercise partial bitmap
coverage (see the "setup partial bitmaps" test and below for more). In
those tests, we don't have bitmap coverage of HEAD, only HEAD~, so we
need to be able to say things like:

    test_rev_exists HEAD~

since just asking for the tip isn't guaranteed to work always.

We could make the argument optional (i.e., `git rev-list --test-bitmap
"${1:-HEAD}"`), but I generally find that it makes the callers more
difficult to read rather than easier.

On the "--test-bitmap" vs "--objects" thing, I don't think it matters
either way. They are both doing basically the same traversal, but
"--test-bitmap" does some additional integrity checks on top.

> >  test_expect_success 'create single-pack midx with bitmaps' '
> >  	git repack -ad &&
> >  	git multi-pack-index write --bitmap &&
> >  	test_path_is_file $midx &&
> > -	test_path_is_file $midx-$(midx_checksum $objdir).bitmap &&
> > -	test_path_is_file $midx-$(midx_checksum $objdir).rev
> > +	test_path_is_file $midx-$(midx_checksum $objdir).bitmap
> >  '
> >
> > +test_rev_exists HEAD
> > +
>
> Perhaps this helper would be more appropriate as a helper method
> within a test, rather than creating a test of its own? I think
> it looks better to include it next to the setup lines, something
> like

Eh, sure. I won't plan on picking this up, but I'm happy to if you feel
strongly about it.

Thanks,
Taylor
diff mbox series

Patch

diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
index 1a9581af30..85a91c2675 100755
--- a/t/t5326-multi-pack-bitmaps.sh
+++ b/t/t5326-multi-pack-bitmaps.sh
@@ -17,16 +17,30 @@  midx_pack_source () {
 	test-tool read-midx --show-objects .git/objects | grep "^$1 " | cut -f2
 }
 
+test_rev_exists () {
+	commit="$1"
+
+	test_expect_success 'reverse index exists' '
+		GIT_TRACE2_EVENT_NESTING=10 \
+		GIT_TRACE2_EVENT=$(pwd)/event.trace \
+			git rev-list --test-bitmap "$commit" &&
+
+		test_path_is_file $midx-$(midx_checksum $objdir).rev &&
+		grep "\"category\":\"load_midx_revindex\",\"key\":\"source\",\"value\":\"rev\"" event.trace
+	'
+}
+
 setup_bitmap_history
 
 test_expect_success 'create single-pack midx with bitmaps' '
 	git repack -ad &&
 	git multi-pack-index write --bitmap &&
 	test_path_is_file $midx &&
-	test_path_is_file $midx-$(midx_checksum $objdir).bitmap &&
-	test_path_is_file $midx-$(midx_checksum $objdir).rev
+	test_path_is_file $midx-$(midx_checksum $objdir).bitmap
 '
 
+test_rev_exists HEAD
+
 basic_bitmap_tests
 
 test_expect_success 'create new additional packs' '
@@ -52,10 +66,11 @@  test_expect_success 'create multi-pack midx with bitmaps' '
 	test_line_count = 25 packs &&
 
 	test_path_is_file $midx &&
-	test_path_is_file $midx-$(midx_checksum $objdir).bitmap &&
-	test_path_is_file $midx-$(midx_checksum $objdir).rev
+	test_path_is_file $midx-$(midx_checksum $objdir).bitmap
 '
 
+test_rev_exists HEAD
+
 basic_bitmap_tests
 
 test_expect_success '--no-bitmap is respected when bitmaps exist' '
@@ -66,7 +81,6 @@  test_expect_success '--no-bitmap is respected when bitmaps exist' '
 
 	test_path_is_file $midx &&
 	test_path_is_file $midx-$(midx_checksum $objdir).bitmap &&
-	test_path_is_file $midx-$(midx_checksum $objdir).rev &&
 
 	git multi-pack-index write --no-bitmap &&
 
@@ -206,10 +220,11 @@  test_expect_success 'setup partial bitmaps' '
 	test_commit loose &&
 	git multi-pack-index write --bitmap 2>err &&
 	test_path_is_file $midx &&
-	test_path_is_file $midx-$(midx_checksum $objdir).bitmap &&
-	test_path_is_file $midx-$(midx_checksum $objdir).rev
+	test_path_is_file $midx-$(midx_checksum $objdir).bitmap
 '
 
+test_rev_exists HEAD~
+
 basic_bitmap_tests HEAD~
 
 test_expect_success 'removing a MIDX clears stale bitmaps' '
@@ -224,7 +239,6 @@  test_expect_success 'removing a MIDX clears stale bitmaps' '
 
 		# Write a MIDX and bitmap; remove the MIDX but leave the bitmap.
 		stale_bitmap=$midx-$(midx_checksum $objdir).bitmap &&
-		stale_rev=$midx-$(midx_checksum $objdir).rev &&
 		rm $midx &&
 
 		# Then write a new MIDX.
@@ -234,9 +248,7 @@  test_expect_success 'removing a MIDX clears stale bitmaps' '
 
 		test_path_is_file $midx &&
 		test_path_is_file $midx-$(midx_checksum $objdir).bitmap &&
-		test_path_is_file $midx-$(midx_checksum $objdir).rev &&
-		test_path_is_missing $stale_bitmap &&
-		test_path_is_missing $stale_rev
+		test_path_is_missing $stale_bitmap
 	)
 '
 
@@ -257,7 +269,6 @@  test_expect_success 'pack.preferBitmapTips' '
 		git multi-pack-index write --bitmap &&
 		test_path_is_file $midx &&
 		test_path_is_file $midx-$(midx_checksum $objdir).bitmap &&
-		test_path_is_file $midx-$(midx_checksum $objdir).rev &&
 
 		test-tool bitmap list-commits | sort >bitmaps &&
 		comm -13 bitmaps commits >before &&
@@ -267,7 +278,6 @@  test_expect_success 'pack.preferBitmapTips' '
 			<before | git update-ref --stdin &&
 
 		rm -fr $midx-$(midx_checksum $objdir).bitmap &&
-		rm -fr $midx-$(midx_checksum $objdir).rev &&
 		rm -fr $midx &&
 
 		git -c pack.preferBitmapTips=refs/tags/include \
@@ -305,7 +315,6 @@  test_expect_success 'writing a bitmap with --refs-snapshot' '
 		grep "$(git rev-parse two)" bitmaps &&
 
 		rm -fr $midx-$(midx_checksum $objdir).bitmap &&
-		rm -fr $midx-$(midx_checksum $objdir).rev &&
 		rm -fr $midx &&
 
 		# Then again, but with a refs snapshot which only sees
@@ -350,7 +359,6 @@  test_expect_success 'write a bitmap with --refs-snapshot (preferred tips)' '
 		) >snapshot &&
 
 		rm -fr $midx-$(midx_checksum $objdir).bitmap &&
-		rm -fr $midx-$(midx_checksum $objdir).rev &&
 		rm -fr $midx &&
 
 		git multi-pack-index write --bitmap --refs-snapshot=snapshot &&