Re: [PATCH 6/8] worktree: prune linked worktree referencing main worktree path

[Junio C Hamano <gitster@pobox.com>]

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