Re: [PATCH v6 1/1] refs: report old values to transaction hooks

>
> If anyone finds the time, I’d appreciate a code review. I’d like to
> close this chapter (hopefully get it upstream) and move on to working
> on the next bug.
>
> Thanks,
> - Maciej Ciemborowicz
>
> On Fri, Sep 25, 2026 at 12:33 AM Maciej Ciemborowicz
> <maciej.ciemborowicz@gmail.com> wrote:
> >
> > The reference-transaction hook reports an all-zero old object ID whenever
> > the caller does not supply an expected old value. Consequently, batched
> > branch, tag, and remote-ref deletions report zero as both the old and new
> > object IDs because refs_delete_refs() intentionally queues unconditional
> > deletions.
> >
> > Changing those callers to provide expected old values would make the
> > deletions conditional and alter existing command behavior. Instead, record
> > the current raw ref value separately for the hook. Read it before the
> > "preparing" hook, then refresh it after the backend has locked the refs so
> > that the "prepared" and later phases report the value protected by the
> > transaction's locks. Keep this value separate from old_oid and old_target so
> > it does not set REF_HAVE_OLD or otherwise constrain the update.
> >
> > Only resolve these values when a reference-transaction hook exists. Preserve
> > symbolic refs as targets, consistent with the hook's existing symref format.
> > Document that an unlocked "preparing" value may differ from later phases if
> > the ref changes before it is locked.
> >
> > Add coverage for batched branch deletion, tag deletion, and remote pruning.
> > Also exercise a concurrent update from the "preparing" hook to verify that
> > the deletion remains unconditional while later hook phases report the value
> > actually removed.
> >
> > Signed-off-by: Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com>
> > ---
> >  Documentation/githooks.adoc      | 17 +++++----
> >  refs.c                           | 54 ++++++++++++++++++++++++---
> >  refs/refs-internal.h             |  8 ++++
> >  t/t1416-ref-transaction-hooks.sh | 64 +++++++++++++++++++++++++++++++-
> >  4 files changed, 128 insertions(+), 15 deletions(-)
> >
> > diff --git a/Documentation/githooks.adoc b/Documentation/githooks.adoc
> > index ed045940d1..f60dd1d582 100644
> > --- a/Documentation/githooks.adoc
> > +++ b/Documentation/githooks.adoc
> > @@ -509,14 +509,15 @@ receives on standard input a line of the format:
> >    <old-value> SP <new-value> SP <ref-name> LF
> >
> >  where `<old-value>` is the old object name passed into the reference
> > -transaction, `<new-value>` is the new object name to be stored in the
> > -ref and `<ref-name>` is the full name of the ref. When force updating
> > -the reference regardless of its current value or when the reference is
> > -to be created anew, `<old-value>` is the all-zeroes object name. To
> > -distinguish these cases, you can inspect the current value of
> > -`<ref-name>` via `git rev-parse`. During the "preparing" state, symbolic
> > -references are not resolved: `<ref-name>` will reflect the symbolic reference
> > -itself rather than the object it points to.
> > +transaction, or the value observed while preparing the transaction if no
> > +old object name was passed. `<new-value>` is the new object name to be
> > +stored in the ref and `<ref-name>` is the full name of the ref. When the
> > +reference does not exist, `<old-value>` is the all-zeroes object name.
> > +Because references are not yet locked in the "preparing" state, its observed
> > +old value may differ from the value reported in subsequent states if the
> > +reference changes before it is locked. During the "preparing" state,
> > +symbolic references are not resolved: `<ref-name>` will reflect the symbolic
> > +reference itself rather than the object it points to.
> >
> >  For symbolic reference updates the `<old_value>` and `<new-value>`
> >  fields could denote references instead of objects. A reference will be
> > diff --git a/refs.c b/refs.c
> > index 92d5df5b71..d2d25402c3 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -1260,6 +1260,7 @@ void ref_transaction_free(struct ref_transaction *transaction)
> >                 free(transaction->updates[i]->committer_info);
> >                 free((char *)transaction->updates[i]->new_target);
> >                 free((char *)transaction->updates[i]->old_target);
> > +               free(transaction->updates[i]->hook_old_target);
> >                 free((char *)transaction->updates[i]->rejection_details);
> >                 free(transaction->updates[i]);
> >         }
> > @@ -2606,6 +2607,8 @@ static int transaction_hook_feed_stdin(int hook_stdin_fd, void *pp_cb, void *pp_
> >         struct transaction_feed_cb_data *feed_cb_data = pp_task_cb;
> >         struct strbuf *buf = &feed_cb_data->buf;
> >         struct ref_update *update;
> > +       const struct object_id *old_oid;
> > +       const char *old_target;
> >         size_t i = feed_cb_data->index++;
> >         int ret;
> >
> > @@ -2619,12 +2622,18 @@ static int transaction_hook_feed_stdin(int hook_stdin_fd, void *pp_cb, void *pp_
> >
> >         strbuf_reset(buf);
> >
> > -       if (!(update->flags & REF_HAVE_OLD))
> > -               strbuf_addf(buf, "%s ", oid_to_hex(null_oid(transaction->ref_store->repo->hash_algo)));
> > -       else if (update->old_target)
> > -               strbuf_addf(buf, "ref:%s ", update->old_target);
> > +       if (update->flags & REF_HAVE_OLD) {
> > +               old_oid = &update->old_oid;
> > +               old_target = update->old_target;
> > +       } else {
> > +               old_oid = &update->hook_old_oid;
> > +               old_target = update->hook_old_target;
> > +       }
> > +
> > +       if (old_target)
> > +               strbuf_addf(buf, "ref:%s ", old_target);
> >         else
> > -               strbuf_addf(buf, "%s ", oid_to_hex(&update->old_oid));
> > +               strbuf_addf(buf, "%s ", oid_to_hex(old_oid));
> >
> >         if (!(update->flags & REF_HAVE_NEW))
> >                 strbuf_addf(buf, "%s ", oid_to_hex(null_oid(transaction->ref_store->repo->hash_algo)));
> > @@ -2660,6 +2669,36 @@ static void transaction_feed_cb_data_free(void *data)
> >         free(d);
> >  }
> >
> > +static void resolve_transaction_hook_old_values(struct ref_transaction *transaction)
> > +{
> > +       struct ref_store *refs = transaction->ref_store;
> > +       struct strbuf referent = STRBUF_INIT;
> > +
> > +       if (!hook_exists(refs->repo, "reference-transaction"))
> > +               return;
> > +
> > +       for (size_t i = 0; i < transaction->nr; i++) {
> > +               struct ref_update *update = transaction->updates[i];
> > +               unsigned int type = 0;
> > +               int failure_errno;
> > +
> > +               if (update->flags & (REF_HAVE_OLD | REF_LOG_ONLY))
> > +                       continue;
> > +
> > +               oidclr(&update->hook_old_oid, refs->repo->hash_algo);
> > +               FREE_AND_NULL(update->hook_old_target);
> > +               strbuf_reset(&referent);
> > +
> > +               if (!refs_read_raw_ref(refs, update->refname,
> > +                                      &update->hook_old_oid, &referent,
> > +                                      &type, &failure_errno) &&
> > +                   (type & REF_ISSYMREF))
> > +                       update->hook_old_target = xstrdup(referent.buf);
> > +       }
> > +
> > +       strbuf_release(&referent);
> > +}
> > +
> >  static int run_transaction_hook(struct ref_transaction *transaction,
> >                                 const char *state)
> >  {
> > @@ -2709,6 +2748,8 @@ int ref_transaction_prepare(struct ref_transaction *transaction,
> >         if (ref_update_reject_duplicates(&transaction->refnames, err))
> >                 return REF_TRANSACTION_ERROR_GENERIC;
> >
> > +       resolve_transaction_hook_old_values(transaction);
> > +
> >         /* Preparing checks before locking references */
> >         ret = run_transaction_hook(transaction, "preparing");
> >         if (ret) {
> > @@ -2720,6 +2761,9 @@ int ref_transaction_prepare(struct ref_transaction *transaction,
> >         if (ret)
> >                 return ret;
> >
> > +       /* Refresh old values now that the references are locked. */
> > +       resolve_transaction_hook_old_values(transaction);
> > +
> >         ret = run_transaction_hook(transaction, "prepared");
> >         if (ret) {
> >                 ref_transaction_abort(transaction, err);
> > diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> > index c3ac7b556f..a7471b2481 100644
> > --- a/refs/refs-internal.h
> > +++ b/refs/refs-internal.h
> > @@ -99,6 +99,14 @@ struct ref_update {
> >          */
> >         struct object_id old_oid;
> >
> > +       /*
> > +        * The old value observed for the reference-transaction hook when the
> > +        * caller did not provide an expected old value. Unlike old_oid and
> > +        * old_target, these fields do not constrain the update.
> > +        */
> > +       struct object_id hook_old_oid;
> > +       char *hook_old_target;
> > +
> >         /*
> >          * If the new_oid points to a tag object, set this to the peeled
> >          * object ID for optimized retrieval without needed to hit the odb.
> > diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
> > index 4fe9d9b234..fcc7404943 100755
> > --- a/t/t1416-ref-transaction-hooks.sh
> > +++ b/t/t1416-ref-transaction-hooks.sh
> > @@ -14,6 +14,66 @@ test_expect_success setup '
> >         POST_OID=$(git rev-parse POST)
> >  '
> >
> > +test_expect_success 'hook gets old values for batched unconditional deletion' '
> > +       test_when_finished "rm -f actual" &&
> > +       test_when_finished "git remote remove origin && rm -rf empty.git" &&
> > +       git init --bare empty.git &&
> > +       git remote add origin ./empty.git &&
> > +       git branch delete-a PRE &&
> > +       git branch delete-b POST &&
> > +       git tag delete-tag POST &&
> > +       git update-ref refs/remotes/origin/to-prune $PRE_OID &&
> > +       test_hook reference-transaction <<-\EOF &&
> > +               if test "$1" = committed
> > +               then
> > +                       cat >>actual
> > +               fi
> > +       EOF
> > +       git branch -D delete-a delete-b &&
> > +       git tag -d delete-tag &&
> > +       git remote prune origin &&
> > +       cat >expect <<-EOF &&
> > +               $PRE_OID $ZERO_OID refs/heads/delete-a
> > +               $POST_OID $ZERO_OID refs/heads/delete-b
> > +               $POST_OID $ZERO_OID refs/tags/delete-tag
> > +               $PRE_OID $ZERO_OID refs/remotes/origin/to-prune
> > +       EOF
> > +       test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'unconditional deletion remains unconditional' '
> > +       test_when_finished "rm -f actual" &&
> > +       test_when_finished "rm -f \"$(git rev-parse --git-path delete-race-once)\"" &&
> > +       git branch delete-race PRE &&
> > +       test_hook reference-transaction <<-\EOF &&
> > +               state=$1
> > +               while read -r old new ref
> > +               do
> > +                       if test "$state" != aborted
> > +                       then
> > +                               case "$new" in
> > +                               *[!0]*) ;;
> > +                               *) echo "$state $old $new $ref" >>actual ;;
> > +                               esac
> > +                       fi
> > +               done
> > +               marker=$(git rev-parse --git-path delete-race-once)
> > +               if test "$state" = preparing && test ! -e "$marker"
> > +               then
> > +                       >"$marker"
> > +                       git update-ref refs/heads/delete-race POST
> > +               fi
> > +       EOF
> > +       git branch -D delete-race &&
> > +       cat >expect <<-EOF &&
> > +               preparing $PRE_OID $ZERO_OID refs/heads/delete-race
> > +               prepared $POST_OID $ZERO_OID refs/heads/delete-race
> > +               committed $POST_OID $ZERO_OID refs/heads/delete-race
> > +       EOF
> > +       test_cmp expect actual &&
> > +       test_must_fail git show-ref --verify refs/heads/delete-race
> > +'
> > +
> >  test_expect_success 'hook allows updating ref if successful' '
> >         git reset --hard PRE &&
> >         test_hook reference-transaction <<-\EOF &&
> > @@ -65,7 +125,7 @@ test_expect_success 'hook gets all queued updates in prepared state' '
> >                 fi
> >         EOF
> >         cat >expect <<-EOF &&
> > -               $ZERO_OID $POST_OID refs/heads/main
> > +               $PRE_OID $POST_OID refs/heads/main
> >         EOF
> >         git update-ref HEAD POST <<-EOF &&
> >                 update HEAD $ZERO_OID $POST_OID
> > @@ -87,7 +147,7 @@ test_expect_success 'hook gets all queued updates in committed state' '
> >                 fi
> >         EOF
> >         cat >expect <<-EOF &&
> > -               $ZERO_OID $POST_OID refs/heads/main
> > +               $PRE_OID $POST_OID refs/heads/main
> >         EOF
> >         git update-ref HEAD POST &&
> >         test_cmp expect actual
> > --
> > 2.39.3 (Apple Git-146)
> >