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

[Junio C Hamano <gitster@pobox.com>]

Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com> 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;