Re: [PATCH v2 1/7] fetch: increase test coverage of fetches

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 3, 2022, 00:25 UTC
Message-ID: <xmqqwnhbevbq.fsf@gitster.g>
In-Reply-To: <b4ca3f1f3baacde2aea7bae4f583f68c211a557a.1645102965.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 18 quoted lines

> +test_expect_success 'atomic fetch with failing backfill' '
> +	git init clone3 &&
> +
> +	# We want to test whether a failure when backfilling tags correctly
> +	# aborts the complete transaction when `--atomic` is passed: we should
> +	# neither create the branch nor should we create the tag when either
> +	# one of both fails to update correctly.
> +	#
> +	# To trigger failure we simply abort when backfilling a tag.
> +	write_script clone3/.git/hooks/reference-transaction <<-\EOF &&
> +		while read oldrev newrev reference
> +		do
> +			if test "$reference" = refs/tags/tag1
> +			then
> +				exit 1
> +			fi
> +		done
> +	EOF

Without the extra indentation level, all your <<here-doc would become easier to read. You are consistently doing this in your patches, which it is better than random mixes of indentation style, though.

Show 7 quoted lines

> +	test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
> +
> +	# Creation of the tag has failed, so ideally refs/heads/something
> +	# should not exist. The fact that it does demonstrates that there is
> +	# a bug in the `--atomic` flag.
> +	test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
> +'

Makes sense.

Show 18 quoted lines

> +test_expect_success 'atomic fetch with backfill should use single transaction' '
> +	git init clone4 &&
> +
> +	# Fetching with the `--atomic` flag should update all references in a
> +	# single transaction, including backfilled tags. We thus expect to see
> +	# a single reference transaction for the created branch and tags.
> +	cat >expected <<-EOF &&
> +		prepared
> +		$ZERO_OID $B refs/heads/something
> +		$ZERO_OID $S refs/tags/tag2
> +		committed
> +		$ZERO_OID $B refs/heads/something
> +		$ZERO_OID $S refs/tags/tag2
> +		prepared
> +		$ZERO_OID $T refs/tags/tag1
> +		committed
> +		$ZERO_OID $T refs/tags/tag1
> +	EOF

I think we see two transactions here, even though the comment says otherwise. Is this expecting a known breakage?

Show 6 quoted lines

> +	write_script clone4/.git/hooks/reference-transaction <<-\EOF &&
> +		( echo "$*" && cat ) >>actual
> +	EOF
> +
> +	git -C clone4 fetch --atomic .. $B:refs/heads/something &&
> +	test_cmp expected clone4/actual

Nice way to make sure what is and is not in each transaction. I feels a bit too rigid (e.g. in the first transaction, there is no inherent reason to expect that the update to head/something is mentioned before the update to tags/tag2, for example).

Show 15 quoted lines

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

I have been wondering if we need to do this from the hook? If we have this ref before we start "fetch", would it have the same effect, or "fetch" notices that this interfering ref exists and removes it to make room for storing refs/tags/tag1, making the whole thing fail to fail?

> +				exit \$!

In any case, "exit 0" or "exit \$?" would be understandable, but exit with "$!", which is ...? The process ID of the most recent background command? Puzzled.

> +			fi
> +		done
> +	EOF