From: Karthik Nayak Date: Mon, 21 Sep 2026 13:19:46 GMT Subject: Re: [PATCH v2 2/3] branch, tag: retain old OIDs in batched deletions Message-ID: In-Reply-To: Maciej Ciemborowicz writes: > 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 any ref reads. If a ref > changes concurrently, the transaction fails and preserves the new value. > > Signed-off-by: Maciej Ciemborowicz > --- > builtin/branch.c | 6 ++++- > builtin/tag.c | 6 ++++- > t/t1416-ref-transaction-hooks.sh | 44 ++++++++++++++++++++++++++++++++ > 3 files changed, 54 insertions(+), 2 deletions(-) > > diff --git a/builtin/branch.c b/builtin/branch.c > index f1abeb681..9f03ebc09 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" > @@ -230,6 +231,7 @@ static int delete_branches(int argc, const char **argv, int force, 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"; > @@ -314,6 +316,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, > } > > item = string_list_append(&refs_to_delete, name); > + oid_array_append(&old_oids, &oid); > item->util = xstrdup((flags & REF_ISBROKEN) ? "broken" > : (flags & REF_ISSYMREF) ? target > : repo_find_unique_abbrev(the_repository, &oid, DEFAULT_ABBREV)); > @@ -323,7 +326,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, > } > > if (refs_delete_refs(get_main_ref_store(the_repository), NULL, > - &refs_to_delete, NULL, REF_NO_DEREF)) > + &refs_to_delete, &old_oids, REF_NO_DEREF)) > ret = 1; > > for_each_string_list_item(item, &refs_to_delete) { > @@ -342,6 +345,7 @@ static int delete_branches(int argc, const char **argv, int force, 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 40874a292..0a3eb70fa 100644 > --- a/builtin/tag.c > +++ b/builtin/tag.c > @@ -119,11 +119,14 @@ static int delete_tags(const char **argv) > { > int result; > struct string_list refs_to_delete = STRING_LIST_INIT_DUP; > + struct oid_array old_oids = OID_ARRAY_INIT; > struct string_list_item *item; > > result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); > + for_each_string_list_item(item, &refs_to_delete) > + oid_array_append(&old_oids, item->util); Nit: wouldn't it make sense to add the oid to `old_oids` within `collect_tags()` instead of iterating over all tags again? You would have to change the callback data sent. If not, we should call this out in the commit message at the least. > if (refs_delete_refs(get_main_ref_store(the_repository), NULL, > - &refs_to_delete, NULL, REF_NO_DEREF)) > + &refs_to_delete, &old_oids, REF_NO_DEREF)) > result = 1; > > for_each_string_list_item(item, &refs_to_delete) { > @@ -137,6 +140,7 @@ static int delete_tags(const char **argv) > free(oid); > } > string_list_clear(&refs_to_delete, 0); > + oid_array_clear(&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) The rest of the patch looks good! :)