Re: [PATCH v2 5/8] t5326: extract `test_rev_exists`

From: Derrick Stolee <stolee@gmail.com>
Date: Dec 20, 2021, 18:33 UTC
Message-ID: <dbc75e29-aceb-051a-8235-e5c912172aed@gmail.com>
In-Reply-To: <73faab9f429221b7be88ea88ce59bc47afb57348.1639446906.git.me@ttaylorr.com>

On 12/13/2021 8:55 PM, Taylor Blau wrote:

Show 13 quoted lines

> 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.

...

Show 5 quoted lines

> +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?

Show 18 quoted lines

> +		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