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

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Thu, 13 Jan 2022 14:00:10 +0100

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 <[email-masked]>
> Signed-off-by: Patrick Steinhardt <[email-masked]>
> ---
>  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.