From: Maciej Ciemborowicz Date: Thu, 01 Oct 2026 17:37:05 GMT Subject: Re: [PATCH v6 1/1] refs: report old values to transaction hooks Message-ID: In-Reply-To: Change of priorities. I originally reported this bug while working on git-hooks-ext and semantic events for reference transactions: https://github.com/ciembor/git-hooks-ext However, today I managed to resolve the issues I had encountered with branch and tag deletion by using the data returned earlier by the transaction in the prepared state. As a result, this bug is now a much lower priority for me. The last issue I still need to resolve (and I don't see a reasonable way to address it without fixing the bug in Git) is branch renaming: https://lore.kernel.org/git/CAOLa=ZTN1TU2A1sgEhiw=ymMYr6Ge11cMEubSaeKqr4WNU=2EQ@mail.gmail.com/T/#t I would appreciate it if more attention could be given to that bug instead. Thanks, Maciej Ciemborowicz On Wed, Sep 30, 2026 at 5:11 AM Maciej Ciemborowicz wrote: > > 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 > 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 > > --- > > 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: > > SP SP LF > > > > where `` is the old object name passed into the reference > > -transaction, `` is the new object name to be stored in the > > -ref and `` 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, `` is the all-zeroes object name. To > > -distinguish these cases, you can inspect the current value of > > -`` via `git rev-parse`. During the "preparing" state, symbolic > > -references are not resolved: `` 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. `` is the new object name to be > > +stored in the ref and `` is the full name of the ref. When the > > +reference does not exist, `` 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: `` will reflect the symbolic > > +reference itself rather than the object it points to. > > > > For symbolic reference updates the `` and `` > > 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) > >