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

[Maciej Ciemborowicz <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. 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 <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);
+	}
 
 	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)