Re: [PATCH v3 5/6] refs: do not execute reference-transaction hook on packing refs

[Patrick Steinhardt <ps@pks.im>]

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

On Thu, Jan 13, 2022 at 02:00:10PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Jan 13 2022, Patrick Steinhardt wrote:
> 
> > [[PGP Signed Part:Undecided]]
> > The reference-transaction hook is supposed to track logical changes to
> > references, but it currently also gets executed when packing refs in a
> > repository. This is unexpected and ultimately not all that useful:
> > packing refs is not supposed to result in any user-visible change to the
> > refs' state, and it ultimately is an implementation detail of how refs
> > stores work.
> >
> > Fix this excessive execution of the hook when packing refs.
> >
> > Reported-by: Waleed Khan <me@waleedkhan.name>
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  refs/files-backend.c             |  6 ++++--
> >  t/t1416-ref-transaction-hooks.sh | 11 +----------
> >  2 files changed, 5 insertions(+), 12 deletions(-)
> >
> > diff --git a/refs/files-backend.c b/refs/files-backend.c
> > index ecf88cee04..3c0f3027fe 100644
> > --- a/refs/files-backend.c
> > +++ b/refs/files-backend.c
> > @@ -1121,7 +1121,8 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)
> >  	if (check_refname_format(r->name, 0))
> >  		return;
> >  
> > -	transaction = ref_store_transaction_begin(&refs->base, 0, &err);
> > +	transaction = ref_store_transaction_begin(&refs->base,
> > +						  REF_TRANSACTION_SKIP_HOOK, &err);
> >  	if (!transaction)
> >  		goto cleanup;
> >  	ref_transaction_add_update(
> > @@ -1192,7 +1193,8 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
> >  	struct strbuf err = STRBUF_INIT;
> >  	struct ref_transaction *transaction;
> >  
> > -	transaction = ref_store_transaction_begin(refs->packed_ref_store, 0, &err);
> > +	transaction = ref_store_transaction_begin(refs->packed_ref_store,
> > +						  REF_TRANSACTION_SKIP_HOOK, &err);
> >  	if (!transaction)
> >  		return -1;
> >  
> > diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
> > index 0567fbdf0b..f9d3d5213f 100755
> > --- a/t/t1416-ref-transaction-hooks.sh
> > +++ b/t/t1416-ref-transaction-hooks.sh
> > @@ -150,21 +150,12 @@ test_expect_success 'hook does not get called on packing refs' '
> >  	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.
> > +	# git-update-ref(1).
> >  	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
> 
> I wondered how we'd deal with cases where the loose ref != the
> corresponding packed ref, but I can't think of ones where it won't be
> invisible externally, i.e. we'll correctly update the packed-refs and
> delete that loose ref as part of this transaction.

With the previous code we'd see two hook executions with different old
OIDs. Given that we only care about logical updates though the user'd
only want to see the one which deletes the user-visible OID, which is
what's stored in the loose ref. And with the fixes in this series that's
the hook invocation we retain.

> I do wonder if the docs also need updating, currently they say:
> 
>     [The hook] executes whenever a reference transaction is prepared,
>     committed or aborted[...]
> 
> But now we'll explicitly exclude certain classes of
> transactions. Perhaps we should expand:
> 
>     "The hook does not cover symbolic references (but that may change in
>     the future)."
> 
> Into some list of types of changes we intentionally exclude, might
> include in the future etc.

Well, from the user's perspective we do execute the hook whenever we
drive a reference transaction: all modifications to the files backend
are still visible to the hook after the changes in this series. The
issue is that with the files backend being a combination of two
backends, we essentially saw a subset of refs executing the hook twice,
which really is an implementation detail.

Patrick

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