Re: [PATCH 6/8] worktree: prune linked worktree referencing main worktree path
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 8, 2020, 21:59 UTC
- Message-ID
- <xmqqmu5djk1s.fsf@gitster.c.googlers.com>
- In-Reply-To
- <20200608062356.40264-7-sunshine@sunshineco.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
Show 9 quoted lines
> 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...
Show 21 quoted lines
> + 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").
Show 23 quoted lines
> 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