| Message ID | 20200608062356.40264-7-sunshine@sunshineco.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | worktree: tighten duplicate path detection | expand |
Eric Sunshine <sunshine@sunshineco.com> writes: > diff --git a/builtin/worktree.c b/builtin/worktree.c > index 2cb95f16d4..eebd77c46d 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -153,6 +153,11 @@ static int prune_cmp(const void *a, const void *b) > > if ((c = fspathcmp(x->string, y->string))) > return c; > + /* paths same; main worktee (util==0) sorts above all others */ missing 'r'. "util ==NULL" or "!util"? Is the "main worktree has NULL in util" an invariant enforced by prune_worktrees()? > + if (!x->util) > + return -1; If x->util and y->util are both NULL, shouldn't they compare equal? Or is there something that enforces an invariant that there is only one such x whose util is NULL? Yes, that is the case... > + if (!y->util) > + return 1; > /* paths same; sort by .git/worktrees/<id> */ > return strcmp(x->util, y->util); > } > @@ -171,6 +176,7 @@ static void prune_dups(struct string_list *l) > static void prune_worktrees(void) > { > struct strbuf reason = STRBUF_INIT; > + struct strbuf main = STRBUF_INIT; > struct string_list kept = STRING_LIST_INIT_NODUP; > DIR *dir = opendir(git_path("worktrees")); > struct dirent *d; > @@ -190,6 +196,10 @@ static void prune_worktrees(void) > } > closedir(dir); > > + strbuf_add_absolute_path(&main, get_git_common_dir()); > + /* massage main worktree absolute path to match 'gitdir' content */ > + strbuf_strip_suffix(&main, "/."); > + string_list_append(&kept, strbuf_detach(&main, 0)); ... of course, it is done here. The existing loop picks up all <id> from .git/worktrees/<id>/ and they always have xstrdup(<id>) that cannot be NULL. But the string list item created here, whose util is NULL, is thrown into &kept and there is nobody who adds a string list item with NULL in util. So yes, there is only one "main" prune_cmp() needs to worry about. And the reason why prune_cmp() tiebreaks entries with the same .string (i.e. "path") so that the primary worktree comes early is because ... > prune_dups(&kept); ... deduping is done by later entries with the same path as an earlier one, keeping the earliest one among the ones with the same path. We want the primary worktree to survive, or we'd be in deep yoghurt. That reason is more important to comment in prune_cmp() than the hint that !util is for primary worktree. IOW, "why do we sort the primary one early?" is more important than "by inspecting .util field, we sort primary one early" (the latter lacks "why"). > string_list_clear(&kept, 1); > > diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh > index 7694f25a16..5f3db93b31 100755 > --- a/t/t2401-worktree-prune.sh > +++ b/t/t2401-worktree-prune.sh > @@ -114,4 +114,16 @@ test_expect_success 'prune duplicate (linked/linked)' ' > ! test -d .git/worktrees/w2 > ' > > +test_expect_success 'prune duplicate (main/linked)' ' > + test_when_finished rm -fr repo wt && > + test_create_repo repo && > + test_commit -C repo x && > + git -C repo worktree add --detach ../wt && > + rm -fr wt && > + mv repo wt && > + git -C wt worktree prune --verbose >actual && > + test_i18ngrep "duplicate entry" actual && > + ! test -d .git/worktrees/wt > +' > + > test_done
On Mon, Jun 8, 2020 at 5:59 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > + /* paths same; main worktee (util==0) sorts above all others */ > > And the reason why prune_cmp() tiebreaks entries with the same .string > (i.e. "path") so that the primary worktree comes early is because ... > > > prune_dups(&kept); > > ... deduping is done by later entries with the same path as an > earlier one, keeping the earliest one among the ones with the same > path. We want the primary worktree to survive, or we'd be in deep > yoghurt. > > That reason is more important to comment in prune_cmp() than the > hint that !util is for primary worktree. IOW, "why do we sort the > primary one early?" is more important than "by inspecting .util > field, we sort primary one early" (the latter lacks "why"). I'll update the comment to explain this. Thanks for the review.
diff --git a/builtin/worktree.c b/builtin/worktree.c index 2cb95f16d4..eebd77c46d 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -153,6 +153,11 @@ static int prune_cmp(const void *a, const void *b) if ((c = fspathcmp(x->string, y->string))) return c; + /* paths same; main worktee (util==0) sorts above all others */ + if (!x->util) + return -1; + if (!y->util) + return 1; /* paths same; sort by .git/worktrees/<id> */ return strcmp(x->util, y->util); } @@ -171,6 +176,7 @@ static void prune_dups(struct string_list *l) static void prune_worktrees(void) { struct strbuf reason = STRBUF_INIT; + struct strbuf main = STRBUF_INIT; struct string_list kept = STRING_LIST_INIT_NODUP; DIR *dir = opendir(git_path("worktrees")); struct dirent *d; @@ -190,6 +196,10 @@ static void prune_worktrees(void) } closedir(dir); + strbuf_add_absolute_path(&main, get_git_common_dir()); + /* massage main worktree absolute path to match 'gitdir' content */ + strbuf_strip_suffix(&main, "/."); + string_list_append(&kept, strbuf_detach(&main, 0)); prune_dups(&kept); string_list_clear(&kept, 1); diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh index 7694f25a16..5f3db93b31 100755 --- a/t/t2401-worktree-prune.sh +++ b/t/t2401-worktree-prune.sh @@ -114,4 +114,16 @@ test_expect_success 'prune duplicate (linked/linked)' ' ! test -d .git/worktrees/w2 ' +test_expect_success 'prune duplicate (main/linked)' ' + test_when_finished rm -fr repo wt && + test_create_repo repo && + test_commit -C repo x && + git -C repo worktree add --detach ../wt && + rm -fr wt && + mv repo wt && + git -C wt worktree prune --verbose >actual && + test_i18ngrep "duplicate entry" actual && + ! test -d .git/worktrees/wt +' + test_done
"git worktree prune" detects when multiple entries are associated with the same path and prunes the duplicates, however, it does not detect when a linked worktree points at the path of the main worktree. Although "git worktree add" disallows creating a new worktree with the same path as the main worktree, such a case can arise outside the control of Git even without the user mucking with .git/worktree/<id>/ administrative files. For instance: $ git clone foo.git $ git -C foo worktree add ../bar $ rm -rf bar $ mv foo bar $ git -C bar worktree list .../bar deadfeeb [master] .../bar deadfeeb [bar] Help the user recover from such corruption by extending "git worktree prune" to also detect when a linked worktree is associated with the path of the main worktree. Reported-by: Jonathan Müller <jonathanmueller.dev@gmail.com> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> --- builtin/worktree.c | 10 ++++++++++ t/t2401-worktree-prune.sh | 12 ++++++++++++ 2 files changed, 22 insertions(+)