Re: [PATCH] t5503: simplify setup of test which exercises failure of backfill

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Mar 03 2022, Patrick Steinhardt wrote:

> [[PGP Signed Part:Undecided]]
> In the testcase to exercise backfilling of tags for fetches we evoke a
> failure of the backfilling mechanism by creating a reference that later
> on causes a D/F conflict. Because the assumption was that git-fetch(1)
> would notice the D/F conflict early on this conflicting reference was
> created via the reference-transaction hook just when we were about to
> write the backfilled tag. As it turns out though this is not the case,
> and the fetch fails in the same way when we create the conflicting ref
> up front.
>
> Simplify the test setup creating the reference up front, which allows us
> to get rid of the hook script.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>
> This simplifies the test setup of t5503 as discussed in [1]. The patch
> applies on top of Junio's ps/fetch-atomic (583bc41923 (fetch: make
> `--atomic` flag cover pruning of refs, 2022-02-17)).

FWIW for something in "next" already the OID will be stable, so it's OK
(and better) to mention 583bc419235 in the commit message itself.

> Patrick
>
>  t/t5503-tagfollow.sh | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
> index e72fdc2534..a3c01014b7 100755
> --- a/t/t5503-tagfollow.sh
> +++ b/t/t5503-tagfollow.sh
> @@ -212,21 +212,11 @@ test_expect_success 'atomic fetch with backfill should use single transaction' '
>  test_expect_success 'backfill failure causes command to fail' '
>  	git init clone5 &&
>  
> -	write_script clone5/.git/hooks/reference-transaction <<-EOF &&
> -		while read oldrev newrev reference
> -		do
> -			if test "\$reference" = refs/tags/tag1
> -			then
> -				# Create a nested tag below the actual tag we
> -				# wanted to write, which causes a D/F conflict
> -				# later when we want to commit refs/tags/tag1.
> -				# We cannot just `exit 1` here given that this
> -				# would cause us to die immediately.
> -				git update-ref refs/tags/tag1/nested $B
> -				exit \$!
> -			fi
> -		done
> -	EOF
> +	# Create a tag that is nested below the tag we are about to fetch via
> +	# the backfill mechanism. This causes a D/F conflict when backfilling
> +	# and should thus cause the command to fail.
> +	empty_blob=$(git -C clone5 hash-object -w --stdin </dev/null) &&
> +	git -C clone5 update-ref refs/tags/tag1/nested $empty_blob &&

This looks better, but FWIW for the discussion about quoted
v.s. unquoted here-doc in this case it's also OK for the pre-image (and
arguably better) to do (and any issues of $? v.s. $! etc. aside):
	
	diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
	index e72fdc25346..eebf0ddc4c2 100755
	--- a/t/t5503-tagfollow.sh
	+++ b/t/t5503-tagfollow.sh
	@@ -212,18 +212,18 @@ test_expect_success 'atomic fetch with backfill should use single transaction' '
	 test_expect_success 'backfill failure causes command to fail' '
	 	git init clone5 &&
	 
	-	write_script clone5/.git/hooks/reference-transaction <<-EOF &&
	+	write_script clone5/.git/hooks/reference-transaction <<-\EOF &&
	 		while read oldrev newrev reference
	 		do
	-			if test "\$reference" = refs/tags/tag1
	+			if test "$reference" = refs/tags/tag1
	 			then
	 				# Create a nested tag below the actual tag we
	 				# wanted to write, which causes a D/F conflict
	 				# later when we want to commit refs/tags/tag1.
	 				# We cannot just `exit 1` here given that this
	 				# would cause us to die immediately.
	-				git update-ref refs/tags/tag1/nested $B
	-				exit \$!
	+				git update-ref refs/tags/tag1/nested '$B'
	+				exit $!
	 			fi
	 		done
	 	EOF

I.e. to end the quote and interpolate $B. IMO also more readable as
e.g. shell syntax highlighting will show that we're interpolating that
outside-test $B.

>  
>  	test_must_fail git -C clone5 fetch .. $B:refs/heads/something &&
>  	test $B = $(git -C clone5 rev-parse --verify refs/heads/something) &&

(In the pre-image, but..) this way of using "test" will hide segfaults
etc. from the invoked git command.

Better to:

    echo $B >expect &&
    git ... >actual &&
    test_cmp expect actual

Or better yet "test_cmp_rev" in this case, but all of that's in "next"
already, so purely optional (and also more food for the "hiding exit
status" microproject) :)