| Message ID | 14b4dacd731a7d9c19029cd8a0c3b6170c31ae25.1712078736.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reftable: optimize write performance | expand |
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 &&
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 &&
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> --- refs/reftable-backend.c | 3 ++- t/t0610-reftable-basics.sh | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-)