From: Maciej Ciemborowicz Date: Wed, 30 Sep 2026 03:11:30 GMT Subject: Re: [PATCH v6 1/1] refs: report old values to transaction hooks Message-ID: In-Reply-To: <2af3eeadd18806c5298d53072428656885cff89d.1790269745.git.maciej.ciemborowicz@gmail.com> 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) >