Thread (52 messages) 52 messages, 5 authors, 6d ago

[PATCH v5 2/3] branch, tag: retain old OIDs in batched deletions

flat view
COOLING6d

From: Maciej Ciemborowicz <hidden>
Date: 2026-09-23 21:04:48
Subsystem: the rest · Maintainer: Linus Torvalds

Revision v5 of 5 in this series.

Revisions (5)
  1. v1 [diff vs current]
  2. v2 [diff vs current]
  3. v3 [diff vs current]
  4. v4 [diff vs current]
  5. v5 current
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 <redacted>
---
 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)
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help