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@xxxxxxxxxxxxxxx>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> 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.
