From: Karthik Nayak Date: Mon, 21 Sep 2026 13:56:29 GMT 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)