From: Karthik Nayak Date: Mon, 21 Sep 2026 13:12:58 GMT Subject: Re: [PATCH v2 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: <5c96a5a1ebafd49a301c5c1dc77a2e19d6677ab0.1789901584.git.maciej.ciemborowicz@gmail.com> Maciej Ciemborowicz writes: > refs_delete_refs() currently performs unconditional deletions. Thus callers > cannot preserve old values that they have already resolved, and > reference-transaction hooks consequently see a null old OID. > > Add an optional oid_array whose entries correspond to the refnames. Pass each > non-null OID to ref_transaction_delete(). Supplying an OID makes the deletion > conditional: if the ref changed after the caller resolved it, the transaction > fails instead of deleting the new value. Existing callers that pass NULL > retain the unconditional behavior. > > Signed-off-by: Maciej Ciemborowicz > --- > bisect.c | 2 +- > builtin/branch.c | 3 ++- > builtin/fetch.c | 2 +- > builtin/remote.c | 5 +++-- > builtin/tag.c | 3 ++- > refs.c | 23 ++++++++++++++--------- > refs.h | 12 ++++++++++-- > t/helper/test-ref-store.c | 2 +- > 8 files changed, 34 insertions(+), 18 deletions(-) > > diff --git a/bisect.c b/bisect.c > index 94c7028d2..9aa3bace9 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -1203,7 +1203,7 @@ int bisect_clean_state(void) > string_list_append(&refs_for_removal, "BISECT_EXPECTED_REV"); > result = refs_delete_refs(get_main_ref_store(the_repository), > "bisect: remove", &refs_for_removal, > - REF_NO_DEREF); > + NULL, REF_NO_DEREF); > string_list_clear(&refs_for_removal, 0); > unlink_or_warn(git_path_bisect_ancestors_ok()); > unlink_or_warn(git_path_bisect_log()); > diff --git a/builtin/branch.c b/builtin/branch.c > index 1572a4f9e..f1abeb681 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -322,7 +322,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, > free(target); > } > > - if (refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) > + if (refs_delete_refs(get_main_ref_store(the_repository), NULL, > + &refs_to_delete, NULL, REF_NO_DEREF)) > ret = 1; > > for_each_string_list_item(item, &refs_to_delete) { > diff --git a/builtin/fetch.c b/builtin/fetch.c > index c1d7c672f..d202147b2 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -1467,7 +1467,7 @@ static int prune_refs(struct display_state *display_state, > } else { > result = refs_delete_refs(get_main_ref_store(the_repository), > "fetch: prune", &refnames, > - 0); > + NULL, 0); > } > } > > diff --git a/builtin/remote.c b/builtin/remote.c > index de989ea3b..13d3cc52d 100644 > --- a/builtin/remote.c > +++ b/builtin/remote.c > @@ -1073,7 +1073,7 @@ static int rm(int argc, const char **argv, const char *prefix, > if (!result) > result = refs_delete_refs(get_main_ref_store(the_repository), > "remote: remove", &branches, > - REF_NO_DEREF); > + NULL, REF_NO_DEREF); > string_list_clear(&branches, 0); > > if (skipped.nr) { > @@ -1645,7 +1645,8 @@ static int prune_remote(const char *remote, int dry_run) > > if (!dry_run) > result |= refs_delete_refs(get_main_ref_store(the_repository), > - "remote: prune", &refs_to_prune, 0); > + "remote: prune", &refs_to_prune, > + NULL, 0); > > for_each_string_list_item(item, &states.stale) { > const char *refname = item->util; > diff --git a/builtin/tag.c b/builtin/tag.c > index 06c125b53..40874a292 100644 > --- a/builtin/tag.c > +++ b/builtin/tag.c > @@ -122,7 +122,8 @@ static int delete_tags(const char **argv) > struct string_list_item *item; > > result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete); > - if (refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF)) > + if (refs_delete_refs(get_main_ref_store(the_repository), NULL, > + &refs_to_delete, NULL, REF_NO_DEREF)) > result = 1; > > for_each_string_list_item(item, &refs_to_delete) { > diff --git a/refs.c b/refs.c > index d3caa9a63..9c593baea 100644 > --- a/refs.c > +++ b/refs.c > @@ -18,6 +18,7 @@ > #include "refs/refs-internal.h" > #include "hook.h" > #include "object-name.h" > +#include "oid-array.h" > #include "odb.h" > #include "object.h" > #include "path.h" > @@ -3056,33 +3057,37 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio > } > > int refs_delete_refs(struct ref_store *refs, const char *logmsg, > - struct string_list *refnames, unsigned int flags) > + struct string_list *refnames, > + const struct oid_array *old_oids, > + unsigned int flags) > { > struct ref_transaction *transaction; > struct strbuf err = STRBUF_INIT; > - struct string_list_item *item; > + size_t i; > int ret = 0, failures = 0; > char *msg; > > + if (old_oids && old_oids->nr != refnames->nr) > + BUG("refname and old OID counts do not match"); > if (!refnames->nr) > return 0; > > msg = normalize_reflog_message(logmsg); > > - /* > - * Since we don't check the references' old_oids, the > - * individual updates can't fail, so we can pack all of the > - * updates into a single transaction. > - */ Okay so we already have the error buf sent to the refs subsystem and the appropriate error will now be displayed. > transaction = ref_store_transaction_begin(refs, 0, &err); > if (!transaction) { > ret = error("%s", err.buf); > goto out; > } > > - for_each_string_list_item(item, refnames) { > + for (i = 0; i < refnames->nr; i++) { > + struct string_list_item *item = &refnames->items[i]; > + const struct object_id *old_oid = old_oids ? &old_oids->oid[i] : NULL; > + > + if (old_oid && is_null_oid(old_oid)) > + old_oid = NULL; We need to do this since `ref_transaction_delete()` doesn't expect old_oids set to zeroes. But why would a callee do this? Shouldn't this also be a bug, if the callee doesn't care about the previous value shouldn't they simply set `old_oids->oid[i] = NULL`? > ret = ref_transaction_delete(transaction, item->string, > - NULL, NULL, flags, msg, &err); > + old_oid, NULL, flags, msg, &err); > if (ret) { > warning(_("could not delete reference %s: %s"), > item->string, err.buf); > diff --git a/refs.h b/refs.h > index 71d5c186d..b76b556cf 100644 > --- a/refs.h > +++ b/refs.h > @@ -9,6 +9,7 @@ > struct fsck_options; > struct object_id; > struct ref_store; > +struct oid_array; > struct strbuf; > struct string_list; > struct string_list_item; > @@ -613,13 +614,20 @@ int refs_delete_ref(struct ref_store *refs, const char *msg, > unsigned int flags); > > /* > - * Delete the specified references. If there are any problems, emit > + * Delete the specified references. If old_oids is non-NULL, it must contain > + * an entry for each refname, in the same order. Each non-null entry is used > + * to verify the current value of the corresponding reference before deleting > + * it. A null entry disables verification for that reference. > + * Here too, we don't talk about zero-oid's. So I think we should skip the implicit conversion. > + * If there are any problems, emit > * errors but attempt to keep going (i.e., the deletes are not done in > * an all-or-nothing transaction). msg and flags are passed through to > * ref_transaction_delete(). > */ > int refs_delete_refs(struct ref_store *refs, const char *msg, > - struct string_list *refnames, unsigned int flags); > + struct string_list *refnames, > + const struct oid_array *old_oids, > + unsigned int flags); > > /** Delete a reflog */ > int refs_delete_reflog(struct ref_store *refs, const char *refname); > diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c > index 3866d0aca..c2c7dfb06 100644 > --- a/t/helper/test-ref-store.c > +++ b/t/helper/test-ref-store.c > @@ -140,7 +140,7 @@ static int cmd_delete_refs(struct ref_store *refs, const char **argv) > while (*argv) > string_list_append(&refnames, *argv++); > > - result = refs_delete_refs(refs, msg, &refnames, flags); > + result = refs_delete_refs(refs, msg, &refnames, NULL, flags); > string_list_clear(&refnames, 0); > return result; > } > -- > 2.39.3 (Apple Git-146)