git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 3/3] fetch, remote: retain old OIDs when pruning refs

From
Karthik Nayak <karthik.188@gmail.com>
Date
Sep 21, 2026, 13:56 UTC
Message-ID
<CAOLa=ZRYTevU5SpkGBQu198Rbaemms4s03pFaZ4DOKCGMOV_vQ@mail.gmail.com>
In-Reply-To
<461c36ccdae09fb827a3c0efc7eed5aef072e09b.1789901584.git.maciej.ciemborowicz@gmail.com>
Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com> writes:
Show 13 quoted lines
> 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`?

Show 23 quoted lines
> Signed-off-by: Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com>
> ---
>  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?

Show 7 quoted lines
>  	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.

Show 11 quoted lines
>  				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?

Show 75 quoted lines
>  	}
>
>  	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.
Show 38 quoted lines
> -	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.

Show 8 quoted lines
> +	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?
Show 16 quoted lines
> +	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?

Show 19 quoted lines
> +		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.
Show 25 quoted lines
> +	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)
Previous: Maciej CiemborowiczNext: Karthik Nayak
Message 15 of 52 in “[BUG] reference-transaction reports zero OIDs for branch and tag deletion”
  1. Maciej CiemborowiczSep 19, 2026
  2. D. Ben KnobleSep 19, 2026
  3. Maciej CiemborowiczSep 19, 2026
  4. 0/3 refs: report old OIDs for batched deletionsMaciej Ciemborowicz, Sep 19, 2026
  5. 1/3 refs: allow callers to supply old OIDs for batch deletionMaciej Ciemborowicz, Sep 19, 2026
  6. Karthik NayakSep 19, 2026
  7. Maciej CiemborowiczSep 20, 2026
  8. 0/3 refs: report old OIDs for batched deletionsMaciej Ciemborowicz, Sep 20, 2026
  9. 1/3 refs: allow callers to supply old OIDs for batch deletionMaciej Ciemborowicz, Sep 20, 2026
  10. Karthik NayakSep 21, 2026
  11. Junio C HamanoSep 21, 2026
  12. 2/3 branch, tag: retain old OIDs in batched deletionsMaciej Ciemborowicz, Sep 20, 2026
  13. Karthik NayakSep 21, 2026
  14. 3/3 fetch, remote: retain old OIDs when pruning refsMaciej Ciemborowicz, Sep 20, 2026
  15. Karthik NayakSep 21, 2026
  16. Karthik NayakSep 21, 2026
  17. Maciej CiemborowiczSep 21, 2026
  18. 0/3 refs: report old OIDs for batched deletionsMaciej Ciemborowicz, Sep 22, 2026
  19. 1/3 refs: allow callers to supply old OIDs for batch deletionMaciej Ciemborowicz, Sep 22, 2026
  20. Junio C HamanoSep 22, 2026
  21. Maciej CiemborowiczSep 22, 2026
  22. Junio C HamanoSep 22, 2026
  23. 2/3 branch, tag: retain old OIDs in batched deletionsMaciej Ciemborowicz, Sep 22, 2026
  24. 3/3 fetch, remote: retain old OIDs when pruning refsMaciej Ciemborowicz, Sep 22, 2026
  25. Junio C HamanoSep 22, 2026
  26. 0/3 refs: report old OIDs for batched deletionsMaciej Ciemborowicz, Sep 22, 2026
  27. 1/3 refs: allow callers to supply old OIDs for batch deletionMaciej Ciemborowicz, Sep 22, 2026
  28. 2/3 branch, tag: retain old OIDs in batched deletionsMaciej Ciemborowicz, Sep 22, 2026
  29. 3/3 fetch, remote: retain old OIDs when pruning refsMaciej Ciemborowicz, Sep 22, 2026
  30. Junio C HamanoSep 23, 2026
  31. Maciej CiemborowiczSep 23, 2026
  32. 0/3 refs: report old OIDs for batched deletionsMaciej Ciemborowicz, Sep 23, 2026
  33. 1/3 refs: allow callers to supply old OIDs for batch deletionMaciej Ciemborowicz, Sep 23, 2026
  34. Karthik NayakSep 24, 2026
  35. Junio C HamanoSep 24, 2026
  36. Maciej CiemborowiczSep 24, 2026
  37. Patrick SteinhardtSep 24, 2026
  38. Junio C HamanoSep 24, 2026
  39. Maciej CiemborowiczSep 24, 2026
  40. Patrick SteinhardtSep 28, 2026
  41. Patrick SteinhardtSep 28, 2026
  42. Maciej CiemborowiczSep 24, 2026
  43. 2/3 branch, tag: retain old OIDs in batched deletionsMaciej Ciemborowicz, Sep 23, 2026
  44. Patrick SteinhardtSep 24, 2026
  45. 3/3 fetch, remote: retain old OIDs when pruning refsMaciej Ciemborowicz, Sep 23, 2026
  46. Junio C HamanoSep 23, 2026
  47. 0/1 refs: report old values to transaction hooksMaciej Ciemborowicz, Sep 24, 2026
  48. 1/1 refs: report old values to transaction hooksMaciej Ciemborowicz, Sep 24, 2026
  49. Maciej CiemborowiczSep 30, 2026
  50. Maciej CiemborowiczOct 1, 2026
  51. 2/3 branch, tag: retain old OIDs in batched deletionsMaciej Ciemborowicz, Sep 19, 2026
  52. 3/3 fetch, remote: retain old OIDs when pruning refsMaciej Ciemborowicz, Sep 19, 2026

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.