Re: [PATCH 1/9] refs/reftable: fix D/F conflict error message on ref copy

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

Patrick Steinhardt <ps@pks.im> writes:

> The `write_copy_table()` function is shared between the reftable
> implementations for renaming and copying refs. The only difference
> between those two cases is that the rename will also delete the old
> reference, whereas copying won't.
>
> This has resulted in a bug though where we don't properly verify refname
> availability. When calling `refs_verify_refname_available()`, we always
> add the old ref name to the list of refs to be skipped when computing
> availability, which indicates that the name would be available even if
> it already exists at the current point in time. This is only the right
> thing to do for renames though, not for copies.
>
> The consequence of this bug is quite harmless because the reftable
> backend has its own checks for D/F conflicts further down in the call
> stack, and thus we refuse the update regardless of the bug. But all the
> user gets in this case is an uninformative message that copying the ref
> has failed, without any further details.
>
> Fix the bug and only add the old name to the skip-list in case we rename
> the ref. Consequently, this error case will now be handled by
> `refs_verify_refname_available()`, which knows to provide a proper error
> message.

OK.  Nicely described.  Instead of letting the reftable code
downstream to notice an update that was left uncaught by an extra
element in &skip, we will let refs_verify_refname_available() to
catch it at the right place.  Makes sense.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs/reftable-backend.c    |  3 ++-
>  t/t0610-reftable-basics.sh | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index e206d5a073..0358da14db 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1351,7 +1351,8 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)
>  	/*
>  	 * Verify that the new refname is available.
>  	 */
> -	string_list_insert(&skip, arg->oldname);
> +	if (arg->delete_old)
> +		string_list_insert(&skip, arg->oldname);
>  	ret = refs_verify_refname_available(&arg->refs->base, arg->newname,
>  					    NULL, &skip, &errbuf);
>  	if (ret < 0) {
> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> index 686781192e..055231a707 100755
> --- a/t/t0610-reftable-basics.sh
> +++ b/t/t0610-reftable-basics.sh
> @@ -730,6 +730,39 @@ test_expect_success 'reflog: updates via HEAD update HEAD reflog' '
>  	)
>  '
>  
> +test_expect_success 'branch: copying branch with D/F conflict' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit A &&
> +		git branch branch &&
> +		cat >expect <<-EOF &&
> +		error: ${SQ}refs/heads/branch${SQ} exists; cannot create ${SQ}refs/heads/branch/moved${SQ}
> +		fatal: branch copy failed
> +		EOF
> +		test_must_fail git branch -c branch branch/moved 2>err &&
> +		test_cmp expect err
> +	)
> +'
> +
> +test_expect_success 'branch: moving branch with D/F conflict' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit A &&
> +		git branch branch &&
> +		git branch conflict &&
> +		cat >expect <<-EOF &&
> +		error: ${SQ}refs/heads/conflict${SQ} exists; cannot create ${SQ}refs/heads/conflict/moved${SQ}
> +		fatal: branch rename failed
> +		EOF
> +		test_must_fail git branch -m branch conflict/moved 2>err &&
> +		test_cmp expect err
> +	)
> +'
> +
>  test_expect_success 'worktree: adding worktree creates separate stack' '
>  	test_when_finished "rm -rf repo worktree" &&
>  	git init repo &&