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

> refs_delete_refs() currently performs unconditional deletions. Thus callers
> cannot preserve old values that they have already resolved, and
> reference-transaction hooks consequently see a null old OID.
>
> Add an optional oid_array whose entries correspond to the refnames. Pass each
> non-null OID to ref_transaction_delete(). Supplying an OID makes the deletion
> conditional: if the ref changed after the caller resolved it, the transaction
> fails instead of deleting the new value. Existing callers that pass NULL
> retain the unconditional behavior.
>
> Signed-off-by: Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com>
> ---
>  bisect.c                  |  2 +-
>  builtin/branch.c          |  3 ++-
>  builtin/fetch.c           |  2 +-
>  builtin/remote.c          |  5 +++--
>  builtin/tag.c             |  3 ++-
>  refs.c                    | 23 ++++++++++++++---------
>  refs.h                    | 12 ++++++++++--
>  t/helper/test-ref-store.c |  2 +-
>  8 files changed, 34 insertions(+), 18 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index 94c7028d2..9aa3bace9 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -1203,7 +1203,7 @@ int bisect_clean_state(void)
>  	string_list_append(&refs_for_removal, "BISECT_EXPECTED_REV");
>  	result = refs_delete_refs(get_main_ref_store(the_repository),
>  				  "bisect: remove", &refs_for_removal,
> -				  REF_NO_DEREF);
> +				  NULL, REF_NO_DEREF);
>  	string_list_clear(&refs_for_removal, 0);
>  	unlink_or_warn(git_path_bisect_ancestors_ok());
>  	unlink_or_warn(git_path_bisect_log());
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 1572a4f9e..f1abeb681 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -322,7 +322,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  		free(target);
>  	}
>
> -	if (refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF))
> +	if (refs_delete_refs(get_main_ref_store(the_repository), NULL,
> +			     &refs_to_delete, NULL, REF_NO_DEREF))
>  		ret = 1;
>
>  	for_each_string_list_item(item, &refs_to_delete) {
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index c1d7c672f..d202147b2 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1467,7 +1467,7 @@ static int prune_refs(struct display_state *display_state,
>  		} else {
>  			result = refs_delete_refs(get_main_ref_store(the_repository),
>  						  "fetch: prune", &refnames,
> -						  0);
> +						  NULL, 0);
>  		}
>  	}
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index de989ea3b..13d3cc52d 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -1073,7 +1073,7 @@ static int rm(int argc, const char **argv, const char *prefix,
>  	if (!result)
>  		result = refs_delete_refs(get_main_ref_store(the_repository),
>  					  "remote: remove", &branches,
> -					  REF_NO_DEREF);
> +					  NULL, REF_NO_DEREF);
>  	string_list_clear(&branches, 0);
>
>  	if (skipped.nr) {
> @@ -1645,7 +1645,8 @@ static int prune_remote(const char *remote, int dry_run)
>
>  	if (!dry_run)
>  		result |= refs_delete_refs(get_main_ref_store(the_repository),
> -					   "remote: prune", &refs_to_prune, 0);
> +					   "remote: prune", &refs_to_prune,
> +					   NULL, 0);
>
>  	for_each_string_list_item(item, &states.stale) {
>  		const char *refname = item->util;
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 06c125b53..40874a292 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -122,7 +122,8 @@ static int delete_tags(const char **argv)
>  	struct string_list_item *item;
>
>  	result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete);
> -	if (refs_delete_refs(get_main_ref_store(the_repository), NULL, &refs_to_delete, REF_NO_DEREF))
> +	if (refs_delete_refs(get_main_ref_store(the_repository), NULL,
> +			     &refs_to_delete, NULL, REF_NO_DEREF))
>  		result = 1;
>
>  	for_each_string_list_item(item, &refs_to_delete) {
> diff --git a/refs.c b/refs.c
> index d3caa9a63..9c593baea 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -18,6 +18,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"
> @@ -3056,33 +3057,37 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio
>  }
>
>  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 ref_transaction *transaction;
>  	struct strbuf err = STRBUF_INIT;
> -	struct string_list_item *item;
> +	size_t i;
>  	int ret = 0, failures = 0;
>  	char *msg;
>
> +	if (old_oids && old_oids->nr != refnames->nr)
> +		BUG("refname and old OID counts do not match");
>  	if (!refnames->nr)
>  		return 0;
>
>  	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);
>  	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;

>  		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);
> diff --git a/refs.h b/refs.h
> index 71d5c186d..b76b556cf 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;
> @@ -613,13 +614,20 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,
>  		    unsigned int flags);
>
>  /*
> - * Delete the specified references. If there are any problems, emit
> + * 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 entry is used
> + * to verify the current value of the corresponding reference before deleting
> + * it. A null entry disables verification for that reference.
> + *

> + * 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,
> +		     unsigned int flags);
>
>  /** Delete a reflog */
>  int refs_delete_reflog(struct ref_store *refs, const char *refname);
> diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
> index 3866d0aca..c2c7dfb06 100644
> --- a/t/helper/test-ref-store.c
> +++ b/t/helper/test-ref-store.c
> @@ -140,7 +140,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, flags);
>  	string_list_clear(&refnames, 0);
>  	return result;
>  }
> --
> 2.39.3 (Apple Git-146)