Re: [PATCH v2 4/6] refs: demonstrate excessive execution of the reference-transaction hook

[Patrick Steinhardt <ps@pks.im>]

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

On Fri, Jan 07, 2022 at 05:31:04PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Add tests which demonstate which demonstrates that we're executing the
> 
> You demonstrate too often, which may be the point of the test, but
> looks wrong.
> 
> I actually think this should be done as part of the fix to the code
> itself, which presumably is a single-liner to tell the "skip when
> running delete in packed-refs backend".  IOW, just fix the code and
> test how the externally observable behaviour of the code should be
> in new tests, in the same commit.

The reason why I chose to split this out into a separate commit is that
it makes it easier to see what behaviour exactly is changing. If it was
a single step, then a reader would only see the post-image behaviour but
cannot reason about the pre-image behaviour without puzzling everything
together.

So personally I'd prefer to keep it as a separate step, but I'm not
opposed to merging them if you still disagree with my reasoning above.

Patrick

> > diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
> > index 6c941027a8..0567fbdf0b 100755
> > --- a/t/t1416-ref-transaction-hooks.sh
> > +++ b/t/t1416-ref-transaction-hooks.sh
> > @@ -136,4 +136,68 @@ test_expect_success 'interleaving hook calls succeed' '
> >  	test_cmp expect target-repo.git/actual
> >  '
> >  
> > +test_expect_success 'hook does not get called on packing refs' '
> > +	# Pack references first such that we are in a known state.
> > +	git pack-refs --all &&
> > +
> > +	write_script .git/hooks/reference-transaction <<-\EOF &&
> > +		echo "$@" >>actual
> > +		cat >>actual
> > +	EOF
> > +	rm -f actual &&
> > +
> > +	git update-ref refs/heads/unpacked-ref $POST_OID &&
> > +	git pack-refs --all &&
> > +
> > +	# We only expect a single hook invocation, which is the call to
> > +	# git-update-ref(1). But currently, packing refs will also trigger the
> > +	# hook.
> > +	cat >expect <<-EOF &&
> > +		prepared
> > +		$ZERO_OID $POST_OID refs/heads/unpacked-ref
> > +		committed
> > +		$ZERO_OID $POST_OID refs/heads/unpacked-ref
> > +		prepared
> > +		$ZERO_OID $POST_OID refs/heads/unpacked-ref
> > +		committed
> > +		$ZERO_OID $POST_OID refs/heads/unpacked-ref
> > +		prepared
> > +		$POST_OID $ZERO_OID refs/heads/unpacked-ref
> > +		committed
> > +		$POST_OID $ZERO_OID refs/heads/unpacked-ref
> > +	EOF
> > +
> > +	test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'deleting packed ref calls hook once' '
> > +	# Create a reference and pack it.
> > +	git update-ref refs/heads/to-be-deleted $POST_OID &&
> > +	git pack-refs --all &&
> > +
> > +	write_script .git/hooks/reference-transaction <<-\EOF &&
> > +		echo "$@" >>actual
> > +		cat >>actual
> > +	EOF
> > +	rm -f actual &&
> > +
> > +	git update-ref -d refs/heads/to-be-deleted $POST_OID &&
> > +
> > +	# We only expect a single hook invocation, which is the logical
> > +	# deletion. But currently, we see two interleaving transactions, once
> > +	# for deleting the loose refs and once for deleting the packed ref.
> > +	cat >expect <<-EOF &&
> > +		prepared
> > +		$ZERO_OID $ZERO_OID refs/heads/to-be-deleted
> > +		prepared
> > +		$POST_OID $ZERO_OID refs/heads/to-be-deleted
> > +		committed
> > +		$ZERO_OID $ZERO_OID refs/heads/to-be-deleted
> > +		committed
> > +		$POST_OID $ZERO_OID refs/heads/to-be-deleted
> > +	EOF
> > +
> > +	test_cmp expect actual
> > +'
> > +
> >  test_done

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