@matheusbsbs / matheus-bagliano / commits / f57cc987a9

refs/reftable: fix D/F conflict error message on ref copy

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. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Apr 8, 2024 at 14:23 UTC f57cc987a937bef3f6c8e4911cf98e0f51eee1e2

2 files changed +35 -1

refs/reftable-backend.c

+2 -1

@@ -1351,7 +1351,8 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)

1351 /*

1352 * Verify that the new refname is available.

1353 */

1354 - string_list_insert(&skip, arg->oldname);

1354 + if (arg->delete_old)

1355 + string_list_insert(&skip, arg->oldname);

1356 ret = refs_verify_refname_available(&arg->refs->base, arg->newname,

1357 NULL, &skip, &errbuf);

1358 if (ret < 0) {

t/t0610-reftable-basics.sh

+33

@@ -730,6 +730,39 @@ test_expect_success 'reflog: updates via HEAD update HEAD reflog' '

730 )

731 '

732

733 +test_expect_success 'branch: copying branch with D/F conflict' '

734 + test_when_finished "rm -rf repo" &&

735 + git init repo &&

736 + (

737 + cd repo &&

738 + test_commit A &&

739 + git branch branch &&

740 + cat >expect <<-EOF &&

741 + error: ${SQ}refs/heads/branch${SQ} exists; cannot create ${SQ}refs/heads/branch/moved${SQ}

742 + fatal: branch copy failed

743 + EOF

744 + test_must_fail git branch -c branch branch/moved 2>err &&

745 + test_cmp expect err

746 + )

747 +'

748 +

749 +test_expect_success 'branch: moving branch with D/F conflict' '

750 + test_when_finished "rm -rf repo" &&

751 + git init repo &&

752 + (

753 + cd repo &&

754 + test_commit A &&

755 + git branch branch &&

756 + git branch conflict &&

757 + cat >expect <<-EOF &&

758 + error: ${SQ}refs/heads/conflict${SQ} exists; cannot create ${SQ}refs/heads/conflict/moved${SQ}

759 + fatal: branch rename failed

760 + EOF

761 + test_must_fail git branch -m branch conflict/moved 2>err &&

762 + test_cmp expect err

763 + )

764 +'

765 +

766 test_expect_success 'worktree: adding worktree creates separate stack' '

767 test_when_finished "rm -rf repo worktree" &&

768 git init repo &&