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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 13, 2022, 13:00 UTC
Message-ID: <220113.86czkvvlf6.gmgdl@evledraar.gmail.com>
In-Reply-To: <d83f309b9c988d7cad9524ac56c0b4c81e2d863f.1642054003.git.ps@pks.im>

On Thu, Jan 13 2022, Patrick Steinhardt wrote:

Show 68 quoted lines

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