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

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

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

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

> +};
> +
> +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);
> +}

> +
>  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++) {

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