From: Karthik Nayak Date: Sat, 19 Sep 2026 20:41:43 GMT Subject: Re: [PATCH 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: <20260919201158.43415-2-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(). Existing callers retain the > unconditional behavior for now. > > 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 | 26 +++++++++++++++----------- > refs.h | 12 ++++++++++-- > t/helper/test-ref-store.c | 2 +- > 8 files changed, 35 insertions(+), 20 deletions(-) > [snip] > diff --git a/refs.c b/refs.c > index d3caa9a633..a9c5397fd7 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,36 +3057,39 @@ 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. > - */ I understand that this is intended to fix a bug. With this change, `refs_delete_refs()`'s behavior has changed from unconditionally deleting all refs to now only deleting the refs if all old OIDs match. Doesn't this introduce possibility of a race since callees of the function who checked the ref's OID before can now expect a failure when the transaction re-checks the old_oid? > transaction = ref_store_transaction_begin(refs, 0, &err); > if (!transaction) { > ret = error("%s", err.buf); > goto out; > } > > - for_each_string_list_item(item, refnames) { > - ret = ref_transaction_delete(transaction, item->string, > - NULL, NULL, flags, msg, &err); > + for (i = 0; i < refnames->nr; i++) { > + const struct object_id *old_oid = old_oids ? &old_oids->oid[i] : NULL; > + Nit: we could add a `struct string_list_item *item = refnames->items[i];` for a nicer diff. > + if (old_oid && is_null_oid(old_oid)) > + old_oid = NULL; > + ret = ref_transaction_delete(transaction, refnames->items[i].string, > + old_oid, NULL, flags, msg, &err); > if (ret) { > warning(_("could not delete reference %s: %s"), > - item->string, err.buf); > + refnames->items[i].string, err.buf); > strbuf_reset(&err); > failures = 1; > } > diff --git a/refs.h b/refs.h > index 71d5c186d0..b76b556cf7 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. > + * > + * 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 3866d0aca4..c2c7dfb065 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)