| Message ID | Zvj-7Rx8ZT_27UpE@ArchLinux (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | add ref content check for files backend | expand |
On Sun, Sep 29, 2024 at 03:17:01PM +0800, shejialuo wrote: > We do allow users to use "git symbolic-ref" to create symrefs which > point to one of the linked worktrees from the primary worktree or one of > the linked worktrees. > > We should not info the user about the escape for above situation. So, > enhance "files_fsck_symref_target" function to check whether the "referent" > starts with the "worktrees/" to make sure that we won't warn the user > when symrefs point to "referent" in the linked worktrees. Shouldn't this commit be squashed into the former one, as it immediately fixes an edge case that was introduced with the parent commit? Patrick
On Mon, Oct 07, 2024 at 08:58:40AM +0200, Patrick Steinhardt wrote: > On Sun, Sep 29, 2024 at 03:17:01PM +0800, shejialuo wrote: > > We do allow users to use "git symbolic-ref" to create symrefs which > > point to one of the linked worktrees from the primary worktree or one of > > the linked worktrees. > > > > We should not info the user about the escape for above situation. So, > > enhance "files_fsck_symref_target" function to check whether the "referent" > > starts with the "worktrees/" to make sure that we won't warn the user > > when symrefs point to "referent" in the linked worktrees. > > Shouldn't this commit be squashed into the former one, as it immediately > fixes an edge case that was introduced with the parent commit? > I partially agree here. I don't think this is an edge case that was introduced with the parent commit. The reason why I use a new commit here is that I want to emphasis the behavior. This is because Junio asked me in the v4 about "escapeReferent" I am not sure starting this as ERROR is wise. Users and third-party tools make creative uses of the system and I cannot offhand think of an argument why it should be forbidden to create a symbolic link to our own HEAD or to some worktree-specific ref in another worktree. Actually, I have never thought we could do this. So, this is my intention. But I do agree that this commit is highly relevant with the parent commit. I will improve this in the next version. > Patrick
diff --git a/refs/files-backend.c b/refs/files-backend.c index bd215c8d08..1182bca108 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3520,10 +3520,11 @@ static int files_fsck_symref_target(struct fsck_options *o, orig_last_byte = referent->buf[orig_len - 1]; strbuf_rtrim(referent); - if (!starts_with(referent->buf, "refs/")) { + if (!starts_with(referent->buf, "refs/") && + !starts_with(referent->buf, "worktrees/")) { ret = fsck_report_ref(o, report, FSCK_MSG_ESCAPE_REFERENT, - "referent '%s' is outside of refs/", + "referent '%s' is outside of refs/ or worktrees/", referent->buf); } diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh index 585f562245..936448f780 100755 --- a/t/t0602-reffiles-fsck.sh +++ b/t/t0602-reffiles-fsck.sh @@ -382,10 +382,42 @@ test_expect_success 'textual symref should be checked whether it is escaped' ' printf "ref: refs-back/heads/main\n" >$branch_dir_prefix/branch-bad-1 && git refs verify 2>err && cat >expect <<-EOF && - warning: refs/heads/branch-bad-1: escapeReferent: referent '\''refs-back/heads/main'\'' is outside of refs/ + warning: refs/heads/branch-bad-1: escapeReferent: referent '\''refs-back/heads/main'\'' is outside of refs/ or worktrees/ EOF rm $branch_dir_prefix/branch-bad-1 && test_cmp expect err ' +test_expect_success 'textual symref escape check should work with worktrees' ' + test_when_finished "rm -rf repo" && + git init repo && + cd repo && + test_commit default && + git branch branch-1 && + git branch branch-2 && + git branch branch-3 && + git worktree add ./worktree-1 branch-2 && + git worktree add ./worktree-2 branch-3 && + + ( + cd worktree-1 && + git branch refs/worktree/w1-branch && + git symbolic-ref refs/worktree/branch-4 refs/heads/branch-1 && + git symbolic-ref refs/worktree/branch-5 worktrees/worktree-2/refs/worktree/w2-branch + ) && + ( + cd worktree-2 && + git branch refs/worktree/w2-branch && + git symbolic-ref refs/worktree/branch-4 refs/heads/branch-1 && + git symbolic-ref refs/worktree/branch-5 worktrees/worktree-1/refs/worktree/w1-branch + ) && + + + git symbolic-ref refs/heads/branch-5 worktrees/worktree-1/refs/worktree/w1-branch && + git symbolic-ref refs/heads/branch-6 worktrees/worktree-2/refs/worktree/w2-branch && + + git refs verify 2>err && + test_must_be_empty err +' + test_done
We do allow users to use "git symbolic-ref" to create symrefs which point to one of the linked worktrees from the primary worktree or one of the linked worktrees. We should not info the user about the escape for above situation. So, enhance "files_fsck_symref_target" function to check whether the "referent" starts with the "worktrees/" to make sure that we won't warn the user when symrefs point to "referent" in the linked worktrees. Mentored-by: Patrick Steinhardt <ps@pks.im> Mentored-by: Karthik Nayak <karthik.188@gmail.com> Signed-off-by: shejialuo <shejialuo@gmail.com> --- refs/files-backend.c | 5 +++-- t/t0602-reffiles-fsck.sh | 34 +++++++++++++++++++++++++++++++++- 2 files changed, 36 insertions(+), 3 deletions(-)