From: Karthik Nayak Date: Thu, 24 Sep 2026 10:04:59 GMT Subject: Re: [PATCH v5 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: <9b76cc2c40a2b1fe727677a9400e3b26ec1ab437.1790196627.git.maciej.ciemborowicz@gmail.com> Maciej Ciemborowicz writes: > refs_delete_refs() performs unconditional deletions, so callers cannot > preserve old values that they have already resolved. Consequently, > reference-transaction hooks see a null old OID. > > Let callers provide an optional array of expected old OIDs in parallel with > the refname list. Delete the ref at position N only if it still points at > the OID at position N. Treat a null OID as an unconditional deletion in > ref_transaction_delete(), allowing callers to include broken refs whose old > value cannot be resolved. Okay this makes sense. > refs_delete_refs() has always promised best-effort deletion. Always use > REF_TRANSACTION_ALLOW_FAILURE and report rejected updates so one failure > does not prevent independent refs in the batch from being deleted. Yup, this seems in line with what we discussed earlier. > Let > callers request the exact set of failed refs when they need to report > partial results So callers to `refs_delete_refs()` need to request the set of failed refs? Okay reading on. > This also completes the conversion that was missed when > batched transaction failure support was introduced. > Not sure what you're trying to say here. What conversion was missed and how is that fixed in this commit? [snip] > diff --git a/refs.c b/refs.c > index 92d5df5b7..13ee2d459 100644 > --- a/refs.c > +++ b/refs.c > @@ -16,6 +16,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" > @@ -1523,7 +1524,7 @@ int ref_transaction_delete(struct ref_transaction *transaction, > struct strbuf *err) > { > if (old_oid && is_null_oid(old_oid)) > - BUG("delete called with old_oid set to zeros"); > + old_oid = NULL; This change is totally different from the rest of the commit, I think it should be a precursor with adequate explanation regarding why this is done and why that's okay. I'm also still of the opinion that this shouldn't be done. A zeroed out null_oid is usually a user bug, where they haven't initialized a `struct object_id` correctly or ignored the return code while reading a ref. This BUG() captures that. We break safety without it. Another point is that `old_oid = NULL` is used to say, I don't care what the value of the ref is, delete it. Whereas `old_oid = null_oid` is more of, the ref shouldn't exist in the first place. Are we mixing up concerns here? > if (old_oid && old_target) > BUG("delete called with both old_oid and old_target set"); > if (old_target && !(flags & REF_NO_DEREF)) > @@ -3069,39 +3070,73 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio > } > } > > +struct delete_refs_rejection_data { > + int failures; > + struct string_list *failed_refs; I'm assuming failures is to count the number of refs which failed, wouldn't `failed_refs->nr` give us the same result? > +}; > + > +static void delete_refs_rejection_handler(const char *refname, > + const struct object_id *old_oid UNUSED, > + const struct object_id *new_oid UNUSED, > + const char *old_target UNUSED, > + const char *new_target UNUSED, > + enum ref_transaction_error err, > + const char *details, > + void *cb_data) > +{ > + struct delete_refs_rejection_data *data = cb_data; > + > + warning(_("could not delete reference %s: %s"), refname, > + details ? details : ref_transaction_error_msg(err)); > + data->failures++; > + if (data->failed_refs) > + string_list_insert(data->failed_refs, refname); > +} Oh so failed_refs is optional? > + > 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, > + struct string_list *failed_refs, > + unsigned int flags) > { > + struct delete_refs_rejection_data rejection_data = { > + .failed_refs = failed_refs, > + }; > struct ref_transaction *transaction; > struct strbuf err = STRBUF_INIT; > - struct string_list_item *item; > - int ret = 0, failures = 0; > + size_t i; > + int ret = 0; > char *msg; > > if (!refnames->nr) > return 0; > + if (old_oids && old_oids->nr != refnames->nr) > + BUG("refname and old OID counts do not match"); > + if (failed_refs && !failed_refs->strdup_strings) > + BUG("failed ref list does not duplicate strings"); > > 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. > - */ > - transaction = ref_store_transaction_begin(refs, 0, &err); > + transaction = ref_store_transaction_begin(refs, > + REF_TRANSACTION_ALLOW_FAILURE, &err); > if (!transaction) { > ret = error("%s", err.buf); > goto out; > } > > - for_each_string_list_item(item, refnames) { > + for (i = 0; i < refnames->nr; i++) { Nit: we could inline the `size_t` here. > + struct string_list_item *item = &refnames->items[i]; > + const struct object_id *old_oid = old_oids ? &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); > strbuf_reset(&err); > - failures = 1; > + rejection_data.failures++; > + if (failed_refs) > + string_list_insert(failed_refs, item->string); > } > } > > @@ -3113,9 +3148,13 @@ int refs_delete_refs(struct ref_store *refs, const char *logmsg, > else > error(_("could not delete references: %s"), err.buf); > } > + if (!ret) > + ref_transaction_for_each_rejected_update(transaction, > + delete_refs_rejection_handler, > + &rejection_data); > > out: > - if (!ret && failures) > + if (!ret && rejection_data.failures) > ret = -1; > ref_transaction_free(transaction); > strbuf_release(&err); > diff --git a/refs.h b/refs.h > index 9979446d1..43f7a32f2 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; > @@ -623,13 +624,26 @@ int refs_delete_ref(struct ref_store *refs, const char *msg, > unsigned int flags); > > /* > - * Delete the specified references. 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(). > + * 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 OID is used to > + * verify the current value of the corresponding reference before deleting > + * it. A null OID requests an unconditional deletion, which allows callers to > + * include broken refs whose old value cannot be resolved. > + * > + * If failed_refs is non-NULL, it must be initialized with > + * STRING_LIST_INIT_DUP. The names of individual updates that cannot be queued > + * or are rejected while processing the best-effort batch are inserted into > + * it. A transaction-wide failure is returned without populating the list. > + * > + * 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, > + struct string_list *failed_refs, > + unsigned int flags); > > /** Delete a reflog */ > int refs_delete_reflog(struct ref_store *refs, const char *refname); > @@ -956,9 +970,10 @@ int ref_transaction_create(struct ref_transaction *transaction, > struct strbuf *err); > > /* > - * Add a reference deletion to transaction. If old_oid is non-NULL, > - * then it holds the value that the reference should have had before > - * the update (which must not be null_oid). > + * Add a reference deletion to transaction. If old_oid is non-NULL and not > + * null_oid, then it holds the value that the reference should have had before > + * the update. Passing null_oid is equivalent to passing NULL and disables the > + * old value check. > * > * See the above comment "Reference transaction updates" for more > * information. > diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c > index db58f0058..29945f2b8 100644 > --- a/t/helper/test-ref-store.c > +++ b/t/helper/test-ref-store.c > @@ -132,7 +132,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, NULL, flags); > string_list_clear(&refnames, 0); > return result; > } > -- > 2.39.3 (Apple Git-146)