Re: [PATCH v3 1/3] refs: allow callers to supply old OIDs for batch deletion

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 22, 2026, 18:55 UTC
Message-ID: <xmqqjyoddsjy.fsf@gitster.g>
In-Reply-To: <3315d5f47ad7d8bcdbeda90b161606507c7040ea.1790079917.git.maciej.ciemborowicz@gmail.com>

Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com> writes:

Show 13 quoted lines

> 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.

Show 11 quoted lines

> 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.

Show 15 quoted lines

> +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.

Show 29 quoted lines

>  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.

Show 12 quoted lines

>  	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.

Show 9 quoted lines

> @@ -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;