Re: [PATCH v2 3/3] fetch, remote: retain old OIDs when pruning refs
From: Karthik Nayak <hidden>
Date: 2026-09-21 13:56:32
Maciej Ciemborowicz [off-list ref] 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`?
quoted hunk ↗ jump to hunk
Signed-off-by: Maciej Ciemborowicz <redacted> --- 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.
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
} 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.
quoted hunk ↗ jump to hunk
- 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)
Attachments
- signature.asc [application/pgp-signature] 690 bytes