# [BUG] reference-transaction reports zero OIDs for branch and tag deletion 52 messages from 2026-09-19 to 2026-10-01. Participants: Maciej Ciemborowicz, D. Ben Knoble, Karthik Nayak, Junio C Hamano, Patrick Steinhardt. Thread: https://gitlist.dev/t/66351 ## Maciej Ciemborowicz, 2026-09-19 13:34 Subject: [BUG] reference-transaction reports zero OIDs for branch and tag deletion Message-ID: ``` Since Git 2.31, the reference-transaction hook receives all-zero old and new object IDs when a branch or tag is deleted with a high-level command. For example, `git branch -d topic` produces: 0000000000000000000000000000000000000000 \ 0000000000000000000000000000000000000000 \ refs/heads/topic The same happens with `git branch -D`, `git tag -d`, and deletion of a remote-tracking ref by `git remote prune`. Git 2.28 through 2.30 reported the previous object ID followed by the all-zero object ID. Starting with Git 2.31, that information is lost. The behavior is still present in Git 2.55 with both the files and reftable backends. A direct deletion with: git update-ref -d refs/heads/topic "$old_oid" continues to report the useful payload: 0000000000000000000000000000000000000000 refs/heads/topic Minimal reproducer: #!/bin/sh set -eu root=$(mktemp -d) trap 'rm -rf "$root"' EXIT repo=$root/repo hooks=$root/hooks log=$root/transactions git init -q "$repo" git -C "$repo" config user.name Reproducer git -C "$repo" config user.email repro@example.com git -C "$repo" commit --allow-empty -qm initial git -C "$repo" branch topic git -C "$repo" tag v1 mkdir "$hooks" cat >"$hooks/reference-transaction" <<'HOOK' #!/bin/sh printf '%s\n' "--- $1" >>"$HOOK_LOG" cat >>"$HOOK_LOG" HOOK chmod +x "$hooks/reference-transaction" git -C "$repo" config core.hooksPath "$hooks" export HOOK_LOG=$log : >"$log" git -C "$repo" branch -d topic git -C "$repo" tag -d v1 cat "$log" Actual output for the committed transactions is equivalent to: 0000000000000000000000000000000000000000 \ 0000000000000000000000000000000000000000 \ refs/heads/topic 0000000000000000000000000000000000000000 \ 0000000000000000000000000000000000000000 \ refs/tags/v1 I expected: 0000000000000000000000000000000000000000 refs/heads/topic 0000000000000000000000000000000000000000 refs/tags/v1 I understand that the reference-transaction documentation permits an all-zero old value when a ref is force-updated without checking its current value. However, `git branch -d` is a safety-checked deletion, Git has already resolved the branch being deleted, and Git 2.28–2.30 provided its old object ID. Was this loss of information intentional? If not, could the previous object ID be restored for these deletion paths? If it is intentional, the documentation may need to clarify that high-level deletion commands can provide a zero-to-zero record. The behavior was tested across Git 2.28–2.55. The compatibility results and test implementation are available here: https://github.com/ciembor/git-hooks-ext/blob/v0.2.0/tests/compat/README.md https://github.com/ciembor/git-hooks-ext/blob/v0.2.0/tests/compat/e2e.sh Thanks, Maciej Ciemborowicz ``` ## D. Ben Knoble, 2026-09-19 14:59 Subject: Re: [BUG] reference-transaction reports zero OIDs for branch and tag deletion Message-ID: In-Reply-To: ``` On Sat, Sep 19, 2026 at 9:34 AM Maciej Ciemborowicz wrote: > > Since Git 2.31, the reference-transaction hook receives all-zero old and > new object IDs when a branch or tag is deleted with a high-level command. [snip] > Was this loss of information intentional? If not, could the previous > object ID be restored for these deletion paths? If it is intentional, > the documentation may need to clarify that high-level deletion commands > can provide a zero-to-zero record. Since you seem to have identified a "good" and "bad" version with a reproduction script, I suspect "git bisect" is a good way to answer your questions about intent. -- D. Ben Knoble ``` ## Maciej Ciemborowicz, 2026-09-19 15:42 Subject: Re: [BUG] reference-transaction reports zero OIDs for branch and tag deletion Message-ID: In-Reply-To: ``` Commit `6754159767` introduced `reference-transaction` in Git 2.28. In Git 2.28-2.30, `git branch -D` and `git tag -d` used `delete_ref()` with the known old OID. Commit `8198907795`, before Git 2.31, replaced single-ref deletion with `delete_refs()`. The new function preserved only the ref names and called: ˋˋˋc ref_transaction_delete(transaction, refname, NULL, NULL, ...) ˋˋˋ Without the old OID, the `REF_HAVE_OLD` flag is not set, so the hook receives: ˋˋˋtext 000000... 000000... refs/heads/topic ˋˋˋ The change was intended to speed up deletion of 24,000 tags from roughly 30 minutes to 5 seconds. The loss of information exposed to the hook appears to have been a side effect. So I assume this is a bug introduced by that optimization, and it should be fixed in a way that preserves the performance improvement. That seems feasible. Cheers, Maciej Ciemborowicz On Sat, Sep 19, 2026 at 4:59 PM D. Ben Knoble wrote: > > On Sat, Sep 19, 2026 at 9:34 AM Maciej Ciemborowicz > wrote: > > > > Since Git 2.31, the reference-transaction hook receives all-zero old and > > new object IDs when a branch or tag is deleted with a high-level command. > > [snip] > > > Was this loss of information intentional? If not, could the previous > > object ID be restored for these deletion paths? If it is intentional, > > the documentation may need to clarify that high-level deletion commands > > can provide a zero-to-zero record. > > Since you seem to have identified a "good" and "bad" version with a > reproduction script, I suspect "git bisect" is a good way to answer > your questions about intent. > > -- > D. Ben Knoble ``` ## Maciej Ciemborowicz, 2026-09-19 20:11 Subject: [PATCH 0/3] refs: report old OIDs for batched deletions Message-ID: <20260919201158.43415-1-maciej.ciemborowicz@gmail.com> In-Reply-To: ``` This follows up on the reference-transaction bug report at [1]. The reference-transaction hook reports an all-zero update when `git branch -d`, `git tag -d`, `git remote prune`, or `git fetch --prune` deletes a ref. This prevents hook consumers from identifying the object that the ref pointed to. For branch and tag deletion, this is a regression caused by 8198907795 (use delete_refs when deleting tags or branches, 2021-01-21). That change made large deletions much faster by batching them, but the batch helper did not accept the old OIDs that both callers had already resolved. The pruning paths have the same omission. Teach refs_delete_refs() to accept optional old OIDs, then pass the values already available to the branch, tag, fetch, and remote callers. This keeps a single batched transaction and does not add any ref reads. Deleting 10,000 packed tags took a median of 0.86 seconds before this series and 0.85 seconds after it across five runs, which is within measurement noise. The regression tests use packed refs and cover the files and reftable backends, distinct old OIDs, and regular and atomic fetch pruning. The full test suite passes with DEVELOPER=1. Additional checks covered SHA-1, SHA-256, broken and symbolic refs, and refs shadowed between loose and packed storage. [1] https://lore.kernel.org/git/CACQ=SRGTTdQ+dHXhN6F52dBv5KxZBRfk_Em2fvmEmGJDoB6oTg@mail.gmail.com/ Maciej Ciemborowicz (3): refs: allow callers to supply old OIDs for batch deletion branch, tag: retain old OIDs in batched deletions fetch, remote: retain old OIDs when pruning refs bisect.c | 2 +- builtin/branch.c | 7 +++- builtin/fetch.c | 11 +++-- builtin/remote.c | 33 ++++++++++++--- builtin/tag.c | 7 +++- refs.c | 26 +++++++----- refs.h | 12 +++++- t/helper/test-ref-store.c | 2 +- t/t1416-ref-transaction-hooks.sh | 70 ++++++++++++++++++++++++++++++++ 9 files changed, 144 insertions(+), 26 deletions(-) base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc -- 2.39.3 (Apple Git-146) ``` ## Maciej Ciemborowicz, 2026-09-19 20:11 Subject: [PATCH 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: <20260919201158.43415-2-maciej.ciemborowicz@gmail.com> In-Reply-To: <20260919201158.43415-1-maciej.ciemborowicz@gmail.com> ``` refs_delete_refs() currently performs unconditional deletions. Thus callers cannot preserve old values that they have already resolved, and reference-transaction hooks consequently see a null old OID. Add an optional oid_array whose entries correspond to the refnames. Pass each non-null OID to ref_transaction_delete(). Existing callers retain the unconditional behavior for now. Signed-off-by: Maciej Ciemborowicz --- bisect.c | 2 +- builtin/branch.c | 3 ++- builtin/fetch.c | 2 +- builtin/remote.c | 5 +++-- builtin/tag.c | 3 ++- refs.c | 26 +++++++++++++++----------- refs.h | 12 ++++++++++-- t/helper/test-ref-store.c | 2 +- 8 files changed, 35 insertions(+), 20 deletions(-) diff --git a/bisect.c b/bisect.c index 94c7028d2a..9aa3bace9f 100644 --- a/bisect.c +++ b/bisect.c @@ -1203,7 +1203,7 @@ int bisect_clean_state(void) string_list_append(&refs_for_removal, "BISECT_EXPECTED_REV"); result = refs_delete_refs(get_main_ref_store(the_repository), "bisect: remove", &refs_for_removal, - REF_NO_DEREF); + NULL, REF_NO_DEREF); string_list_clear(&refs_for_removal, 0); unlink_or_warn(git_path_bisect_ancestors_ok()); unlink_or_warn(git_path_bisect_log()); diff --git a/builtin/branch.c b/builtin/branch.c index 1572a4f9ef..f1abeb681d 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -322,7 +322,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, free(target); } - if (refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) + if (refs_delete_refs(get_main_ref_store(the_repository), NULL, + &refs_to_delete, NULL, REF_NO_DEREF)) ret = 1; for_each_string_list_item(item, &refs_to_delete) { diff --git a/builtin/fetch.c b/builtin/fetch.c index c1d7c672f4..d202147b21 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1467,7 +1467,7 @@ static int prune_refs(struct display_state *display_state, } else { result = refs_delete_refs(get_main_ref_store(the_repository), "fetch: prune", &refnames, - 0); + NULL, 0); } } diff --git a/builtin/remote.c b/builtin/remote.c index de989ea3ba..13d3cc52dd 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1073,7 +1073,7 @@ static int rm(int argc, const char **argv, const char *prefix, if (!result) result = refs_delete_refs(get_main_ref_store(the_repository), "remote: remove", &branches, - REF_NO_DEREF); + NULL, REF_NO_DEREF); string_list_clear(&branches, 0); if (skipped.nr) { @@ -1645,7 +1645,8 @@ static int prune_remote(const char *remote, int dry_run) if (!dry_run) result |= refs_delete_refs(get_main_ref_store(the_repository), - "remote: prune", &refs_to_prune, 0); + "remote: prune", &refs_to_prune, + NULL, 0); for_each_string_list_item(item, &states.stale) { const char *refname = item->util; diff --git a/builtin/tag.c b/builtin/tag.c index 06c125b53c..40874a2923 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -122,7 +122,8 @@ static int delete_tags(const char **argv) struct string_list_item *item; result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); - if (refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) + if (refs_delete_refs(get_main_ref_store(the_repository), NULL, + &refs_to_delete, NULL, REF_NO_DEREF)) result = 1; for_each_string_list_item(item, &refs_to_delete) { diff --git a/refs.c b/refs.c index d3caa9a633..a9c5397fd7 100644 --- a/refs.c +++ b/refs.c @@ -18,6 +18,7 @@ #include "refs/refs-internal.h" #include "hook.h" #include "object-name.h" +#include "oid-array.h" #include "odb.h" #include "object.h" #include "path.h" @@ -3056,36 +3057,39 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio } int refs_delete_refs(struct ref_store *refs, const char *logmsg, - struct string_list *refnames, unsigned int flags) + struct string_list *refnames, + const struct oid_array *old_oids, + unsigned int flags) { struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; - struct string_list_item *item; + size_t i; int ret = 0, failures = 0; char *msg; + if (old_oids && old_oids->nr != refnames->nr) + BUG("refname and old OID counts do not match"); if (!refnames->nr) return 0; msg = normalize_reflog_message(logmsg); - /* - * Since we don't check the references' old_oids, the - * individual updates can't fail, so we can pack all of the - * updates into a single transaction. - */ transaction = ref_store_transaction_begin(refs, 0, &err); if (!transaction) { ret = error("%s", err.buf); goto out; } - for_each_string_list_item(item, refnames) { - ret = ref_transaction_delete(transaction, item->string, - NULL, NULL, flags, msg, &err); + for (i = 0; i < refnames->nr; i++) { + const struct object_id *old_oid = old_oids ? &old_oids->oid[i] : NULL; + + if (old_oid && is_null_oid(old_oid)) + old_oid = NULL; + ret = ref_transaction_delete(transaction, refnames->items[i].string, + old_oid, NULL, flags, msg, &err); if (ret) { warning(_("could not delete reference %s: %s"), - item->string, err.buf); + refnames->items[i].string, err.buf); strbuf_reset(&err); failures = 1; } diff --git a/refs.h b/refs.h index 71d5c186d0..b76b556cf7 100644 --- a/refs.h +++ b/refs.h @@ -9,6 +9,7 @@ struct fsck_options; struct object_id; struct ref_store; +struct oid_array; struct strbuf; struct string_list; struct string_list_item; @@ -613,13 +614,20 @@ int refs_delete_ref(struct ref_store *refs, const char *msg, unsigned int flags); /* - * Delete the specified references. If there are any problems, emit + * Delete the specified references. If old_oids is non-NULL, it must contain + * an entry for each refname, in the same order. Each non-null entry is used + * to verify the current value of the corresponding reference before deleting + * it. A null entry disables verification for that reference. + * + * If there are any problems, emit * errors but attempt to keep going (i.e., the deletes are not done in * an all-or-nothing transaction). msg and flags are passed through to * ref_transaction_delete(). */ int refs_delete_refs(struct ref_store *refs, const char *msg, - struct string_list *refnames, unsigned int flags); + struct string_list *refnames, + const struct oid_array *old_oids, + unsigned int flags); /** Delete a reflog */ int refs_delete_reflog(struct ref_store *refs, const char *refname); diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c index 3866d0aca4..c2c7dfb065 100644 --- a/t/helper/test-ref-store.c +++ b/t/helper/test-ref-store.c @@ -140,7 +140,7 @@ static int cmd_delete_refs(struct ref_store *refs, const char **argv) while (*argv) string_list_append(&refnames, *argv++); - result = refs_delete_refs(refs, msg, &refnames, flags); + result = refs_delete_refs(refs, msg, &refnames, NULL, flags); string_list_clear(&refnames, 0); return result; } -- 2.39.3 (Apple Git-146) ``` ## Maciej Ciemborowicz, 2026-09-19 20:11 Subject: [PATCH 2/3] branch, tag: retain old OIDs in batched deletions Message-ID: <20260919201158.43415-3-maciej.ciemborowicz@gmail.com> In-Reply-To: <20260919201158.43415-1-maciej.ciemborowicz@gmail.com> ``` Since 8198907795 (use delete_refs when deleting tags or branches, 2021-01-21), branch and tag deletion pass no old OIDs to the ref transaction. As a result, reference-transaction hooks report zero as both the old and new OID. Both commands already resolve the old OIDs before starting the deletion. Pass those values to refs_delete_refs() so hooks receive useful old values without adding any ref reads. Signed-off-by: Maciej Ciemborowicz --- builtin/branch.c | 6 +++++- builtin/tag.c | 6 +++++- t/t1416-ref-transaction-hooks.sh | 28 ++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index f1abeb681d..9f03ebc095 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -16,6 +16,7 @@ #include "commit.h" #include "gettext.h" #include "object-name.h" +#include "oid-array.h" #include "remote.h" #include "parse-options.h" #include "branch.h" @@ -230,6 +231,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, struct strbuf bname = STRBUF_INIT; enum interpret_branch_kind allowed_interpret; struct string_list refs_to_delete = STRING_LIST_INIT_DUP; + struct oid_array old_oids = OID_ARRAY_INIT; struct string_list_item *item; int branch_name_pos; const char *fmt_remotes = "refs/remotes/%s"; @@ -314,6 +316,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, } item = string_list_append(&refs_to_delete, name); + oid_array_append(&old_oids, &oid); item->util = xstrdup((flags & REF_ISBROKEN) ? "broken" : (flags & REF_ISSYMREF) ? target : repo_find_unique_abbrev(the_repository, &oid, DEFAULT_ABBREV)); @@ -323,7 +326,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, } if (refs_delete_refs(get_main_ref_store(the_repository), NULL, - &refs_to_delete, NULL, REF_NO_DEREF)) + &refs_to_delete, &old_oids, REF_NO_DEREF)) ret = 1; for_each_string_list_item(item, &refs_to_delete) { @@ -342,6 +345,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, free(describe_ref); } string_list_clear(&refs_to_delete, 0); + oid_array_clear(&old_oids); free(name); strbuf_release(&bname); diff --git a/builtin/tag.c b/builtin/tag.c index 40874a2923..0a3eb70faf 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -119,11 +119,14 @@ static int delete_tags(const char **argv) { int result; struct string_list refs_to_delete = STRING_LIST_INIT_DUP; + struct oid_array old_oids = OID_ARRAY_INIT; struct string_list_item *item; result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); + for_each_string_list_item(item, &refs_to_delete) + oid_array_append(&old_oids, item->util); if (refs_delete_refs(get_main_ref_store(the_repository), NULL, - &refs_to_delete, NULL, REF_NO_DEREF)) + &refs_to_delete, &old_oids, REF_NO_DEREF)) result = 1; for_each_string_list_item(item, &refs_to_delete) { @@ -137,6 +140,7 @@ static int delete_tags(const char **argv) free(oid); } string_list_clear(&refs_to_delete, 0); + oid_array_clear(&old_oids); return result; } diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh index 4fe9d9b234..8d400cd7ac 100755 --- a/t/t1416-ref-transaction-hooks.sh +++ b/t/t1416-ref-transaction-hooks.sh @@ -14,6 +14,34 @@ test_expect_success setup ' POST_OID=$(git rev-parse POST) ' +test_expect_success 'hook gets old values for batched branch/tag deletion' ' + test_when_finished "rm -f actual" && + git branch to-delete PRE && + git tag delete-tag POST && + git pack-refs --all && + test_hook reference-transaction <<-\EOF && + if test "$1" = committed + then + # Ignore backend-internal zero-to-zero records. + while read -r old new ref + do + case "$old" in + *[!0]*) + echo "$old $new $ref" + ;; + esac + done >>actual + fi + EOF + cat >expect <<-EOF && + $PRE_OID $ZERO_OID refs/heads/to-delete + $POST_OID $ZERO_OID refs/tags/delete-tag + EOF + git branch -D to-delete && + git tag -d delete-tag && + test_cmp expect actual +' + test_expect_success 'hook allows updating ref if successful' ' git reset --hard PRE && test_hook reference-transaction <<-\EOF && -- 2.39.3 (Apple Git-146) ``` ## Maciej Ciemborowicz, 2026-09-19 20:11 Subject: [PATCH 3/3] fetch, remote: retain old OIDs when pruning refs Message-ID: <20260919201158.43415-4-maciej.ciemborowicz@gmail.com> In-Reply-To: <20260919201158.43415-1-maciej.ciemborowicz@gmail.com> ``` get_stale_heads() records the current value of each stale local ref in its new_oid member. The pruning paths discard that value and request unconditional deletion, so reference-transaction hooks receive a null old OID. Carry the recorded values into the deletion transactions. This reuses data collected while finding stale refs and therefore requires no additional ref reads. Signed-off-by: Maciej Ciemborowicz --- builtin/fetch.c | 11 ++++++--- builtin/remote.c | 30 +++++++++++++++++++---- t/t1416-ref-transaction-hooks.sh | 42 ++++++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 8 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index d202147b21..a982d7541f 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1452,14 +1452,18 @@ static int prune_refs(struct display_state *display_state, struct ref *ref, *stale_refs = get_stale_heads(rs, ref_map); struct strbuf err = STRBUF_INIT; struct string_list refnames = STRING_LIST_INIT_NODUP; + struct oid_array old_oids = OID_ARRAY_INIT; - for (ref = stale_refs; ref; ref = ref->next) + for (ref = stale_refs; ref; ref = ref->next) { string_list_append(&refnames, ref->name); + oid_array_append(&old_oids, &ref->new_oid); + } if (!dry_run) { if (transaction) { for (ref = stale_refs; ref; ref = ref->next) { - result = ref_transaction_delete(transaction, ref->name, NULL, + result = ref_transaction_delete(transaction, ref->name, + &ref->new_oid, NULL, 0, "fetch: prune", &err); if (result) goto cleanup; @@ -1467,7 +1471,7 @@ static int prune_refs(struct display_state *display_state, } else { result = refs_delete_refs(get_main_ref_store(the_repository), "fetch: prune", &refnames, - NULL, 0); + &old_oids, 0); } } @@ -1487,6 +1491,7 @@ static int prune_refs(struct display_state *display_state, cleanup: string_list_clear(&refnames, 0); + oid_array_clear(&old_oids); strbuf_release(&err); free_refs(stale_refs); return result; diff --git a/builtin/remote.c b/builtin/remote.c index 13d3cc52dd..7b0ad13342 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -17,6 +17,7 @@ #include "refs.h" #include "refspec.h" #include "odb.h" +#include "oid-array.h" #include "strvec.h" #include "commit-reach.h" #include "progress.h" @@ -380,6 +381,11 @@ struct ref_states { int queried; }; +struct stale_ref { + struct object_id oid; + char name[FLEX_ARRAY]; +}; + #define REF_STATES_INIT { \ .new_refs = STRING_LIST_INIT_DUP, \ .skipped = STRING_LIST_INIT_DUP, \ @@ -410,9 +416,13 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat } stale_refs = get_stale_heads(&states->remote->fetch, fetch_map); for (ref = stale_refs; ref; ref = ref->next) { + struct stale_ref *stale_ref; struct string_list_item *item = string_list_append(&states->stale, abbrev_branch(ref->name)); - item->util = xstrdup(ref->name); + + FLEX_ALLOC_STR(stale_ref, name, ref->name); + oidcpy(&stale_ref->oid, &ref->new_oid); + item->util = stale_ref; } free_refs(stale_refs); free_refs(fetch_map); @@ -1627,6 +1637,7 @@ static int prune_remote(const char *remote, int dry_run) int result = 0; struct ref_states states = REF_STATES_INIT; struct string_list refs_to_prune = STRING_LIST_INIT_NODUP; + struct oid_array old_oids = OID_ARRAY_INIT; struct string_list_item *item; get_remote_ref_states(remote, &states, GET_REF_STATES); @@ -1639,17 +1650,25 @@ static int prune_remote(const char *remote, int dry_run) printf_ln(_("Pruning %s"), remote); printf_ln(_("URL: %s"), states.remote->url.v[0]); - for_each_string_list_item(item, &states.stale) - string_list_append(&refs_to_prune, item->util); + for_each_string_list_item(item, &states.stale) { + struct stale_ref *stale_ref = item->util; + struct string_list_item *to_prune; + + to_prune = string_list_append(&refs_to_prune, stale_ref->name); + to_prune->util = &stale_ref->oid; + } string_list_sort(&refs_to_prune); + for_each_string_list_item(item, &refs_to_prune) + oid_array_append(&old_oids, item->util); if (!dry_run) result |= refs_delete_refs(get_main_ref_store(the_repository), "remote: prune", &refs_to_prune, - NULL, 0); + &old_oids, 0); for_each_string_list_item(item, &states.stale) { - const char *refname = item->util; + struct stale_ref *stale_ref = item->util; + const char *refname = stale_ref->name; if (dry_run) printf_ln(_(" * [would prune] %s"), @@ -1663,6 +1682,7 @@ static int prune_remote(const char *remote, int dry_run) stdout, " ", dry_run, &refs_to_prune); string_list_clear(&refs_to_prune, 0); + oid_array_clear(&old_oids); free_remote_ref_states(&states); return result; } diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh index 8d400cd7ac..39bdc1bc26 100755 --- a/t/t1416-ref-transaction-hooks.sh +++ b/t/t1416-ref-transaction-hooks.sh @@ -42,6 +42,48 @@ test_expect_success 'hook gets old values for batched branch/tag deletion' ' test_cmp expect actual ' +test_expect_success 'hook gets old values when pruning remote refs' ' + test_create_repo empty.git --bare && + test_create_repo prune && + git -C prune remote add origin ../empty.git && + test_commit -C prune one && + one=$(git -C prune rev-parse HEAD) && + test_commit -C prune two && + two=$(git -C prune rev-parse HEAD) && + git -C prune update-ref refs/remotes/origin/remote-prune-z "$one" && + git -C prune update-ref refs/remotes/origin/remote-prune-a "$two" && + git -C prune pack-refs --all && + test_hook -C prune reference-transaction <<-\EOF && + if test "$1" = committed + then + # Ignore backend-internal zero-to-zero records. + while read -r old new ref + do + case "$old" in + *[!0]*) + echo "$old $new $ref" + ;; + esac + done >>actual + fi + EOF + ( + cd prune && + git remote prune origin && + git update-ref refs/remotes/origin/fetch-prune "$one" && + git fetch --prune origin && + git update-ref refs/remotes/origin/atomic-prune "$one" && + git fetch --atomic --prune origin && + cat >expect <<-EOF && + $two $ZERO_OID refs/remotes/origin/remote-prune-a + $one $ZERO_OID refs/remotes/origin/remote-prune-z + $one $ZERO_OID refs/remotes/origin/fetch-prune + $one $ZERO_OID refs/remotes/origin/atomic-prune + EOF + test_cmp expect actual + ) +' + test_expect_success 'hook allows updating ref if successful' ' git reset --hard PRE && test_hook reference-transaction <<-\EOF && -- 2.39.3 (Apple Git-146) ``` ## Karthik Nayak, 2026-09-19 20:41 Subject: Re: [PATCH 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: <20260919201158.43415-2-maciej.ciemborowicz@gmail.com> ``` Maciej Ciemborowicz writes: > refs_delete_refs() currently performs unconditional deletions. Thus callers > cannot preserve old values that they have already resolved, and > reference-transaction hooks consequently see a null old OID. > > Add an optional oid_array whose entries correspond to the refnames. Pass each > non-null OID to ref_transaction_delete(). Existing callers retain the > unconditional behavior for now. > > Signed-off-by: Maciej Ciemborowicz > --- > bisect.c | 2 +- > builtin/branch.c | 3 ++- > builtin/fetch.c | 2 +- > builtin/remote.c | 5 +++-- > builtin/tag.c | 3 ++- > refs.c | 26 +++++++++++++++----------- > refs.h | 12 ++++++++++-- > t/helper/test-ref-store.c | 2 +- > 8 files changed, 35 insertions(+), 20 deletions(-) > [snip] > diff --git a/refs.c b/refs.c > index d3caa9a633..a9c5397fd7 100644 > --- a/refs.c > +++ b/refs.c > @@ -18,6 +18,7 @@ > #include "refs/refs-internal.h" > #include "hook.h" > #include "object-name.h" > +#include "oid-array.h" > #include "odb.h" > #include "object.h" > #include "path.h" > @@ -3056,36 +3057,39 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio > } > > int refs_delete_refs(struct ref_store *refs, const char *logmsg, > - struct string_list *refnames, unsigned int flags) > + struct string_list *refnames, > + const struct oid_array *old_oids, > + unsigned int flags) > { > struct ref_transaction *transaction; > struct strbuf err = STRBUF_INIT; > - struct string_list_item *item; > + size_t i; > int ret = 0, failures = 0; > char *msg; > > + if (old_oids && old_oids->nr != refnames->nr) > + BUG("refname and old OID counts do not match"); > if (!refnames->nr) > return 0; > > msg = normalize_reflog_message(logmsg); > > - /* > - * Since we don't check the references' old_oids, the > - * individual updates can't fail, so we can pack all of the > - * updates into a single transaction. > - */ I understand that this is intended to fix a bug. With this change, `refs_delete_refs()`'s behavior has changed from unconditionally deleting all refs to now only deleting the refs if all old OIDs match. Doesn't this introduce possibility of a race since callees of the function who checked the ref's OID before can now expect a failure when the transaction re-checks the old_oid? > transaction = ref_store_transaction_begin(refs, 0, &err); > if (!transaction) { > ret = error("%s", err.buf); > goto out; > } > > - for_each_string_list_item(item, refnames) { > - ret = ref_transaction_delete(transaction, item->string, > - NULL, NULL, flags, msg, &err); > + for (i = 0; i < refnames->nr; i++) { > + const struct object_id *old_oid = old_oids ? &old_oids->oid[i] : NULL; > + Nit: we could add a `struct string_list_item *item = refnames->items[i];` for a nicer diff. > + if (old_oid && is_null_oid(old_oid)) > + old_oid = NULL; > + ret = ref_transaction_delete(transaction, refnames->items[i].string, > + old_oid, NULL, flags, msg, &err); > if (ret) { > warning(_("could not delete reference %s: %s"), > - item->string, err.buf); > + refnames->items[i].string, err.buf); > strbuf_reset(&err); > failures = 1; > } > diff --git a/refs.h b/refs.h > index 71d5c186d0..b76b556cf7 100644 > --- a/refs.h > +++ b/refs.h > @@ -9,6 +9,7 @@ > struct fsck_options; > struct object_id; > struct ref_store; > +struct oid_array; > struct strbuf; > struct string_list; > struct string_list_item; > @@ -613,13 +614,20 @@ int refs_delete_ref(struct ref_store *refs, const char *msg, > unsigned int flags); > > /* > - * Delete the specified references. If there are any problems, emit > + * Delete the specified references. If old_oids is non-NULL, it must contain > + * an entry for each refname, in the same order. Each non-null entry is used > + * to verify the current value of the corresponding reference before deleting > + * it. A null entry disables verification for that reference. > + * > + * If there are any problems, emit > * errors but attempt to keep going (i.e., the deletes are not done in > * an all-or-nothing transaction). msg and flags are passed through to > * ref_transaction_delete(). > */ > int refs_delete_refs(struct ref_store *refs, const char *msg, > - struct string_list *refnames, unsigned int flags); > + struct string_list *refnames, > + const struct oid_array *old_oids, > + unsigned int flags); > > /** Delete a reflog */ > int refs_delete_reflog(struct ref_store *refs, const char *refname); > diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c > index 3866d0aca4..c2c7dfb065 100644 > --- a/t/helper/test-ref-store.c > +++ b/t/helper/test-ref-store.c > @@ -140,7 +140,7 @@ static int cmd_delete_refs(struct ref_store *refs, const char **argv) > while (*argv) > string_list_append(&refnames, *argv++); > > - result = refs_delete_refs(refs, msg, &refnames, flags); > + result = refs_delete_refs(refs, msg, &refnames, NULL, flags); > string_list_clear(&refnames, 0); > return result; > } > -- > 2.39.3 (Apple Git-146) ``` ## Maciej Ciemborowicz, 2026-09-20 10:38 Subject: Re: [PATCH 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: <20260920103855.19874-1-maciej.ciemborowicz@gmail.com> In-Reply-To: ``` Thanks. Yes, supplying old_oid changes these deletions from unconditional to compare-and-delete, so a concurrent ref change can make the transaction fail. I should have called that out explicitly. I think that failure is desirable here: otherwise the command can delete a value that it never examined. For branch and tag deletion this also restores the behavior from before 8198907795 (use delete_refs when deleting tags or branches, 2021-01-21), where delete_ref() was passed the OID that had been resolved by the caller. That commit batched the deletes through delete_refs(), but the expected OIDs were lost in the conversion. I reproduced the race with a reference-transaction hook that updates the branch during the "preparing" phase, after delete_branches() has collected its OID. Current Git returns success and deletes the concurrently updated branch. With this series, the outer transaction fails its old-OID check and leaves the new value intact. The same check prevents pruning based on a stale scan from deleting a ref that another process updated meanwhile. I will make this behavior change explicit in the commit messages and add a regression test for the concurrent update. Your comment also exposed that remote prune can print "[pruned]" after such a deletion failure; I will fix that reporting in v2 as well. And agreed on using a local item variable for the loop; I will include that in v2. Thanks, Maciej ``` ## Maciej Ciemborowicz, 2026-09-20 10:54 Subject: [PATCH v2 0/3] refs: report old OIDs for batched deletions Message-ID: In-Reply-To: ``` The reference-transaction hook receives zero as both the old and new OID when branch, tag, fetch, and remote delete refs through refs_delete_refs(). Those callers already know the values that they selected for deletion. Teach refs_delete_refs() to accept aligned old OIDs and pass them into the transaction. Besides making the hook records useful, this makes the selected callers reject concurrent changes instead of deleting values that they did not inspect. For branch and tag, this restores the compare-and-delete behavior that existed before 8198907795 converted them to batched deletion. For pruning, it prevents a stale scan from deleting a ref updated by another process. The values are already available at every updated call site, so the series adds no ref reads and retains batched performance. Changes since v1: * Document the conditional deletion behavior and its race protection. * Add tests that update refs from the hook's preparing phase and verify that branch deletion and remote pruning preserve the concurrent update. * Avoid printing deletion status when a non-atomic prune fails. * Use a local string_list_item in refs_delete_refs(), as suggested by Karthik. Based on maint at e9019fcafe (Git 2.55). Tests: * t1416-ref-transaction-hooks.sh (files and reftable) * t3200-branch.sh * t7004-tag.sh * t5510-fetch.sh * t5505-remote.sh Maciej Ciemborowicz (3): refs: allow callers to supply old OIDs for batch deletion branch, tag: retain old OIDs in batched deletions fetch, remote: retain old OIDs when pruning refs bisect.c | 2 +- builtin/branch.c | 7 +- builtin/fetch.c | 13 +++- builtin/remote.c | 39 +++++++++-- builtin/tag.c | 7 +- refs.c | 23 ++++--- refs.h | 12 +++- t/helper/test-ref-store.c | 2 +- t/t1416-ref-transaction-hooks.sh | 110 +++++++++++++++++++++++++++++++ 9 files changed, 190 insertions(+), 25 deletions(-) Range-diff against v1: 1: e1c72cfba ! 1: 5c96a5a1e refs: allow callers to supply old OIDs for batch deletion @@ Commit message reference-transaction hooks consequently see a null old OID. Add an optional oid_array whose entries correspond to the refnames. Pass each - non-null OID to ref_transaction_delete(). Existing callers retain the - unconditional behavior for now. + non-null OID to ref_transaction_delete(). Supplying an OID makes the deletion + conditional: if the ref changed after the caller resolved it, the transaction + fails instead of deleting the new value. Existing callers that pass NULL + retain the unconditional behavior. Signed-off-by: Maciej Ciemborowicz @@ refs.c: void ref_transaction_for_each_rejected_update(struct ref_transaction *tr } - for_each_string_list_item(item, refnames) { -- ret = ref_transaction_delete(transaction, item->string, -- NULL, NULL, flags, msg, &err); + for (i = 0; i < refnames->nr; i++) { ++ struct string_list_item *item = &refnames->items[i]; + const struct object_id *old_oid = old_oids ? &old_oids->oid[i] : NULL; + + if (old_oid && is_null_oid(old_oid)) + old_oid = NULL; -+ ret = ref_transaction_delete(transaction, refnames->items[i].string, + ret = ref_transaction_delete(transaction, item->string, +- NULL, NULL, flags, msg, &err); + old_oid, NULL, flags, msg, &err); if (ret) { warning(_("could not delete reference %s: %s"), -- item->string, err.buf); -+ refnames->items[i].string, err.buf); - strbuf_reset(&err); - failures = 1; - } + item->string, err.buf); ## refs.h ## @@ 2: 09e0b8557 ! 2: d00fdeba2 branch, tag: retain old OIDs in batched deletions @@ Metadata ## Commit message ## branch, tag: retain old OIDs in batched deletions - Since 8198907795 (use delete_refs when deleting tags or branches, - 2021-01-21), branch and tag deletion pass no old OIDs to the ref transaction. - As a result, reference-transaction hooks report zero as both the old and new - OID. + Before 8198907795 (use delete_refs when deleting tags or branches, + 2021-01-21), branch and tag deletion passed each resolved old OID to + delete_ref(). This prevented the command from deleting a ref that another + process had changed after it was inspected. - Both commands already resolve the old OIDs before starting the deletion. Pass - those values to refs_delete_refs() so hooks receive useful old values without - adding any ref reads. + The conversion to batched deletion dropped those old OIDs. Besides making the + deletions unconditional, this causes reference-transaction hooks to report + zero as both the old and new OID. + + Both commands still resolve the old OIDs before starting the deletion. Pass + those values to refs_delete_refs(). This restores the old race protection and + lets hooks receive useful old values without adding any ref reads. If a ref + changes concurrently, the transaction fails and preserves the new value. Signed-off-by: Maciej Ciemborowicz @@ t/t1416-ref-transaction-hooks.sh: test_expect_success setup ' + git tag -d delete-tag && + test_cmp expect actual +' ++ ++test_expect_success 'branch deletion rejects a concurrent update' ' ++ git branch delete-race PRE && ++ test_hook reference-transaction <<-\EOF && ++ marker=$(git rev-parse --git-path delete-race-once) ++ if test "$1" = preparing && test ! -e "$marker" ++ then ++ >"$marker" ++ git update-ref refs/heads/delete-race POST ++ fi ++ exit 0 ++ EOF ++ test_must_fail git branch -D delete-race 2>err && ++ test_grep "is at $POST_OID but expected $PRE_OID" err && ++ test_cmp_rev POST refs/heads/delete-race ++' + test_expect_success 'hook allows updating ref if successful' ' git reset --hard PRE && 3: 95c8abce3 ! 3: 461c36ccd fetch, remote: retain old OIDs when pruning refs @@ Commit message new_oid member. The pruning paths discard that value and request unconditional deletion, so reference-transaction hooks receive a null old OID. - Carry the recorded values into the deletion transactions. This reuses data - collected while finding stale refs and therefore requires no additional ref - reads. + Carry the recorded values into the deletion transactions. Besides giving the + hooks useful values, this stops a stale scan from deleting a ref that another + process updated before the transaction acquired its locks. A concurrent + change now makes the prune fail and preserves the new value. + + This reuses data collected while finding stale refs and therefore requires no + additional ref reads. Do not print deletion status when a non-atomic prune + fails its old-OID check. Signed-off-by: Maciej Ciemborowicz @@ builtin/fetch.c: static int prune_refs(struct display_state *display_state, - NULL, 0); + &old_oids, 0); } ++ if (result) ++ goto cleanup; } + if (verbosity >= 0) { @@ builtin/fetch.c: static int prune_refs(struct display_state *display_state, cleanup: @@ builtin/remote.c: static int prune_remote(const char *remote, int dry_run) + for_each_string_list_item(item, &refs_to_prune) + oid_array_append(&old_oids, item->util); - if (!dry_run) +- if (!dry_run) ++ if (!dry_run) { result |= refs_delete_refs(get_main_ref_store(the_repository), "remote: prune", &refs_to_prune, - NULL, 0); + &old_oids, 0); ++ if (result) ++ goto cleanup; ++ } for_each_string_list_item(item, &states.stale) { - const char *refname = item->util; @@ builtin/remote.c: static int prune_remote(const char *remote, int dry_run) if (dry_run) printf_ln(_(" * [would prune] %s"), @@ builtin/remote.c: static int prune_remote(const char *remote, int dry_run) + refs_warn_dangling_symrefs(get_main_ref_store(the_repository), stdout, " ", dry_run, &refs_to_prune); ++cleanup: string_list_clear(&refs_to_prune, 0); + oid_array_clear(&old_oids); free_remote_ref_states(&states); @@ builtin/remote.c: static int prune_remote(const char *remote, int dry_run) } ## t/t1416-ref-transaction-hooks.sh ## -@@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'hook gets old values for batched branch/tag deletion' ' - test_cmp expect actual +@@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'branch deletion rejects a concurrent update' ' + test_cmp_rev POST refs/heads/delete-race ' +test_expect_success 'hook gets old values when pruning remote refs' ' @@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'hook gets old values for + test_cmp expect actual + ) +' ++ ++test_expect_success 'remote prune rejects a concurrent update' ' ++ test_when_finished "rm -rf race-empty.git race-prune" && ++ test_create_repo race-empty.git --bare && ++ test_create_repo race-prune && ++ test_commit -C race-prune one && ++ one=$(git -C race-prune rev-parse HEAD) && ++ test_commit -C race-prune two && ++ two=$(git -C race-prune rev-parse HEAD) && ++ git -C race-prune remote add origin ../race-empty.git && ++ git -C race-prune update-ref refs/remotes/origin/race "$one" && ++ test_hook -C race-prune reference-transaction <<-\EOF && ++ marker=$(git rev-parse --git-path prune-race-once) ++ if test "$1" = preparing && test ! -e "$marker" ++ then ++ >"$marker" ++ git update-ref refs/remotes/origin/race HEAD ++ fi ++ exit 0 ++ EOF ++ test_must_fail git -C race-prune remote prune origin >out 2>err && ++ test "$two" = "$(git -C race-prune rev-parse refs/remotes/origin/race)" && ++ ! grep "\[pruned\]" out ++' + test_expect_success 'hook allows updating ref if successful' ' git reset --hard PRE && -- 2.39.3 (Apple Git-146) ``` ## Maciej Ciemborowicz, 2026-09-20 10:54 Subject: [PATCH v2 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: <5c96a5a1ebafd49a301c5c1dc77a2e19d6677ab0.1789901584.git.maciej.ciemborowicz@gmail.com> In-Reply-To: ``` refs_delete_refs() currently performs unconditional deletions. Thus callers cannot preserve old values that they have already resolved, and reference-transaction hooks consequently see a null old OID. Add an optional oid_array whose entries correspond to the refnames. Pass each non-null OID to ref_transaction_delete(). Supplying an OID makes the deletion conditional: if the ref changed after the caller resolved it, the transaction fails instead of deleting the new value. Existing callers that pass NULL retain the unconditional behavior. Signed-off-by: Maciej Ciemborowicz --- bisect.c | 2 +- builtin/branch.c | 3 ++- builtin/fetch.c | 2 +- builtin/remote.c | 5 +++-- builtin/tag.c | 3 ++- refs.c | 23 ++++++++++++++--------- refs.h | 12 ++++++++++-- t/helper/test-ref-store.c | 2 +- 8 files changed, 34 insertions(+), 18 deletions(-) diff --git a/bisect.c b/bisect.c index 94c7028d2..9aa3bace9 100644 --- a/bisect.c +++ b/bisect.c @@ -1203,7 +1203,7 @@ int bisect_clean_state(void) string_list_append(&refs_for_removal, "BISECT_EXPECTED_REV"); result = refs_delete_refs(get_main_ref_store(the_repository), "bisect: remove", &refs_for_removal, - REF_NO_DEREF); + NULL, REF_NO_DEREF); string_list_clear(&refs_for_removal, 0); unlink_or_warn(git_path_bisect_ancestors_ok()); unlink_or_warn(git_path_bisect_log()); diff --git a/builtin/branch.c b/builtin/branch.c index 1572a4f9e..f1abeb681 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -322,7 +322,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, free(target); } - if (refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) + if (refs_delete_refs(get_main_ref_store(the_repository), NULL, + &refs_to_delete, NULL, REF_NO_DEREF)) ret = 1; for_each_string_list_item(item, &refs_to_delete) { diff --git a/builtin/fetch.c b/builtin/fetch.c index c1d7c672f..d202147b2 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1467,7 +1467,7 @@ static int prune_refs(struct display_state *display_state, } else { result = refs_delete_refs(get_main_ref_store(the_repository), "fetch: prune", &refnames, - 0); + NULL, 0); } } diff --git a/builtin/remote.c b/builtin/remote.c index de989ea3b..13d3cc52d 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1073,7 +1073,7 @@ static int rm(int argc, const char **argv, const char *prefix, if (!result) result = refs_delete_refs(get_main_ref_store(the_repository), "remote: remove", &branches, - REF_NO_DEREF); + NULL, REF_NO_DEREF); string_list_clear(&branches, 0); if (skipped.nr) { @@ -1645,7 +1645,8 @@ static int prune_remote(const char *remote, int dry_run) if (!dry_run) result |= refs_delete_refs(get_main_ref_store(the_repository), - "remote: prune", &refs_to_prune, 0); + "remote: prune", &refs_to_prune, + NULL, 0); for_each_string_list_item(item, &states.stale) { const char *refname = item->util; diff --git a/builtin/tag.c b/builtin/tag.c index 06c125b53..40874a292 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -122,7 +122,8 @@ static int delete_tags(const char **argv) struct string_list_item *item; result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); - if (refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) + if (refs_delete_refs(get_main_ref_store(the_repository), NULL, + &refs_to_delete, NULL, REF_NO_DEREF)) result = 1; for_each_string_list_item(item, &refs_to_delete) { diff --git a/refs.c b/refs.c index d3caa9a63..9c593baea 100644 --- a/refs.c +++ b/refs.c @@ -18,6 +18,7 @@ #include "refs/refs-internal.h" #include "hook.h" #include "object-name.h" +#include "oid-array.h" #include "odb.h" #include "object.h" #include "path.h" @@ -3056,33 +3057,37 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio } int refs_delete_refs(struct ref_store *refs, const char *logmsg, - struct string_list *refnames, unsigned int flags) + struct string_list *refnames, + const struct oid_array *old_oids, + unsigned int flags) { struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; - struct string_list_item *item; + size_t i; int ret = 0, failures = 0; char *msg; + if (old_oids && old_oids->nr != refnames->nr) + BUG("refname and old OID counts do not match"); if (!refnames->nr) return 0; msg = normalize_reflog_message(logmsg); - /* - * Since we don't check the references' old_oids, the - * individual updates can't fail, so we can pack all of the - * updates into a single transaction. - */ transaction = ref_store_transaction_begin(refs, 0, &err); if (!transaction) { ret = error("%s", err.buf); goto out; } - for_each_string_list_item(item, refnames) { + for (i = 0; i < refnames->nr; i++) { + struct string_list_item *item = &refnames->items[i]; + const struct object_id *old_oid = old_oids ? &old_oids->oid[i] : NULL; + + if (old_oid && is_null_oid(old_oid)) + old_oid = NULL; ret = ref_transaction_delete(transaction, item->string, - NULL, NULL, flags, msg, &err); + old_oid, NULL, flags, msg, &err); if (ret) { warning(_("could not delete reference %s: %s"), item->string, err.buf); diff --git a/refs.h b/refs.h index 71d5c186d..b76b556cf 100644 --- a/refs.h +++ b/refs.h @@ -9,6 +9,7 @@ struct fsck_options; struct object_id; struct ref_store; +struct oid_array; struct strbuf; struct string_list; struct string_list_item; @@ -613,13 +614,20 @@ int refs_delete_ref(struct ref_store *refs, const char *msg, unsigned int flags); /* - * Delete the specified references. If there are any problems, emit + * Delete the specified references. If old_oids is non-NULL, it must contain + * an entry for each refname, in the same order. Each non-null entry is used + * to verify the current value of the corresponding reference before deleting + * it. A null entry disables verification for that reference. + * + * If there are any problems, emit * errors but attempt to keep going (i.e., the deletes are not done in * an all-or-nothing transaction). msg and flags are passed through to * ref_transaction_delete(). */ int refs_delete_refs(struct ref_store *refs, const char *msg, - struct string_list *refnames, unsigned int flags); + struct string_list *refnames, + const struct oid_array *old_oids, + unsigned int flags); /** Delete a reflog */ int refs_delete_reflog(struct ref_store *refs, const char *refname); diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c index 3866d0aca..c2c7dfb06 100644 --- a/t/helper/test-ref-store.c +++ b/t/helper/test-ref-store.c @@ -140,7 +140,7 @@ static int cmd_delete_refs(struct ref_store *refs, const char **argv) while (*argv) string_list_append(&refnames, *argv++); - result = refs_delete_refs(refs, msg, &refnames, flags); + result = refs_delete_refs(refs, msg, &refnames, NULL, flags); string_list_clear(&refnames, 0); return result; } -- 2.39.3 (Apple Git-146) ``` ## Maciej Ciemborowicz, 2026-09-20 10:54 Subject: [PATCH v2 2/3] branch, tag: retain old OIDs in batched deletions Message-ID: In-Reply-To: ``` Before 8198907795 (use delete_refs when deleting tags or branches, 2021-01-21), branch and tag deletion passed each resolved old OID to delete_ref(). This prevented the command from deleting a ref that another process had changed after it was inspected. The conversion to batched deletion dropped those old OIDs. Besides making the deletions unconditional, this causes reference-transaction hooks to report zero as both the old and new OID. Both commands still resolve the old OIDs before starting the deletion. Pass those values to refs_delete_refs(). This restores the old race protection and lets hooks receive useful old values without adding any ref reads. If a ref changes concurrently, the transaction fails and preserves the new value. Signed-off-by: Maciej Ciemborowicz --- builtin/branch.c | 6 ++++- builtin/tag.c | 6 ++++- t/t1416-ref-transaction-hooks.sh | 44 ++++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 2 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index f1abeb681..9f03ebc09 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -16,6 +16,7 @@ #include "commit.h" #include "gettext.h" #include "object-name.h" +#include "oid-array.h" #include "remote.h" #include "parse-options.h" #include "branch.h" @@ -230,6 +231,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, struct strbuf bname = STRBUF_INIT; enum interpret_branch_kind allowed_interpret; struct string_list refs_to_delete = STRING_LIST_INIT_DUP; + struct oid_array old_oids = OID_ARRAY_INIT; struct string_list_item *item; int branch_name_pos; const char *fmt_remotes = "refs/remotes/%s"; @@ -314,6 +316,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, } item = string_list_append(&refs_to_delete, name); + oid_array_append(&old_oids, &oid); item->util = xstrdup((flags & REF_ISBROKEN) ? "broken" : (flags & REF_ISSYMREF) ? target : repo_find_unique_abbrev(the_repository, &oid, DEFAULT_ABBREV)); @@ -323,7 +326,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, } if (refs_delete_refs(get_main_ref_store(the_repository), NULL, - &refs_to_delete, NULL, REF_NO_DEREF)) + &refs_to_delete, &old_oids, REF_NO_DEREF)) ret = 1; for_each_string_list_item(item, &refs_to_delete) { @@ -342,6 +345,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, free(describe_ref); } string_list_clear(&refs_to_delete, 0); + oid_array_clear(&old_oids); free(name); strbuf_release(&bname); diff --git a/builtin/tag.c b/builtin/tag.c index 40874a292..0a3eb70fa 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -119,11 +119,14 @@ static int delete_tags(const char **argv) { int result; struct string_list refs_to_delete = STRING_LIST_INIT_DUP; + struct oid_array old_oids = OID_ARRAY_INIT; struct string_list_item *item; result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); + for_each_string_list_item(item, &refs_to_delete) + oid_array_append(&old_oids, item->util); if (refs_delete_refs(get_main_ref_store(the_repository), NULL, - &refs_to_delete, NULL, REF_NO_DEREF)) + &refs_to_delete, &old_oids, REF_NO_DEREF)) result = 1; for_each_string_list_item(item, &refs_to_delete) { @@ -137,6 +140,7 @@ static int delete_tags(const char **argv) free(oid); } string_list_clear(&refs_to_delete, 0); + oid_array_clear(&old_oids); return result; } diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh index 4fe9d9b23..01b5ba8c4 100755 --- a/t/t1416-ref-transaction-hooks.sh +++ b/t/t1416-ref-transaction-hooks.sh @@ -14,6 +14,50 @@ test_expect_success setup ' POST_OID=$(git rev-parse POST) ' +test_expect_success 'hook gets old values for batched branch/tag deletion' ' + test_when_finished "rm -f actual" && + git branch to-delete PRE && + git tag delete-tag POST && + git pack-refs --all && + test_hook reference-transaction <<-\EOF && + if test "$1" = committed + then + # Ignore backend-internal zero-to-zero records. + while read -r old new ref + do + case "$old" in + *[!0]*) + echo "$old $new $ref" + ;; + esac + done >>actual + fi + EOF + cat >expect <<-EOF && + $PRE_OID $ZERO_OID refs/heads/to-delete + $POST_OID $ZERO_OID refs/tags/delete-tag + EOF + git branch -D to-delete && + git tag -d delete-tag && + test_cmp expect actual +' + +test_expect_success 'branch deletion rejects a concurrent update' ' + git branch delete-race PRE && + test_hook reference-transaction <<-\EOF && + marker=$(git rev-parse --git-path delete-race-once) + if test "$1" = preparing && test ! -e "$marker" + then + >"$marker" + git update-ref refs/heads/delete-race POST + fi + exit 0 + EOF + test_must_fail git branch -D delete-race 2>err && + test_grep "is at $POST_OID but expected $PRE_OID" err && + test_cmp_rev POST refs/heads/delete-race +' + test_expect_success 'hook allows updating ref if successful' ' git reset --hard PRE && test_hook reference-transaction <<-\EOF && -- 2.39.3 (Apple Git-146) ``` ## Maciej Ciemborowicz, 2026-09-20 10:54 Subject: [PATCH v2 3/3] fetch, remote: retain old OIDs when pruning refs Message-ID: <461c36ccdae09fb827a3c0efc7eed5aef072e09b.1789901584.git.maciej.ciemborowicz@gmail.com> In-Reply-To: ``` get_stale_heads() records the current value of each stale local ref in its new_oid member. The pruning paths discard that value and request unconditional deletion, so reference-transaction hooks receive a null old OID. Carry the recorded values into the deletion transactions. Besides giving the hooks useful values, this stops a stale scan from deleting a ref that another process updated before the transaction acquired its locks. A concurrent change now makes the prune fail and preserves the new value. This reuses data collected while finding stale refs and therefore requires no additional ref reads. Do not print deletion status when a non-atomic prune fails its old-OID check. Signed-off-by: Maciej Ciemborowicz --- builtin/fetch.c | 13 +++++-- builtin/remote.c | 36 ++++++++++++++--- t/t1416-ref-transaction-hooks.sh | 66 ++++++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 9 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index d202147b2..da413ace0 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1452,14 +1452,18 @@ static int prune_refs(struct display_state *display_state, struct ref *ref, *stale_refs = get_stale_heads(rs, ref_map); struct strbuf err = STRBUF_INIT; struct string_list refnames = STRING_LIST_INIT_NODUP; + struct oid_array old_oids = OID_ARRAY_INIT; - for (ref = stale_refs; ref; ref = ref->next) + for (ref = stale_refs; ref; ref = ref->next) { string_list_append(&refnames, ref->name); + oid_array_append(&old_oids, &ref->new_oid); + } if (!dry_run) { if (transaction) { for (ref = stale_refs; ref; ref = ref->next) { - result = ref_transaction_delete(transaction, ref->name, NULL, + result = ref_transaction_delete(transaction, ref->name, + &ref->new_oid, NULL, 0, "fetch: prune", &err); if (result) goto cleanup; @@ -1467,8 +1471,10 @@ static int prune_refs(struct display_state *display_state, } else { result = refs_delete_refs(get_main_ref_store(the_repository), "fetch: prune", &refnames, - NULL, 0); + &old_oids, 0); } + if (result) + goto cleanup; } if (verbosity >= 0) { @@ -1487,6 +1493,7 @@ static int prune_refs(struct display_state *display_state, cleanup: string_list_clear(&refnames, 0); + oid_array_clear(&old_oids); strbuf_release(&err); free_refs(stale_refs); return result; diff --git a/builtin/remote.c b/builtin/remote.c index 13d3cc52d..b899bec55 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -17,6 +17,7 @@ #include "refs.h" #include "refspec.h" #include "odb.h" +#include "oid-array.h" #include "strvec.h" #include "commit-reach.h" #include "progress.h" @@ -380,6 +381,11 @@ struct ref_states { int queried; }; +struct stale_ref { + struct object_id oid; + char name[FLEX_ARRAY]; +}; + #define REF_STATES_INIT { \ .new_refs = STRING_LIST_INIT_DUP, \ .skipped = STRING_LIST_INIT_DUP, \ @@ -410,9 +416,13 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat } stale_refs = get_stale_heads(&states->remote->fetch, fetch_map); for (ref = stale_refs; ref; ref = ref->next) { + struct stale_ref *stale_ref; struct string_list_item *item = string_list_append(&states->stale, abbrev_branch(ref->name)); - item->util = xstrdup(ref->name); + + FLEX_ALLOC_STR(stale_ref, name, ref->name); + oidcpy(&stale_ref->oid, &ref->new_oid); + item->util = stale_ref; } free_refs(stale_refs); free_refs(fetch_map); @@ -1627,6 +1637,7 @@ static int prune_remote(const char *remote, int dry_run) int result = 0; struct ref_states states = REF_STATES_INIT; struct string_list refs_to_prune = STRING_LIST_INIT_NODUP; + struct oid_array old_oids = OID_ARRAY_INIT; struct string_list_item *item; get_remote_ref_states(remote, &states, GET_REF_STATES); @@ -1639,17 +1650,28 @@ static int prune_remote(const char *remote, int dry_run) printf_ln(_("Pruning %s"), remote); printf_ln(_("URL: %s"), states.remote->url.v[0]); - for_each_string_list_item(item, &states.stale) - string_list_append(&refs_to_prune, item->util); + for_each_string_list_item(item, &states.stale) { + struct stale_ref *stale_ref = item->util; + struct string_list_item *to_prune; + + to_prune = string_list_append(&refs_to_prune, stale_ref->name); + to_prune->util = &stale_ref->oid; + } string_list_sort(&refs_to_prune); + for_each_string_list_item(item, &refs_to_prune) + oid_array_append(&old_oids, item->util); - if (!dry_run) + if (!dry_run) { result |= refs_delete_refs(get_main_ref_store(the_repository), "remote: prune", &refs_to_prune, - NULL, 0); + &old_oids, 0); + if (result) + goto cleanup; + } for_each_string_list_item(item, &states.stale) { - const char *refname = item->util; + struct stale_ref *stale_ref = item->util; + const char *refname = stale_ref->name; if (dry_run) printf_ln(_(" * [would prune] %s"), @@ -1662,7 +1684,9 @@ static int prune_remote(const char *remote, int dry_run) refs_warn_dangling_symrefs(get_main_ref_store(the_repository), stdout, " ", dry_run, &refs_to_prune); +cleanup: string_list_clear(&refs_to_prune, 0); + oid_array_clear(&old_oids); free_remote_ref_states(&states); return result; } diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh index 01b5ba8c4..2b51b216b 100755 --- a/t/t1416-ref-transaction-hooks.sh +++ b/t/t1416-ref-transaction-hooks.sh @@ -58,6 +58,72 @@ test_expect_success 'branch deletion rejects a concurrent update' ' test_cmp_rev POST refs/heads/delete-race ' +test_expect_success 'hook gets old values when pruning remote refs' ' + test_create_repo empty.git --bare && + test_create_repo prune && + git -C prune remote add origin ../empty.git && + test_commit -C prune one && + one=$(git -C prune rev-parse HEAD) && + test_commit -C prune two && + two=$(git -C prune rev-parse HEAD) && + git -C prune update-ref refs/remotes/origin/remote-prune-z "$one" && + git -C prune update-ref refs/remotes/origin/remote-prune-a "$two" && + git -C prune pack-refs --all && + test_hook -C prune reference-transaction <<-\EOF && + if test "$1" = committed + then + # Ignore backend-internal zero-to-zero records. + while read -r old new ref + do + case "$old" in + *[!0]*) + echo "$old $new $ref" + ;; + esac + done >>actual + fi + EOF + ( + cd prune && + git remote prune origin && + git update-ref refs/remotes/origin/fetch-prune "$one" && + git fetch --prune origin && + git update-ref refs/remotes/origin/atomic-prune "$one" && + git fetch --atomic --prune origin && + cat >expect <<-EOF && + $two $ZERO_OID refs/remotes/origin/remote-prune-a + $one $ZERO_OID refs/remotes/origin/remote-prune-z + $one $ZERO_OID refs/remotes/origin/fetch-prune + $one $ZERO_OID refs/remotes/origin/atomic-prune + EOF + test_cmp expect actual + ) +' + +test_expect_success 'remote prune rejects a concurrent update' ' + test_when_finished "rm -rf race-empty.git race-prune" && + test_create_repo race-empty.git --bare && + test_create_repo race-prune && + test_commit -C race-prune one && + one=$(git -C race-prune rev-parse HEAD) && + test_commit -C race-prune two && + two=$(git -C race-prune rev-parse HEAD) && + git -C race-prune remote add origin ../race-empty.git && + git -C race-prune update-ref refs/remotes/origin/race "$one" && + test_hook -C race-prune reference-transaction <<-\EOF && + marker=$(git rev-parse --git-path prune-race-once) + if test "$1" = preparing && test ! -e "$marker" + then + >"$marker" + git update-ref refs/remotes/origin/race HEAD + fi + exit 0 + EOF + test_must_fail git -C race-prune remote prune origin >out 2>err && + test "$two" = "$(git -C race-prune rev-parse refs/remotes/origin/race)" && + ! grep "\[pruned\]" out +' + test_expect_success 'hook allows updating ref if successful' ' git reset --hard PRE && test_hook reference-transaction <<-\EOF && -- 2.39.3 (Apple Git-146) ``` ## Karthik Nayak, 2026-09-21 13:12 Subject: Re: [PATCH v2 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: <5c96a5a1ebafd49a301c5c1dc77a2e19d6677ab0.1789901584.git.maciej.ciemborowicz@gmail.com> ``` Maciej Ciemborowicz writes: > refs_delete_refs() currently performs unconditional deletions. Thus callers > cannot preserve old values that they have already resolved, and > reference-transaction hooks consequently see a null old OID. > > Add an optional oid_array whose entries correspond to the refnames. Pass each > non-null OID to ref_transaction_delete(). Supplying an OID makes the deletion > conditional: if the ref changed after the caller resolved it, the transaction > fails instead of deleting the new value. Existing callers that pass NULL > retain the unconditional behavior. > > Signed-off-by: Maciej Ciemborowicz > --- > bisect.c | 2 +- > builtin/branch.c | 3 ++- > builtin/fetch.c | 2 +- > builtin/remote.c | 5 +++-- > builtin/tag.c | 3 ++- > refs.c | 23 ++++++++++++++--------- > refs.h | 12 ++++++++++-- > t/helper/test-ref-store.c | 2 +- > 8 files changed, 34 insertions(+), 18 deletions(-) > > diff --git a/bisect.c b/bisect.c > index 94c7028d2..9aa3bace9 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -1203,7 +1203,7 @@ int bisect_clean_state(void) > string_list_append(&refs_for_removal, "BISECT_EXPECTED_REV"); > result = refs_delete_refs(get_main_ref_store(the_repository), > "bisect: remove", &refs_for_removal, > - REF_NO_DEREF); > + NULL, REF_NO_DEREF); > string_list_clear(&refs_for_removal, 0); > unlink_or_warn(git_path_bisect_ancestors_ok()); > unlink_or_warn(git_path_bisect_log()); > diff --git a/builtin/branch.c b/builtin/branch.c > index 1572a4f9e..f1abeb681 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -322,7 +322,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, > free(target); > } > > - if (refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) > + if (refs_delete_refs(get_main_ref_store(the_repository), NULL, > + &refs_to_delete, NULL, REF_NO_DEREF)) > ret = 1; > > for_each_string_list_item(item, &refs_to_delete) { > diff --git a/builtin/fetch.c b/builtin/fetch.c > index c1d7c672f..d202147b2 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -1467,7 +1467,7 @@ static int prune_refs(struct display_state *display_state, > } else { > result = refs_delete_refs(get_main_ref_store(the_repository), > "fetch: prune", &refnames, > - 0); > + NULL, 0); > } > } > > diff --git a/builtin/remote.c b/builtin/remote.c > index de989ea3b..13d3cc52d 100644 > --- a/builtin/remote.c > +++ b/builtin/remote.c > @@ -1073,7 +1073,7 @@ static int rm(int argc, const char **argv, const char *prefix, > if (!result) > result = refs_delete_refs(get_main_ref_store(the_repository), > "remote: remove", &branches, > - REF_NO_DEREF); > + NULL, REF_NO_DEREF); > string_list_clear(&branches, 0); > > if (skipped.nr) { > @@ -1645,7 +1645,8 @@ static int prune_remote(const char *remote, int dry_run) > > if (!dry_run) > result |= refs_delete_refs(get_main_ref_store(the_repository), > - "remote: prune", &refs_to_prune, 0); > + "remote: prune", &refs_to_prune, > + NULL, 0); > > for_each_string_list_item(item, &states.stale) { > const char *refname = item->util; > diff --git a/builtin/tag.c b/builtin/tag.c > index 06c125b53..40874a292 100644 > --- a/builtin/tag.c > +++ b/builtin/tag.c > @@ -122,7 +122,8 @@ static int delete_tags(const char **argv) > struct string_list_item *item; > > result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); > - if (refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) > + if (refs_delete_refs(get_main_ref_store(the_repository), NULL, > + &refs_to_delete, NULL, REF_NO_DEREF)) > result = 1; > > for_each_string_list_item(item, &refs_to_delete) { > diff --git a/refs.c b/refs.c > index d3caa9a63..9c593baea 100644 > --- a/refs.c > +++ b/refs.c > @@ -18,6 +18,7 @@ > #include "refs/refs-internal.h" > #include "hook.h" > #include "object-name.h" > +#include "oid-array.h" > #include "odb.h" > #include "object.h" > #include "path.h" > @@ -3056,33 +3057,37 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio > } > > int refs_delete_refs(struct ref_store *refs, const char *logmsg, > - struct string_list *refnames, unsigned int flags) > + struct string_list *refnames, > + const struct oid_array *old_oids, > + unsigned int flags) > { > struct ref_transaction *transaction; > struct strbuf err = STRBUF_INIT; > - struct string_list_item *item; > + size_t i; > int ret = 0, failures = 0; > char *msg; > > + if (old_oids && old_oids->nr != refnames->nr) > + BUG("refname and old OID counts do not match"); > if (!refnames->nr) > return 0; > > msg = normalize_reflog_message(logmsg); > > - /* > - * Since we don't check the references' old_oids, the > - * individual updates can't fail, so we can pack all of the > - * updates into a single transaction. > - */ Okay so we already have the error buf sent to the refs subsystem and the appropriate error will now be displayed. > transaction = ref_store_transaction_begin(refs, 0, &err); > if (!transaction) { > ret = error("%s", err.buf); > goto out; > } > > - for_each_string_list_item(item, refnames) { > + for (i = 0; i < refnames->nr; i++) { > + struct string_list_item *item = &refnames->items[i]; > + const struct object_id *old_oid = old_oids ? &old_oids->oid[i] : NULL; > + > + if (old_oid && is_null_oid(old_oid)) > + old_oid = NULL; We need to do this since `ref_transaction_delete()` doesn't expect old_oids set to zeroes. But why would a callee do this? Shouldn't this also be a bug, if the callee doesn't care about the previous value shouldn't they simply set `old_oids->oid[i] = NULL`? > ret = ref_transaction_delete(transaction, item->string, > - NULL, NULL, flags, msg, &err); > + old_oid, NULL, flags, msg, &err); > if (ret) { > warning(_("could not delete reference %s: %s"), > item->string, err.buf); > diff --git a/refs.h b/refs.h > index 71d5c186d..b76b556cf 100644 > --- a/refs.h > +++ b/refs.h > @@ -9,6 +9,7 @@ > struct fsck_options; > struct object_id; > struct ref_store; > +struct oid_array; > struct strbuf; > struct string_list; > struct string_list_item; > @@ -613,13 +614,20 @@ int refs_delete_ref(struct ref_store *refs, const char *msg, > unsigned int flags); > > /* > - * Delete the specified references. If there are any problems, emit > + * Delete the specified references. If old_oids is non-NULL, it must contain > + * an entry for each refname, in the same order. Each non-null entry is used > + * to verify the current value of the corresponding reference before deleting > + * it. A null entry disables verification for that reference. > + * Here too, we don't talk about zero-oid's. So I think we should skip the implicit conversion. > + * If there are any problems, emit > * errors but attempt to keep going (i.e., the deletes are not done in > * an all-or-nothing transaction). msg and flags are passed through to > * ref_transaction_delete(). > */ > int refs_delete_refs(struct ref_store *refs, const char *msg, > - struct string_list *refnames, unsigned int flags); > + struct string_list *refnames, > + const struct oid_array *old_oids, > + unsigned int flags); > > /** Delete a reflog */ > int refs_delete_reflog(struct ref_store *refs, const char *refname); > diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c > index 3866d0aca..c2c7dfb06 100644 > --- a/t/helper/test-ref-store.c > +++ b/t/helper/test-ref-store.c > @@ -140,7 +140,7 @@ static int cmd_delete_refs(struct ref_store *refs, const char **argv) > while (*argv) > string_list_append(&refnames, *argv++); > > - result = refs_delete_refs(refs, msg, &refnames, flags); > + result = refs_delete_refs(refs, msg, &refnames, NULL, flags); > string_list_clear(&refnames, 0); > return result; > } > -- > 2.39.3 (Apple Git-146) ``` ## Karthik Nayak, 2026-09-21 13:19 Subject: Re: [PATCH v2 2/3] branch, tag: retain old OIDs in batched deletions Message-ID: In-Reply-To: ``` Maciej Ciemborowicz writes: > Before 8198907795 (use delete_refs when deleting tags or branches, > 2021-01-21), branch and tag deletion passed each resolved old OID to > delete_ref(). This prevented the command from deleting a ref that another > process had changed after it was inspected. > > The conversion to batched deletion dropped those old OIDs. Besides making the > deletions unconditional, this causes reference-transaction hooks to report > zero as both the old and new OID. > > Both commands still resolve the old OIDs before starting the deletion. Pass > those values to refs_delete_refs(). This restores the old race protection and > lets hooks receive useful old values without adding any ref reads. If a ref > changes concurrently, the transaction fails and preserves the new value. > > Signed-off-by: Maciej Ciemborowicz > --- > builtin/branch.c | 6 ++++- > builtin/tag.c | 6 ++++- > t/t1416-ref-transaction-hooks.sh | 44 ++++++++++++++++++++++++++++++++ > 3 files changed, 54 insertions(+), 2 deletions(-) > > diff --git a/builtin/branch.c b/builtin/branch.c > index f1abeb681..9f03ebc09 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -16,6 +16,7 @@ > #include "commit.h" > #include "gettext.h" > #include "object-name.h" > +#include "oid-array.h" > #include "remote.h" > #include "parse-options.h" > #include "branch.h" > @@ -230,6 +231,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, > struct strbuf bname = STRBUF_INIT; > enum interpret_branch_kind allowed_interpret; > struct string_list refs_to_delete = STRING_LIST_INIT_DUP; > + struct oid_array old_oids = OID_ARRAY_INIT; > struct string_list_item *item; > int branch_name_pos; > const char *fmt_remotes = "refs/remotes/%s"; > @@ -314,6 +316,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, > } > > item = string_list_append(&refs_to_delete, name); > + oid_array_append(&old_oids, &oid); > item->util = xstrdup((flags & REF_ISBROKEN) ? "broken" > : (flags & REF_ISSYMREF) ? target > : repo_find_unique_abbrev(the_repository, &oid, DEFAULT_ABBREV)); > @@ -323,7 +326,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, > } > > if (refs_delete_refs(get_main_ref_store(the_repository), NULL, > - &refs_to_delete, NULL, REF_NO_DEREF)) > + &refs_to_delete, &old_oids, REF_NO_DEREF)) > ret = 1; > > for_each_string_list_item(item, &refs_to_delete) { > @@ -342,6 +345,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, > free(describe_ref); > } > string_list_clear(&refs_to_delete, 0); > + oid_array_clear(&old_oids); > > free(name); > strbuf_release(&bname); > diff --git a/builtin/tag.c b/builtin/tag.c > index 40874a292..0a3eb70fa 100644 > --- a/builtin/tag.c > +++ b/builtin/tag.c > @@ -119,11 +119,14 @@ static int delete_tags(const char **argv) > { > int result; > struct string_list refs_to_delete = STRING_LIST_INIT_DUP; > + struct oid_array old_oids = OID_ARRAY_INIT; > struct string_list_item *item; > > result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); > + for_each_string_list_item(item, &refs_to_delete) > + oid_array_append(&old_oids, item->util); Nit: wouldn't it make sense to add the oid to `old_oids` within `collect_tags()` instead of iterating over all tags again? You would have to change the callback data sent. If not, we should call this out in the commit message at the least. > if (refs_delete_refs(get_main_ref_store(the_repository), NULL, > - &refs_to_delete, NULL, REF_NO_DEREF)) > + &refs_to_delete, &old_oids, REF_NO_DEREF)) > result = 1; > > for_each_string_list_item(item, &refs_to_delete) { > @@ -137,6 +140,7 @@ static int delete_tags(const char **argv) > free(oid); > } > string_list_clear(&refs_to_delete, 0); > + oid_array_clear(&old_oids); > return result; > } > > diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh > index 4fe9d9b23..01b5ba8c4 100755 > --- a/t/t1416-ref-transaction-hooks.sh > +++ b/t/t1416-ref-transaction-hooks.sh > @@ -14,6 +14,50 @@ test_expect_success setup ' > POST_OID=$(git rev-parse POST) > ' > > +test_expect_success 'hook gets old values for batched branch/tag deletion' ' > + test_when_finished "rm -f actual" && > + git branch to-delete PRE && > + git tag delete-tag POST && > + git pack-refs --all && > + test_hook reference-transaction <<-\EOF && > + if test "$1" = committed > + then > + # Ignore backend-internal zero-to-zero records. > + while read -r old new ref > + do > + case "$old" in > + *[!0]*) > + echo "$old $new $ref" > + ;; > + esac > + done >>actual > + fi > + EOF > + cat >expect <<-EOF && > + $PRE_OID $ZERO_OID refs/heads/to-delete > + $POST_OID $ZERO_OID refs/tags/delete-tag > + EOF > + git branch -D to-delete && > + git tag -d delete-tag && > + test_cmp expect actual > +' > + > +test_expect_success 'branch deletion rejects a concurrent update' ' > + git branch delete-race PRE && > + test_hook reference-transaction <<-\EOF && > + marker=$(git rev-parse --git-path delete-race-once) > + if test "$1" = preparing && test ! -e "$marker" > + then > + >"$marker" > + git update-ref refs/heads/delete-race POST > + fi > + exit 0 > + EOF > + test_must_fail git branch -D delete-race 2>err && > + test_grep "is at $POST_OID but expected $PRE_OID" err && > + test_cmp_rev POST refs/heads/delete-race > +' > + > test_expect_success 'hook allows updating ref if successful' ' > git reset --hard PRE && > test_hook reference-transaction <<-\EOF && > -- > 2.39.3 (Apple Git-146) The rest of the patch looks good! :) ``` ## Karthik Nayak, 2026-09-21 13:56 Subject: Re: [PATCH v2 3/3] fetch, remote: retain old OIDs when pruning refs Message-ID: In-Reply-To: <461c36ccdae09fb827a3c0efc7eed5aef072e09b.1789901584.git.maciej.ciemborowicz@gmail.com> ``` Maciej Ciemborowicz writes: > get_stale_heads() records the current value of each stale local ref in its > new_oid member. The pruning paths discard that value and request unconditional > deletion, so reference-transaction hooks receive a null old OID. > > Carry the recorded values into the deletion transactions. Besides giving the > hooks useful values, this stops a stale scan from deleting a ref that another > process updated before the transaction acquired its locks. A concurrent > change now makes the prune fail and preserves the new value. > > This reuses data collected while finding stale refs and therefore requires no > additional ref reads. Do not print deletion status when a non-atomic prune > fails its old-OID check. > This does break user behavior though, previously we would never fail on pruning refs, but now we would and in a all-or-nothing manner. So perhaps a better way would be to use the `REF_TRANSACTION_ALLOW_FAILURE`? > Signed-off-by: Maciej Ciemborowicz > --- > builtin/fetch.c | 13 +++++-- > builtin/remote.c | 36 ++++++++++++++--- > t/t1416-ref-transaction-hooks.sh | 66 ++++++++++++++++++++++++++++++++ > 3 files changed, 106 insertions(+), 9 deletions(-) > > diff --git a/builtin/fetch.c b/builtin/fetch.c > index d202147b2..da413ace0 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -1452,14 +1452,18 @@ static int prune_refs(struct display_state *display_state, > struct ref *ref, *stale_refs = get_stale_heads(rs, ref_map); > struct strbuf err = STRBUF_INIT; > struct string_list refnames = STRING_LIST_INIT_NODUP; > + struct oid_array old_oids = OID_ARRAY_INIT; > > - for (ref = stale_refs; ref; ref = ref->next) > + for (ref = stale_refs; ref; ref = ref->next) { > string_list_append(&refnames, ref->name); > + oid_array_append(&old_oids, &ref->new_oid); > + } > Here `refnames` is built, but below it is only used for the non-atomic flow. Perhaps, we should move this into the `else` block? > if (!dry_run) { > if (transaction) { > for (ref = stale_refs; ref; ref = ref->next) { > - result = ref_transaction_delete(transaction, ref->name, NULL, > + result = ref_transaction_delete(transaction, ref->name, > + &ref->new_oid, > NULL, 0, "fetch: prune", &err); Nit: the formatting seems off. Curiously, was an LLM used to create these patches? If so, please do read our policy in 'Documentation/SubmittingPatches' regarding AI usage. > if (result) > goto cleanup; > @@ -1467,8 +1471,10 @@ static int prune_refs(struct display_state *display_state, > } else { > result = refs_delete_refs(get_main_ref_store(the_repository), > "fetch: prune", &refnames, > - NULL, 0); > + &old_oids, 0); > } > + if (result) > + goto cleanup; So, previously, we'd always prune all references without checking the old_oid. Now we should expect that this can fail. So we do need to check for the `result`. Seems like the other branch condition also does the same, we can extract this out? > } > > if (verbosity >= 0) { > @@ -1487,6 +1493,7 @@ static int prune_refs(struct display_state *display_state, > > cleanup: > string_list_clear(&refnames, 0); > + oid_array_clear(&old_oids); > strbuf_release(&err); > free_refs(stale_refs); > return result; > diff --git a/builtin/remote.c b/builtin/remote.c > index 13d3cc52d..b899bec55 100644 > --- a/builtin/remote.c > +++ b/builtin/remote.c > @@ -17,6 +17,7 @@ > #include "refs.h" > #include "refspec.h" > #include "odb.h" > +#include "oid-array.h" > #include "strvec.h" > #include "commit-reach.h" > #include "progress.h" > @@ -380,6 +381,11 @@ struct ref_states { > int queried; > }; > > +struct stale_ref { > + struct object_id oid; > + char name[FLEX_ARRAY]; > +}; > + > #define REF_STATES_INIT { \ > .new_refs = STRING_LIST_INIT_DUP, \ > .skipped = STRING_LIST_INIT_DUP, \ > @@ -410,9 +416,13 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat > } > stale_refs = get_stale_heads(&states->remote->fetch, fetch_map); > for (ref = stale_refs; ref; ref = ref->next) { > + struct stale_ref *stale_ref; > struct string_list_item *item = > string_list_append(&states->stale, abbrev_branch(ref->name)); > - item->util = xstrdup(ref->name); > + > + FLEX_ALLOC_STR(stale_ref, name, ref->name); > + oidcpy(&stale_ref->oid, &ref->new_oid); > + item->util = stale_ref; > } > free_refs(stale_refs); > free_refs(fetch_map); > @@ -1627,6 +1637,7 @@ static int prune_remote(const char *remote, int dry_run) > int result = 0; > struct ref_states states = REF_STATES_INIT; > struct string_list refs_to_prune = STRING_LIST_INIT_NODUP; > + struct oid_array old_oids = OID_ARRAY_INIT; > struct string_list_item *item; > > get_remote_ref_states(remote, &states, GET_REF_STATES); > @@ -1639,17 +1650,28 @@ static int prune_remote(const char *remote, int dry_run) > printf_ln(_("Pruning %s"), remote); > printf_ln(_("URL: %s"), states.remote->url.v[0]); > > - for_each_string_list_item(item, &states.stale) > - string_list_append(&refs_to_prune, item->util); > + for_each_string_list_item(item, &states.stale) { > + struct stale_ref *stale_ref = item->util; > + struct string_list_item *to_prune; > + > + to_prune = string_list_append(&refs_to_prune, stale_ref->name); > + to_prune->util = &stale_ref->oid; > + } > string_list_sort(&refs_to_prune); > + for_each_string_list_item(item, &refs_to_prune) > + oid_array_append(&old_oids, item->util); > We do this in the previous block? We don't need a new iterator here. > - if (!dry_run) > + if (!dry_run) { > result |= refs_delete_refs(get_main_ref_store(the_repository), > "remote: prune", &refs_to_prune, > - NULL, 0); > + &old_oids, 0); > + if (result) > + goto cleanup; > + } > > for_each_string_list_item(item, &states.stale) { > - const char *refname = item->util; > + struct stale_ref *stale_ref = item->util; > + const char *refname = stale_ref->name; > > if (dry_run) > printf_ln(_(" * [would prune] %s"), > @@ -1662,7 +1684,9 @@ static int prune_remote(const char *remote, int dry_run) > refs_warn_dangling_symrefs(get_main_ref_store(the_repository), > stdout, " ", dry_run, &refs_to_prune); > > +cleanup: > string_list_clear(&refs_to_prune, 0); > + oid_array_clear(&old_oids); > free_remote_ref_states(&states); > return result; > } > diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh > index 01b5ba8c4..2b51b216b 100755 > --- a/t/t1416-ref-transaction-hooks.sh > +++ b/t/t1416-ref-transaction-hooks.sh > @@ -58,6 +58,72 @@ test_expect_success 'branch deletion rejects a concurrent update' ' > test_cmp_rev POST refs/heads/delete-race > ' > > +test_expect_success 'hook gets old values when pruning remote refs' ' > + test_create_repo empty.git --bare && > + test_create_repo prune && test_create_repo is considered deprecated, let's use `git init` directly. While we're at it, we should also cleanup the directories we create here. > + git -C prune remote add origin ../empty.git && > + test_commit -C prune one && > + one=$(git -C prune rev-parse HEAD) && > + test_commit -C prune two && > + two=$(git -C prune rev-parse HEAD) && > + git -C prune update-ref refs/remotes/origin/remote-prune-z "$one" && > + git -C prune update-ref refs/remotes/origin/remote-prune-a "$two" && > + git -C prune pack-refs --all && Why do we need to pack-refs? > + test_hook -C prune reference-transaction <<-\EOF && > + if test "$1" = committed > + then > + # Ignore backend-internal zero-to-zero records. > + while read -r old new ref > + do > + case "$old" in > + *[!0]*) > + echo "$old $new $ref" > + ;; > + esac > + done >>actual > + fi > + EOF > + ( > + cd prune && All the commands above also run in the 'prune' directory, can we put all of them in this subshell? > + git remote prune origin && > + git update-ref refs/remotes/origin/fetch-prune "$one" && > + git fetch --prune origin && > + git update-ref refs/remotes/origin/atomic-prune "$one" && > + git fetch --atomic --prune origin && > + cat >expect <<-EOF && > + $two $ZERO_OID refs/remotes/origin/remote-prune-a > + $one $ZERO_OID refs/remotes/origin/remote-prune-z > + $one $ZERO_OID refs/remotes/origin/fetch-prune > + $one $ZERO_OID refs/remotes/origin/atomic-prune > + EOF > + test_cmp expect actual > + ) > +' > + > +test_expect_success 'remote prune rejects a concurrent update' ' > + test_when_finished "rm -rf race-empty.git race-prune" && > + test_create_repo race-empty.git --bare && > + test_create_repo race-prune && same as above. > + test_commit -C race-prune one && > + one=$(git -C race-prune rev-parse HEAD) && > + test_commit -C race-prune two && > + two=$(git -C race-prune rev-parse HEAD) && > + git -C race-prune remote add origin ../race-empty.git && > + git -C race-prune update-ref refs/remotes/origin/race "$one" && > + test_hook -C race-prune reference-transaction <<-\EOF && > + marker=$(git rev-parse --git-path prune-race-once) > + if test "$1" = preparing && test ! -e "$marker" > + then > + >"$marker" > + git update-ref refs/remotes/origin/race HEAD > + fi > + exit 0 > + EOF > + test_must_fail git -C race-prune remote prune origin >out 2>err && > + test "$two" = "$(git -C race-prune rev-parse refs/remotes/origin/race)" && > + ! grep "\[pruned\]" out > +' > + > test_expect_success 'hook allows updating ref if successful' ' > git reset --hard PRE && > test_hook reference-transaction <<-\EOF && > -- > 2.39.3 (Apple Git-146) ``` ## Karthik Nayak, 2026-09-21 13:57 Subject: Re: [PATCH v2 0/3] refs: report old OIDs for batched deletions Message-ID: In-Reply-To: ``` Maciej Ciemborowicz writes: > The reference-transaction hook receives zero as both the old and new OID > when branch, tag, fetch, and remote delete refs through refs_delete_refs(). > Those callers already know the values that they selected for deletion. > > Teach refs_delete_refs() to accept aligned old OIDs and pass them into the > transaction. Besides making the hook records useful, this makes the selected > callers reject concurrent changes instead of deleting values that they did > not inspect. For branch and tag, this restores the compare-and-delete > behavior that existed before 8198907795 converted them to batched deletion. > For pruning, it prevents a stale scan from deleting a ref updated by another > process. > > The values are already available at every updated call site, so the series > adds no ref reads and retains batched performance. I still have some concerns about backward comparability here, since we go from a delete all without any checks to a all-or-nothing situation, which can be an issue with commands like `git fetch --prune`. Left some comments on the individual patches. > > Changes since v1: > > * Document the conditional deletion behavior and its race protection. > * Add tests that update refs from the hook's preparing phase and verify that > branch deletion and remote pruning preserve the concurrent update. > * Avoid printing deletion status when a non-atomic prune fails. > * Use a local string_list_item in refs_delete_refs(), as suggested by > Karthik. > > Based on maint at e9019fcafe (Git 2.55). Might be worthwhile to rebase on top of master. Seems like there are conflicts with d38352cd43 (A few more fixes before -rc2, 2026-09-17). [snip] Thanks! ``` ## Maciej Ciemborowicz, 2026-09-21 20:01 Subject: Re: [PATCH v2 0/3] refs: report old OIDs for batched deletions Message-ID: In-Reply-To: ``` Karthik Nayak writes: > This does break user behavior though, previously we would never fail on > pruning refs, but now we would and in a all-or-nothing manner. So > perhaps a better way would be to use the REF_TRANSACTION_ALLOW_FAILURE? That makes sense. I will use it. > Here refnames is built, but below it is only used for the non-atomic > flow. Perhaps, we should move this into the else block? Yes, I will move construction of the refname and OID arrays into the non-atomic branch. > Nit: the formatting seems off. Will fix. > Curiously, was an LLM used to create these patches? If so, please do > read our policy in 'Documentation/SubmittingPatches' regarding AI usage. Yes, I use an AI coding agent. Thank you for pointing me to the policy. I have now read it. I reviewed the resulting changes and tests, understand the implementation, so I take responsibility for the version I submit. > Seems like the other branch condition also does the same, we can extract > this out? Will do. > We do this in the previous block? We don't need a new iterator here. Right. I will append the refname and corresponding OID in the same loop. > test_create_repo is considered deprecated, let's use git init > directly. While we're at it, we should also cleanup the directories we > create here. Will do it. > Why do we need to pack-refs? It is not required to reproduce this problem. I will remove it. > All the commands above also run in the 'prune' directory, can we put all > of them in this subshell? Yes, I will move the repository operations into the subshell where possible. Thanks for the review. I hope tomorow I will prepare v3. Cheers, - Maciej Ciemborowicz On Mon, Sep 21, 2026 at 3:57 PM Karthik Nayak wrote: > > Maciej Ciemborowicz writes: > > > The reference-transaction hook receives zero as both the old and new OID > > when branch, tag, fetch, and remote delete refs through refs_delete_refs(). > > Those callers already know the values that they selected for deletion. > > > > Teach refs_delete_refs() to accept aligned old OIDs and pass them into the > > transaction. Besides making the hook records useful, this makes the selected > > callers reject concurrent changes instead of deleting values that they did > > not inspect. For branch and tag, this restores the compare-and-delete > > behavior that existed before 8198907795 converted them to batched deletion. > > For pruning, it prevents a stale scan from deleting a ref updated by another > > process. > > > > The values are already available at every updated call site, so the series > > adds no ref reads and retains batched performance. > > I still have some concerns about backward comparability here, since we > go from a delete all without any checks to a all-or-nothing situation, > which can be an issue with commands like `git fetch --prune`. > > Left some comments on the individual patches. > > > > > Changes since v1: > > > > * Document the conditional deletion behavior and its race protection. > > * Add tests that update refs from the hook's preparing phase and verify that > > branch deletion and remote pruning preserve the concurrent update. > > * Avoid printing deletion status when a non-atomic prune fails. > > * Use a local string_list_item in refs_delete_refs(), as suggested by > > Karthik. > > > > Based on maint at e9019fcafe (Git 2.55). > > Might be worthwhile to rebase on top of master. Seems like there are > conflicts with d38352cd43 (A few more fixes before -rc2, 2026-09-17). > > [snip] > > Thanks! ``` ## Junio C Hamano, 2026-09-21 23:55 Subject: Re: [PATCH v2 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: <5c96a5a1ebafd49a301c5c1dc77a2e19d6677ab0.1789901584.git.maciej.ciemborowicz@gmail.com> ``` Maciej Ciemborowicz writes: > refs_delete_refs() currently performs unconditional deletions. Thus callers > cannot preserve old values that they have already resolved, and > reference-transaction hooks consequently see a null old OID. > > Add an optional oid_array whose entries correspond to the refnames. I had to read this sentence three times and still couldn't guess what it wanted to say. I _think_ the code is passing a list of refnames, and your new parameter that is oid_array serves as a parallel list, where the ref, identified by the Nth element of the list of refnames, is protected from deletion with the Nth element of the list of oids in such a way that ref is not removed unless it points at the specified object. You'd need to find a concise way to tell that story instead of the above sentence that does not give readers any meaningful information. > int refs_delete_refs(struct ref_store *refs, const char *logmsg, > - struct string_list *refnames, unsigned int flags) > + struct string_list *refnames, > + const struct oid_array *old_oids, > + unsigned int flags) > { > struct ref_transaction *transaction; > struct strbuf err = STRBUF_INIT; > - struct string_list_item *item; > + size_t i; > int ret = 0, failures = 0; > char *msg; > > + if (old_oids && old_oids->nr != refnames->nr) > + BUG("refname and old OID counts do not match"); OK. So it is not end-users' but calling code's responsibility to ensure that the optional list of object names have exactly the same number of entries as the list of refs. > if (!refnames->nr) > return 0; And this is as before. Shouldn't the new test above be placed below this? After all, if we are removing no refs, we really do not care what garbage is in the old oids array---we won't even look at it. > msg = normalize_reflog_message(logmsg); > > - /* > - * Since we don't check the references' old_oids, the > - * individual updates can't fail, so we can pack all of the > - * updates into a single transaction. > - */ To me, this reads more like "We want to make sure that each deletion is independent and philosophically each of them should belong in separate transactions so that even when some fails the rest would proceed. Luckily, the current API does not allow you to check the current value to protect refs from deletion, so we can cram all delete operations in a single transaction and still claim that we are not making it all-or-none!". Natural continuation of that argument is "If we ever extend the API so that refs are optionally protected from deletion, we can get into a situation where some refs can be successfully removed while others cannot. Keeping everything in a single transaction WILL BECOME A WRONG DESIGN CHOICE when it happens." And this new code is doing exactly that, making all the deletions, of possibly unrelated refs, into an all-or-none matter. Don't we need to have separate transactions to delete each ref to retain the "delete them independently" semantics? If the caller (e.g., "git fetch --prune" without "--atomic") wants to delete 1000 refs, and a single ref fails its old-oid check due to a concurrent update, none of the 1000 refs will be removed and the transaction would be aborted. explains this function like so: /* * Delete the specified references. If there are any problems, emit * errors but attempt to keep going (i.e., the deletes are not done in * an all-or-nothing transaction). msg and flags are passed through to * ref_transaction_delete(). */ int refs_delete_refs(struct ref_store *refs, const char *msg, struct string_list *refnames, unsigned int flags); because we want to avoid exactly such a failure mode. I do not offhand remember if our ref transactions have a mode where it acts more like a glorified "batch" job and commit does not necessarily require everything succeeding, but if it do, then it is OK to keep using a single transaction but to run it in such a "best effort" mode. > transaction = ref_store_transaction_begin(refs, 0, &err); > if (!transaction) { > ret = error("%s", err.buf); > goto out; > } > > - for_each_string_list_item(item, refnames) { > + for (i = 0; i < refnames->nr; i++) { > + struct string_list_item *item = &refnames->items[i]; > + const struct object_id *old_oid = old_oids ? &old_oids->oid[i] : NULL; > + > + if (old_oid && is_null_oid(old_oid)) > + old_oid = NULL; > ret = ref_transaction_delete(transaction, item->string, > - NULL, NULL, flags, msg, &err); > + old_oid, NULL, flags, msg, &err); > if (ret) { > warning(_("could not delete reference %s: %s"), > item->string, err.buf); ``` ## Maciej Ciemborowicz, 2026-09-22 12:26 Subject: [PATCH v3 0/3] refs: report old OIDs for batched deletions Message-ID: In-Reply-To: ``` This follows up on the reference-transaction bug report at [1]. The reference-transaction hook receives zero as both the old and new OID when branch, tag, fetch, and remote delete refs through refs_delete_refs(). Those callers already know the values that they selected for deletion. Teach refs_delete_refs() to accept a parallel array of expected old OIDs and pass them into the transaction. Besides making hook records useful, this restores conditional deletion for branch and tag and adds it to pruning without additional ref reads. Use REF_TRANSACTION_ALLOW_FAILURE for non-atomic batches so a concurrent change rejects only that deletion while unrelated stale refs are still removed. Atomic fetches retain their all-or-nothing behavior. Changes since v2: * Rebase onto master at d38352cd43. * Use REF_TRANSACTION_ALLOW_FAILURE for conditional batch deletion and report individual rejections. * Clarify how the parallel refname and old-OID arrays correspond, and skip their length check when there are no refs to delete. * Document null OIDs as the unconditional-deletion sentinel needed for broken refs. * Append tag OIDs in collect_tags() instead of making a second pass. * Build the fetch refname/OID arrays only for non-atomic pruning and fix the atomic-path formatting. * Replace deprecated test_create_repo calls, clean up test repositories, remove unnecessary packed-ref setup, and cover partial prune failure. The full test suite passes with DEVELOPER=1. The focused tests also pass with SHA-1 and SHA-256 using both the files and reftable backends. [1] https://lore.kernel.org/git/CACQ=SRGTTdQ+dHXhN6F52dBv5KxZBRfk_Em2fvmEmGJDoB6oTg@mail.gmail.com/ Maciej Ciemborowicz (3): refs: allow callers to supply old OIDs for batch deletion branch, tag: retain old OIDs in batched deletions fetch, remote: retain old OIDs when pruning refs bisect.c | 2 +- builtin/branch.c | 7 +- builtin/fetch.c | 21 ++++-- builtin/remote.c | 37 +++++++-- builtin/tag.c | 28 +++++-- refs.c | 54 +++++++++++--- refs.h | 13 +++- t/helper/test-ref-store.c | 2 +- t/t1416-ref-transaction-hooks.sh | 124 +++++++++++++++++++++++++++++++ 9 files changed, 250 insertions(+), 38 deletions(-) Range-diff against v2: 1: 2e36ce00e ! 1: 3315d5f47 refs: allow callers to supply old OIDs for batch deletion @@ Metadata ## Commit message ## refs: allow callers to supply old OIDs for batch deletion - refs_delete_refs() currently performs unconditional deletions. Thus callers - cannot preserve old values that they have already resolved, and - reference-transaction hooks consequently see a null old OID. + refs_delete_refs() performs unconditional deletions, so callers cannot + preserve old values that they have already resolved. Consequently, + reference-transaction hooks see a null old OID. - Add an optional oid_array whose entries correspond to the refnames. Pass each - non-null OID to ref_transaction_delete(). Supplying an OID makes the deletion - conditional: if the ref changed after the caller resolved it, the transaction - fails instead of deleting the new value. Existing callers that pass NULL - retain the unconditional behavior. + Let callers provide an optional array of expected old OIDs in parallel with + the refname list. When the array is provided, delete the ref at position N + only if it still points at the OID at position N. A null OID requests an + unconditional deletion for refs whose old value cannot be resolved, such as + broken refs. + + Use REF_TRANSACTION_ALLOW_FAILURE when old OIDs are supplied. This retains + the helper's best-effort behavior: an old-OID mismatch rejects that deletion + while independent deletions in the batch can still proceed. Signed-off-by: Maciej Ciemborowicz @@ bisect.c: int bisect_clean_state(void) unlink_or_warn(git_path_bisect_log()); ## builtin/branch.c ## -@@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int force, int kinds, - free(target); +@@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int kinds, } -- if (refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) -+ if (refs_delete_refs(get_main_ref_store(the_repository), NULL, + if (!(flags & DELETE_BRANCH_DRY_RUN) && +- refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) ++ refs_delete_refs(get_main_ref_store(the_repository), NULL, + &refs_to_delete, NULL, REF_NO_DEREF)) ret = 1; @@ refs.c #include "object.h" #include "path.h" @@ refs.c: void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio + } } ++struct delete_refs_rejection_data { ++ int failures; ++}; ++ ++static void delete_refs_rejection_handler(const char *refname, ++ const struct object_id *old_oid UNUSED, ++ const struct object_id *new_oid UNUSED, ++ const char *old_target UNUSED, ++ const char *new_target UNUSED, ++ enum ref_transaction_error err, ++ const char *details, ++ void *cb_data) ++{ ++ struct delete_refs_rejection_data *data = cb_data; ++ ++ warning(_("could not delete reference %s: %s"), refname, ++ details ? details : ref_transaction_error_msg(err)); ++ data->failures = 1; ++} ++ int refs_delete_refs(struct ref_store *refs, const char *logmsg, - struct string_list *refnames, unsigned int flags) + struct string_list *refnames, + const struct oid_array *old_oids, + unsigned int flags) { ++ struct delete_refs_rejection_data rejection_data = { 0 }; struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; - struct string_list_item *item; @@ refs.c: void ref_transaction_for_each_rejected_update(struct ref_transaction *tr int ret = 0, failures = 0; char *msg; -+ if (old_oids && old_oids->nr != refnames->nr) -+ BUG("refname and old OID counts do not match"); if (!refnames->nr) return 0; ++ if (old_oids && old_oids->nr != refnames->nr) ++ BUG("refname and old OID counts do not match"); msg = normalize_reflog_message(logmsg); @@ refs.c: void ref_transaction_for_each_rejected_update(struct ref_transaction *tr - * individual updates can't fail, so we can pack all of the - * updates into a single transaction. - */ - transaction = ref_store_transaction_begin(refs, 0, &err); +- transaction = ref_store_transaction_begin(refs, 0, &err); ++ transaction = ref_store_transaction_begin(refs, ++ old_oids ? REF_TRANSACTION_ALLOW_FAILURE : 0, &err); if (!transaction) { ret = error("%s", err.buf); goto out; @@ refs.c: void ref_transaction_for_each_rejected_update(struct ref_transaction *tr if (ret) { warning(_("could not delete reference %s: %s"), item->string, err.buf); +@@ refs.c: int refs_delete_refs(struct ref_store *refs, const char *logmsg, + refnames->items[0].string, err.buf); + else + error(_("could not delete references: %s"), err.buf); +- } ++ } else if (old_oids) ++ ref_transaction_for_each_rejected_update(transaction, ++ delete_refs_rejection_handler, ++ &rejection_data); + + out: ++ if (rejection_data.failures) ++ failures = 1; + if (!ret && failures) + ret = -1; + ref_transaction_free(transaction); ## refs.h ## @@ @@ refs.h: int refs_delete_ref(struct ref_store *refs, const char *msg, /* - * Delete the specified references. If there are any problems, emit + * Delete the specified references. If old_oids is non-NULL, it must contain -+ * an entry for each refname, in the same order. Each non-null entry is used -+ * to verify the current value of the corresponding reference before deleting -+ * it. A null entry disables verification for that reference. ++ * an entry for each refname, in the same order. Each non-null OID is used to ++ * verify the current value of the corresponding reference before deleting ++ * it. A null OID requests an unconditional deletion, which allows callers to ++ * include broken refs whose old value cannot be resolved. + * + * If there are any problems, emit * errors but attempt to keep going (i.e., the deletes are not done in 2: e8b867f8e ! 2: 2065188aa branch, tag: retain old OIDs in batched deletions @@ Commit message delete_ref(). This prevented the command from deleting a ref that another process had changed after it was inspected. - The conversion to batched deletion dropped those old OIDs. Besides making the - deletions unconditional, this causes reference-transaction hooks to report - zero as both the old and new OID. + The conversion to batched deletion dropped those old OIDs. Besides making + the deletions unconditional, this causes reference-transaction hooks to + report zero as both the old and new OID. Both commands still resolve the old OIDs before starting the deletion. Pass - those values to refs_delete_refs(). This restores the old race protection and - lets hooks receive useful old values without adding any ref reads. If a ref - changes concurrently, the transaction fails and preserves the new value. + those values to refs_delete_refs(). This restores the old race protection + and lets hooks receive useful old values without adding ref reads. If a ref + changes concurrently, reject its deletion and preserve the new value. Signed-off-by: Maciej Ciemborowicz @@ builtin/branch.c #include "remote.h" #include "parse-options.h" #include "branch.h" -@@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int force, int kinds, +@@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int kinds, struct strbuf bname = STRBUF_INIT; enum interpret_branch_kind allowed_interpret; struct string_list refs_to_delete = STRING_LIST_INIT_DUP; @@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int fo struct string_list_item *item; int branch_name_pos; const char *fmt_remotes = "refs/remotes/%s"; -@@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int force, int kinds, +@@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int kinds, } item = string_list_append(&refs_to_delete, name); + oid_array_append(&old_oids, &oid); - item->util = xstrdup((flags & REF_ISBROKEN) ? "broken" - : (flags & REF_ISSYMREF) ? target + item->util = xstrdup((ref_flags & REF_ISBROKEN) ? "broken" + : (ref_flags & REF_ISSYMREF) ? target : repo_find_unique_abbrev(the_repository, &oid, DEFAULT_ABBREV)); -@@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int force, int kinds, - } +@@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int kinds, - if (refs_delete_refs(get_main_ref_store(the_repository), NULL, + if (!(flags & DELETE_BRANCH_DRY_RUN) && + refs_delete_refs(get_main_ref_store(the_repository), NULL, - &refs_to_delete, NULL, REF_NO_DEREF)) + &refs_to_delete, &old_oids, REF_NO_DEREF)) ret = 1; for_each_string_list_item(item, &refs_to_delete) { -@@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int force, int kinds, +@@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int kinds, free(describe_ref); } string_list_clear(&refs_to_delete, 0); @@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int fo strbuf_release(&bname); ## builtin/tag.c ## -@@ builtin/tag.c: static int delete_tags(const char **argv) +@@ builtin/tag.c: static int for_each_tag_name(const char **argv, each_tag_name_fn fn, + return had_error; + } + ++struct tags_to_delete { ++ struct string_list refs; ++ struct oid_array old_oids; ++}; ++ + static int collect_tags(const char *name UNUSED, const char *ref, + const struct object_id *oid, void *cb_data) + { +- struct string_list *ref_list = cb_data; ++ struct tags_to_delete *data = cb_data; ++ struct string_list_item *item; + +- string_list_append(ref_list, ref); +- ref_list->items[ref_list->nr - 1].util = oiddup(oid); ++ item = string_list_append(&data->refs, ref); ++ item->util = oiddup(oid); ++ oid_array_append(&data->old_oids, oid); + return 0; + } + + static int delete_tags(const char **argv) { int result; - struct string_list refs_to_delete = STRING_LIST_INIT_DUP; -+ struct oid_array old_oids = OID_ARRAY_INIT; +- struct string_list refs_to_delete = STRING_LIST_INIT_DUP; ++ struct tags_to_delete data = { ++ .refs = STRING_LIST_INIT_DUP, ++ .old_oids = OID_ARRAY_INIT, ++ }; struct string_list_item *item; - result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); -+ for_each_string_list_item(item, &refs_to_delete) -+ oid_array_append(&old_oids, item->util); +- result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); ++ result = for_each_tag_name(argv, collect_tags, &data); if (refs_delete_refs(get_main_ref_store(the_repository), NULL, - &refs_to_delete, NULL, REF_NO_DEREF)) -+ &refs_to_delete, &old_oids, REF_NO_DEREF)) ++ &data.refs, &data.old_oids, REF_NO_DEREF)) result = 1; - for_each_string_list_item(item, &refs_to_delete) { +- for_each_string_list_item(item, &refs_to_delete) { ++ for_each_string_list_item(item, &data.refs) { + const char *name = item->string; + struct object_id *oid = item->util; + if (!refs_ref_exists(get_main_ref_store(the_repository), name)) @@ builtin/tag.c: static int delete_tags(const char **argv) + free(oid); } - string_list_clear(&refs_to_delete, 0); -+ oid_array_clear(&old_oids); +- string_list_clear(&refs_to_delete, 0); ++ string_list_clear(&data.refs, 0); ++ oid_array_clear(&data.old_oids); return result; } 3: 6aebfac97 ! 3: 3f3062252 fetch, remote: retain old OIDs when pruning refs @@ Commit message fetch, remote: retain old OIDs when pruning refs get_stale_heads() records the current value of each stale local ref in its - new_oid member. The pruning paths discard that value and request unconditional - deletion, so reference-transaction hooks receive a null old OID. + new_oid member. The pruning paths discard that value and request + unconditional deletion, so reference-transaction hooks receive a null old + OID. - Carry the recorded values into the deletion transactions. Besides giving the - hooks useful values, this stops a stale scan from deleting a ref that another - process updated before the transaction acquired its locks. A concurrent - change now makes the prune fail and preserves the new value. + Pass the recorded values into the deletion transactions. If a ref changes + after the stale scan, reject that deletion and preserve the new value. + Non-atomic pruning uses refs_delete_refs(), whose partial-failure mode still + deletes unaffected stale refs. An atomic fetch remains all-or-nothing. - This reuses data collected while finding stale refs and therefore requires no - additional ref reads. Do not print deletion status when a non-atomic prune - fails its old-OID check. + Reuse values collected while finding stale refs, avoiding additional ref + reads. Avoid reporting deletion status when pruning encounters a rejected + update. Signed-off-by: Maciej Ciemborowicz @@ builtin/fetch.c: static int prune_refs(struct display_state *display_state, struct ref *ref, *stale_refs = get_stale_heads(rs, ref_map); struct strbuf err = STRBUF_INIT; struct string_list refnames = STRING_LIST_INIT_NODUP; -+ struct oid_array old_oids = OID_ARRAY_INIT; - +- - for (ref = stale_refs; ref; ref = ref->next) -+ for (ref = stale_refs; ref; ref = ref->next) { - string_list_append(&refnames, ref->name); -+ oid_array_append(&old_oids, &ref->new_oid); -+ } +- string_list_append(&refnames, ref->name); ++ struct oid_array old_oids = OID_ARRAY_INIT; if (!dry_run) { if (transaction) { for (ref = stale_refs; ref; ref = ref->next) { - result = ref_transaction_delete(transaction, ref->name, NULL, +- NULL, 0, "fetch: prune", &err); + result = ref_transaction_delete(transaction, ref->name, -+ &ref->new_oid, - NULL, 0, "fetch: prune", &err); ++ &ref->new_oid, NULL, 0, ++ "fetch: prune", &err); if (result) goto cleanup; -@@ builtin/fetch.c: static int prune_refs(struct display_state *display_state, + } } else { ++ for (ref = stale_refs; ref; ref = ref->next) { ++ string_list_append(&refnames, ref->name); ++ oid_array_append(&old_oids, &ref->new_oid); ++ } result = refs_delete_refs(get_main_ref_store(the_repository), "fetch: prune", &refnames, - NULL, 0); @@ builtin/fetch.c: static int prune_refs(struct display_state *display_state, } if (verbosity >= 0) { + int summary_width = transport_summary_width(stale_refs); + ++ if (!refnames.nr) ++ for (ref = stale_refs; ref; ref = ref->next) ++ string_list_append(&refnames, ref->name); + for (ref = stale_refs; ref; ref = ref->next) { + display_ref_update(display_state, '-', _("[deleted]"), NULL, + _("(none)"), ref->name, @@ builtin/fetch.c: static int prune_refs(struct display_state *display_state, cleanup: @@ builtin/remote.c: static int prune_remote(const char *remote, int dry_run) - for_each_string_list_item(item, &states.stale) - string_list_append(&refs_to_prune, item->util); +- string_list_sort(&refs_to_prune); + for_each_string_list_item(item, &states.stale) { + struct stale_ref *stale_ref = item->util; -+ struct string_list_item *to_prune; + -+ to_prune = string_list_append(&refs_to_prune, stale_ref->name); -+ to_prune->util = &stale_ref->oid; ++ string_list_append(&refs_to_prune, stale_ref->name); ++ oid_array_append(&old_oids, &stale_ref->oid); + } - string_list_sort(&refs_to_prune); -+ for_each_string_list_item(item, &refs_to_prune) -+ oid_array_append(&old_oids, item->util); - if (!dry_run) + if (!dry_run) { @@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'branch deletion rejects a ' +test_expect_success 'hook gets old values when pruning remote refs' ' -+ test_create_repo empty.git --bare && -+ test_create_repo prune && -+ git -C prune remote add origin ../empty.git && -+ test_commit -C prune one && -+ one=$(git -C prune rev-parse HEAD) && -+ test_commit -C prune two && -+ two=$(git -C prune rev-parse HEAD) && -+ git -C prune update-ref refs/remotes/origin/remote-prune-z "$one" && -+ git -C prune update-ref refs/remotes/origin/remote-prune-a "$two" && -+ git -C prune pack-refs --all && ++ test_when_finished "rm -rf empty.git prune" && ++ git init --bare empty.git && ++ git init prune && ++ ( ++ cd prune && ++ git remote add origin ../empty.git && ++ git commit --allow-empty -m one && ++ one=$(git rev-parse HEAD) && ++ git commit --allow-empty -m two && ++ two=$(git rev-parse HEAD) && ++ git update-ref refs/remotes/origin/remote-prune-z "$one" && ++ git update-ref refs/remotes/origin/remote-prune-a "$two" ++ ) && + test_hook -C prune reference-transaction <<-\EOF && + if test "$1" = committed + then @@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'branch deletion rejects a + EOF + ( + cd prune && ++ one=$(git rev-parse HEAD^) && ++ two=$(git rev-parse HEAD) && + git remote prune origin && + git update-ref refs/remotes/origin/fetch-prune "$one" && + git fetch --prune origin && @@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'branch deletion rejects a + +test_expect_success 'remote prune rejects a concurrent update' ' + test_when_finished "rm -rf race-empty.git race-prune" && -+ test_create_repo race-empty.git --bare && -+ test_create_repo race-prune && -+ test_commit -C race-prune one && -+ one=$(git -C race-prune rev-parse HEAD) && -+ test_commit -C race-prune two && -+ two=$(git -C race-prune rev-parse HEAD) && -+ git -C race-prune remote add origin ../race-empty.git && -+ git -C race-prune update-ref refs/remotes/origin/race "$one" && ++ git init --bare race-empty.git && ++ git init race-prune && ++ ( ++ cd race-prune && ++ git commit --allow-empty -m one && ++ one=$(git rev-parse HEAD) && ++ git commit --allow-empty -m two && ++ two=$(git rev-parse HEAD) && ++ git remote add origin ../race-empty.git && ++ git update-ref refs/remotes/origin/race "$one" && ++ git update-ref refs/remotes/origin/other "$one" ++ ) && + test_hook -C race-prune reference-transaction <<-\EOF && + marker=$(git rev-parse --git-path prune-race-once) + if test "$1" = preparing && test ! -e "$marker" @@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'branch deletion rejects a + fi + exit 0 + EOF -+ test_must_fail git -C race-prune remote prune origin >out 2>err && -+ test "$two" = "$(git -C race-prune rev-parse refs/remotes/origin/race)" && -+ ! grep "\[pruned\]" out ++ ( ++ cd race-prune && ++ two=$(git rev-parse HEAD) && ++ test_must_fail git remote prune origin >out 2>err && ++ test_cmp_rev "$two" refs/remotes/origin/race && ++ test_must_fail git rev-parse --verify refs/remotes/origin/other && ++ test_grep ! "\[pruned\]" out ++ ) +' + test_expect_success 'hook allows updating ref if successful' ' base-commit: d38352cd43ab9745686d697872408bc3249a153f -- 2.39.3 (Apple Git-146) ``` ## Maciej Ciemborowicz, 2026-09-22 12:26 Subject: [PATCH v3 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: <3315d5f47ad7d8bcdbeda90b161606507c7040ea.1790079917.git.maciej.ciemborowicz@gmail.com> In-Reply-To: ``` refs_delete_refs() performs unconditional deletions, so callers cannot preserve old values that they have already resolved. Consequently, reference-transaction hooks see a null old OID. Let callers provide an optional array of expected old OIDs in parallel with the refname list. When the array is provided, delete the ref at position N only if it still points at the OID at position N. A null OID requests an unconditional deletion for refs whose old value cannot be resolved, such as broken refs. Use REF_TRANSACTION_ALLOW_FAILURE when old OIDs are supplied. This retains the helper's best-effort behavior: an old-OID mismatch rejects that deletion while independent deletions in the batch can still proceed. Signed-off-by: Maciej Ciemborowicz --- bisect.c | 2 +- builtin/branch.c | 3 ++- builtin/fetch.c | 2 +- builtin/remote.c | 5 ++-- builtin/tag.c | 3 ++- refs.c | 54 +++++++++++++++++++++++++++++++-------- refs.h | 13 ++++++++-- t/helper/test-ref-store.c | 2 +- 8 files changed, 64 insertions(+), 20 deletions(-) diff --git a/bisect.c b/bisect.c index 9cbb3dc67..931a80098 100644 --- a/bisect.c +++ b/bisect.c @@ -1206,7 +1206,7 @@ int bisect_clean_state(void) string_list_append(&refs_for_removal, "BISECT_EXPECTED_REV"); result = refs_delete_refs(get_main_ref_store(the_repository), "bisect: remove", &refs_for_removal, - REF_NO_DEREF); + NULL, REF_NO_DEREF); string_list_clear(&refs_for_removal, 0); unlink_or_warn(git_path_bisect_ancestors_ok()); unlink_or_warn(git_path_bisect_log()); diff --git a/builtin/branch.c b/builtin/branch.c index a613148fc..c9f259d04 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -351,7 +351,8 @@ static int delete_branches(int argc, const char **argv, int kinds, } if (!(flags & DELETE_BRANCH_DRY_RUN) && - refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) + refs_delete_refs(get_main_ref_store(the_repository), NULL, + &refs_to_delete, NULL, REF_NO_DEREF)) ret = 1; for_each_string_list_item(item, &refs_to_delete) { diff --git a/builtin/fetch.c b/builtin/fetch.c index 533fdfe7d..b662216bf 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1486,7 +1486,7 @@ static int prune_refs(struct display_state *display_state, } else { result = refs_delete_refs(get_main_ref_store(the_repository), "fetch: prune", &refnames, - 0); + NULL, 0); } } diff --git a/builtin/remote.c b/builtin/remote.c index de989ea3b..13d3cc52d 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1073,7 +1073,7 @@ static int rm(int argc, const char **argv, const char *prefix, if (!result) result = refs_delete_refs(get_main_ref_store(the_repository), "remote: remove", &branches, - REF_NO_DEREF); + NULL, REF_NO_DEREF); string_list_clear(&branches, 0); if (skipped.nr) { @@ -1645,7 +1645,8 @@ static int prune_remote(const char *remote, int dry_run) if (!dry_run) result |= refs_delete_refs(get_main_ref_store(the_repository), - "remote: prune", &refs_to_prune, 0); + "remote: prune", &refs_to_prune, + NULL, 0); for_each_string_list_item(item, &states.stale) { const char *refname = item->util; diff --git a/builtin/tag.c b/builtin/tag.c index 06c125b53..40874a292 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -122,7 +122,8 @@ static int delete_tags(const char **argv) struct string_list_item *item; result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); - if (refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) + if (refs_delete_refs(get_main_ref_store(the_repository), NULL, + &refs_to_delete, NULL, REF_NO_DEREF)) result = 1; for_each_string_list_item(item, &refs_to_delete) { diff --git a/refs.c b/refs.c index 92d5df5b7..1e0f432ed 100644 --- a/refs.c +++ b/refs.c @@ -16,6 +16,7 @@ #include "refs/refs-internal.h" #include "hook.h" #include "object-name.h" +#include "oid-array.h" #include "odb.h" #include "object.h" #include "path.h" @@ -3069,34 +3070,60 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio } } +struct delete_refs_rejection_data { + int failures; +}; + +static void delete_refs_rejection_handler(const char *refname, + const struct object_id *old_oid UNUSED, + const struct object_id *new_oid UNUSED, + const char *old_target UNUSED, + const char *new_target UNUSED, + enum ref_transaction_error err, + const char *details, + void *cb_data) +{ + struct delete_refs_rejection_data *data = cb_data; + + warning(_("could not delete reference %s: %s"), refname, + details ? details : ref_transaction_error_msg(err)); + data->failures = 1; +} + int refs_delete_refs(struct ref_store *refs, const char *logmsg, - struct string_list *refnames, unsigned int flags) + struct string_list *refnames, + const struct oid_array *old_oids, + unsigned int flags) { + struct delete_refs_rejection_data rejection_data = { 0 }; struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; - struct string_list_item *item; + size_t i; int ret = 0, failures = 0; char *msg; if (!refnames->nr) return 0; + if (old_oids && old_oids->nr != refnames->nr) + BUG("refname and old OID counts do not match"); msg = normalize_reflog_message(logmsg); - /* - * Since we don't check the references' old_oids, the - * individual updates can't fail, so we can pack all of the - * updates into a single transaction. - */ - transaction = ref_store_transaction_begin(refs, 0, &err); + transaction = ref_store_transaction_begin(refs, + old_oids ? REF_TRANSACTION_ALLOW_FAILURE : 0, &err); if (!transaction) { ret = error("%s", err.buf); goto out; } - for_each_string_list_item(item, refnames) { + for (i = 0; i < refnames->nr; i++) { + struct string_list_item *item = &refnames->items[i]; + const struct object_id *old_oid = old_oids ? &old_oids->oid[i] : NULL; + + if (old_oid && is_null_oid(old_oid)) + old_oid = NULL; ret = ref_transaction_delete(transaction, item->string, - NULL, NULL, flags, msg, &err); + old_oid, NULL, flags, msg, &err); if (ret) { warning(_("could not delete reference %s: %s"), item->string, err.buf); @@ -3112,9 +3139,14 @@ int refs_delete_refs(struct ref_store *refs, const char *logmsg, refnames->items[0].string, err.buf); else error(_("could not delete references: %s"), err.buf); - } + } else if (old_oids) + ref_transaction_for_each_rejected_update(transaction, + delete_refs_rejection_handler, + &rejection_data); out: + if (rejection_data.failures) + failures = 1; if (!ret && failures) ret = -1; ref_transaction_free(transaction); diff --git a/refs.h b/refs.h index 9979446d1..3a7aacefe 100644 --- a/refs.h +++ b/refs.h @@ -9,6 +9,7 @@ struct fsck_options; struct object_id; struct ref_store; +struct oid_array; struct strbuf; struct string_list; struct string_list_item; @@ -623,13 +624,21 @@ int refs_delete_ref(struct ref_store *refs, const char *msg, unsigned int flags); /* - * Delete the specified references. If there are any problems, emit + * Delete the specified references. If old_oids is non-NULL, it must contain + * an entry for each refname, in the same order. Each non-null OID is used to + * verify the current value of the corresponding reference before deleting + * it. A null OID requests an unconditional deletion, which allows callers to + * include broken refs whose old value cannot be resolved. + * + * If there are any problems, emit * errors but attempt to keep going (i.e., the deletes are not done in * an all-or-nothing transaction). msg and flags are passed through to * ref_transaction_delete(). */ int refs_delete_refs(struct ref_store *refs, const char *msg, - struct string_list *refnames, unsigned int flags); + struct string_list *refnames, + const struct oid_array *old_oids, + unsigned int flags); /** Delete a reflog */ int refs_delete_reflog(struct ref_store *refs, const char *refname); diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c index db58f0058..6d6857cbd 100644 --- a/t/helper/test-ref-store.c +++ b/t/helper/test-ref-store.c @@ -132,7 +132,7 @@ static int cmd_delete_refs(struct ref_store *refs, const char **argv) while (*argv) string_list_append(&refnames, *argv++); - result = refs_delete_refs(refs, msg, &refnames, flags); + result = refs_delete_refs(refs, msg, &refnames, NULL, flags); string_list_clear(&refnames, 0); return result; } -- 2.39.3 (Apple Git-146) ``` ## Maciej Ciemborowicz, 2026-09-22 12:26 Subject: [PATCH v3 2/3] branch, tag: retain old OIDs in batched deletions Message-ID: <2065188aabecd857456b3f2d791edf9f7c8c6c6a.1790079917.git.maciej.ciemborowicz@gmail.com> In-Reply-To: ``` Before 8198907795 (use delete_refs when deleting tags or branches, 2021-01-21), branch and tag deletion passed each resolved old OID to delete_ref(). This prevented the command from deleting a ref that another process had changed after it was inspected. The conversion to batched deletion dropped those old OIDs. Besides making the deletions unconditional, this causes reference-transaction hooks to report zero as both the old and new OID. Both commands still resolve the old OIDs before starting the deletion. Pass those values to refs_delete_refs(). This restores the old race protection and lets hooks receive useful old values without adding ref reads. If a ref changes concurrently, reject its deletion and preserve the new value. Signed-off-by: Maciej Ciemborowicz --- builtin/branch.c | 6 ++++- builtin/tag.c | 27 ++++++++++++++------ t/t1416-ref-transaction-hooks.sh | 44 ++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 9 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index c9f259d04..f222a2644 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -16,6 +16,7 @@ #include "commit.h" #include "gettext.h" #include "object-name.h" +#include "oid-array.h" #include "remote.h" #include "parse-options.h" #include "branch.h" @@ -248,6 +249,7 @@ static int delete_branches(int argc, const char **argv, int kinds, struct strbuf bname = STRBUF_INIT; enum interpret_branch_kind allowed_interpret; struct string_list refs_to_delete = STRING_LIST_INIT_DUP; + struct oid_array old_oids = OID_ARRAY_INIT; struct string_list_item *item; int branch_name_pos; const char *fmt_remotes = "refs/remotes/%s"; @@ -342,6 +344,7 @@ static int delete_branches(int argc, const char **argv, int kinds, } item = string_list_append(&refs_to_delete, name); + oid_array_append(&old_oids, &oid); item->util = xstrdup((ref_flags & REF_ISBROKEN) ? "broken" : (ref_flags & REF_ISSYMREF) ? target : repo_find_unique_abbrev(the_repository, &oid, DEFAULT_ABBREV)); @@ -352,7 +355,7 @@ static int delete_branches(int argc, const char **argv, int kinds, if (!(flags & DELETE_BRANCH_DRY_RUN) && refs_delete_refs(get_main_ref_store(the_repository), NULL, - &refs_to_delete, NULL, REF_NO_DEREF)) + &refs_to_delete, &old_oids, REF_NO_DEREF)) ret = 1; for_each_string_list_item(item, &refs_to_delete) { @@ -377,6 +380,7 @@ static int delete_branches(int argc, const char **argv, int kinds, free(describe_ref); } string_list_clear(&refs_to_delete, 0); + oid_array_clear(&old_oids); free(name); strbuf_release(&bname); diff --git a/builtin/tag.c b/builtin/tag.c index 40874a292..32b70c369 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -105,28 +105,38 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn, return had_error; } +struct tags_to_delete { + struct string_list refs; + struct oid_array old_oids; +}; + static int collect_tags(const char *name UNUSED, const char *ref, const struct object_id *oid, void *cb_data) { - struct string_list *ref_list = cb_data; + struct tags_to_delete *data = cb_data; + struct string_list_item *item; - string_list_append(ref_list, ref); - ref_list->items[ref_list->nr - 1].util = oiddup(oid); + item = string_list_append(&data->refs, ref); + item->util = oiddup(oid); + oid_array_append(&data->old_oids, oid); return 0; } static int delete_tags(const char **argv) { int result; - struct string_list refs_to_delete = STRING_LIST_INIT_DUP; + struct tags_to_delete data = { + .refs = STRING_LIST_INIT_DUP, + .old_oids = OID_ARRAY_INIT, + }; struct string_list_item *item; - result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); + result = for_each_tag_name(argv, collect_tags, &data); if (refs_delete_refs(get_main_ref_store(the_repository), NULL, - &refs_to_delete, NULL, REF_NO_DEREF)) + &data.refs, &data.old_oids, REF_NO_DEREF)) result = 1; - for_each_string_list_item(item, &refs_to_delete) { + for_each_string_list_item(item, &data.refs) { const char *name = item->string; struct object_id *oid = item->util; if (!refs_ref_exists(get_main_ref_store(the_repository), name)) @@ -136,7 +146,8 @@ static int delete_tags(const char **argv) free(oid); } - string_list_clear(&refs_to_delete, 0); + string_list_clear(&data.refs, 0); + oid_array_clear(&data.old_oids); return result; } diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh index 4fe9d9b23..01b5ba8c4 100755 --- a/t/t1416-ref-transaction-hooks.sh +++ b/t/t1416-ref-transaction-hooks.sh @@ -14,6 +14,50 @@ test_expect_success setup ' POST_OID=$(git rev-parse POST) ' +test_expect_success 'hook gets old values for batched branch/tag deletion' ' + test_when_finished "rm -f actual" && + git branch to-delete PRE && + git tag delete-tag POST && + git pack-refs --all && + test_hook reference-transaction <<-\EOF && + if test "$1" = committed + then + # Ignore backend-internal zero-to-zero records. + while read -r old new ref + do + case "$old" in + *[!0]*) + echo "$old $new $ref" + ;; + esac + done >>actual + fi + EOF + cat >expect <<-EOF && + $PRE_OID $ZERO_OID refs/heads/to-delete + $POST_OID $ZERO_OID refs/tags/delete-tag + EOF + git branch -D to-delete && + git tag -d delete-tag && + test_cmp expect actual +' + +test_expect_success 'branch deletion rejects a concurrent update' ' + git branch delete-race PRE && + test_hook reference-transaction <<-\EOF && + marker=$(git rev-parse --git-path delete-race-once) + if test "$1" = preparing && test ! -e "$marker" + then + >"$marker" + git update-ref refs/heads/delete-race POST + fi + exit 0 + EOF + test_must_fail git branch -D delete-race 2>err && + test_grep "is at $POST_OID but expected $PRE_OID" err && + test_cmp_rev POST refs/heads/delete-race +' + test_expect_success 'hook allows updating ref if successful' ' git reset --hard PRE && test_hook reference-transaction <<-\EOF && -- 2.39.3 (Apple Git-146) ``` ## Maciej Ciemborowicz, 2026-09-22 12:26 Subject: [PATCH v3 3/3] fetch, remote: retain old OIDs when pruning refs Message-ID: <3f3062252ac1aa057b9ee9a2dd9892e629ba7a82.1790079917.git.maciej.ciemborowicz@gmail.com> In-Reply-To: ``` get_stale_heads() records the current value of each stale local ref in its new_oid member. The pruning paths discard that value and request unconditional deletion, so reference-transaction hooks receive a null old OID. Pass the recorded values into the deletion transactions. If a ref changes after the stale scan, reject that deletion and preserve the new value. Non-atomic pruning uses refs_delete_refs(), whose partial-failure mode still deletes unaffected stale refs. An atomic fetch remains all-or-nothing. Reuse values collected while finding stale refs, avoiding additional ref reads. Avoid reporting deletion status when pruning encounters a rejected update. Signed-off-by: Maciej Ciemborowicz --- builtin/fetch.c | 21 ++++++--- builtin/remote.c | 34 +++++++++++--- t/t1416-ref-transaction-hooks.sh | 80 ++++++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 13 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index b662216bf..2a59ac10f 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1471,28 +1471,36 @@ static int prune_refs(struct display_state *display_state, struct ref *ref, *stale_refs = get_stale_heads(rs, ref_map); struct strbuf err = STRBUF_INIT; struct string_list refnames = STRING_LIST_INIT_NODUP; - - for (ref = stale_refs; ref; ref = ref->next) - string_list_append(&refnames, ref->name); + struct oid_array old_oids = OID_ARRAY_INIT; if (!dry_run) { if (transaction) { for (ref = stale_refs; ref; ref = ref->next) { - result = ref_transaction_delete(transaction, ref->name, NULL, - NULL, 0, "fetch: prune", &err); + result = ref_transaction_delete(transaction, ref->name, + &ref->new_oid, NULL, 0, + "fetch: prune", &err); if (result) goto cleanup; } } else { + for (ref = stale_refs; ref; ref = ref->next) { + string_list_append(&refnames, ref->name); + oid_array_append(&old_oids, &ref->new_oid); + } result = refs_delete_refs(get_main_ref_store(the_repository), "fetch: prune", &refnames, - NULL, 0); + &old_oids, 0); } + if (result) + goto cleanup; } if (verbosity >= 0) { int summary_width = transport_summary_width(stale_refs); + if (!refnames.nr) + for (ref = stale_refs; ref; ref = ref->next) + string_list_append(&refnames, ref->name); for (ref = stale_refs; ref; ref = ref->next) { display_ref_update(display_state, '-', _("[deleted]"), NULL, _("(none)"), ref->name, @@ -1506,6 +1514,7 @@ static int prune_refs(struct display_state *display_state, cleanup: string_list_clear(&refnames, 0); + oid_array_clear(&old_oids); strbuf_release(&err); free_refs(stale_refs); return result; diff --git a/builtin/remote.c b/builtin/remote.c index 13d3cc52d..a99d18832 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -17,6 +17,7 @@ #include "refs.h" #include "refspec.h" #include "odb.h" +#include "oid-array.h" #include "strvec.h" #include "commit-reach.h" #include "progress.h" @@ -380,6 +381,11 @@ struct ref_states { int queried; }; +struct stale_ref { + struct object_id oid; + char name[FLEX_ARRAY]; +}; + #define REF_STATES_INIT { \ .new_refs = STRING_LIST_INIT_DUP, \ .skipped = STRING_LIST_INIT_DUP, \ @@ -410,9 +416,13 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat } stale_refs = get_stale_heads(&states->remote->fetch, fetch_map); for (ref = stale_refs; ref; ref = ref->next) { + struct stale_ref *stale_ref; struct string_list_item *item = string_list_append(&states->stale, abbrev_branch(ref->name)); - item->util = xstrdup(ref->name); + + FLEX_ALLOC_STR(stale_ref, name, ref->name); + oidcpy(&stale_ref->oid, &ref->new_oid); + item->util = stale_ref; } free_refs(stale_refs); free_refs(fetch_map); @@ -1627,6 +1637,7 @@ static int prune_remote(const char *remote, int dry_run) int result = 0; struct ref_states states = REF_STATES_INIT; struct string_list refs_to_prune = STRING_LIST_INIT_NODUP; + struct oid_array old_oids = OID_ARRAY_INIT; struct string_list_item *item; get_remote_ref_states(remote, &states, GET_REF_STATES); @@ -1639,17 +1650,24 @@ static int prune_remote(const char *remote, int dry_run) printf_ln(_("Pruning %s"), remote); printf_ln(_("URL: %s"), states.remote->url.v[0]); - for_each_string_list_item(item, &states.stale) - string_list_append(&refs_to_prune, item->util); - string_list_sort(&refs_to_prune); + for_each_string_list_item(item, &states.stale) { + struct stale_ref *stale_ref = item->util; + + string_list_append(&refs_to_prune, stale_ref->name); + oid_array_append(&old_oids, &stale_ref->oid); + } - if (!dry_run) + if (!dry_run) { result |= refs_delete_refs(get_main_ref_store(the_repository), "remote: prune", &refs_to_prune, - NULL, 0); + &old_oids, 0); + if (result) + goto cleanup; + } for_each_string_list_item(item, &states.stale) { - const char *refname = item->util; + struct stale_ref *stale_ref = item->util; + const char *refname = stale_ref->name; if (dry_run) printf_ln(_(" * [would prune] %s"), @@ -1662,7 +1680,9 @@ static int prune_remote(const char *remote, int dry_run) refs_warn_dangling_symrefs(get_main_ref_store(the_repository), stdout, " ", dry_run, &refs_to_prune); +cleanup: string_list_clear(&refs_to_prune, 0); + oid_array_clear(&old_oids); free_remote_ref_states(&states); return result; } diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh index 01b5ba8c4..8b52f2366 100755 --- a/t/t1416-ref-transaction-hooks.sh +++ b/t/t1416-ref-transaction-hooks.sh @@ -58,6 +58,86 @@ test_expect_success 'branch deletion rejects a concurrent update' ' test_cmp_rev POST refs/heads/delete-race ' +test_expect_success 'hook gets old values when pruning remote refs' ' + test_when_finished "rm -rf empty.git prune" && + git init --bare empty.git && + git init prune && + ( + cd prune && + git remote add origin ../empty.git && + git commit --allow-empty -m one && + one=$(git rev-parse HEAD) && + git commit --allow-empty -m two && + two=$(git rev-parse HEAD) && + git update-ref refs/remotes/origin/remote-prune-z "$one" && + git update-ref refs/remotes/origin/remote-prune-a "$two" + ) && + test_hook -C prune reference-transaction <<-\EOF && + if test "$1" = committed + then + # Ignore backend-internal zero-to-zero records. + while read -r old new ref + do + case "$old" in + *[!0]*) + echo "$old $new $ref" + ;; + esac + done >>actual + fi + EOF + ( + cd prune && + one=$(git rev-parse HEAD^) && + two=$(git rev-parse HEAD) && + git remote prune origin && + git update-ref refs/remotes/origin/fetch-prune "$one" && + git fetch --prune origin && + git update-ref refs/remotes/origin/atomic-prune "$one" && + git fetch --atomic --prune origin && + cat >expect <<-EOF && + $two $ZERO_OID refs/remotes/origin/remote-prune-a + $one $ZERO_OID refs/remotes/origin/remote-prune-z + $one $ZERO_OID refs/remotes/origin/fetch-prune + $one $ZERO_OID refs/remotes/origin/atomic-prune + EOF + test_cmp expect actual + ) +' + +test_expect_success 'remote prune rejects a concurrent update' ' + test_when_finished "rm -rf race-empty.git race-prune" && + git init --bare race-empty.git && + git init race-prune && + ( + cd race-prune && + git commit --allow-empty -m one && + one=$(git rev-parse HEAD) && + git commit --allow-empty -m two && + two=$(git rev-parse HEAD) && + git remote add origin ../race-empty.git && + git update-ref refs/remotes/origin/race "$one" && + git update-ref refs/remotes/origin/other "$one" + ) && + test_hook -C race-prune reference-transaction <<-\EOF && + marker=$(git rev-parse --git-path prune-race-once) + if test "$1" = preparing && test ! -e "$marker" + then + >"$marker" + git update-ref refs/remotes/origin/race HEAD + fi + exit 0 + EOF + ( + cd race-prune && + two=$(git rev-parse HEAD) && + test_must_fail git remote prune origin >out 2>err && + test_cmp_rev "$two" refs/remotes/origin/race && + test_must_fail git rev-parse --verify refs/remotes/origin/other && + test_grep ! "\[pruned\]" out + ) +' + test_expect_success 'hook allows updating ref if successful' ' git reset --hard PRE && test_hook reference-transaction <<-\EOF && -- 2.39.3 (Apple Git-146) ``` ## Junio C Hamano, 2026-09-22 18:55 Subject: Re: [PATCH v3 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: <3315d5f47ad7d8bcdbeda90b161606507c7040ea.1790079917.git.maciej.ciemborowicz@gmail.com> ``` Maciej Ciemborowicz writes: > refs_delete_refs() performs unconditional deletions, so callers cannot > preserve old values that they have already resolved. Consequently, > reference-transaction hooks see a null old OID. > > Let callers provide an optional array of expected old OIDs in parallel with > the refname list. When the array is provided, delete the ref at position N > only if it still points at the OID at position N. A null OID requests an > unconditional deletion for refs whose old value cannot be resolved, such as > broken refs. > > Use REF_TRANSACTION_ALLOW_FAILURE when old OIDs are supplied. This retains > the helper's best-effort behavior: an old-OID mismatch rejects that deletion > while independent deletions in the batch can still proceed. The history around this area seems to look like this (you can use "git blame" to figure this out yourself). * 98ffd5ff67 (delete_refs(): new function for the refs API, 2015-06-22) started the API function to allow multiple refs in bulk. The comment in refs.h that says refs_delete_refs() is not done in an all-or-nothing transaction has been there ever since. * 2fb330ca72 (packed_delete_refs(): implement method, 2017-09-08) started the "because these operations cannot fail, we can afford to run bulk deletion in a transaction without having to worry about making it all-or-none" for the packed backends. * e85e5dd78a (refs/files: use transactions to delete references, 2023-11-14) did the same for the files backends. * d6f8e72982 (refs: deduplicate code to delete references, 2023-11-14) consolidated the "because these cannot fail, we can afford to run bulk deletion in a transaction without making it all-or-none" codepaths between files and packed backends. * 23fc8e4f61 (refs: implement batch reference update support, 2025-04-08) introduced REF_TRANSACTION_ALLOW_FAILURE so that some callers can take advantage of "ref transactions" as a batched update mechanism, without having to roll everything back upon a failure. Doesn't the above observation suggest us that we should always be passing to ref_store_transaction_begin() inside refs_delete_refs() the REF_TRANSACTION_ALLOW_FAILURE flag? I would say that it was a missed clean-up opportunity at 23fc8e4f61 that we didn't do so back then. > diff --git a/refs.c b/refs.c > index 92d5df5b7..1e0f432ed 100644 > --- a/refs.c > +++ b/refs.c > @@ -3069,34 +3070,60 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio > } > } > > +struct delete_refs_rejection_data { > + int failures; > +}; This makes readers expect that we would be counting failures, e.g., the caller may request deletion of 100 refs and we report 30 of them failed to be deleted. > +static void delete_refs_rejection_handler(const char *refname, > + const struct object_id *old_oid UNUSED, > + const struct object_id *new_oid UNUSED, > + const char *old_target UNUSED, > + const char *new_target UNUSED, > + enum ref_transaction_error err, > + const char *details, > + void *cb_data) > +{ > + struct delete_refs_rejection_data *data = cb_data; > + > + warning(_("could not delete reference %s: %s"), refname, > + details ? details : ref_transaction_error_msg(err)); > + data->failures = 1; > +} But that is not what is happening. If we wanted to count, it is a simple matter of incrementing the data->failures member instead of assigning 1 to it, of course. It also might be annoying to see 30 warning messages in such a case---or it may be what the caller is asking. I cannot tell. If we wanted to squelch excessive warning messages, we could count and cut-off after N failures, of course. > int refs_delete_refs(struct ref_store *refs, const char *logmsg, > - struct string_list *refnames, unsigned int flags) > + struct string_list *refnames, > + const struct oid_array *old_oids, > + unsigned int flags) > { > + struct delete_refs_rejection_data rejection_data = { 0 }; > struct ref_transaction *transaction; > struct strbuf err = STRBUF_INIT; > - struct string_list_item *item; > + size_t i; > int ret = 0, failures = 0; > char *msg; > > if (!refnames->nr) > return 0; > + if (old_oids && old_oids->nr != refnames->nr) > + BUG("refname and old OID counts do not match"); > > msg = normalize_reflog_message(logmsg); > > - /* > - * Since we don't check the references' old_oids, the > - * individual updates can't fail, so we can pack all of the > - * updates into a single transaction. > - */ > - transaction = ref_store_transaction_begin(refs, 0, &err); > + transaction = ref_store_transaction_begin(refs, > + old_oids ? REF_TRANSACTION_ALLOW_FAILURE : 0, &err); This is the conditional/unconditional REF_TRANSACTION_ALLOW_FAILURE I discussed earlier. > if (!transaction) { > ret = error("%s", err.buf); > goto out; > } > > - for_each_string_list_item(item, refnames) { > + for (i = 0; i < refnames->nr; i++) { > + struct string_list_item *item = &refnames->items[i]; > + const struct object_id *old_oid = old_oids ? &old_oids->oid[i] : NULL; > + > + if (old_oid && is_null_oid(old_oid)) > + old_oid = NULL; I think there was a comment by another reviewer on the previous round around this area, which was never answered. In general, it is a polite thing to respond to review messages and see that your response is acknowledged before you send an updated patch. I _think_ the reason why you need to treat null_oid specially is because you are using a flat array of object names, not an array of pointers to individual object names, but in that case, I wonder if ref_transaction_delete() should be the one who pays attention to the NULL-ness of its old_oid parameter? The current code does detect and reject (old_oid && is_null_oid(old_oid)) case, but I am not sure what we are gaining by that limitation. Rather I wonder if the first two lines of the function should read more like if (old_oid && is_null_oid(old_oid)) - BUG("delete called with old_oid set to zeros"); + old_oid = NULL; not forcing the callers (like we see above) to do the same. > @@ -3112,9 +3139,14 @@ int refs_delete_refs(struct ref_store *refs, const char *logmsg, > refnames->items[0].string, err.buf); > else > error(_("could not delete references: %s"), err.buf); > - } > + } else if (old_oids) > + ref_transaction_for_each_rejected_update(transaction, > + delete_refs_rejection_handler, > + &rejection_data); I personally feel that we should be weaning ourselves off of the assumption that presence of old_oids[] is the ONLY thing to cause rejection. IOW, always call for-each-rejected-update here regardless of old_oids != NULL. > out: > + if (rejection_data.failures) > + failures = 1; This is quite roundabout thing to do. rejection_data.failures, unlike my initial assumption, is not counting but is either 0 or 1, so failures here is also either 0 or 1, and then ... > if (!ret && failures) > ret = -1; ... if we have the failures computed to non-zero, we make sure ret is not zero. Shouldn't we at least get rid of the local variable failures? And if a variable FOO is not counting the number of FOO, do not name it FOOs. If it is a Boolean recording if we got FOOed, call it as such. My preference in this code path is to actually count failures in the member "int failures" of rejection_data structure, but if we are not counting, then call it "bool failed", perhaps. out: if (!ret && rejection_data.failed) ret = -1; ``` ## Junio C Hamano, 2026-09-22 19:16 Subject: Re: [PATCH v3 3/3] fetch, remote: retain old OIDs when pruning refs Message-ID: In-Reply-To: <3f3062252ac1aa057b9ee9a2dd9892e629ba7a82.1790079917.git.maciej.ciemborowicz@gmail.com> ``` Maciej Ciemborowicz writes: > if (!dry_run) { > if (transaction) { > for (ref = stale_refs; ref; ref = ref->next) { > - result = ref_transaction_delete(transaction, ref->name, NULL, > - NULL, 0, "fetch: prune", &err); > + result = ref_transaction_delete(transaction, ref->name, > + &ref->new_oid, NULL, 0, > + "fetch: prune", &err); > if (result) > goto cleanup; > } > } else { > + for (ref = stale_refs; ref; ref = ref->next) { > + string_list_append(&refnames, ref->name); > + oid_array_append(&old_oids, &ref->new_oid); > + } > result = refs_delete_refs(get_main_ref_store(the_repository), > "fetch: prune", &refnames, > - NULL, 0); > + &old_oids, 0); > } > + if (result) > + goto cleanup; > } Hmph, I may not be reading the code correctly, but the last "goto cleanup" in the above block can happen when refs_delete_refs() call that internally uses the best effort transaction sees an error. If we were about to prune 30 refs but failed to prune one of them, and if we are running with non-negative verbosity, don't we still want to make the "[deleted]" report for the 29 of them and possibly report "[failed to delete]" for the one that failed? > > if (verbosity >= 0) { > int summary_width = transport_summary_width(stale_refs); > > + if (!refnames.nr) > + for (ref = stale_refs; ref; ref = ref->next) > + string_list_append(&refnames, ref->name); > for (ref = stale_refs; ref; ref = ref->next) { > display_ref_update(display_state, '-', _("[deleted]"), NULL, > _("(none)"), ref->name, > @@ -1639,17 +1650,24 @@ static int prune_remote(const char *remote, int dry_run) > printf_ln(_("Pruning %s"), remote); > printf_ln(_("URL: %s"), states.remote->url.v[0]); > > - for_each_string_list_item(item, &states.stale) > - string_list_append(&refs_to_prune, item->util); > - string_list_sort(&refs_to_prune); > + for_each_string_list_item(item, &states.stale) { > + struct stale_ref *stale_ref = item->util; > + > + string_list_append(&refs_to_prune, stale_ref->name); > + oid_array_append(&old_oids, &stale_ref->oid); > + } > > - if (!dry_run) > + if (!dry_run) { > result |= refs_delete_refs(get_main_ref_store(the_repository), > "remote: prune", &refs_to_prune, > - NULL, 0); > + &old_oids, 0); > + if (result) > + goto cleanup; > + } Ditto. Beyond the post context of this hunk ... > for_each_string_list_item(item, &states.stale) { > - const char *refname = item->util; > + struct stale_ref *stale_ref = item->util; > + const char *refname = stale_ref->name; > > if (dry_run) > printf_ln(_(" * [would prune] %s"), ... around here is a code that reports "* [pruned]" for the ones that we successfully removed, which is now ignored when even one of the bulk removal fails. ``` ## Maciej Ciemborowicz, 2026-09-22 19:21 Subject: Re: [PATCH v3 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: ``` On Tue, Sep 22, 2026 at 8:55 PM Junio C Hamano wrote: > I think there was a comment by another reviewer on the previous > round around this area, which was never answered. In general, it is > a polite thing to respond to review messages and see that your > response is acknowledged before you send an updated patch. I'm very sorry, I didn't check my email before submitting the patch. I'll take a look at this. By the way, I expected the review process to be tough, but I'm starting to wonder if I'll ever get through it :). You mentioned earlier that you could prepare a patch. Is that offer still on the table? Cheers, Maciej Ciemborowicz ``` ## Maciej Ciemborowicz, 2026-09-22 22:29 Subject: [PATCH v4 0/3] refs: report old OIDs for batched deletions Message-ID: In-Reply-To: ``` Okay, I will keep trying. I believe this version addresses all comments from the previous round, and I hope I have not missed anything. This follows up on the reference-transaction bug report at [1]. The reference-transaction hook receives zero as both the old and new OID when branch, tag, fetch, and remote delete refs through refs_delete_refs(). Those callers already know the values that they selected for deletion. Teach refs_delete_refs() to accept a parallel array of expected old OIDs and pass them into the transaction. Besides making hook records useful, this restores conditional deletion for branch and tag and adds it to pruning without additional ref reads. Non-atomic batches preserve best-effort behavior, while atomic fetches remain all-or-nothing. Changes since v3: * Rebase onto master at 3bc0341127 (Git 2.56-rc2). * Always use REF_TRANSACTION_ALLOW_FAILURE in refs_delete_refs(), including unconditional batches, and always inspect rejected updates. * Count individual failures directly and remove the redundant failures variable. * Treat a null old OID as an unconditional deletion in ref_transaction_delete(), instead of requiring each caller to convert it. * Let refs_delete_refs() return the exact set of individually failed refs. * Continue reporting successful fetch and remote prune deletions after a partial failure, while omitting rejected refs from deletion and dangling symref reports. * Add coverage for partial fetch pruning as well as remote pruning. The full test suite passes. The focused reference-transaction tests also pass with SHA-1 and SHA-256 using both the files and reftable backends. [1] https://lore.kernel.org/git/CACQ=SRGTTdQ+dHXhN6F52dBv5KxZBRfk_Em2fvmEmGJDoB6oTg@mail.gmail.com/ Maciej Ciemborowicz (3): refs: allow callers to supply old OIDs for batch deletion branch, tag: retain old OIDs in batched deletions fetch, remote: retain old OIDs when pruning refs bisect.c | 2 +- builtin/branch.c | 7 +- builtin/fetch.c | 30 ++++-- builtin/remote.c | 47 +++++++-- builtin/tag.c | 28 ++++-- refs.c | 67 ++++++++++--- refs.h | 31 ++++-- t/helper/test-ref-store.c | 2 +- t/t1416-ref-transaction-hooks.sh | 160 +++++++++++++++++++++++++++++++ 9 files changed, 324 insertions(+), 50 deletions(-) Range-diff against v3: 1: 3315d5f47 ! 1: f4a9d065c refs: allow callers to supply old OIDs for batch deletion @@ Commit message reference-transaction hooks see a null old OID. Let callers provide an optional array of expected old OIDs in parallel with - the refname list. When the array is provided, delete the ref at position N - only if it still points at the OID at position N. A null OID requests an - unconditional deletion for refs whose old value cannot be resolved, such as - broken refs. + the refname list. Delete the ref at position N only if it still points at + the OID at position N. Treat a null OID as an unconditional deletion in + ref_transaction_delete(), allowing callers to include broken refs whose old + value cannot be resolved. - Use REF_TRANSACTION_ALLOW_FAILURE when old OIDs are supplied. This retains - the helper's best-effort behavior: an old-OID mismatch rejects that deletion - while independent deletions in the batch can still proceed. + refs_delete_refs() has always promised best-effort deletion. Always use + REF_TRANSACTION_ALLOW_FAILURE and report rejected updates so one failure + does not prevent independent refs in the batch from being deleted. Let + callers request the exact set of failed refs when they need to report + partial results. This also completes the conversion that was missed when + batched transaction failure support was introduced. Signed-off-by: Maciej Ciemborowicz @@ bisect.c: int bisect_clean_state(void) result = refs_delete_refs(get_main_ref_store(the_repository), "bisect: remove", &refs_for_removal, - REF_NO_DEREF); -+ NULL, REF_NO_DEREF); ++ NULL, NULL, REF_NO_DEREF); string_list_clear(&refs_for_removal, 0); unlink_or_warn(git_path_bisect_ancestors_ok()); unlink_or_warn(git_path_bisect_log()); @@ builtin/remote.c: static int rm(int argc, const char **argv, const char *prefix, result = refs_delete_refs(get_main_ref_store(the_repository), "remote: remove", &branches, - REF_NO_DEREF); -+ NULL, REF_NO_DEREF); ++ NULL, NULL, REF_NO_DEREF); string_list_clear(&branches, 0); if (skipped.nr) { @@ refs.c #include "odb.h" #include "object.h" #include "path.h" +@@ refs.c: int ref_transaction_delete(struct ref_transaction *transaction, + struct strbuf *err) + { + if (old_oid && is_null_oid(old_oid)) +- BUG("delete called with old_oid set to zeros"); ++ old_oid = NULL; + if (old_oid && old_target) + BUG("delete called with both old_oid and old_target set"); + if (old_target && !(flags & REF_NO_DEREF)) @@ refs.c: void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio } } +struct delete_refs_rejection_data { + int failures; ++ struct string_list *failed_refs; +}; + +static void delete_refs_rejection_handler(const char *refname, @@ refs.c: void ref_transaction_for_each_rejected_update(struct ref_transaction *tr + + warning(_("could not delete reference %s: %s"), refname, + details ? details : ref_transaction_error_msg(err)); -+ data->failures = 1; ++ data->failures++; ++ if (data->failed_refs) ++ string_list_insert(data->failed_refs, refname); +} + int refs_delete_refs(struct ref_store *refs, const char *logmsg, - struct string_list *refnames, unsigned int flags) + struct string_list *refnames, + const struct oid_array *old_oids, ++ struct string_list *failed_refs, + unsigned int flags) { -+ struct delete_refs_rejection_data rejection_data = { 0 }; ++ struct delete_refs_rejection_data rejection_data = { ++ .failed_refs = failed_refs, ++ }; struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; - struct string_list_item *item; +- int ret = 0, failures = 0; + size_t i; - int ret = 0, failures = 0; ++ int ret = 0; char *msg; if (!refnames->nr) return 0; + if (old_oids && old_oids->nr != refnames->nr) + BUG("refname and old OID counts do not match"); ++ if (failed_refs && !failed_refs->strdup_strings) ++ BUG("failed ref list does not duplicate strings"); msg = normalize_reflog_message(logmsg); @@ refs.c: void ref_transaction_for_each_rejected_update(struct ref_transaction *tr - */ - transaction = ref_store_transaction_begin(refs, 0, &err); + transaction = ref_store_transaction_begin(refs, -+ old_oids ? REF_TRANSACTION_ALLOW_FAILURE : 0, &err); ++ REF_TRANSACTION_ALLOW_FAILURE, &err); if (!transaction) { ret = error("%s", err.buf); goto out; @@ refs.c: void ref_transaction_for_each_rejected_update(struct ref_transaction *tr + struct string_list_item *item = &refnames->items[i]; + const struct object_id *old_oid = old_oids ? &old_oids->oid[i] : NULL; + -+ if (old_oid && is_null_oid(old_oid)) -+ old_oid = NULL; ret = ref_transaction_delete(transaction, item->string, - NULL, NULL, flags, msg, &err); + old_oid, NULL, flags, msg, &err); if (ret) { warning(_("could not delete reference %s: %s"), item->string, err.buf); + strbuf_reset(&err); +- failures = 1; ++ rejection_data.failures++; ++ if (failed_refs) ++ string_list_insert(failed_refs, item->string); + } + } + @@ refs.c: int refs_delete_refs(struct ref_store *refs, const char *logmsg, - refnames->items[0].string, err.buf); else error(_("could not delete references: %s"), err.buf); -- } -+ } else if (old_oids) + } ++ if (!ret) + ref_transaction_for_each_rejected_update(transaction, -+ delete_refs_rejection_handler, -+ &rejection_data); ++ delete_refs_rejection_handler, ++ &rejection_data); out: -+ if (rejection_data.failures) -+ failures = 1; - if (!ret && failures) +- if (!ret && failures) ++ if (!ret && rejection_data.failures) ret = -1; ref_transaction_free(transaction); + strbuf_release(&err); ## refs.h ## @@ @@ refs.h: int refs_delete_ref(struct ref_store *refs, const char *msg, /* - * Delete the specified references. If there are any problems, emit +- * errors but attempt to keep going (i.e., the deletes are not done in +- * an all-or-nothing transaction). msg and flags are passed through to +- * ref_transaction_delete(). + * Delete the specified references. If old_oids is non-NULL, it must contain + * an entry for each refname, in the same order. Each non-null OID is used to + * verify the current value of the corresponding reference before deleting + * it. A null OID requests an unconditional deletion, which allows callers to + * include broken refs whose old value cannot be resolved. + * -+ * If there are any problems, emit - * errors but attempt to keep going (i.e., the deletes are not done in - * an all-or-nothing transaction). msg and flags are passed through to - * ref_transaction_delete(). ++ * If failed_refs is non-NULL, it must be initialized with ++ * STRING_LIST_INIT_DUP. The names of individual updates that cannot be queued ++ * or are rejected while processing the best-effort batch are inserted into ++ * it. A transaction-wide failure is returned without populating the list. ++ * ++ * If there are any problems, emit errors but attempt to keep going (i.e., ++ * the deletes are not done in an all-or-nothing transaction). msg and flags ++ * are passed through to ref_transaction_delete(). */ int refs_delete_refs(struct ref_store *refs, const char *msg, - struct string_list *refnames, unsigned int flags); + struct string_list *refnames, + const struct oid_array *old_oids, ++ struct string_list *failed_refs, + unsigned int flags); /** Delete a reflog */ int refs_delete_reflog(struct ref_store *refs, const char *refname); +@@ refs.h: int ref_transaction_create(struct ref_transaction *transaction, + struct strbuf *err); + + /* +- * Add a reference deletion to transaction. If old_oid is non-NULL, +- * then it holds the value that the reference should have had before +- * the update (which must not be null_oid). ++ * Add a reference deletion to transaction. If old_oid is non-NULL and not ++ * null_oid, then it holds the value that the reference should have had before ++ * the update. Passing null_oid is equivalent to passing NULL and disables the ++ * old value check. + * + * See the above comment "Reference transaction updates" for more + * information. ## t/helper/test-ref-store.c ## @@ t/helper/test-ref-store.c: static int cmd_delete_refs(struct ref_store *refs, const char **argv) @@ t/helper/test-ref-store.c: static int cmd_delete_refs(struct ref_store *refs, co string_list_append(&refnames, *argv++); - result = refs_delete_refs(refs, msg, &refnames, flags); -+ result = refs_delete_refs(refs, msg, &refnames, NULL, flags); ++ result = refs_delete_refs(refs, msg, &refnames, NULL, NULL, flags); string_list_clear(&refnames, 0); return result; } 2: 2065188aa ! 2: 918c97d2b branch, tag: retain old OIDs in batched deletions @@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int ki if (!(flags & DELETE_BRANCH_DRY_RUN) && refs_delete_refs(get_main_ref_store(the_repository), NULL, - &refs_to_delete, NULL, REF_NO_DEREF)) -+ &refs_to_delete, &old_oids, REF_NO_DEREF)) ++ &refs_to_delete, &old_oids, NULL, REF_NO_DEREF)) ret = 1; for_each_string_list_item(item, &refs_to_delete) { @@ builtin/tag.c: static int for_each_tag_name(const char **argv, each_tag_name_fn + result = for_each_tag_name(argv, collect_tags, &data); if (refs_delete_refs(get_main_ref_store(the_repository), NULL, - &refs_to_delete, NULL, REF_NO_DEREF)) -+ &data.refs, &data.old_oids, REF_NO_DEREF)) ++ &data.refs, &data.old_oids, NULL, REF_NO_DEREF)) result = 1; - for_each_string_list_item(item, &refs_to_delete) { 3: 3f3062252 ! 3: 6f34853c7 fetch, remote: retain old OIDs when pruning refs @@ Commit message Non-atomic pruning uses refs_delete_refs(), whose partial-failure mode still deletes unaffected stale refs. An atomic fetch remains all-or-nothing. - Reuse values collected while finding stale refs, avoiding additional ref - reads. Avoid reporting deletion status when pruning encounters a rejected - update. + Continue reporting successful non-atomic deletions when another deletion is + rejected, but do not report the rejected ref as deleted or use it when + checking for newly dangling symrefs. Use the rejected-ref list returned by + refs_delete_refs() so reporting reflects the transaction result without + additional ref reads. Signed-off-by: Maciej Ciemborowicz @@ builtin/fetch.c: static int prune_refs(struct display_state *display_state, - - for (ref = stale_refs; ref; ref = ref->next) - string_list_append(&refnames, ref->name); ++ struct string_list deleted_refs = STRING_LIST_INIT_NODUP; ++ struct string_list failed_refs = STRING_LIST_INIT_DUP; + struct oid_array old_oids = OID_ARRAY_INIT; if (!dry_run) { @@ builtin/fetch.c: static int prune_refs(struct display_state *display_state, - result = ref_transaction_delete(transaction, ref->name, NULL, - NULL, 0, "fetch: prune", &err); + result = ref_transaction_delete(transaction, ref->name, -+ &ref->new_oid, NULL, 0, -+ "fetch: prune", &err); ++ &ref->new_oid, NULL, 0, ++ "fetch: prune", &err); if (result) goto cleanup; } @@ builtin/fetch.c: static int prune_refs(struct display_state *display_state, result = refs_delete_refs(get_main_ref_store(the_repository), "fetch: prune", &refnames, - NULL, 0); -+ &old_oids, 0); ++ &old_oids, &failed_refs, 0); ++ if (result && !failed_refs.nr) ++ goto cleanup; } -+ if (result) -+ goto cleanup; } - if (verbosity >= 0) { +@@ builtin/fetch.c: static int prune_refs(struct display_state *display_state, int summary_width = transport_summary_width(stale_refs); -+ if (!refnames.nr) -+ for (ref = stale_refs; ref; ref = ref->next) -+ string_list_append(&refnames, ref->name); for (ref = stale_refs; ref; ref = ref->next) { ++ if (string_list_has_string(&failed_refs, ref->name)) ++ continue; ++ display_ref_update(display_state, '-', _("[deleted]"), NULL, _("(none)"), ref->name, -@@ builtin/fetch.c: static int prune_refs(struct display_state *display_state, + &ref->new_oid, &ref->old_oid, + summary_width); ++ string_list_append(&deleted_refs, ref->name); + } +- string_list_sort(&refnames); ++ string_list_sort(&deleted_refs); + refs_warn_dangling_symrefs(get_main_ref_store(the_repository), +- stderr, " ", dry_run, &refnames); ++ stderr, " ", dry_run, &deleted_refs); + } cleanup: string_list_clear(&refnames, 0); ++ string_list_clear(&deleted_refs, 0); ++ string_list_clear(&failed_refs, 0); + oid_array_clear(&old_oids); strbuf_release(&err); free_refs(stale_refs); @@ builtin/remote.c: static int prune_remote(const char *remote, int dry_run) int result = 0; struct ref_states states = REF_STATES_INIT; struct string_list refs_to_prune = STRING_LIST_INIT_NODUP; ++ struct string_list pruned_refs = STRING_LIST_INIT_NODUP; ++ struct string_list failed_refs = STRING_LIST_INIT_DUP; + struct oid_array old_oids = OID_ARRAY_INIT; struct string_list_item *item; @@ builtin/remote.c: static int prune_remote(const char *remote, int dry_run) result |= refs_delete_refs(get_main_ref_store(the_repository), "remote: prune", &refs_to_prune, - NULL, 0); -+ &old_oids, 0); -+ if (result) ++ &old_oids, &failed_refs, 0); ++ if (result && !failed_refs.nr) + goto cleanup; + } @@ builtin/remote.c: static int prune_remote(const char *remote, int dry_run) - const char *refname = item->util; + struct stale_ref *stale_ref = item->util; + const char *refname = stale_ref->name; ++ ++ if (string_list_has_string(&failed_refs, refname)) ++ continue; if (dry_run) printf_ln(_(" * [would prune] %s"), @@ builtin/remote.c: static int prune_remote(const char *remote, int dry_run) + else + printf_ln(_(" * [pruned] %s"), + abbrev_ref(refname, "refs/remotes/")); ++ string_list_append(&pruned_refs, refname); + } + refs_warn_dangling_symrefs(get_main_ref_store(the_repository), - stdout, " ", dry_run, &refs_to_prune); +- stdout, " ", dry_run, &refs_to_prune); ++ stdout, " ", dry_run, &pruned_refs); +cleanup: string_list_clear(&refs_to_prune, 0); ++ string_list_clear(&pruned_refs, 0); ++ string_list_clear(&failed_refs, 0); + oid_array_clear(&old_oids); free_remote_ref_states(&states); return result; @@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'branch deletion rejects a + ) +' + -+test_expect_success 'remote prune rejects a concurrent update' ' ++test_expect_success 'remote prune reports deletions around a concurrent update' ' + test_when_finished "rm -rf race-empty.git race-prune" && + git init --bare race-empty.git && + git init race-prune && @@ t/t1416-ref-transaction-hooks.sh: test_expect_success 'branch deletion rejects a + test_must_fail git remote prune origin >out 2>err && + test_cmp_rev "$two" refs/remotes/origin/race && + test_must_fail git rev-parse --verify refs/remotes/origin/other && -+ test_grep ! "\[pruned\]" out ++ test_grep "\[pruned\].*origin/other" out && ++ test_grep ! "\[pruned\].*origin/race" out && ++ test_grep "could not delete reference refs/remotes/origin/race" err ++ ) ++' ++ ++test_expect_success 'fetch prune reports deletions around a concurrent update' ' ++ test_when_finished "rm -rf fetch-empty.git fetch-prune" && ++ git init --bare fetch-empty.git && ++ git init fetch-prune && ++ ( ++ cd fetch-prune && ++ git commit --allow-empty -m one && ++ one=$(git rev-parse HEAD) && ++ git commit --allow-empty -m two && ++ git remote add origin ../fetch-empty.git && ++ git update-ref refs/remotes/origin/race "$one" && ++ git update-ref refs/remotes/origin/other "$one" ++ ) && ++ test_hook -C fetch-prune reference-transaction <<-\EOF && ++ marker=$(git rev-parse --git-path prune-race-once) ++ if test "$1" = preparing && test ! -e "$marker" ++ then ++ >"$marker" ++ git update-ref refs/remotes/origin/race HEAD ++ fi ++ exit 0 ++ EOF ++ ( ++ cd fetch-prune && ++ two=$(git rev-parse HEAD) && ++ test_must_fail git fetch --prune origin >out 2>err && ++ test_cmp_rev "$two" refs/remotes/origin/race && ++ test_must_fail git rev-parse --verify refs/remotes/origin/other && ++ test_grep "\[deleted\].*origin/other" err && ++ test_grep ! "\[deleted\].*origin/race" err && ++ test_grep "could not delete reference refs/remotes/origin/race" err + ) +' + -- 2.39.3 (Apple Git-146) ``` ## Maciej Ciemborowicz, 2026-09-22 22:31 Subject: [PATCH v4 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: ``` refs_delete_refs() performs unconditional deletions, so callers cannot preserve old values that they have already resolved. Consequently, reference-transaction hooks see a null old OID. Let callers provide an optional array of expected old OIDs in parallel with the refname list. Delete the ref at position N only if it still points at the OID at position N. Treat a null OID as an unconditional deletion in ref_transaction_delete(), allowing callers to include broken refs whose old value cannot be resolved. refs_delete_refs() has always promised best-effort deletion. Always use REF_TRANSACTION_ALLOW_FAILURE and report rejected updates so one failure does not prevent independent refs in the batch from being deleted. Let callers request the exact set of failed refs when they need to report partial results. This also completes the conversion that was missed when batched transaction failure support was introduced. Signed-off-by: Maciej Ciemborowicz --- bisect.c | 2 +- builtin/branch.c | 3 +- builtin/fetch.c | 2 +- builtin/remote.c | 5 +-- builtin/tag.c | 3 +- refs.c | 67 +++++++++++++++++++++++++++++++-------- refs.h | 31 +++++++++++++----- t/helper/test-ref-store.c | 2 +- 8 files changed, 86 insertions(+), 29 deletions(-) diff --git a/bisect.c b/bisect.c index 9cbb3dc67..c8ab16d1e 100644 --- a/bisect.c +++ b/bisect.c @@ -1206,7 +1206,7 @@ int bisect_clean_state(void) string_list_append(&refs_for_removal, "BISECT_EXPECTED_REV"); result = refs_delete_refs(get_main_ref_store(the_repository), "bisect: remove", &refs_for_removal, - REF_NO_DEREF); + NULL, NULL, REF_NO_DEREF); string_list_clear(&refs_for_removal, 0); unlink_or_warn(git_path_bisect_ancestors_ok()); unlink_or_warn(git_path_bisect_log()); diff --git a/builtin/branch.c b/builtin/branch.c index a613148fc..c9f259d04 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -351,7 +351,8 @@ static int delete_branches(int argc, const char **argv, int kinds, } if (!(flags & DELETE_BRANCH_DRY_RUN) && - refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) + refs_delete_refs(get_main_ref_store(the_repository), NULL, + &refs_to_delete, NULL, REF_NO_DEREF)) ret = 1; for_each_string_list_item(item, &refs_to_delete) { diff --git a/builtin/fetch.c b/builtin/fetch.c index 533fdfe7d..b662216bf 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1486,7 +1486,7 @@ static int prune_refs(struct display_state *display_state, } else { result = refs_delete_refs(get_main_ref_store(the_repository), "fetch: prune", &refnames, - 0); + NULL, 0); } } diff --git a/builtin/remote.c b/builtin/remote.c index de989ea3b..56b06845b 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1073,7 +1073,7 @@ static int rm(int argc, const char **argv, const char *prefix, if (!result) result = refs_delete_refs(get_main_ref_store(the_repository), "remote: remove", &branches, - REF_NO_DEREF); + NULL, NULL, REF_NO_DEREF); string_list_clear(&branches, 0); if (skipped.nr) { @@ -1645,7 +1645,8 @@ static int prune_remote(const char *remote, int dry_run) if (!dry_run) result |= refs_delete_refs(get_main_ref_store(the_repository), - "remote: prune", &refs_to_prune, 0); + "remote: prune", &refs_to_prune, + NULL, 0); for_each_string_list_item(item, &states.stale) { const char *refname = item->util; diff --git a/builtin/tag.c b/builtin/tag.c index 06c125b53..40874a292 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -122,7 +122,8 @@ static int delete_tags(const char **argv) struct string_list_item *item; result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); - if (refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) + if (refs_delete_refs(get_main_ref_store(the_repository), NULL, + &refs_to_delete, NULL, REF_NO_DEREF)) result = 1; for_each_string_list_item(item, &refs_to_delete) { diff --git a/refs.c b/refs.c index 92d5df5b7..13ee2d459 100644 --- a/refs.c +++ b/refs.c @@ -16,6 +16,7 @@ #include "refs/refs-internal.h" #include "hook.h" #include "object-name.h" +#include "oid-array.h" #include "odb.h" #include "object.h" #include "path.h" @@ -1523,7 +1524,7 @@ int ref_transaction_delete(struct ref_transaction *transaction, struct strbuf *err) { if (old_oid && is_null_oid(old_oid)) - BUG("delete called with old_oid set to zeros"); + old_oid = NULL; if (old_oid && old_target) BUG("delete called with both old_oid and old_target set"); if (old_target && !(flags & REF_NO_DEREF)) @@ -3069,39 +3070,73 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio } } +struct delete_refs_rejection_data { + int failures; + struct string_list *failed_refs; +}; + +static void delete_refs_rejection_handler(const char *refname, + const struct object_id *old_oid UNUSED, + const struct object_id *new_oid UNUSED, + const char *old_target UNUSED, + const char *new_target UNUSED, + enum ref_transaction_error err, + const char *details, + void *cb_data) +{ + struct delete_refs_rejection_data *data = cb_data; + + warning(_("could not delete reference %s: %s"), refname, + details ? details : ref_transaction_error_msg(err)); + data->failures++; + if (data->failed_refs) + string_list_insert(data->failed_refs, refname); +} + int refs_delete_refs(struct ref_store *refs, const char *logmsg, - struct string_list *refnames, unsigned int flags) + struct string_list *refnames, + const struct oid_array *old_oids, + struct string_list *failed_refs, + unsigned int flags) { + struct delete_refs_rejection_data rejection_data = { + .failed_refs = failed_refs, + }; struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; - struct string_list_item *item; - int ret = 0, failures = 0; + size_t i; + int ret = 0; char *msg; if (!refnames->nr) return 0; + if (old_oids && old_oids->nr != refnames->nr) + BUG("refname and old OID counts do not match"); + if (failed_refs && !failed_refs->strdup_strings) + BUG("failed ref list does not duplicate strings"); msg = normalize_reflog_message(logmsg); - /* - * Since we don't check the references' old_oids, the - * individual updates can't fail, so we can pack all of the - * updates into a single transaction. - */ - transaction = ref_store_transaction_begin(refs, 0, &err); + transaction = ref_store_transaction_begin(refs, + REF_TRANSACTION_ALLOW_FAILURE, &err); if (!transaction) { ret = error("%s", err.buf); goto out; } - for_each_string_list_item(item, refnames) { + for (i = 0; i < refnames->nr; i++) { + struct string_list_item *item = &refnames->items[i]; + const struct object_id *old_oid = old_oids ? &old_oids->oid[i] : NULL; + ret = ref_transaction_delete(transaction, item->string, - NULL, NULL, flags, msg, &err); + old_oid, NULL, flags, msg, &err); if (ret) { warning(_("could not delete reference %s: %s"), item->string, err.buf); strbuf_reset(&err); - failures = 1; + rejection_data.failures++; + if (failed_refs) + string_list_insert(failed_refs, item->string); } } @@ -3113,9 +3148,13 @@ int refs_delete_refs(struct ref_store *refs, const char *logmsg, else error(_("could not delete references: %s"), err.buf); } + if (!ret) + ref_transaction_for_each_rejected_update(transaction, + delete_refs_rejection_handler, + &rejection_data); out: - if (!ret && failures) + if (!ret && rejection_data.failures) ret = -1; ref_transaction_free(transaction); strbuf_release(&err); diff --git a/refs.h b/refs.h index 9979446d1..43f7a32f2 100644 --- a/refs.h +++ b/refs.h @@ -9,6 +9,7 @@ struct fsck_options; struct object_id; struct ref_store; +struct oid_array; struct strbuf; struct string_list; struct string_list_item; @@ -623,13 +624,26 @@ int refs_delete_ref(struct ref_store *refs, const char *msg, unsigned int flags); /* - * Delete the specified references. If there are any problems, emit - * errors but attempt to keep going (i.e., the deletes are not done in - * an all-or-nothing transaction). msg and flags are passed through to - * ref_transaction_delete(). + * Delete the specified references. If old_oids is non-NULL, it must contain + * an entry for each refname, in the same order. Each non-null OID is used to + * verify the current value of the corresponding reference before deleting + * it. A null OID requests an unconditional deletion, which allows callers to + * include broken refs whose old value cannot be resolved. + * + * If failed_refs is non-NULL, it must be initialized with + * STRING_LIST_INIT_DUP. The names of individual updates that cannot be queued + * or are rejected while processing the best-effort batch are inserted into + * it. A transaction-wide failure is returned without populating the list. + * + * If there are any problems, emit errors but attempt to keep going (i.e., + * the deletes are not done in an all-or-nothing transaction). msg and flags + * are passed through to ref_transaction_delete(). */ int refs_delete_refs(struct ref_store *refs, const char *msg, - struct string_list *refnames, unsigned int flags); + struct string_list *refnames, + const struct oid_array *old_oids, + struct string_list *failed_refs, + unsigned int flags); /** Delete a reflog */ int refs_delete_reflog(struct ref_store *refs, const char *refname); @@ -956,9 +970,10 @@ int ref_transaction_create(struct ref_transaction *transaction, struct strbuf *err); /* - * Add a reference deletion to transaction. If old_oid is non-NULL, - * then it holds the value that the reference should have had before - * the update (which must not be null_oid). + * Add a reference deletion to transaction. If old_oid is non-NULL and not + * null_oid, then it holds the value that the reference should have had before + * the update. Passing null_oid is equivalent to passing NULL and disables the + * old value check. * * See the above comment "Reference transaction updates" for more * information. diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c index db58f0058..29945f2b8 100644 --- a/t/helper/test-ref-store.c +++ b/t/helper/test-ref-store.c @@ -132,7 +132,7 @@ static int cmd_delete_refs(struct ref_store *refs, const char **argv) while (*argv) string_list_append(&refnames, *argv++); - result = refs_delete_refs(refs, msg, &refnames, flags); + result = refs_delete_refs(refs, msg, &refnames, NULL, NULL, flags); string_list_clear(&refnames, 0); return result; } -- 2.39.3 (Apple Git-146) ``` ## Maciej Ciemborowicz, 2026-09-22 22:31 Subject: [PATCH v4 2/3] branch, tag: retain old OIDs in batched deletions Message-ID: <918c97d2b4589f6616de33dced7471119ba86fde.1790113781.git.maciej.ciemborowicz@gmail.com> In-Reply-To: ``` Before 8198907795 (use delete_refs when deleting tags or branches, 2021-01-21), branch and tag deletion passed each resolved old OID to delete_ref(). This prevented the command from deleting a ref that another process had changed after it was inspected. The conversion to batched deletion dropped those old OIDs. Besides making the deletions unconditional, this causes reference-transaction hooks to report zero as both the old and new OID. Both commands still resolve the old OIDs before starting the deletion. Pass those values to refs_delete_refs(). This restores the old race protection and lets hooks receive useful old values without adding ref reads. If a ref changes concurrently, reject its deletion and preserve the new value. Signed-off-by: Maciej Ciemborowicz --- builtin/branch.c | 6 ++++- builtin/tag.c | 27 ++++++++++++++------ t/t1416-ref-transaction-hooks.sh | 44 ++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 9 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index c9f259d04..4ce1407bc 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -16,6 +16,7 @@ #include "commit.h" #include "gettext.h" #include "object-name.h" +#include "oid-array.h" #include "remote.h" #include "parse-options.h" #include "branch.h" @@ -248,6 +249,7 @@ static int delete_branches(int argc, const char **argv, int kinds, struct strbuf bname = STRBUF_INIT; enum interpret_branch_kind allowed_interpret; struct string_list refs_to_delete = STRING_LIST_INIT_DUP; + struct oid_array old_oids = OID_ARRAY_INIT; struct string_list_item *item; int branch_name_pos; const char *fmt_remotes = "refs/remotes/%s"; @@ -342,6 +344,7 @@ static int delete_branches(int argc, const char **argv, int kinds, } item = string_list_append(&refs_to_delete, name); + oid_array_append(&old_oids, &oid); item->util = xstrdup((ref_flags & REF_ISBROKEN) ? "broken" : (ref_flags & REF_ISSYMREF) ? target : repo_find_unique_abbrev(the_repository, &oid, DEFAULT_ABBREV)); @@ -352,7 +355,7 @@ static int delete_branches(int argc, const char **argv, int kinds, if (!(flags & DELETE_BRANCH_DRY_RUN) && refs_delete_refs(get_main_ref_store(the_repository), NULL, - &refs_to_delete, NULL, REF_NO_DEREF)) + &refs_to_delete, &old_oids, NULL, REF_NO_DEREF)) ret = 1; for_each_string_list_item(item, &refs_to_delete) { @@ -377,6 +380,7 @@ static int delete_branches(int argc, const char **argv, int kinds, free(describe_ref); } string_list_clear(&refs_to_delete, 0); + oid_array_clear(&old_oids); free(name); strbuf_release(&bname); diff --git a/builtin/tag.c b/builtin/tag.c index 40874a292..07116664d 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -105,28 +105,38 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn, return had_error; } +struct tags_to_delete { + struct string_list refs; + struct oid_array old_oids; +}; + static int collect_tags(const char *name UNUSED, const char *ref, const struct object_id *oid, void *cb_data) { - struct string_list *ref_list = cb_data; + struct tags_to_delete *data = cb_data; + struct string_list_item *item; - string_list_append(ref_list, ref); - ref_list->items[ref_list->nr - 1].util = oiddup(oid); + item = string_list_append(&data->refs, ref); + item->util = oiddup(oid); + oid_array_append(&data->old_oids, oid); return 0; } static int delete_tags(const char **argv) { int result; - struct string_list refs_to_delete = STRING_LIST_INIT_DUP; + struct tags_to_delete data = { + .refs = STRING_LIST_INIT_DUP, + .old_oids = OID_ARRAY_INIT, + }; struct string_list_item *item; - result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); + result = for_each_tag_name(argv, collect_tags, &data); if (refs_delete_refs(get_main_ref_store(the_repository), NULL, - &refs_to_delete, NULL, REF_NO_DEREF)) + &data.refs, &data.old_oids, NULL, REF_NO_DEREF)) result = 1; - for_each_string_list_item(item, &refs_to_delete) { + for_each_string_list_item(item, &data.refs) { const char *name = item->string; struct object_id *oid = item->util; if (!refs_ref_exists(get_main_ref_store(the_repository), name)) @@ -136,7 +146,8 @@ static int delete_tags(const char **argv) free(oid); } - string_list_clear(&refs_to_delete, 0); + string_list_clear(&data.refs, 0); + oid_array_clear(&data.old_oids); return result; } diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh index 4fe9d9b23..01b5ba8c4 100755 --- a/t/t1416-ref-transaction-hooks.sh +++ b/t/t1416-ref-transaction-hooks.sh @@ -14,6 +14,50 @@ test_expect_success setup ' POST_OID=$(git rev-parse POST) ' +test_expect_success 'hook gets old values for batched branch/tag deletion' ' + test_when_finished "rm -f actual" && + git branch to-delete PRE && + git tag delete-tag POST && + git pack-refs --all && + test_hook reference-transaction <<-\EOF && + if test "$1" = committed + then + # Ignore backend-internal zero-to-zero records. + while read -r old new ref + do + case "$old" in + *[!0]*) + echo "$old $new $ref" + ;; + esac + done >>actual + fi + EOF + cat >expect <<-EOF && + $PRE_OID $ZERO_OID refs/heads/to-delete + $POST_OID $ZERO_OID refs/tags/delete-tag + EOF + git branch -D to-delete && + git tag -d delete-tag && + test_cmp expect actual +' + +test_expect_success 'branch deletion rejects a concurrent update' ' + git branch delete-race PRE && + test_hook reference-transaction <<-\EOF && + marker=$(git rev-parse --git-path delete-race-once) + if test "$1" = preparing && test ! -e "$marker" + then + >"$marker" + git update-ref refs/heads/delete-race POST + fi + exit 0 + EOF + test_must_fail git branch -D delete-race 2>err && + test_grep "is at $POST_OID but expected $PRE_OID" err && + test_cmp_rev POST refs/heads/delete-race +' + test_expect_success 'hook allows updating ref if successful' ' git reset --hard PRE && test_hook reference-transaction <<-\EOF && -- 2.39.3 (Apple Git-146) ``` ## Maciej Ciemborowicz, 2026-09-22 22:31 Subject: [PATCH v4 3/3] fetch, remote: retain old OIDs when pruning refs Message-ID: <6f34853c79625794d2eb364d227660be57d1539b.1790113781.git.maciej.ciemborowicz@gmail.com> In-Reply-To: ``` get_stale_heads() records the current value of each stale local ref in its new_oid member. The pruning paths discard that value and request unconditional deletion, so reference-transaction hooks receive a null old OID. Pass the recorded values into the deletion transactions. If a ref changes after the stale scan, reject that deletion and preserve the new value. Non-atomic pruning uses refs_delete_refs(), whose partial-failure mode still deletes unaffected stale refs. An atomic fetch remains all-or-nothing. Continue reporting successful non-atomic deletions when another deletion is rejected, but do not report the rejected ref as deleted or use it when checking for newly dangling symrefs. Use the rejected-ref list returned by refs_delete_refs() so reporting reflects the transaction result without additional ref reads. Signed-off-by: Maciej Ciemborowicz --- builtin/fetch.c | 30 +++++--- builtin/remote.c | 44 +++++++++--- t/t1416-ref-transaction-hooks.sh | 116 +++++++++++++++++++++++++++++++ 3 files changed, 174 insertions(+), 16 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index b662216bf..95789edb8 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1471,22 +1471,29 @@ static int prune_refs(struct display_state *display_state, struct ref *ref, *stale_refs = get_stale_heads(rs, ref_map); struct strbuf err = STRBUF_INIT; struct string_list refnames = STRING_LIST_INIT_NODUP; - - for (ref = stale_refs; ref; ref = ref->next) - string_list_append(&refnames, ref->name); + struct string_list deleted_refs = STRING_LIST_INIT_NODUP; + struct string_list failed_refs = STRING_LIST_INIT_DUP; + struct oid_array old_oids = OID_ARRAY_INIT; if (!dry_run) { if (transaction) { for (ref = stale_refs; ref; ref = ref->next) { - result = ref_transaction_delete(transaction, ref->name, NULL, - NULL, 0, "fetch: prune", &err); + result = ref_transaction_delete(transaction, ref->name, + &ref->new_oid, NULL, 0, + "fetch: prune", &err); if (result) goto cleanup; } } else { + for (ref = stale_refs; ref; ref = ref->next) { + string_list_append(&refnames, ref->name); + oid_array_append(&old_oids, &ref->new_oid); + } result = refs_delete_refs(get_main_ref_store(the_repository), "fetch: prune", &refnames, - NULL, 0); + &old_oids, &failed_refs, 0); + if (result && !failed_refs.nr) + goto cleanup; } } @@ -1494,18 +1501,25 @@ static int prune_refs(struct display_state *display_state, int summary_width = transport_summary_width(stale_refs); for (ref = stale_refs; ref; ref = ref->next) { + if (string_list_has_string(&failed_refs, ref->name)) + continue; + display_ref_update(display_state, '-', _("[deleted]"), NULL, _("(none)"), ref->name, &ref->new_oid, &ref->old_oid, summary_width); + string_list_append(&deleted_refs, ref->name); } - string_list_sort(&refnames); + string_list_sort(&deleted_refs); refs_warn_dangling_symrefs(get_main_ref_store(the_repository), - stderr, " ", dry_run, &refnames); + stderr, " ", dry_run, &deleted_refs); } cleanup: string_list_clear(&refnames, 0); + string_list_clear(&deleted_refs, 0); + string_list_clear(&failed_refs, 0); + oid_array_clear(&old_oids); strbuf_release(&err); free_refs(stale_refs); return result; diff --git a/builtin/remote.c b/builtin/remote.c index 56b06845b..2d9ee6db1 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -17,6 +17,7 @@ #include "refs.h" #include "refspec.h" #include "odb.h" +#include "oid-array.h" #include "strvec.h" #include "commit-reach.h" #include "progress.h" @@ -380,6 +381,11 @@ struct ref_states { int queried; }; +struct stale_ref { + struct object_id oid; + char name[FLEX_ARRAY]; +}; + #define REF_STATES_INIT { \ .new_refs = STRING_LIST_INIT_DUP, \ .skipped = STRING_LIST_INIT_DUP, \ @@ -410,9 +416,13 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat } stale_refs = get_stale_heads(&states->remote->fetch, fetch_map); for (ref = stale_refs; ref; ref = ref->next) { + struct stale_ref *stale_ref; struct string_list_item *item = string_list_append(&states->stale, abbrev_branch(ref->name)); - item->util = xstrdup(ref->name); + + FLEX_ALLOC_STR(stale_ref, name, ref->name); + oidcpy(&stale_ref->oid, &ref->new_oid); + item->util = stale_ref; } free_refs(stale_refs); free_refs(fetch_map); @@ -1627,6 +1637,9 @@ static int prune_remote(const char *remote, int dry_run) int result = 0; struct ref_states states = REF_STATES_INIT; struct string_list refs_to_prune = STRING_LIST_INIT_NODUP; + struct string_list pruned_refs = STRING_LIST_INIT_NODUP; + struct string_list failed_refs = STRING_LIST_INIT_DUP; + struct oid_array old_oids = OID_ARRAY_INIT; struct string_list_item *item; get_remote_ref_states(remote, &states, GET_REF_STATES); @@ -1639,17 +1652,27 @@ static int prune_remote(const char *remote, int dry_run) printf_ln(_("Pruning %s"), remote); printf_ln(_("URL: %s"), states.remote->url.v[0]); - for_each_string_list_item(item, &states.stale) - string_list_append(&refs_to_prune, item->util); - string_list_sort(&refs_to_prune); + for_each_string_list_item(item, &states.stale) { + struct stale_ref *stale_ref = item->util; + + string_list_append(&refs_to_prune, stale_ref->name); + oid_array_append(&old_oids, &stale_ref->oid); + } - if (!dry_run) + if (!dry_run) { result |= refs_delete_refs(get_main_ref_store(the_repository), "remote: prune", &refs_to_prune, - NULL, 0); + &old_oids, &failed_refs, 0); + if (result && !failed_refs.nr) + goto cleanup; + } for_each_string_list_item(item, &states.stale) { - const char *refname = item->util; + struct stale_ref *stale_ref = item->util; + const char *refname = stale_ref->name; + + if (string_list_has_string(&failed_refs, refname)) + continue; if (dry_run) printf_ln(_(" * [would prune] %s"), @@ -1657,12 +1680,17 @@ static int prune_remote(const char *remote, int dry_run) else printf_ln(_(" * [pruned] %s"), abbrev_ref(refname, "refs/remotes/")); + string_list_append(&pruned_refs, refname); } refs_warn_dangling_symrefs(get_main_ref_store(the_repository), - stdout, " ", dry_run, &refs_to_prune); + stdout, " ", dry_run, &pruned_refs); +cleanup: string_list_clear(&refs_to_prune, 0); + string_list_clear(&pruned_refs, 0); + string_list_clear(&failed_refs, 0); + oid_array_clear(&old_oids); free_remote_ref_states(&states); return result; } diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh index 01b5ba8c4..e7c16cd87 100755 --- a/t/t1416-ref-transaction-hooks.sh +++ b/t/t1416-ref-transaction-hooks.sh @@ -58,6 +58,122 @@ test_expect_success 'branch deletion rejects a concurrent update' ' test_cmp_rev POST refs/heads/delete-race ' +test_expect_success 'hook gets old values when pruning remote refs' ' + test_when_finished "rm -rf empty.git prune" && + git init --bare empty.git && + git init prune && + ( + cd prune && + git remote add origin ../empty.git && + git commit --allow-empty -m one && + one=$(git rev-parse HEAD) && + git commit --allow-empty -m two && + two=$(git rev-parse HEAD) && + git update-ref refs/remotes/origin/remote-prune-z "$one" && + git update-ref refs/remotes/origin/remote-prune-a "$two" + ) && + test_hook -C prune reference-transaction <<-\EOF && + if test "$1" = committed + then + # Ignore backend-internal zero-to-zero records. + while read -r old new ref + do + case "$old" in + *[!0]*) + echo "$old $new $ref" + ;; + esac + done >>actual + fi + EOF + ( + cd prune && + one=$(git rev-parse HEAD^) && + two=$(git rev-parse HEAD) && + git remote prune origin && + git update-ref refs/remotes/origin/fetch-prune "$one" && + git fetch --prune origin && + git update-ref refs/remotes/origin/atomic-prune "$one" && + git fetch --atomic --prune origin && + cat >expect <<-EOF && + $two $ZERO_OID refs/remotes/origin/remote-prune-a + $one $ZERO_OID refs/remotes/origin/remote-prune-z + $one $ZERO_OID refs/remotes/origin/fetch-prune + $one $ZERO_OID refs/remotes/origin/atomic-prune + EOF + test_cmp expect actual + ) +' + +test_expect_success 'remote prune reports deletions around a concurrent update' ' + test_when_finished "rm -rf race-empty.git race-prune" && + git init --bare race-empty.git && + git init race-prune && + ( + cd race-prune && + git commit --allow-empty -m one && + one=$(git rev-parse HEAD) && + git commit --allow-empty -m two && + two=$(git rev-parse HEAD) && + git remote add origin ../race-empty.git && + git update-ref refs/remotes/origin/race "$one" && + git update-ref refs/remotes/origin/other "$one" + ) && + test_hook -C race-prune reference-transaction <<-\EOF && + marker=$(git rev-parse --git-path prune-race-once) + if test "$1" = preparing && test ! -e "$marker" + then + >"$marker" + git update-ref refs/remotes/origin/race HEAD + fi + exit 0 + EOF + ( + cd race-prune && + two=$(git rev-parse HEAD) && + test_must_fail git remote prune origin >out 2>err && + test_cmp_rev "$two" refs/remotes/origin/race && + test_must_fail git rev-parse --verify refs/remotes/origin/other && + test_grep "\[pruned\].*origin/other" out && + test_grep ! "\[pruned\].*origin/race" out && + test_grep "could not delete reference refs/remotes/origin/race" err + ) +' + +test_expect_success 'fetch prune reports deletions around a concurrent update' ' + test_when_finished "rm -rf fetch-empty.git fetch-prune" && + git init --bare fetch-empty.git && + git init fetch-prune && + ( + cd fetch-prune && + git commit --allow-empty -m one && + one=$(git rev-parse HEAD) && + git commit --allow-empty -m two && + git remote add origin ../fetch-empty.git && + git update-ref refs/remotes/origin/race "$one" && + git update-ref refs/remotes/origin/other "$one" + ) && + test_hook -C fetch-prune reference-transaction <<-\EOF && + marker=$(git rev-parse --git-path prune-race-once) + if test "$1" = preparing && test ! -e "$marker" + then + >"$marker" + git update-ref refs/remotes/origin/race HEAD + fi + exit 0 + EOF + ( + cd fetch-prune && + two=$(git rev-parse HEAD) && + test_must_fail git fetch --prune origin >out 2>err && + test_cmp_rev "$two" refs/remotes/origin/race && + test_must_fail git rev-parse --verify refs/remotes/origin/other && + test_grep "\[deleted\].*origin/other" err && + test_grep ! "\[deleted\].*origin/race" err && + test_grep "could not delete reference refs/remotes/origin/race" err + ) +' + test_expect_success 'hook allows updating ref if successful' ' git reset --hard PRE && test_hook reference-transaction <<-\EOF && -- 2.39.3 (Apple Git-146) ``` ## Junio C Hamano, 2026-09-22 23:31 Subject: Re: [PATCH v3 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: ``` Maciej Ciemborowicz writes: > On Tue, Sep 22, 2026 at 8:55 PM Junio C Hamano wrote: > >> I think there was a comment by another reviewer on the previous >> round around this area, which was never answered. In general, it is >> a polite thing to respond to review messages and see that your >> response is acknowledged before you send an updated patch. > > I'm very sorry, I didn't check my email before submitting the patch. > I'll take a look at this. By the way, I expected the review process to > be tough, but I'm starting to wonder if I'll ever get through it :). I do not think it is the review process, but the fact that the problem you chose to tackle is not trivial to solve cleanly to begin with, and you are doing very well. Often non-trivial topics take multiple iterations to get right. > You mentioned earlier that you could prepare a patch. Is that offer > still on the table? I do not recall that, or I do not know a patch to do what was being discussed back then, sorry. ``` ## Junio C Hamano, 2026-09-23 20:03 Subject: Re: [PATCH v4 3/3] fetch, remote: retain old OIDs when pruning refs Message-ID: In-Reply-To: <6f34853c79625794d2eb364d227660be57d1539b.1790113781.git.maciej.ciemborowicz@gmail.com> ``` Maciej Ciemborowicz writes: > diff --git a/builtin/fetch.c b/builtin/fetch.c > index b662216bf..95789edb8 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -1471,22 +1471,29 @@ static int prune_refs(struct display_state *display_state, > ... > } else { > + for (ref = stale_refs; ref; ref = ref->next) { > + string_list_append(&refnames, ref->name); > + oid_array_append(&old_oids, &ref->new_oid); > + } > result = refs_delete_refs(get_main_ref_store(the_repository), > "fetch: prune", &refnames, > - NULL, 0); > + &old_oids, &failed_refs, 0); Isn't adding a new parameter to refs_delete_refs() needed before this step? The corresponding changes to refs.[ch] was done in [1/3], and the fact that the callsite receives this update to add an extra parameter this late in the series means [1/3] and [2/3] does not even compile, right? Thanks. ``` ## Maciej Ciemborowicz, 2026-09-23 21:02 Subject: Re: [PATCH v4 3/3] fetch, remote: retain old OIDs when pruning refs Message-ID: <20260923210205.59543-1-maciej.ciemborowicz@gmail.com> In-Reply-To: ``` > Isn't adding a new parameter to refs_delete_refs() needed before > this step? The corresponding changes to refs.[ch] was done in > [1/3], and the fact that the callsite receives this update to add an > extra parameter this late in the series means [1/3] and [2/3] does > not even compile, right? You are right. I updated only some of the call sites in 1/3 and left the others for the patches that begin supplying the new arguments. As a result, the intermediate commits do not compile. I have corrected the local series by updating every call site for the new signature in 1/3, with NULL for optional data that is supplied only by the later patches. I also built the tree after each patch. The final tree remains identical to v4. Thanks for catching this. ``` ## Maciej Ciemborowicz, 2026-09-23 21:04 Subject: [PATCH v5 0/3] refs: report old OIDs for batched deletions Message-ID: In-Reply-To: ``` Thanks for catching the broken intermediate commits. This version updates every refs_delete_refs() call site in 1/3, using NULL for the optional data that is not supplied until the later patches. Each patch now builds on its predecessor. The final tree is identical to v4. This follows up on the reference-transaction bug report at [1]. The reference-transaction hook receives zero as both the old and new OID when branch, tag, fetch, and remote delete refs through refs_delete_refs(). Those callers already know the values that they selected for deletion. Teach refs_delete_refs() to accept a parallel array of expected old OIDs and pass them into the transaction. Besides making hook records useful, this restores conditional deletion for branch and tag and adds it to pruning without additional ref reads. Non-atomic batches preserve best-effort behavior, while atomic fetches remain all-or-nothing. Changes since v4: * Update all refs_delete_refs() call sites in 1/3 for the new signature. * Verify that 1/3, 1/3--2/3, and the complete series each build with DEVELOPER=1. The focused reference-transaction tests pass with SHA-1 and SHA-256 using both the files and reftable backends. The full test suite passed on the identical final tree in v4. [1] https://lore.kernel.org/git/CACQ=SRGTTdQ+dHXhN6F52dBv5KxZBRfk_Em2fvmEmGJDoB6oTg@mail.gmail.com/ Maciej Ciemborowicz (3): refs: allow callers to supply old OIDs for batch deletion branch, tag: retain old OIDs in batched deletions fetch, remote: retain old OIDs when pruning refs bisect.c | 2 +- builtin/branch.c | 7 +- builtin/fetch.c | 30 ++++-- builtin/remote.c | 47 +++++++-- builtin/tag.c | 28 ++++-- refs.c | 67 ++++++++++--- refs.h | 31 ++++-- t/helper/test-ref-store.c | 2 +- t/t1416-ref-transaction-hooks.sh | 160 +++++++++++++++++++++++++++++++ 9 files changed, 324 insertions(+), 50 deletions(-) Range-diff against v4: 1: f4a9d065c ! 1: 9b76cc2c4 refs: allow callers to supply old OIDs for batch deletion @@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int ki if (!(flags & DELETE_BRANCH_DRY_RUN) && - refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) + refs_delete_refs(get_main_ref_store(the_repository), NULL, -+ &refs_to_delete, NULL, REF_NO_DEREF)) ++ &refs_to_delete, NULL, NULL, REF_NO_DEREF)) ret = 1; for_each_string_list_item(item, &refs_to_delete) { @@ builtin/fetch.c: static int prune_refs(struct display_state *display_state, result = refs_delete_refs(get_main_ref_store(the_repository), "fetch: prune", &refnames, - 0); -+ NULL, 0); ++ NULL, NULL, 0); } } @@ builtin/remote.c: static int prune_remote(const char *remote, int dry_run) result |= refs_delete_refs(get_main_ref_store(the_repository), - "remote: prune", &refs_to_prune, 0); + "remote: prune", &refs_to_prune, -+ NULL, 0); ++ NULL, NULL, 0); for_each_string_list_item(item, &states.stale) { const char *refname = item->util; @@ builtin/tag.c: static int delete_tags(const char **argv) result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); - if (refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) + if (refs_delete_refs(get_main_ref_store(the_repository), NULL, -+ &refs_to_delete, NULL, REF_NO_DEREF)) ++ &refs_to_delete, NULL, NULL, REF_NO_DEREF)) result = 1; for_each_string_list_item(item, &refs_to_delete) { 2: 918c97d2b ! 2: 6a8401c44 branch, tag: retain old OIDs in batched deletions @@ builtin/branch.c: static int delete_branches(int argc, const char **argv, int ki if (!(flags & DELETE_BRANCH_DRY_RUN) && refs_delete_refs(get_main_ref_store(the_repository), NULL, -- &refs_to_delete, NULL, REF_NO_DEREF)) +- &refs_to_delete, NULL, NULL, REF_NO_DEREF)) + &refs_to_delete, &old_oids, NULL, REF_NO_DEREF)) ret = 1; @@ builtin/tag.c: static int for_each_tag_name(const char **argv, each_tag_name_fn - result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); + result = for_each_tag_name(argv, collect_tags, &data); if (refs_delete_refs(get_main_ref_store(the_repository), NULL, -- &refs_to_delete, NULL, REF_NO_DEREF)) +- &refs_to_delete, NULL, NULL, REF_NO_DEREF)) + &data.refs, &data.old_oids, NULL, REF_NO_DEREF)) result = 1; 3: 6f34853c7 ! 3: 541da44c3 fetch, remote: retain old OIDs when pruning refs @@ builtin/fetch.c: static int prune_refs(struct display_state *display_state, + } result = refs_delete_refs(get_main_ref_store(the_repository), "fetch: prune", &refnames, -- NULL, 0); +- NULL, NULL, 0); + &old_oids, &failed_refs, 0); + if (result && !failed_refs.nr) + goto cleanup; @@ builtin/remote.c: static int prune_remote(const char *remote, int dry_run) + if (!dry_run) { result |= refs_delete_refs(get_main_ref_store(the_repository), "remote: prune", &refs_to_prune, -- NULL, 0); +- NULL, NULL, 0); + &old_oids, &failed_refs, 0); + if (result && !failed_refs.nr) + goto cleanup; -- 2.39.3 (Apple Git-146) ``` ## Maciej Ciemborowicz, 2026-09-23 21:04 Subject: [PATCH v5 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: <9b76cc2c40a2b1fe727677a9400e3b26ec1ab437.1790196627.git.maciej.ciemborowicz@gmail.com> In-Reply-To: ``` refs_delete_refs() performs unconditional deletions, so callers cannot preserve old values that they have already resolved. Consequently, reference-transaction hooks see a null old OID. Let callers provide an optional array of expected old OIDs in parallel with the refname list. Delete the ref at position N only if it still points at the OID at position N. Treat a null OID as an unconditional deletion in ref_transaction_delete(), allowing callers to include broken refs whose old value cannot be resolved. refs_delete_refs() has always promised best-effort deletion. Always use REF_TRANSACTION_ALLOW_FAILURE and report rejected updates so one failure does not prevent independent refs in the batch from being deleted. Let callers request the exact set of failed refs when they need to report partial results. This also completes the conversion that was missed when batched transaction failure support was introduced. Signed-off-by: Maciej Ciemborowicz --- bisect.c | 2 +- builtin/branch.c | 3 +- builtin/fetch.c | 2 +- builtin/remote.c | 5 +-- builtin/tag.c | 3 +- refs.c | 67 +++++++++++++++++++++++++++++++-------- refs.h | 31 +++++++++++++----- t/helper/test-ref-store.c | 2 +- 8 files changed, 86 insertions(+), 29 deletions(-) diff --git a/bisect.c b/bisect.c index 9cbb3dc67..c8ab16d1e 100644 --- a/bisect.c +++ b/bisect.c @@ -1206,7 +1206,7 @@ int bisect_clean_state(void) string_list_append(&refs_for_removal, "BISECT_EXPECTED_REV"); result = refs_delete_refs(get_main_ref_store(the_repository), "bisect: remove", &refs_for_removal, - REF_NO_DEREF); + NULL, NULL, REF_NO_DEREF); string_list_clear(&refs_for_removal, 0); unlink_or_warn(git_path_bisect_ancestors_ok()); unlink_or_warn(git_path_bisect_log()); diff --git a/builtin/branch.c b/builtin/branch.c index a613148fc..baccefc77 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -351,7 +351,8 @@ static int delete_branches(int argc, const char **argv, int kinds, } if (!(flags & DELETE_BRANCH_DRY_RUN) && - refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) + refs_delete_refs(get_main_ref_store(the_repository), NULL, + &refs_to_delete, NULL, NULL, REF_NO_DEREF)) ret = 1; for_each_string_list_item(item, &refs_to_delete) { diff --git a/builtin/fetch.c b/builtin/fetch.c index 533fdfe7d..11caa6b4a 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1486,7 +1486,7 @@ static int prune_refs(struct display_state *display_state, } else { result = refs_delete_refs(get_main_ref_store(the_repository), "fetch: prune", &refnames, - 0); + NULL, NULL, 0); } } diff --git a/builtin/remote.c b/builtin/remote.c index de989ea3b..840c842e2 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1073,7 +1073,7 @@ static int rm(int argc, const char **argv, const char *prefix, if (!result) result = refs_delete_refs(get_main_ref_store(the_repository), "remote: remove", &branches, - REF_NO_DEREF); + NULL, NULL, REF_NO_DEREF); string_list_clear(&branches, 0); if (skipped.nr) { @@ -1645,7 +1645,8 @@ static int prune_remote(const char *remote, int dry_run) if (!dry_run) result |= refs_delete_refs(get_main_ref_store(the_repository), - "remote: prune", &refs_to_prune, 0); + "remote: prune", &refs_to_prune, + NULL, NULL, 0); for_each_string_list_item(item, &states.stale) { const char *refname = item->util; diff --git a/builtin/tag.c b/builtin/tag.c index 06c125b53..40157e834 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -122,7 +122,8 @@ static int delete_tags(const char **argv) struct string_list_item *item; result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); - if (refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) + if (refs_delete_refs(get_main_ref_store(the_repository), NULL, + &refs_to_delete, NULL, NULL, REF_NO_DEREF)) result = 1; for_each_string_list_item(item, &refs_to_delete) { diff --git a/refs.c b/refs.c index 92d5df5b7..13ee2d459 100644 --- a/refs.c +++ b/refs.c @@ -16,6 +16,7 @@ #include "refs/refs-internal.h" #include "hook.h" #include "object-name.h" +#include "oid-array.h" #include "odb.h" #include "object.h" #include "path.h" @@ -1523,7 +1524,7 @@ int ref_transaction_delete(struct ref_transaction *transaction, struct strbuf *err) { if (old_oid && is_null_oid(old_oid)) - BUG("delete called with old_oid set to zeros"); + old_oid = NULL; if (old_oid && old_target) BUG("delete called with both old_oid and old_target set"); if (old_target && !(flags & REF_NO_DEREF)) @@ -3069,39 +3070,73 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio } } +struct delete_refs_rejection_data { + int failures; + struct string_list *failed_refs; +}; + +static void delete_refs_rejection_handler(const char *refname, + const struct object_id *old_oid UNUSED, + const struct object_id *new_oid UNUSED, + const char *old_target UNUSED, + const char *new_target UNUSED, + enum ref_transaction_error err, + const char *details, + void *cb_data) +{ + struct delete_refs_rejection_data *data = cb_data; + + warning(_("could not delete reference %s: %s"), refname, + details ? details : ref_transaction_error_msg(err)); + data->failures++; + if (data->failed_refs) + string_list_insert(data->failed_refs, refname); +} + int refs_delete_refs(struct ref_store *refs, const char *logmsg, - struct string_list *refnames, unsigned int flags) + struct string_list *refnames, + const struct oid_array *old_oids, + struct string_list *failed_refs, + unsigned int flags) { + struct delete_refs_rejection_data rejection_data = { + .failed_refs = failed_refs, + }; struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; - struct string_list_item *item; - int ret = 0, failures = 0; + size_t i; + int ret = 0; char *msg; if (!refnames->nr) return 0; + if (old_oids && old_oids->nr != refnames->nr) + BUG("refname and old OID counts do not match"); + if (failed_refs && !failed_refs->strdup_strings) + BUG("failed ref list does not duplicate strings"); msg = normalize_reflog_message(logmsg); - /* - * Since we don't check the references' old_oids, the - * individual updates can't fail, so we can pack all of the - * updates into a single transaction. - */ - transaction = ref_store_transaction_begin(refs, 0, &err); + transaction = ref_store_transaction_begin(refs, + REF_TRANSACTION_ALLOW_FAILURE, &err); if (!transaction) { ret = error("%s", err.buf); goto out; } - for_each_string_list_item(item, refnames) { + for (i = 0; i < refnames->nr; i++) { + struct string_list_item *item = &refnames->items[i]; + const struct object_id *old_oid = old_oids ? &old_oids->oid[i] : NULL; + ret = ref_transaction_delete(transaction, item->string, - NULL, NULL, flags, msg, &err); + old_oid, NULL, flags, msg, &err); if (ret) { warning(_("could not delete reference %s: %s"), item->string, err.buf); strbuf_reset(&err); - failures = 1; + rejection_data.failures++; + if (failed_refs) + string_list_insert(failed_refs, item->string); } } @@ -3113,9 +3148,13 @@ int refs_delete_refs(struct ref_store *refs, const char *logmsg, else error(_("could not delete references: %s"), err.buf); } + if (!ret) + ref_transaction_for_each_rejected_update(transaction, + delete_refs_rejection_handler, + &rejection_data); out: - if (!ret && failures) + if (!ret && rejection_data.failures) ret = -1; ref_transaction_free(transaction); strbuf_release(&err); diff --git a/refs.h b/refs.h index 9979446d1..43f7a32f2 100644 --- a/refs.h +++ b/refs.h @@ -9,6 +9,7 @@ struct fsck_options; struct object_id; struct ref_store; +struct oid_array; struct strbuf; struct string_list; struct string_list_item; @@ -623,13 +624,26 @@ int refs_delete_ref(struct ref_store *refs, const char *msg, unsigned int flags); /* - * Delete the specified references. If there are any problems, emit - * errors but attempt to keep going (i.e., the deletes are not done in - * an all-or-nothing transaction). msg and flags are passed through to - * ref_transaction_delete(). + * Delete the specified references. If old_oids is non-NULL, it must contain + * an entry for each refname, in the same order. Each non-null OID is used to + * verify the current value of the corresponding reference before deleting + * it. A null OID requests an unconditional deletion, which allows callers to + * include broken refs whose old value cannot be resolved. + * + * If failed_refs is non-NULL, it must be initialized with + * STRING_LIST_INIT_DUP. The names of individual updates that cannot be queued + * or are rejected while processing the best-effort batch are inserted into + * it. A transaction-wide failure is returned without populating the list. + * + * If there are any problems, emit errors but attempt to keep going (i.e., + * the deletes are not done in an all-or-nothing transaction). msg and flags + * are passed through to ref_transaction_delete(). */ int refs_delete_refs(struct ref_store *refs, const char *msg, - struct string_list *refnames, unsigned int flags); + struct string_list *refnames, + const struct oid_array *old_oids, + struct string_list *failed_refs, + unsigned int flags); /** Delete a reflog */ int refs_delete_reflog(struct ref_store *refs, const char *refname); @@ -956,9 +970,10 @@ int ref_transaction_create(struct ref_transaction *transaction, struct strbuf *err); /* - * Add a reference deletion to transaction. If old_oid is non-NULL, - * then it holds the value that the reference should have had before - * the update (which must not be null_oid). + * Add a reference deletion to transaction. If old_oid is non-NULL and not + * null_oid, then it holds the value that the reference should have had before + * the update. Passing null_oid is equivalent to passing NULL and disables the + * old value check. * * See the above comment "Reference transaction updates" for more * information. diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c index db58f0058..29945f2b8 100644 --- a/t/helper/test-ref-store.c +++ b/t/helper/test-ref-store.c @@ -132,7 +132,7 @@ static int cmd_delete_refs(struct ref_store *refs, const char **argv) while (*argv) string_list_append(&refnames, *argv++); - result = refs_delete_refs(refs, msg, &refnames, flags); + result = refs_delete_refs(refs, msg, &refnames, NULL, NULL, flags); string_list_clear(&refnames, 0); return result; } -- 2.39.3 (Apple Git-146) ``` ## Maciej Ciemborowicz, 2026-09-23 21:04 Subject: [PATCH v5 2/3] branch, tag: retain old OIDs in batched deletions Message-ID: <6a8401c448f527fd80c162908a2736811a723096.1790196627.git.maciej.ciemborowicz@gmail.com> In-Reply-To: ``` Before 8198907795 (use delete_refs when deleting tags or branches, 2021-01-21), branch and tag deletion passed each resolved old OID to delete_ref(). This prevented the command from deleting a ref that another process had changed after it was inspected. The conversion to batched deletion dropped those old OIDs. Besides making the deletions unconditional, this causes reference-transaction hooks to report zero as both the old and new OID. Both commands still resolve the old OIDs before starting the deletion. Pass those values to refs_delete_refs(). This restores the old race protection and lets hooks receive useful old values without adding ref reads. If a ref changes concurrently, reject its deletion and preserve the new value. Signed-off-by: Maciej Ciemborowicz --- builtin/branch.c | 6 ++++- builtin/tag.c | 27 ++++++++++++++------ t/t1416-ref-transaction-hooks.sh | 44 ++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 9 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index baccefc77..4ce1407bc 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -16,6 +16,7 @@ #include "commit.h" #include "gettext.h" #include "object-name.h" +#include "oid-array.h" #include "remote.h" #include "parse-options.h" #include "branch.h" @@ -248,6 +249,7 @@ static int delete_branches(int argc, const char **argv, int kinds, struct strbuf bname = STRBUF_INIT; enum interpret_branch_kind allowed_interpret; struct string_list refs_to_delete = STRING_LIST_INIT_DUP; + struct oid_array old_oids = OID_ARRAY_INIT; struct string_list_item *item; int branch_name_pos; const char *fmt_remotes = "refs/remotes/%s"; @@ -342,6 +344,7 @@ static int delete_branches(int argc, const char **argv, int kinds, } item = string_list_append(&refs_to_delete, name); + oid_array_append(&old_oids, &oid); item->util = xstrdup((ref_flags & REF_ISBROKEN) ? "broken" : (ref_flags & REF_ISSYMREF) ? target : repo_find_unique_abbrev(the_repository, &oid, DEFAULT_ABBREV)); @@ -352,7 +355,7 @@ static int delete_branches(int argc, const char **argv, int kinds, if (!(flags & DELETE_BRANCH_DRY_RUN) && refs_delete_refs(get_main_ref_store(the_repository), NULL, - &refs_to_delete, NULL, NULL, REF_NO_DEREF)) + &refs_to_delete, &old_oids, NULL, REF_NO_DEREF)) ret = 1; for_each_string_list_item(item, &refs_to_delete) { @@ -377,6 +380,7 @@ static int delete_branches(int argc, const char **argv, int kinds, free(describe_ref); } string_list_clear(&refs_to_delete, 0); + oid_array_clear(&old_oids); free(name); strbuf_release(&bname); diff --git a/builtin/tag.c b/builtin/tag.c index 40157e834..07116664d 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -105,28 +105,38 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn, return had_error; } +struct tags_to_delete { + struct string_list refs; + struct oid_array old_oids; +}; + static int collect_tags(const char *name UNUSED, const char *ref, const struct object_id *oid, void *cb_data) { - struct string_list *ref_list = cb_data; + struct tags_to_delete *data = cb_data; + struct string_list_item *item; - string_list_append(ref_list, ref); - ref_list->items[ref_list->nr - 1].util = oiddup(oid); + item = string_list_append(&data->refs, ref); + item->util = oiddup(oid); + oid_array_append(&data->old_oids, oid); return 0; } static int delete_tags(const char **argv) { int result; - struct string_list refs_to_delete = STRING_LIST_INIT_DUP; + struct tags_to_delete data = { + .refs = STRING_LIST_INIT_DUP, + .old_oids = OID_ARRAY_INIT, + }; struct string_list_item *item; - result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); + result = for_each_tag_name(argv, collect_tags, &data); if (refs_delete_refs(get_main_ref_store(the_repository), NULL, - &refs_to_delete, NULL, NULL, REF_NO_DEREF)) + &data.refs, &data.old_oids, NULL, REF_NO_DEREF)) result = 1; - for_each_string_list_item(item, &refs_to_delete) { + for_each_string_list_item(item, &data.refs) { const char *name = item->string; struct object_id *oid = item->util; if (!refs_ref_exists(get_main_ref_store(the_repository), name)) @@ -136,7 +146,8 @@ static int delete_tags(const char **argv) free(oid); } - string_list_clear(&refs_to_delete, 0); + string_list_clear(&data.refs, 0); + oid_array_clear(&data.old_oids); return result; } diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh index 4fe9d9b23..01b5ba8c4 100755 --- a/t/t1416-ref-transaction-hooks.sh +++ b/t/t1416-ref-transaction-hooks.sh @@ -14,6 +14,50 @@ test_expect_success setup ' POST_OID=$(git rev-parse POST) ' +test_expect_success 'hook gets old values for batched branch/tag deletion' ' + test_when_finished "rm -f actual" && + git branch to-delete PRE && + git tag delete-tag POST && + git pack-refs --all && + test_hook reference-transaction <<-\EOF && + if test "$1" = committed + then + # Ignore backend-internal zero-to-zero records. + while read -r old new ref + do + case "$old" in + *[!0]*) + echo "$old $new $ref" + ;; + esac + done >>actual + fi + EOF + cat >expect <<-EOF && + $PRE_OID $ZERO_OID refs/heads/to-delete + $POST_OID $ZERO_OID refs/tags/delete-tag + EOF + git branch -D to-delete && + git tag -d delete-tag && + test_cmp expect actual +' + +test_expect_success 'branch deletion rejects a concurrent update' ' + git branch delete-race PRE && + test_hook reference-transaction <<-\EOF && + marker=$(git rev-parse --git-path delete-race-once) + if test "$1" = preparing && test ! -e "$marker" + then + >"$marker" + git update-ref refs/heads/delete-race POST + fi + exit 0 + EOF + test_must_fail git branch -D delete-race 2>err && + test_grep "is at $POST_OID but expected $PRE_OID" err && + test_cmp_rev POST refs/heads/delete-race +' + test_expect_success 'hook allows updating ref if successful' ' git reset --hard PRE && test_hook reference-transaction <<-\EOF && -- 2.39.3 (Apple Git-146) ``` ## Maciej Ciemborowicz, 2026-09-23 21:04 Subject: [PATCH v5 3/3] fetch, remote: retain old OIDs when pruning refs Message-ID: <541da44c371807e22a368cbc60b6fc26be7c64a3.1790196627.git.maciej.ciemborowicz@gmail.com> In-Reply-To: ``` get_stale_heads() records the current value of each stale local ref in its new_oid member. The pruning paths discard that value and request unconditional deletion, so reference-transaction hooks receive a null old OID. Pass the recorded values into the deletion transactions. If a ref changes after the stale scan, reject that deletion and preserve the new value. Non-atomic pruning uses refs_delete_refs(), whose partial-failure mode still deletes unaffected stale refs. An atomic fetch remains all-or-nothing. Continue reporting successful non-atomic deletions when another deletion is rejected, but do not report the rejected ref as deleted or use it when checking for newly dangling symrefs. Use the rejected-ref list returned by refs_delete_refs() so reporting reflects the transaction result without additional ref reads. Signed-off-by: Maciej Ciemborowicz --- builtin/fetch.c | 30 +++++--- builtin/remote.c | 44 +++++++++--- t/t1416-ref-transaction-hooks.sh | 116 +++++++++++++++++++++++++++++++ 3 files changed, 174 insertions(+), 16 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 11caa6b4a..95789edb8 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1471,22 +1471,29 @@ static int prune_refs(struct display_state *display_state, struct ref *ref, *stale_refs = get_stale_heads(rs, ref_map); struct strbuf err = STRBUF_INIT; struct string_list refnames = STRING_LIST_INIT_NODUP; - - for (ref = stale_refs; ref; ref = ref->next) - string_list_append(&refnames, ref->name); + struct string_list deleted_refs = STRING_LIST_INIT_NODUP; + struct string_list failed_refs = STRING_LIST_INIT_DUP; + struct oid_array old_oids = OID_ARRAY_INIT; if (!dry_run) { if (transaction) { for (ref = stale_refs; ref; ref = ref->next) { - result = ref_transaction_delete(transaction, ref->name, NULL, - NULL, 0, "fetch: prune", &err); + result = ref_transaction_delete(transaction, ref->name, + &ref->new_oid, NULL, 0, + "fetch: prune", &err); if (result) goto cleanup; } } else { + for (ref = stale_refs; ref; ref = ref->next) { + string_list_append(&refnames, ref->name); + oid_array_append(&old_oids, &ref->new_oid); + } result = refs_delete_refs(get_main_ref_store(the_repository), "fetch: prune", &refnames, - NULL, NULL, 0); + &old_oids, &failed_refs, 0); + if (result && !failed_refs.nr) + goto cleanup; } } @@ -1494,18 +1501,25 @@ static int prune_refs(struct display_state *display_state, int summary_width = transport_summary_width(stale_refs); for (ref = stale_refs; ref; ref = ref->next) { + if (string_list_has_string(&failed_refs, ref->name)) + continue; + display_ref_update(display_state, '-', _("[deleted]"), NULL, _("(none)"), ref->name, &ref->new_oid, &ref->old_oid, summary_width); + string_list_append(&deleted_refs, ref->name); } - string_list_sort(&refnames); + string_list_sort(&deleted_refs); refs_warn_dangling_symrefs(get_main_ref_store(the_repository), - stderr, " ", dry_run, &refnames); + stderr, " ", dry_run, &deleted_refs); } cleanup: string_list_clear(&refnames, 0); + string_list_clear(&deleted_refs, 0); + string_list_clear(&failed_refs, 0); + oid_array_clear(&old_oids); strbuf_release(&err); free_refs(stale_refs); return result; diff --git a/builtin/remote.c b/builtin/remote.c index 840c842e2..2d9ee6db1 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -17,6 +17,7 @@ #include "refs.h" #include "refspec.h" #include "odb.h" +#include "oid-array.h" #include "strvec.h" #include "commit-reach.h" #include "progress.h" @@ -380,6 +381,11 @@ struct ref_states { int queried; }; +struct stale_ref { + struct object_id oid; + char name[FLEX_ARRAY]; +}; + #define REF_STATES_INIT { \ .new_refs = STRING_LIST_INIT_DUP, \ .skipped = STRING_LIST_INIT_DUP, \ @@ -410,9 +416,13 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat } stale_refs = get_stale_heads(&states->remote->fetch, fetch_map); for (ref = stale_refs; ref; ref = ref->next) { + struct stale_ref *stale_ref; struct string_list_item *item = string_list_append(&states->stale, abbrev_branch(ref->name)); - item->util = xstrdup(ref->name); + + FLEX_ALLOC_STR(stale_ref, name, ref->name); + oidcpy(&stale_ref->oid, &ref->new_oid); + item->util = stale_ref; } free_refs(stale_refs); free_refs(fetch_map); @@ -1627,6 +1637,9 @@ static int prune_remote(const char *remote, int dry_run) int result = 0; struct ref_states states = REF_STATES_INIT; struct string_list refs_to_prune = STRING_LIST_INIT_NODUP; + struct string_list pruned_refs = STRING_LIST_INIT_NODUP; + struct string_list failed_refs = STRING_LIST_INIT_DUP; + struct oid_array old_oids = OID_ARRAY_INIT; struct string_list_item *item; get_remote_ref_states(remote, &states, GET_REF_STATES); @@ -1639,17 +1652,27 @@ static int prune_remote(const char *remote, int dry_run) printf_ln(_("Pruning %s"), remote); printf_ln(_("URL: %s"), states.remote->url.v[0]); - for_each_string_list_item(item, &states.stale) - string_list_append(&refs_to_prune, item->util); - string_list_sort(&refs_to_prune); + for_each_string_list_item(item, &states.stale) { + struct stale_ref *stale_ref = item->util; + + string_list_append(&refs_to_prune, stale_ref->name); + oid_array_append(&old_oids, &stale_ref->oid); + } - if (!dry_run) + if (!dry_run) { result |= refs_delete_refs(get_main_ref_store(the_repository), "remote: prune", &refs_to_prune, - NULL, NULL, 0); + &old_oids, &failed_refs, 0); + if (result && !failed_refs.nr) + goto cleanup; + } for_each_string_list_item(item, &states.stale) { - const char *refname = item->util; + struct stale_ref *stale_ref = item->util; + const char *refname = stale_ref->name; + + if (string_list_has_string(&failed_refs, refname)) + continue; if (dry_run) printf_ln(_(" * [would prune] %s"), @@ -1657,12 +1680,17 @@ static int prune_remote(const char *remote, int dry_run) else printf_ln(_(" * [pruned] %s"), abbrev_ref(refname, "refs/remotes/")); + string_list_append(&pruned_refs, refname); } refs_warn_dangling_symrefs(get_main_ref_store(the_repository), - stdout, " ", dry_run, &refs_to_prune); + stdout, " ", dry_run, &pruned_refs); +cleanup: string_list_clear(&refs_to_prune, 0); + string_list_clear(&pruned_refs, 0); + string_list_clear(&failed_refs, 0); + oid_array_clear(&old_oids); free_remote_ref_states(&states); return result; } diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh index 01b5ba8c4..e7c16cd87 100755 --- a/t/t1416-ref-transaction-hooks.sh +++ b/t/t1416-ref-transaction-hooks.sh @@ -58,6 +58,122 @@ test_expect_success 'branch deletion rejects a concurrent update' ' test_cmp_rev POST refs/heads/delete-race ' +test_expect_success 'hook gets old values when pruning remote refs' ' + test_when_finished "rm -rf empty.git prune" && + git init --bare empty.git && + git init prune && + ( + cd prune && + git remote add origin ../empty.git && + git commit --allow-empty -m one && + one=$(git rev-parse HEAD) && + git commit --allow-empty -m two && + two=$(git rev-parse HEAD) && + git update-ref refs/remotes/origin/remote-prune-z "$one" && + git update-ref refs/remotes/origin/remote-prune-a "$two" + ) && + test_hook -C prune reference-transaction <<-\EOF && + if test "$1" = committed + then + # Ignore backend-internal zero-to-zero records. + while read -r old new ref + do + case "$old" in + *[!0]*) + echo "$old $new $ref" + ;; + esac + done >>actual + fi + EOF + ( + cd prune && + one=$(git rev-parse HEAD^) && + two=$(git rev-parse HEAD) && + git remote prune origin && + git update-ref refs/remotes/origin/fetch-prune "$one" && + git fetch --prune origin && + git update-ref refs/remotes/origin/atomic-prune "$one" && + git fetch --atomic --prune origin && + cat >expect <<-EOF && + $two $ZERO_OID refs/remotes/origin/remote-prune-a + $one $ZERO_OID refs/remotes/origin/remote-prune-z + $one $ZERO_OID refs/remotes/origin/fetch-prune + $one $ZERO_OID refs/remotes/origin/atomic-prune + EOF + test_cmp expect actual + ) +' + +test_expect_success 'remote prune reports deletions around a concurrent update' ' + test_when_finished "rm -rf race-empty.git race-prune" && + git init --bare race-empty.git && + git init race-prune && + ( + cd race-prune && + git commit --allow-empty -m one && + one=$(git rev-parse HEAD) && + git commit --allow-empty -m two && + two=$(git rev-parse HEAD) && + git remote add origin ../race-empty.git && + git update-ref refs/remotes/origin/race "$one" && + git update-ref refs/remotes/origin/other "$one" + ) && + test_hook -C race-prune reference-transaction <<-\EOF && + marker=$(git rev-parse --git-path prune-race-once) + if test "$1" = preparing && test ! -e "$marker" + then + >"$marker" + git update-ref refs/remotes/origin/race HEAD + fi + exit 0 + EOF + ( + cd race-prune && + two=$(git rev-parse HEAD) && + test_must_fail git remote prune origin >out 2>err && + test_cmp_rev "$two" refs/remotes/origin/race && + test_must_fail git rev-parse --verify refs/remotes/origin/other && + test_grep "\[pruned\].*origin/other" out && + test_grep ! "\[pruned\].*origin/race" out && + test_grep "could not delete reference refs/remotes/origin/race" err + ) +' + +test_expect_success 'fetch prune reports deletions around a concurrent update' ' + test_when_finished "rm -rf fetch-empty.git fetch-prune" && + git init --bare fetch-empty.git && + git init fetch-prune && + ( + cd fetch-prune && + git commit --allow-empty -m one && + one=$(git rev-parse HEAD) && + git commit --allow-empty -m two && + git remote add origin ../fetch-empty.git && + git update-ref refs/remotes/origin/race "$one" && + git update-ref refs/remotes/origin/other "$one" + ) && + test_hook -C fetch-prune reference-transaction <<-\EOF && + marker=$(git rev-parse --git-path prune-race-once) + if test "$1" = preparing && test ! -e "$marker" + then + >"$marker" + git update-ref refs/remotes/origin/race HEAD + fi + exit 0 + EOF + ( + cd fetch-prune && + two=$(git rev-parse HEAD) && + test_must_fail git fetch --prune origin >out 2>err && + test_cmp_rev "$two" refs/remotes/origin/race && + test_must_fail git rev-parse --verify refs/remotes/origin/other && + test_grep "\[deleted\].*origin/other" err && + test_grep ! "\[deleted\].*origin/race" err && + test_grep "could not delete reference refs/remotes/origin/race" err + ) +' + test_expect_success 'hook allows updating ref if successful' ' git reset --hard PRE && test_hook reference-transaction <<-\EOF && -- 2.39.3 (Apple Git-146) ``` ## Junio C Hamano, 2026-09-23 21:55 Subject: Re: [PATCH v5 0/3] refs: report old OIDs for batched deletions Message-ID: In-Reply-To: ``` Maciej Ciemborowicz writes: > Changes since v4: > > * Update all refs_delete_refs() call sites in 1/3 for the new signature. > * Verify that 1/3, 1/3--2/3, and the complete series each build with > DEVELOPER=1. Thanks. In the past few weeks, I've been trying a new element in my workflow to try compiling each and every step of a new round of patches (I cannot afford cycles to run full test suite on them, which would slow me down too much), after getting scolded by a long-time contributor for queuing a topic whose end state built OK but intermediate states did not compile. This time three patches all built OK. ``` ## Karthik Nayak, 2026-09-24 10:04 Subject: Re: [PATCH v5 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: <9b76cc2c40a2b1fe727677a9400e3b26ec1ab437.1790196627.git.maciej.ciemborowicz@gmail.com> ``` Maciej Ciemborowicz writes: > refs_delete_refs() performs unconditional deletions, so callers cannot > preserve old values that they have already resolved. Consequently, > reference-transaction hooks see a null old OID. > > Let callers provide an optional array of expected old OIDs in parallel with > the refname list. Delete the ref at position N only if it still points at > the OID at position N. Treat a null OID as an unconditional deletion in > ref_transaction_delete(), allowing callers to include broken refs whose old > value cannot be resolved. Okay this makes sense. > refs_delete_refs() has always promised best-effort deletion. Always use > REF_TRANSACTION_ALLOW_FAILURE and report rejected updates so one failure > does not prevent independent refs in the batch from being deleted. Yup, this seems in line with what we discussed earlier. > Let > callers request the exact set of failed refs when they need to report > partial results So callers to `refs_delete_refs()` need to request the set of failed refs? Okay reading on. > This also completes the conversion that was missed when > batched transaction failure support was introduced. > Not sure what you're trying to say here. What conversion was missed and how is that fixed in this commit? [snip] > diff --git a/refs.c b/refs.c > index 92d5df5b7..13ee2d459 100644 > --- a/refs.c > +++ b/refs.c > @@ -16,6 +16,7 @@ > #include "refs/refs-internal.h" > #include "hook.h" > #include "object-name.h" > +#include "oid-array.h" > #include "odb.h" > #include "object.h" > #include "path.h" > @@ -1523,7 +1524,7 @@ int ref_transaction_delete(struct ref_transaction *transaction, > struct strbuf *err) > { > if (old_oid && is_null_oid(old_oid)) > - BUG("delete called with old_oid set to zeros"); > + old_oid = NULL; This change is totally different from the rest of the commit, I think it should be a precursor with adequate explanation regarding why this is done and why that's okay. I'm also still of the opinion that this shouldn't be done. A zeroed out null_oid is usually a user bug, where they haven't initialized a `struct object_id` correctly or ignored the return code while reading a ref. This BUG() captures that. We break safety without it. Another point is that `old_oid = NULL` is used to say, I don't care what the value of the ref is, delete it. Whereas `old_oid = null_oid` is more of, the ref shouldn't exist in the first place. Are we mixing up concerns here? > if (old_oid && old_target) > BUG("delete called with both old_oid and old_target set"); > if (old_target && !(flags & REF_NO_DEREF)) > @@ -3069,39 +3070,73 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio > } > } > > +struct delete_refs_rejection_data { > + int failures; > + struct string_list *failed_refs; I'm assuming failures is to count the number of refs which failed, wouldn't `failed_refs->nr` give us the same result? > +}; > + > +static void delete_refs_rejection_handler(const char *refname, > + const struct object_id *old_oid UNUSED, > + const struct object_id *new_oid UNUSED, > + const char *old_target UNUSED, > + const char *new_target UNUSED, > + enum ref_transaction_error err, > + const char *details, > + void *cb_data) > +{ > + struct delete_refs_rejection_data *data = cb_data; > + > + warning(_("could not delete reference %s: %s"), refname, > + details ? details : ref_transaction_error_msg(err)); > + data->failures++; > + if (data->failed_refs) > + string_list_insert(data->failed_refs, refname); > +} Oh so failed_refs is optional? > + > int refs_delete_refs(struct ref_store *refs, const char *logmsg, > - struct string_list *refnames, unsigned int flags) > + struct string_list *refnames, > + const struct oid_array *old_oids, > + struct string_list *failed_refs, > + unsigned int flags) > { > + struct delete_refs_rejection_data rejection_data = { > + .failed_refs = failed_refs, > + }; > struct ref_transaction *transaction; > struct strbuf err = STRBUF_INIT; > - struct string_list_item *item; > - int ret = 0, failures = 0; > + size_t i; > + int ret = 0; > char *msg; > > if (!refnames->nr) > return 0; > + if (old_oids && old_oids->nr != refnames->nr) > + BUG("refname and old OID counts do not match"); > + if (failed_refs && !failed_refs->strdup_strings) > + BUG("failed ref list does not duplicate strings"); > > msg = normalize_reflog_message(logmsg); > > - /* > - * Since we don't check the references' old_oids, the > - * individual updates can't fail, so we can pack all of the > - * updates into a single transaction. > - */ > - transaction = ref_store_transaction_begin(refs, 0, &err); > + transaction = ref_store_transaction_begin(refs, > + REF_TRANSACTION_ALLOW_FAILURE, &err); > if (!transaction) { > ret = error("%s", err.buf); > goto out; > } > > - for_each_string_list_item(item, refnames) { > + for (i = 0; i < refnames->nr; i++) { Nit: we could inline the `size_t` here. > + struct string_list_item *item = &refnames->items[i]; > + const struct object_id *old_oid = old_oids ? &old_oids->oid[i] : NULL; > + > ret = ref_transaction_delete(transaction, item->string, > - NULL, NULL, flags, msg, &err); > + old_oid, NULL, flags, msg, &err); > if (ret) { > warning(_("could not delete reference %s: %s"), > item->string, err.buf); > strbuf_reset(&err); > - failures = 1; > + rejection_data.failures++; > + if (failed_refs) > + string_list_insert(failed_refs, item->string); > } > } > > @@ -3113,9 +3148,13 @@ int refs_delete_refs(struct ref_store *refs, const char *logmsg, > else > error(_("could not delete references: %s"), err.buf); > } > + if (!ret) > + ref_transaction_for_each_rejected_update(transaction, > + delete_refs_rejection_handler, > + &rejection_data); > > out: > - if (!ret && failures) > + if (!ret && rejection_data.failures) > ret = -1; > ref_transaction_free(transaction); > strbuf_release(&err); > diff --git a/refs.h b/refs.h > index 9979446d1..43f7a32f2 100644 > --- a/refs.h > +++ b/refs.h > @@ -9,6 +9,7 @@ > struct fsck_options; > struct object_id; > struct ref_store; > +struct oid_array; > struct strbuf; > struct string_list; > struct string_list_item; > @@ -623,13 +624,26 @@ int refs_delete_ref(struct ref_store *refs, const char *msg, > unsigned int flags); > > /* > - * Delete the specified references. If there are any problems, emit > - * errors but attempt to keep going (i.e., the deletes are not done in > - * an all-or-nothing transaction). msg and flags are passed through to > - * ref_transaction_delete(). > + * Delete the specified references. If old_oids is non-NULL, it must contain > + * an entry for each refname, in the same order. Each non-null OID is used to > + * verify the current value of the corresponding reference before deleting > + * it. A null OID requests an unconditional deletion, which allows callers to > + * include broken refs whose old value cannot be resolved. > + * > + * If failed_refs is non-NULL, it must be initialized with > + * STRING_LIST_INIT_DUP. The names of individual updates that cannot be queued > + * or are rejected while processing the best-effort batch are inserted into > + * it. A transaction-wide failure is returned without populating the list. > + * > + * If there are any problems, emit errors but attempt to keep going (i.e., > + * the deletes are not done in an all-or-nothing transaction). msg and flags > + * are passed through to ref_transaction_delete(). > > */ > int refs_delete_refs(struct ref_store *refs, const char *msg, > - struct string_list *refnames, unsigned int flags); > + struct string_list *refnames, > + const struct oid_array *old_oids, > + struct string_list *failed_refs, > + unsigned int flags); > > /** Delete a reflog */ > int refs_delete_reflog(struct ref_store *refs, const char *refname); > @@ -956,9 +970,10 @@ int ref_transaction_create(struct ref_transaction *transaction, > struct strbuf *err); > > /* > - * Add a reference deletion to transaction. If old_oid is non-NULL, > - * then it holds the value that the reference should have had before > - * the update (which must not be null_oid). > + * Add a reference deletion to transaction. If old_oid is non-NULL and not > + * null_oid, then it holds the value that the reference should have had before > + * the update. Passing null_oid is equivalent to passing NULL and disables the > + * old value check. > * > * See the above comment "Reference transaction updates" for more > * information. > diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c > index db58f0058..29945f2b8 100644 > --- a/t/helper/test-ref-store.c > +++ b/t/helper/test-ref-store.c > @@ -132,7 +132,7 @@ static int cmd_delete_refs(struct ref_store *refs, const char **argv) > while (*argv) > string_list_append(&refnames, *argv++); > > - result = refs_delete_refs(refs, msg, &refnames, flags); > + result = refs_delete_refs(refs, msg, &refnames, NULL, NULL, flags); > string_list_clear(&refnames, 0); > return result; > } > -- > 2.39.3 (Apple Git-146) ``` ## Patrick Steinhardt, 2026-09-24 11:07 Subject: Re: [PATCH v5 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: <9b76cc2c40a2b1fe727677a9400e3b26ec1ab437.1790196627.git.maciej.ciemborowicz@gmail.com> ``` On Wed, Sep 23, 2026 at 11:04:40PM +0200, Maciej Ciemborowicz wrote: > refs_delete_refs() performs unconditional deletions, so callers cannot > preserve old values that they have already resolved. Consequently, > reference-transaction hooks see a null old OID. > > Let callers provide an optional array of expected old OIDs in parallel with > the refname list. Delete the ref at position N only if it still points at > the OID at position N. Treat a null OID as an unconditional deletion in > ref_transaction_delete(), allowing callers to include broken refs whose old > value cannot be resolved. > > refs_delete_refs() has always promised best-effort deletion. Always use > REF_TRANSACTION_ALLOW_FAILURE and report rejected updates so one failure > does not prevent independent refs in the batch from being deleted. Let > callers request the exact set of failed refs when they need to report > partial results. This also completes the conversion that was missed when > batched transaction failure support was introduced. Taking a step back though... the only reason that this function really exists is to provide a convenience wrapper that deletes references while we don't care for the old state. If we want to not do that anymore and instead want to expect a specific old OID, is this function still the right function to use? In other words, shouldn't the callers instead be updated to drive their own transaction if they want more complex behaviour? > diff --git a/refs.c b/refs.c > index 92d5df5b7..13ee2d459 100644 > --- a/refs.c > +++ b/refs.c > @@ -1523,7 +1524,7 @@ int ref_transaction_delete(struct ref_transaction *transaction, > struct strbuf *err) > { > if (old_oid && is_null_oid(old_oid)) > - BUG("delete called with old_oid set to zeros"); > + old_oid = NULL; > if (old_oid && old_target) > BUG("delete called with both old_oid and old_target set"); > if (old_target && !(flags & REF_NO_DEREF)) I'm not a huge fan of starting to treat a null OID as something other than "this branch should not exist". Everywhere else it still does, so mixing this feels fishy to me. Also, this change wouldn't have to exist if we instead started to drive a proper transaction. > @@ -3069,39 +3070,73 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio > } > } > > +struct delete_refs_rejection_data { > + int failures; > + struct string_list *failed_refs; > +}; > + > +static void delete_refs_rejection_handler(const char *refname, > + const struct object_id *old_oid UNUSED, > + const struct object_id *new_oid UNUSED, > + const char *old_target UNUSED, > + const char *new_target UNUSED, > + enum ref_transaction_error err, > + const char *details, > + void *cb_data) > +{ > + struct delete_refs_rejection_data *data = cb_data; > + > + warning(_("could not delete reference %s: %s"), refname, > + details ? details : ref_transaction_error_msg(err)); > + data->failures++; > + if (data->failed_refs) > + string_list_insert(data->failed_refs, refname); > +} > + > int refs_delete_refs(struct ref_store *refs, const char *logmsg, > - struct string_list *refnames, unsigned int flags) > + struct string_list *refnames, > + const struct oid_array *old_oids, > + struct string_list *failed_refs, > + unsigned int flags) And here we also have to yield failed refs now because we don't have a better mechanism. Same as before though, if we used a ref transaction we'd already have that mechanism. So overall I'm not quite on board with this change, as I think it's going down the wrong route. If you want more complex behaviour when deleting refs you should use a ref transaction, as it would already handle all of what you're trying to do here. Patrick ``` ## Patrick Steinhardt, 2026-09-24 11:08 Subject: Re: [PATCH v5 2/3] branch, tag: retain old OIDs in batched deletions Message-ID: In-Reply-To: <6a8401c448f527fd80c162908a2736811a723096.1790196627.git.maciej.ciemborowicz@gmail.com> ``` On Wed, Sep 23, 2026 at 11:04:41PM +0200, Maciej Ciemborowicz wrote: > Before 8198907795 (use delete_refs when deleting tags or branches, > 2021-01-21), branch and tag deletion passed each resolved old OID to > delete_ref(). This prevented the command from deleting a ref that another > process had changed after it was inspected. > > The conversion to batched deletion dropped those old OIDs. Besides making > the deletions unconditional, this causes reference-transaction hooks to > report zero as both the old and new OID. > > Both commands still resolve the old OIDs before starting the deletion. Pass > those values to refs_delete_refs(). This restores the old race protection > and lets hooks receive useful old values without adding ref reads. If a ref > changes concurrently, reject its deletion and preserve the new value. Hm. The motivation makes sense to me, but I have to wonder whether we're approaching it on the wrong level. With your proposed changes, we're now not force-deleting the refs anymore, which is a user-visible change in behaviour. What you're after though is to always have an old object ID available when the reference-transaction hook kicks in. But if that's the goal, shouldn't we consider whether we can instead resolve the old value during the transaction and queue that for the reftx hook, regardless of whether or not the user has asked for an old object ID? That would now cover _all_ users that modify refs without us having to update every single callsite. Sure, strictly speaking it's a backwards-incompatible change. But we've always considered the reftx hook to be exposing internals, so we aren't all that strict about retaining its behaviour and have allowed changes in behaviour in the past. So I wouldn't mind if we adapted the hook to always yield the old object ID. Patrick ``` ## Junio C Hamano, 2026-09-24 16:34 Subject: Re: [PATCH v5 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: ``` Karthik Nayak writes: > I'm also still of the opinion that this shouldn't be done. A zeroed out > null_oid is usually a user bug, where they haven't initialized a `struct > object_id` correctly or ignored the return code while reading a ref. ... unless they are using an element in an object_array and want to selectively have object names to some but not all of the elements in the array. In such a use case, a pointer to a null_oid is just as good a representation as a NULL pointer of "N/A" for a parameter to a function like this one that takes an optional object name. You could force each such callers to notice the entry they are about to call this function with has a null_oid and pass a NULL instead, but why force the caller to do so when the callee is capable of doing so centrally? ``` ## Junio C Hamano, 2026-09-24 16:45 Subject: Re: [PATCH v5 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: ``` Patrick Steinhardt writes: > On Wed, Sep 23, 2026 at 11:04:40PM +0200, Maciej Ciemborowicz wrote: >> refs_delete_refs() performs unconditional deletions, so callers cannot >> preserve old values that they have already resolved. Consequently, >> reference-transaction hooks see a null old OID. I wasn't paying attention when I gave my reviews, but the above puzzles me. "callers cannot preserve", meaning "after deletion the values cannot be read anymore"? Of course, but then callers can read them beforehand and use the stored value when calling hooks later. Patrick, do you understand these three lines above? I don't, and I am asking you because below what you say mostly seems to make sense. >> refs_delete_refs() has always promised best-effort deletion. Always use >> REF_TRANSACTION_ALLOW_FAILURE and report rejected updates so one failure >> does not prevent independent refs in the batch from being deleted. Let >> callers request the exact set of failed refs when they need to report >> partial results. This also completes the conversion that was missed when >> batched transaction failure support was introduced. > > Taking a step back though... the only reason that this function really > exists is to provide a convenience wrapper that deletes references while > we don't care for the old state. If we want to not do that anymore and > instead want to expect a specific old OID, is this function still the > right function to use? > > In other words, shouldn't the callers instead be updated to drive their > own transaction if they want more complex behaviour? That is a valid question to ask. I think the bulk deletion of refs is done via this function, so you certainly can update those callers of it that wants to protect references that are being updated from getting deleted with their own transaction and remember what refs are and are not removed, but I am not so convinced as you seem to be that adding an optional transaction support to the existing function they all call to do so, as the amount of the necessary call would be more or less the same. >> int refs_delete_refs(struct ref_store *refs, const char *logmsg, >> - struct string_list *refnames, unsigned int flags) >> + struct string_list *refnames, >> + const struct oid_array *old_oids, >> + struct string_list *failed_refs, >> + unsigned int flags) > > And here we also have to yield failed refs now because we don't have a > better mechanism. Same as before though, if we used a ref transaction > we'd already have that mechanism. > > So overall I'm not quite on board with this change, as I think it's going > down the wrong route. If you want more complex behaviour when deleting > refs you should use a ref transaction, as it would already handle all of > what you're trying to do here. I am neutral and would need to see what the code would look like to decide which one is more reasonable. Thanks. ``` ## Maciej Ciemborowicz, 2026-09-24 19:43 Subject: Re: [PATCH v5 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: ``` On Thu, Sep 24, 2026 at 12:05 PM Karthik Nayak wrote: > This also completes the conversion that was missed when > batched transaction failure support was introduced. Agreed, that sentence is unclear. I will try to remove the batch deletion API changes, along with this sentence and the caller-specific approach. > This change is totally different from the rest of the commit, I think it > should be a precursor with adequate explanation regarding why this is > done and why that's okay. > > I'm also still of the opinion that this shouldn't be done. I will try to leave ref_transaction_delete() and its null_oid BUG() unchanged. Instead, store the value observed for the hook in separate fields that do not set REF_HAVE_OLD and therefore do not constrain the transaction. > I'm assuming failures is to count the number of refs which failed, > wouldn't failed_refs->nr give us the same result? failed_refs is optional, which is why a separate counter is needed. If I move the solution into the common transaction layer, both the failed_refs parameter and the counter should no longer be needed. > Nit: we could inline the size_t here. I will fix this in the next version. Thanks for the review. - Maciej Ciemborowicz ``` ## Maciej Ciemborowicz, 2026-09-24 19:56 Subject: Re: [PATCH v5 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: ``` > Taking a step back though... the only reason that this function really > exists is to provide a convenience wrapper that deletes references while > we don't care for the old state. If we want to not do that anymore and > instead want to expect a specific old OID, is this function still the > right function to use? Agreed. Extending refs_delete_refs() seems to be the wrong layer for this. I will try to rework the patch so that refs_delete_refs(), ref_transaction_delete(), and callers remain unchanged. Instead, the common transaction hook layer could record a separate observed old value for updates whose callers did not supply one. That value would be used only as hook input and would not set REF_HAVE_OLD or otherwise constrain the update. This should also allow me to remove the failed_refs interface and the special handling of null_oid from the current version. Thanks, - Maciej Ciemborowicz On Thu, Sep 24, 2026 at 1:07 PM Patrick Steinhardt wrote: > > On Wed, Sep 23, 2026 at 11:04:40PM +0200, Maciej Ciemborowicz wrote: > > refs_delete_refs() performs unconditional deletions, so callers cannot > > preserve old values that they have already resolved. Consequently, > > reference-transaction hooks see a null old OID. > > > > Let callers provide an optional array of expected old OIDs in parallel with > > the refname list. Delete the ref at position N only if it still points at > > the OID at position N. Treat a null OID as an unconditional deletion in > > ref_transaction_delete(), allowing callers to include broken refs whose old > > value cannot be resolved. > > > > refs_delete_refs() has always promised best-effort deletion. Always use > > REF_TRANSACTION_ALLOW_FAILURE and report rejected updates so one failure > > does not prevent independent refs in the batch from being deleted. Let > > callers request the exact set of failed refs when they need to report > > partial results. This also completes the conversion that was missed when > > batched transaction failure support was introduced. > > Taking a step back though... the only reason that this function really > exists is to provide a convenience wrapper that deletes references while > we don't care for the old state. If we want to not do that anymore and > instead want to expect a specific old OID, is this function still the > right function to use? > > In other words, shouldn't the callers instead be updated to drive their > own transaction if they want more complex behaviour? > > > diff --git a/refs.c b/refs.c > > index 92d5df5b7..13ee2d459 100644 > > --- a/refs.c > > +++ b/refs.c > > @@ -1523,7 +1524,7 @@ int ref_transaction_delete(struct ref_transaction *transaction, > > struct strbuf *err) > > { > > if (old_oid && is_null_oid(old_oid)) > > - BUG("delete called with old_oid set to zeros"); > > + old_oid = NULL; > > if (old_oid && old_target) > > BUG("delete called with both old_oid and old_target set"); > > if (old_target && !(flags & REF_NO_DEREF)) > > I'm not a huge fan of starting to treat a null OID as something other > than "this branch should not exist". Everywhere else it still does, so > mixing this feels fishy to me. > > Also, this change wouldn't have to exist if we instead started to drive > a proper transaction. > > > @@ -3069,39 +3070,73 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio > > } > > } > > > > +struct delete_refs_rejection_data { > > + int failures; > > + struct string_list *failed_refs; > > +}; > > + > > +static void delete_refs_rejection_handler(const char *refname, > > + const struct object_id *old_oid UNUSED, > > + const struct object_id *new_oid UNUSED, > > + const char *old_target UNUSED, > > + const char *new_target UNUSED, > > + enum ref_transaction_error err, > > + const char *details, > > + void *cb_data) > > +{ > > + struct delete_refs_rejection_data *data = cb_data; > > + > > + warning(_("could not delete reference %s: %s"), refname, > > + details ? details : ref_transaction_error_msg(err)); > > + data->failures++; > > + if (data->failed_refs) > > + string_list_insert(data->failed_refs, refname); > > +} > > + > > int refs_delete_refs(struct ref_store *refs, const char *logmsg, > > - struct string_list *refnames, unsigned int flags) > > + struct string_list *refnames, > > + const struct oid_array *old_oids, > > + struct string_list *failed_refs, > > + unsigned int flags) > > And here we also have to yield failed refs now because we don't have a > better mechanism. Same as before though, if we used a ref transaction > we'd already have that mechanism. > > So overall I'm not quite on board with this change, as I think it's going > down the wrong route. If you want more complex behaviour when deleting > refs you should use a ref transaction, as it would already handle all of > what you're trying to do here. > > Patrick ``` ## Maciej Ciemborowicz, 2026-09-24 20:13 Subject: Re: [PATCH v5 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: ``` On Thu, Sep 24, 2026 at 6:45 PM Junio C Hamano wrote: > "callers cannot preserve", meaning "after deletion the values cannot > be read anymore"? Of course, but then callers can read them > beforehand and use the stored value when calling hooks later. I meant that refs_delete_refs() has no parameter for the values its callers have already resolved, so those values are not carried into the transaction and are therefore not available to the hook. Patrick's later suggestion to resolve missing old values in the common hook layer seems to avoid this API question altogether. Thanks, - Maciej Ciemborowicz ``` ## Maciej Ciemborowicz, 2026-09-24 22:33 Subject: [PATCH v6 0/1] refs: report old values to transaction hooks Message-ID: In-Reply-To: ``` This follows up on the reference-transaction bug report at [1]. The reference-transaction hook currently reports an all-zero old object ID when a caller queues an unconditional update. As a result, batched branch, tag, and remote-ref deletions report zero for both the old and new values. Earlier versions changed refs_delete_refs() and its callers to pass expected old OIDs. As Patrick pointed out, that makes previously unconditional deletions conditional and adds complexity at the wrong layer. Resolve the old value in the common transaction hook layer instead. Keep it separate from the caller-supplied old_oid and old_target so it cannot constrain the update. Resolve it before the "preparing" hook and refresh it after the backend locks the references, allowing later phases to report the value protected by those locks. Do this work only when a reference-transaction hook exists. Changes since v5: * Replace the three-patch caller-specific approach with one transaction layer change. * Leave refs_delete_refs(), ref_transaction_delete(), and all command call sites unchanged. * Preserve unconditional deletion semantics and test a concurrent update from the "preparing" hook. * Report observed old values for all unconditional transactions, including symbolic ref targets. * Document that the unlocked value reported in "preparing" may differ from later phases if the reference changes before it is locked. The full test suite passes (1060 files, 34660 tests). The focused reference-transaction tests also pass with SHA-1 and SHA-256 using both the files and reftable backends. [1] https://lore.kernel.org/git/CACQ=SRGTTdQ+dHXhN6F52dBv5KxZBRfk_Em2fvmEmGJDoB6oTg@mail.gmail.com/ Maciej Ciemborowicz (1): refs: report old values to transaction hooks 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(-) Range-diff against v5: 1: 9b76cc2c40 < -: ---------- refs: allow callers to supply old OIDs for batch deletion 2: 6a8401c448 < -: ---------- branch, tag: retain old OIDs in batched deletions 3: 541da44c37 < -: ---------- fetch, remote: retain old OIDs when pruning refs -: ---------- > 1: 2af3eeadd1 refs: report old values to transaction hooks -- 2.39.3 (Apple Git-146) ``` ## Maciej Ciemborowicz, 2026-09-24 22:33 Subject: [PATCH v6 1/1] refs: report old values to transaction hooks Message-ID: <2af3eeadd18806c5298d53072428656885cff89d.1790269745.git.maciej.ciemborowicz@gmail.com> In-Reply-To: ``` 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) ``` ## Patrick Steinhardt, 2026-09-28 06:43 Subject: Re: [PATCH v5 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: ``` On Thu, Sep 24, 2026 at 09:45:27AM -0700, Junio C Hamano wrote: > Patrick Steinhardt writes: > > > On Wed, Sep 23, 2026 at 11:04:40PM +0200, Maciej Ciemborowicz wrote: > >> refs_delete_refs() performs unconditional deletions, so callers cannot > >> preserve old values that they have already resolved. Consequently, > >> reference-transaction hooks see a null old OID. > > I wasn't paying attention when I gave my reviews, but the above > puzzles me. > > "callers cannot preserve", meaning "after deletion the values cannot > be read anymore"? Of course, but then callers can read them > beforehand and use the stored value when calling hooks later. > > Patrick, do you understand these three lines above? I don't, and I > am asking you because below what you say mostly seems to make sense. Yeah, I think it's less of a "cannot" but more of a "we do not". I mentioned this in a later patch, but I think the proper fix for what the author is after to have reference transactions always resolve the status quo and provide old object IDs regardless of whether the user provided one or not. If so we wouldn't have to change any of the interfaces at all, and we make sure that the reftx hook always gets invoked with proper old and new OIDs. Patrick ``` ## Patrick Steinhardt, 2026-09-28 06:44 Subject: Re: [PATCH v5 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: ``` On Thu, Sep 24, 2026 at 10:13:33PM +0200, Maciej Ciemborowicz wrote: > On Thu, Sep 24, 2026 at 6:45 PM Junio C Hamano wrote: > > > "callers cannot preserve", meaning "after deletion the values cannot > > be read anymore"? Of course, but then callers can read them > > beforehand and use the stored value when calling hooks later. > > I meant that refs_delete_refs() has no parameter for > the values its callers have already resolved, so those values are not > carried into the transaction and are therefore not available to the hook. > > Patrick's later suggestion to resolve missing old values in the common hook > layer seems to avoid this API question altogether. Yup, exactly. All users of reference transactions would always supply both old and new object ID to the reftx hook without changes to any of the callers. And I think that's a sensible change to make. Patrick ``` ## Maciej Ciemborowicz, 2026-09-30 03:11 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) > ``` ## Maciej Ciemborowicz, 2026-10-01 17:37 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) > > ```