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

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 10, 2022, 12:54 UTC
Message-ID: <Ydwsa0LiVfj+YP7N@ncase>
In-Reply-To: <xmqqiluv3tev.fsf@gitster.g>

On Fri, Jan 07, 2022 at 05:31:04PM -0800, Junio C Hamano wrote:

Show 12 quoted lines

> 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

Show 73 quoted lines

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