Re: [PATCH v2 2/3] branch, tag: retain old OIDs in batched deletions

> Before 8198907795 (use delete_refs when deleting tags or branches,
> 2021-01-21), branch and tag deletion passed each resolved old OID to
> delete_ref(). This prevented the command from deleting a ref that another
> process had changed after it was inspected.
>
> The conversion to batched deletion dropped those old OIDs. Besides making the
> deletions unconditional, this causes reference-transaction hooks to report
> zero as both the old and new OID.
>
> Both commands still resolve the old OIDs before starting the deletion. Pass
> those values to refs_delete_refs(). This restores the old race protection and
> lets hooks receive useful old values without adding any ref reads. If a ref
> changes concurrently, the transaction fails and preserves the new value.
>
> Signed-off-by: Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com>
> ---
>  builtin/branch.c                 |  6 ++++-
>  builtin/tag.c                    |  6 ++++-
>  t/t1416-ref-transaction-hooks.sh | 44 ++++++++++++++++++++++++++++++++
>  3 files changed, 54 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index f1abeb681..9f03ebc09 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -16,6 +16,7 @@
>  #include "commit.h"
>  #include "gettext.h"
>  #include "object-name.h"
> +#include "oid-array.h"
>  #include "remote.h"
>  #include "parse-options.h"
>  #include "branch.h"
> @@ -230,6 +231,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  	struct strbuf bname = STRBUF_INIT;
>  	enum interpret_branch_kind allowed_interpret;
>  	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
> +	struct oid_array old_oids = OID_ARRAY_INIT;
>  	struct string_list_item *item;
>  	int branch_name_pos;
>  	const char *fmt_remotes = "refs/remotes/%s";
> @@ -314,6 +316,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  		}
>
>  		item = string_list_append(&refs_to_delete, name);
> +		oid_array_append(&old_oids, &oid);
>  		item->util = xstrdup((flags & REF_ISBROKEN) ? "broken"
>  				    : (flags & REF_ISSYMREF) ? target
>  				    : repo_find_unique_abbrev(the_repository, &oid, DEFAULT_ABBREV));
> @@ -323,7 +326,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  	}
>
>  	if (refs_delete_refs(get_main_ref_store(the_repository), NULL,
> -			     &refs_to_delete, NULL, REF_NO_DEREF))
> +			     &refs_to_delete, &old_oids, REF_NO_DEREF))
>  		ret = 1;
>
>  	for_each_string_list_item(item, &refs_to_delete) {
> @@ -342,6 +345,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  		free(describe_ref);
>  	}
>  	string_list_clear(&refs_to_delete, 0);
> +	oid_array_clear(&old_oids);
>
>  	free(name);
>  	strbuf_release(&bname);
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 40874a292..0a3eb70fa 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -119,11 +119,14 @@ static int delete_tags(const char **argv)
>  {
>  	int result;
>  	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
> +	struct oid_array old_oids = OID_ARRAY_INIT;
>  	struct string_list_item *item;
>
>  	result = for_each_tag_name(argv, collect_tags, (void *)&refs_to_delete);
> +	for_each_string_list_item(item, &refs_to_delete)
> +		oid_array_append(&old_oids, item->util);

>  	if (refs_delete_refs(get_main_ref_store(the_repository), NULL,
> -			     &refs_to_delete, NULL, REF_NO_DEREF))
> +			     &refs_to_delete, &old_oids, REF_NO_DEREF))
>  		result = 1;
>
>  	for_each_string_list_item(item, &refs_to_delete) {
> @@ -137,6 +140,7 @@ static int delete_tags(const char **argv)
>  		free(oid);
>  	}
>  	string_list_clear(&refs_to_delete, 0);
> +	oid_array_clear(&old_oids);
>  	return result;
>  }
>
> diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
> index 4fe9d9b23..01b5ba8c4 100755
> --- a/t/t1416-ref-transaction-hooks.sh
> +++ b/t/t1416-ref-transaction-hooks.sh
> @@ -14,6 +14,50 @@ test_expect_success setup '
>  	POST_OID=$(git rev-parse POST)
>  '
>
> +test_expect_success 'hook gets old values for batched branch/tag deletion' '
> +	test_when_finished "rm -f actual" &&
> +	git branch to-delete PRE &&
> +	git tag delete-tag POST &&
> +	git pack-refs --all &&
> +	test_hook reference-transaction <<-\EOF &&
> +		if test "$1" = committed
> +		then
> +			# Ignore backend-internal zero-to-zero records.
> +			while read -r old new ref
> +			do
> +				case "$old" in
> +				*[!0]*)
> +					echo "$old $new $ref"
> +					;;
> +				esac
> +			done >>actual
> +		fi
> +	EOF
> +	cat >expect <<-EOF &&
> +		$PRE_OID $ZERO_OID refs/heads/to-delete
> +		$POST_OID $ZERO_OID refs/tags/delete-tag
> +	EOF
> +	git branch -D to-delete &&
> +	git tag -d delete-tag &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'branch deletion rejects a concurrent update' '
> +	git branch delete-race PRE &&
> +	test_hook reference-transaction <<-\EOF &&
> +		marker=$(git rev-parse --git-path delete-race-once)
> +		if test "$1" = preparing && test ! -e "$marker"
> +		then
> +			>"$marker"
> +			git update-ref refs/heads/delete-race POST
> +		fi
> +		exit 0
> +	EOF
> +	test_must_fail git branch -D delete-race 2>err &&
> +	test_grep "is at $POST_OID but expected $PRE_OID" err &&
> +	test_cmp_rev POST refs/heads/delete-race
> +'
> +
>  test_expect_success 'hook allows updating ref if successful' '
>  	git reset --hard PRE &&
>  	test_hook reference-transaction <<-\EOF &&
> --
> 2.39.3 (Apple Git-146)