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

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

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