Patrick Steinhardt <ps@xxxxxx> 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@xxxxxx>
> ---
> 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 &&
