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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 4215 bytes --]

On Wed, Mar 02, 2022 at 04:25:13PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > +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.

Personally I think it's easier to read this way, but grepping through
the codebase shows that what you're proposing is used consistently.
I'll change it.

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

Yes, it is. I've added a comment in this patch to document the breakage,
which is then removed when the bug is fixed.

> > +	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).
> 
> > +'
> > +
> > +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?

No, it indeed is not, thanks!

Patrick

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]