From: Junio C Hamano Date: Tue, 22 Sep 2026 18:55:45 GMT Subject: Re: [PATCH v3 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: <3315d5f47ad7d8bcdbeda90b161606507c7040ea.1790079917.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. When the array is provided, delete the ref at position N > only if it still points at the OID at position N. A null OID requests an > unconditional deletion for refs whose old value cannot be resolved, such as > broken refs. > > Use REF_TRANSACTION_ALLOW_FAILURE when old OIDs are supplied. This retains > the helper's best-effort behavior: an old-OID mismatch rejects that deletion > while independent deletions in the batch can still proceed. The history around this area seems to look like this (you can use "git blame" to figure this out yourself). * 98ffd5ff67 (delete_refs(): new function for the refs API, 2015-06-22) started the API function to allow multiple refs in bulk. The comment in refs.h that says refs_delete_refs() is not done in an all-or-nothing transaction has been there ever since. * 2fb330ca72 (packed_delete_refs(): implement method, 2017-09-08) started the "because these operations cannot fail, we can afford to run bulk deletion in a transaction without having to worry about making it all-or-none" for the packed backends. * e85e5dd78a (refs/files: use transactions to delete references, 2023-11-14) did the same for the files backends. * d6f8e72982 (refs: deduplicate code to delete references, 2023-11-14) consolidated the "because these cannot fail, we can afford to run bulk deletion in a transaction without making it all-or-none" codepaths between files and packed backends. * 23fc8e4f61 (refs: implement batch reference update support, 2025-04-08) introduced REF_TRANSACTION_ALLOW_FAILURE so that some callers can take advantage of "ref transactions" as a batched update mechanism, without having to roll everything back upon a failure. Doesn't the above observation suggest us that we should always be passing to ref_store_transaction_begin() inside refs_delete_refs() the REF_TRANSACTION_ALLOW_FAILURE flag? I would say that it was a missed clean-up opportunity at 23fc8e4f61 that we didn't do so back then. > diff --git a/refs.c b/refs.c > index 92d5df5b7..1e0f432ed 100644 > --- a/refs.c > +++ b/refs.c > @@ -3069,34 +3070,60 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio > } > } > > +struct delete_refs_rejection_data { > + int failures; > +}; This makes readers expect that we would be counting failures, e.g., the caller may request deletion of 100 refs and we report 30 of them failed to be deleted. > +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 = 1; > +} But that is not what is happening. If we wanted to count, it is a simple matter of incrementing the data->failures member instead of assigning 1 to it, of course. It also might be annoying to see 30 warning messages in such a case---or it may be what the caller is asking. I cannot tell. If we wanted to squelch excessive warning messages, we could count and cut-off after N failures, of course. > 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 delete_refs_rejection_data rejection_data = { 0 }; > 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 (!refnames->nr) > return 0; > + if (old_oids && old_oids->nr != refnames->nr) > + BUG("refname and old OID counts do not match"); > > 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, > + old_oids ? REF_TRANSACTION_ALLOW_FAILURE : 0, &err); This is the conditional/unconditional REF_TRANSACTION_ALLOW_FAILURE I discussed earlier. > 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; I think there was a comment by another reviewer on the previous round around this area, which was never answered. In general, it is a polite thing to respond to review messages and see that your response is acknowledged before you send an updated patch. I _think_ the reason why you need to treat null_oid specially is because you are using a flat array of object names, not an array of pointers to individual object names, but in that case, I wonder if ref_transaction_delete() should be the one who pays attention to the NULL-ness of its old_oid parameter? The current code does detect and reject (old_oid && is_null_oid(old_oid)) case, but I am not sure what we are gaining by that limitation. Rather I wonder if the first two lines of the function should read more like if (old_oid && is_null_oid(old_oid)) - BUG("delete called with old_oid set to zeros"); + old_oid = NULL; not forcing the callers (like we see above) to do the same. > @@ -3112,9 +3139,14 @@ int refs_delete_refs(struct ref_store *refs, const char *logmsg, > refnames->items[0].string, err.buf); > else > error(_("could not delete references: %s"), err.buf); > - } > + } else if (old_oids) > + ref_transaction_for_each_rejected_update(transaction, > + delete_refs_rejection_handler, > + &rejection_data); I personally feel that we should be weaning ourselves off of the assumption that presence of old_oids[] is the ONLY thing to cause rejection. IOW, always call for-each-rejected-update here regardless of old_oids != NULL. > out: > + if (rejection_data.failures) > + failures = 1; This is quite roundabout thing to do. rejection_data.failures, unlike my initial assumption, is not counting but is either 0 or 1, so failures here is also either 0 or 1, and then ... > if (!ret && failures) > ret = -1; ... if we have the failures computed to non-zero, we make sure ret is not zero. Shouldn't we at least get rid of the local variable failures? And if a variable FOO is not counting the number of FOO, do not name it FOOs. If it is a Boolean recording if we got FOOed, call it as such. My preference in this code path is to actually count failures in the member "int failures" of rejection_data structure, but if we are not counting, then call it "bool failed", perhaps. out: if (!ret && rejection_data.failed) ret = -1;