[PATCH 0/8] worktree: tighten duplicate path detection

[PATCH 0/8] worktree: tighten duplicate path detection

[Eric Sunshine]

This patch series started as a (presumably) simple enhancement to make
"git worktree prune" more helpful to users who shoot themselves in the
foot[1] by causing a linked worktree to point at the main worktree, a
situation which "git worktree add" tries hard to disallow but which
users can accomplish merely be deleting and renaming directories
(without even mucking around inside the .git/worktrees/<id> directory).
The series grew as I realized that "git worktree move" does not employ
the same protections as "git worktree add" and happily allows multiple
linked worktrees to references the same location, thus that also had to
be fixed.

[1]: https://lore.kernel.org/git/CAPig+cTU8+N6Chimpoa2_T-TcXxw-3B9-9pjCLz7WeOh472P_A@mail.gmail.com/

Eric Sunshine (8):
  worktree: factor out repeated string literal
  worktree: prune corrupted worktree even if locked
  worktree: give "should be pruned?" function more meaningful name
  worktree: make high-level pruning re-usable
  worktree: prune duplicate entries referencing same worktree path
  worktree: prune linked worktree referencing main worktree path
  worktree: generalize candidate worktree path validation
  worktree: make "move" refuse to move atop missing registered worktree

 Documentation/git-worktree.txt |   4 +-
 builtin/worktree.c             | 142 ++++++++++++++++++++++-----------
 t/t2401-worktree-prune.sh      |  38 ++++++++-
 t/t2403-worktree-move.sh       |  21 +++++
 4 files changed, 157 insertions(+), 48 deletions(-)

-- 
2.27.0.290.gba653c62da

[PATCH 1/8] worktree: factor out repeated string literal

[Eric Sunshine]

For each worktree removed by "git worktree prune", it reports the reason
for the removal. All reasons share the common prefix "Removing
worktrees/%s:". As new removal reasons are added, this prefix needs to
be duplicated, which is error-prone and potentially cumbersome.
Therefore, factor out the common prefix.

Although this change seems to increase the "sentence lego quotient", it
should be reasonably safe, as the reason for removal is a distinct
clause, not strictly related to the prefix. Moreover, the "worktrees" in
"Removing worktrees/%s:" is a path literal which ought not be localized,
so by factoring it out, we can more easily avoid exposing that path
fragment to translators.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index d99db35668..9b15f19fc5 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -76,19 +76,19 @@ static int prune_worktree(const char *id, struct strbuf *reason)
 	ssize_t read_result;
 
 	if (!is_directory(git_path("worktrees/%s", id))) {
-		strbuf_addf(reason, _("Removing worktrees/%s: not a valid directory"), id);
+		strbuf_addstr(reason, _("not a valid directory"));
 		return 1;
 	}
 	if (file_exists(git_path("worktrees/%s/locked", id)))
 		return 0;
 	if (stat(git_path("worktrees/%s/gitdir", id), &st)) {
-		strbuf_addf(reason, _("Removing worktrees/%s: gitdir file does not exist"), id);
+		strbuf_addstr(reason, _("gitdir file does not exist"));
 		return 1;
 	}
 	fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY);
 	if (fd < 0) {
-		strbuf_addf(reason, _("Removing worktrees/%s: unable to read gitdir file (%s)"),
-			    id, strerror(errno));
+		strbuf_addf(reason, _("unable to read gitdir file (%s)"),
+			    strerror(errno));
 		return 1;
 	}
 	len = xsize_t(st.st_size);
@@ -96,8 +96,8 @@ static int prune_worktree(const char *id, struct strbuf *reason)
 
 	read_result = read_in_full(fd, path, len);
 	if (read_result < 0) {
-		strbuf_addf(reason, _("Removing worktrees/%s: unable to read gitdir file (%s)"),
-			    id, strerror(errno));
+		strbuf_addf(reason, _("unable to read gitdir file (%s)"),
+			    strerror(errno));
 		close(fd);
 		free(path);
 		return 1;
@@ -106,15 +106,15 @@ static int prune_worktree(const char *id, struct strbuf *reason)
 
 	if (read_result != len) {
 		strbuf_addf(reason,
-			    _("Removing worktrees/%s: short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"),
-			    id, (uintmax_t)len, (uintmax_t)read_result);
+			    _("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"),
+			    (uintmax_t)len, (uintmax_t)read_result);
 		free(path);
 		return 1;
 	}
 	while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
 		len--;
 	if (!len) {
-		strbuf_addf(reason, _("Removing worktrees/%s: invalid gitdir file"), id);
+		strbuf_addstr(reason, _("invalid gitdir file"));
 		free(path);
 		return 1;
 	}
@@ -123,7 +123,7 @@ static int prune_worktree(const char *id, struct strbuf *reason)
 		free(path);
 		if (stat(git_path("worktrees/%s/index", id), &st) ||
 		    st.st_mtime <= expire) {
-			strbuf_addf(reason, _("Removing worktrees/%s: gitdir file points to non-existent location"), id);
+			strbuf_addstr(reason, _("gitdir file points to non-existent location"));
 			return 1;
 		} else {
 			return 0;
@@ -147,7 +147,8 @@ static void prune_worktrees(void)
 		if (!prune_worktree(d->d_name, &reason))
 			continue;
 		if (show_only || verbose)
-			printf("%s\n", reason.buf);
+			printf_ln(_("Removing %s/%s: %s"),
+				  "worktrees", d->d_name, reason.buf);
 		if (show_only)
 			continue;
 		delete_git_dir(d->d_name);
-- 
2.27.0.290.gba653c62da

Re: [PATCH 1/8] worktree: factor out repeated string literal

[Shourya Shukla]

On 08/06 02:23, Eric Sunshine wrote:
Hello Eric!

> Although this change seems to increase the "sentence lego quotient", it
> should be reasonably safe, as the reason for removal is a distinct
> clause, not strictly related to the prefix. Moreover, the "worktrees" in
> "Removing worktrees/%s:" is a path literal which ought not be localized,
> so by factoring it out, we can more easily avoid exposing that path
> fragment to translators.

Could you explain the above paragraph just a bit more? The English is
confusing me a bit.

Re: [PATCH 1/8] worktree: factor out repeated string literal

[Eric Sunshine]

On Mon, Jun 8, 2020 at 3:38 PM Shourya Shukla
<shouryashukla.oo@gmail.com> wrote:
> On 08/06 02:23, Eric Sunshine wrote:
> > Although this change seems to increase the "sentence lego quotient", it
> > should be reasonably safe, as the reason for removal is a distinct
> > clause, not strictly related to the prefix. Moreover, the "worktrees" in
> > "Removing worktrees/%s:" is a path literal which ought not be localized,
> > so by factoring it out, we can more easily avoid exposing that path
> > fragment to translators.
>
> Could you explain the above paragraph just a bit more? The English is
> confusing me a bit.

Which part is confusing you? The "sentence lego" bit or the bit about
not wanting translators touching a path literal? Or something else?

The path element bit is easy: A literal path, such as
".git/worktrees/foo" should not be translated to a user's local
language.

Sentence lego refers to the process of composing text from several
pieces. Doing so can make it difficult for a translator to localize it
to a particular language because the standalone pieces don't
necessarily provide sufficient context for a proper translation. For
instance, this example is from [1]:

    Translatable strings should be entire sentences. It is often not
    possible to translate single verbs or adjectives in a
    substitutable way.

    printf ("File %s is %s protected", filename, rw ? "write" : "read");

    Most translators will not look at the source and will thus only see
    the string "File %s is %s protected", which is
    unintelligible. Change this to

    printf (rw ? "File %s is write protected" :
             "File %s is read protected", filename);

    This way the translator will not only understand the message, she
    will also be able to find the appropriate grammatical
    construction. A French translator for example translates "write
    protected" like "protected against writing".

[1]: https://www.gnu.org/software/gettext/manual/html_node/Preparing-Strings.html

[PATCH 2/8] worktree: prune corrupted worktree even if locked

[Eric Sunshine]

The .git/worktrees/<id>/locked file created by "git worktree lock" is
intended to prevent a missing worktree -- which might reside on a
removable device or network share -- from being pruned. It is not meant
to prevent a corrupt worktree from being pruned, yet it short-circuits
almost all "git worktree prune" corruption checks. This can make it
impossible[1] to prune a worktree which becomes corrupt after the lock
is placed since "git worktree prune" won't prune it, and it may not even
be possible to unlock it with "git worktree unlock", depending upon the
nature of the corruption.

Therefore, delay the check for .git/worktrees/<id>/locked until after
all forms of corruption have been checked so that it behaves as
originally intended (to wit: preventing pruning of a missing worktree
only).

[1]: Impossible, that is, without manually mucking around with
     .git/worktrees/<id>/ administrative files.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c        |  4 ++--
 t/t2401-worktree-prune.sh | 14 ++++++++++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9b15f19fc5..f7351413af 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -79,8 +79,6 @@ static int prune_worktree(const char *id, struct strbuf *reason)
 		strbuf_addstr(reason, _("not a valid directory"));
 		return 1;
 	}
-	if (file_exists(git_path("worktrees/%s/locked", id)))
-		return 0;
 	if (stat(git_path("worktrees/%s/gitdir", id), &st)) {
 		strbuf_addstr(reason, _("gitdir file does not exist"));
 		return 1;
@@ -121,6 +119,8 @@ static int prune_worktree(const char *id, struct strbuf *reason)
 	path[len] = '\0';
 	if (!file_exists(path)) {
 		free(path);
+		if (file_exists(git_path("worktrees/%s/locked", id)))
+			return 0;
 		if (stat(git_path("worktrees/%s/index", id), &st) ||
 		    st.st_mtime <= expire) {
 			strbuf_addstr(reason, _("gitdir file points to non-existent location"));
diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh
index b7d6d5d45a..9be8e97d66 100755
--- a/t/t2401-worktree-prune.sh
+++ b/t/t2401-worktree-prune.sh
@@ -69,13 +69,23 @@ test_expect_success 'prune directories with gitdir pointing to nowhere' '
 '
 
 test_expect_success 'not prune locked checkout' '
-	test_when_finished rm -r .git/worktrees &&
-	mkdir -p .git/worktrees/ghi &&
+	test_when_finished rm -fr .git/worktrees ghi &&
+	git worktree add ghi &&
 	: >.git/worktrees/ghi/locked &&
+	rm -r ghi &&
 	git worktree prune &&
 	test -d .git/worktrees/ghi
 '
 
+test_expect_success 'prune corrupt despite lock' '
+	test_when_finished rm -fr .git/worktrees ghi &&
+	mkdir -p .git/worktrees/ghi &&
+	: >.git/worktrees/ghi/gitdir &&
+	: >.git/worktrees/ghi/locked &&
+	git worktree prune &&
+	! test -d .git/worktrees/ghi
+'
+
 test_expect_success 'not prune recent checkouts' '
 	test_when_finished rm -r .git/worktrees &&
 	git worktree add jlm HEAD &&
-- 
2.27.0.290.gba653c62da

Re: [PATCH 2/8] worktree: prune corrupted worktree even if locked

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> The .git/worktrees/<id>/locked file created by "git worktree lock" is
> intended to prevent a missing worktree -- which might reside on a
> removable device or network share -- from being pruned. It is not meant
> to prevent a corrupt worktree from being pruned, yet it short-circuits
> almost all "git worktree prune" corruption checks.

The '.git/worktrees/<id>/locked' file is what 'It' in "It is not
meant to" refers to, but the 'it' in "yet it short-circuits" cannot
refer to the same thing---my reading hiccuped there.

"Its presence causes most of the corruption checks skipped by 'git
worktree prune'", perhaps.

> This can make it
> impossible[1] to prune a worktree which becomes corrupt after the lock
> is placed since "git worktree prune" won't prune it, and it may not even
> be possible to unlock it with "git worktree unlock", depending upon the
> nature of the corruption.

The latter is because... "worktree unlock" does not skip corruption
check and refuses to unlock a corrupted worktree, or something?

> Therefore, delay the check for .git/worktrees/<id>/locked until after
> all forms of corruption have been checked so that it behaves as
> originally intended (to wit: preventing pruning of a missing worktree
> only).

An alternative could be to allow unlocking a worktree even if it is
corrupt, and with that, such an unprunable corrupt worktree can
first be unlocked and then pruned?  A naive first thought is that
might make it slightly safer, but the reason why this approach was
taken is because the end user already said 'prune' so that should
trump whatever ".git/worktrees/<id>/" has?

But the intent of locking a worktree is "make sure that the end user
is aware of the fact that it is locked before allowing the worktree
to be pruned", isn't it?  Unless there is a way for a corruption to
add the "locked" file the end-user did not intend to have, if we
sense the "locked" file given to a worktree, shouldn't we honor that
existing "locked" file's intent?

I am growing skeptical about the approach taken by this step.  There
must be something missing that I may become aware of after reading
the remainder of the series.

	... goes back and digs a bit ...

This came from 23af91d1 (prune: strategies for linked checkouts,
2014-11-30) which explains:

    To prevent `git prune --worktrees` from deleting a
    $GIT_DIR/worktrees entry (which can be useful in some
    situations, such as when the entry's working tree is stored on a
    portable device), add a file named 'locked' to the entry's
    directory.

Notice that "in some situations, such as" gives just one example,
and it is clear that it is the only envisioned use case.

It therefore feels more sensible to honor the "locked" file whether
the actual worktree (or just a part of it) is accessible or not when
"prune" gets exercised.  After all, if some parts of the actual
worktree gets moved to removal media when not in use, such a partial
removal may make the worktree appear as if it is "corrupt".  We do
not want to declare that it is corrupt and we ignore the locked
state, or do we?

Thanks.

> [1]: Impossible, that is, without manually mucking around with
>      .git/worktrees/<id>/ administrative files.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  builtin/worktree.c        |  4 ++--
>  t/t2401-worktree-prune.sh | 14 ++++++++++++--
>  2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 9b15f19fc5..f7351413af 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -79,8 +79,6 @@ static int prune_worktree(const char *id, struct strbuf *reason)
>  		strbuf_addstr(reason, _("not a valid directory"));
>  		return 1;
>  	}
> -	if (file_exists(git_path("worktrees/%s/locked", id)))
> -		return 0;
>  	if (stat(git_path("worktrees/%s/gitdir", id), &st)) {
>  		strbuf_addstr(reason, _("gitdir file does not exist"));
>  		return 1;
> @@ -121,6 +119,8 @@ static int prune_worktree(const char *id, struct strbuf *reason)
>  	path[len] = '\0';
>  	if (!file_exists(path)) {
>  		free(path);
> +		if (file_exists(git_path("worktrees/%s/locked", id)))
> +			return 0;
>  		if (stat(git_path("worktrees/%s/index", id), &st) ||
>  		    st.st_mtime <= expire) {
>  			strbuf_addstr(reason, _("gitdir file points to non-existent location"));
> diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh
> index b7d6d5d45a..9be8e97d66 100755
> --- a/t/t2401-worktree-prune.sh
> +++ b/t/t2401-worktree-prune.sh
> @@ -69,13 +69,23 @@ test_expect_success 'prune directories with gitdir pointing to nowhere' '
>  '
>  
>  test_expect_success 'not prune locked checkout' '
> -	test_when_finished rm -r .git/worktrees &&
> -	mkdir -p .git/worktrees/ghi &&
> +	test_when_finished rm -fr .git/worktrees ghi &&
> +	git worktree add ghi &&
>  	: >.git/worktrees/ghi/locked &&
> +	rm -r ghi &&
>  	git worktree prune &&
>  	test -d .git/worktrees/ghi
>  '
>  
> +test_expect_success 'prune corrupt despite lock' '
> +	test_when_finished rm -fr .git/worktrees ghi &&
> +	mkdir -p .git/worktrees/ghi &&
> +	: >.git/worktrees/ghi/gitdir &&
> +	: >.git/worktrees/ghi/locked &&
> +	git worktree prune &&
> +	! test -d .git/worktrees/ghi
> +'
> +
>  test_expect_success 'not prune recent checkouts' '
>  	test_when_finished rm -r .git/worktrees &&
>  	git worktree add jlm HEAD &&

Re: [PATCH 2/8] worktree: prune corrupted worktree even if locked

[Eric Sunshine]

On Mon, Jun 8, 2020 at 5:24 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > The .git/worktrees/<id>/locked file created by "git worktree lock" is
> > intended to prevent a missing worktree -- which might reside on a
> > removable device or network share -- from being pruned. It is not meant
> > to prevent a corrupt worktree from being pruned, yet it short-circuits
> > almost all "git worktree prune" corruption checks.
>
> The '.git/worktrees/<id>/locked' file is what 'It' in "It is not
> meant to" refers to, but the 'it' in "yet it short-circuits" cannot
> refer to the same thing---my reading hiccuped there.
>
> "Its presence causes most of the corruption checks skipped by 'git
> worktree prune'", perhaps.

I can adopt that wording, but see below...

> > This can make it
> > impossible[1] to prune a worktree which becomes corrupt after the lock
> > is placed since "git worktree prune" won't prune it, and it may not even
> > be possible to unlock it with "git worktree unlock", depending upon the
> > nature of the corruption.
>
> The latter is because... "worktree unlock" does not skip corruption
> check and refuses to unlock a corrupted worktree, or something?

That bit of the commit message was a late addition and somewhat
intentionally hand-wavy. I don't think "git worktree unlock" will
currently die or misbehave due to corruption, but was thinking that it
someday might if additional checks are ever added. But, it's not
worth pursuing since...

> But the intent of locking a worktree is "make sure that the end user
> is aware of the fact that it is locked before allowing the worktree
> to be pruned", isn't it? Unless there is a way for a corruption to
> add the "locked" file the end-user did not intend to have, if we
> sense the "locked" file given to a worktree, shouldn't we honor that
> existing "locked" file's intent?
>
> I am growing skeptical about the approach taken by this step. There
> must be something missing that I may become aware of after reading
> the remainder of the series.

You're not the only person skeptical about this patch. I flip-flopped
on it multiple times, first convincing myself it was the right thing
to do, then convincing myself that the original code was correct, and
so forth. That's a good indication that such a change overall is
questionable.

Aside from that, this patch is unrelated to the intent of this series.
So, I'll drop it when I re-roll.

[PATCH 3/8] worktree: give "should be pruned?" function more meaningful name

[Eric Sunshine]

Readers of the name prune_worktree() are likely to expect the function
to actually prune a worktree, however, it only answers the question
"should this worktree be pruned?". Give it a name more reflective of its
true purpose to avoid such confusion.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index f7351413af..27681a1396 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -67,7 +67,7 @@ static void delete_worktrees_dir_if_empty(void)
 	rmdir(git_path("worktrees")); /* ignore failed removal */
 }
 
-static int prune_worktree(const char *id, struct strbuf *reason)
+static int should_prune_worktree(const char *id, struct strbuf *reason)
 {
 	struct stat st;
 	char *path;
@@ -144,7 +144,7 @@ static void prune_worktrees(void)
 		if (is_dot_or_dotdot(d->d_name))
 			continue;
 		strbuf_reset(&reason);
-		if (!prune_worktree(d->d_name, &reason))
+		if (!should_prune_worktree(d->d_name, &reason))
 			continue;
 		if (show_only || verbose)
 			printf_ln(_("Removing %s/%s: %s"),
-- 
2.27.0.290.gba653c62da

Re: [PATCH 3/8] worktree: give "should be pruned?" function more meaningful name

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Readers of the name prune_worktree() are likely to expect the function
> to actually prune a worktree, however, it only answers the question
> "should this worktree be pruned?". Give it a name more reflective of its
> true purpose to avoid such confusion.

100% agreed.

[PATCH 4/8] worktree: make high-level pruning re-usable

[Eric Sunshine]

The low-level logic for removing a worktree is well encapsulated in
delete_git_dir(). However, high-level details related to pruning a
worktree -- such as dealing with verbosity and dry-run mode -- are not
encapsulated. Factor out this high-level logic into its own function so
it can be re-used as new worktree corruption detectors are added.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 27681a1396..d0c046e885 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -133,6 +133,14 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)
 	return 0;
 }
 
+static void prune_worktree(const char *id, const char *reason)
+{
+	if (show_only || verbose)
+		printf_ln(_("Removing %s/%s: %s"), "worktrees", id, reason);
+	if (!show_only)
+		delete_git_dir(id);
+}
+
 static void prune_worktrees(void)
 {
 	struct strbuf reason = STRBUF_INIT;
@@ -146,12 +154,7 @@ static void prune_worktrees(void)
 		strbuf_reset(&reason);
 		if (!should_prune_worktree(d->d_name, &reason))
 			continue;
-		if (show_only || verbose)
-			printf_ln(_("Removing %s/%s: %s"),
-				  "worktrees", d->d_name, reason.buf);
-		if (show_only)
-			continue;
-		delete_git_dir(d->d_name);
+		prune_worktree(d->d_name, reason.buf);
 	}
 	closedir(dir);
 	if (!show_only)
-- 
2.27.0.290.gba653c62da

Re: [PATCH 4/8] worktree: make high-level pruning re-usable

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> The low-level logic for removing a worktree is well encapsulated in
> delete_git_dir(). However, high-level details related to pruning a
> worktree -- such as dealing with verbosity and dry-run mode -- are not
> encapsulated. Factor out this high-level logic into its own function so
> it can be re-used as new worktree corruption detectors are added.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  builtin/worktree.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 27681a1396..d0c046e885 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -133,6 +133,14 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)
>  	return 0;
>  }
>  
> +static void prune_worktree(const char *id, const char *reason)
> +{
> +	if (show_only || verbose)
> +		printf_ln(_("Removing %s/%s: %s"), "worktrees", id, reason);
> +	if (!show_only)
> +		delete_git_dir(id);
> +}

Makes sense, and this sensible name can be used only because we've
cleaned up the other one in the previous step.

Good so far (except that I still do not know why 2/8 is a good move
after reading the series up to this point).

>  static void prune_worktrees(void)
>  {
>  	struct strbuf reason = STRBUF_INIT;
> @@ -146,12 +154,7 @@ static void prune_worktrees(void)
>  		strbuf_reset(&reason);
>  		if (!should_prune_worktree(d->d_name, &reason))
>  			continue;
> -		if (show_only || verbose)
> -			printf_ln(_("Removing %s/%s: %s"),
> -				  "worktrees", d->d_name, reason.buf);
> -		if (show_only)
> -			continue;
> -		delete_git_dir(d->d_name);
> +		prune_worktree(d->d_name, reason.buf);
>  	}
>  	closedir(dir);
>  	if (!show_only)

[PATCH 5/8] worktree: prune duplicate entries referencing same worktree path

[Eric Sunshine]

A fundamental restriction of linked working trees is that there must
only ever be a single worktree associated with a particular path, thus
"git worktree add" explicitly disallows creation of a new worktree at
the same location as an existing registered worktree. Nevertheless,
users can still "shoot themselves in the foot" by mucking with
administrative files in .git/worktree/<id>/. Worse, "git worktree move"
is careless[1] and allows a worktree to be moved atop a registered but
missing worktree (which can happen, for instance, if the worktree is on
removable media). For instance:

    $ git clone foo.git
    $ cd foo
    $ git worktree add ../bar
    $ git worktree add ../baz
    $ rm -rf ../bar
    $ git worktree move ../baz ../bar
    $ git worktree list
    .../foo beefd00f [master]
    .../bar beefd00f [bar]
    .../bar beefd00f [baz]

Help users recover from this form of corruption by teaching "git
worktree prune" to detect when multiple worktrees are associated with
the same path.

[1]: A subsequent commit will fix "git worktree move" validation to be
     more strict.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c        | 64 ++++++++++++++++++++++++++++++---------
 t/t2401-worktree-prune.sh | 12 ++++++++
 2 files changed, 62 insertions(+), 14 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index d0c046e885..2cb95f16d4 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -67,7 +67,12 @@ static void delete_worktrees_dir_if_empty(void)
 	rmdir(git_path("worktrees")); /* ignore failed removal */
 }
 
-static int should_prune_worktree(const char *id, struct strbuf *reason)
+/*
+ * Return NULL if worktree entry should be pruned (along with reason for
+ * pruning), otherwise return the path of the worktree itself. Caller is
+ * responsible for freeing return value.
+ */
+static char *worktree_disposition(const char *id, struct strbuf *reason)
 {
 	struct stat st;
 	char *path;
@@ -77,17 +82,17 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)
 
 	if (!is_directory(git_path("worktrees/%s", id))) {
 		strbuf_addstr(reason, _("not a valid directory"));
-		return 1;
+		return NULL;
 	}
 	if (stat(git_path("worktrees/%s/gitdir", id), &st)) {
 		strbuf_addstr(reason, _("gitdir file does not exist"));
-		return 1;
+		return NULL;
 	}
 	fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY);
 	if (fd < 0) {
 		strbuf_addf(reason, _("unable to read gitdir file (%s)"),
 			    strerror(errno));
-		return 1;
+		return NULL;
 	}
 	len = xsize_t(st.st_size);
 	path = xmallocz(len);
@@ -98,7 +103,7 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)
 			    strerror(errno));
 		close(fd);
 		free(path);
-		return 1;
+		return NULL;
 	}
 	close(fd);
 
@@ -107,30 +112,29 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)
 			    _("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"),
 			    (uintmax_t)len, (uintmax_t)read_result);
 		free(path);
-		return 1;
+		return NULL;
 	}
 	while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
 		len--;
 	if (!len) {
 		strbuf_addstr(reason, _("invalid gitdir file"));
 		free(path);
-		return 1;
+		return NULL;
 	}
 	path[len] = '\0';
 	if (!file_exists(path)) {
-		free(path);
 		if (file_exists(git_path("worktrees/%s/locked", id)))
-			return 0;
+			return path;
 		if (stat(git_path("worktrees/%s/index", id), &st) ||
 		    st.st_mtime <= expire) {
 			strbuf_addstr(reason, _("gitdir file points to non-existent location"));
-			return 1;
+			free(path);
+			return NULL;
 		} else {
-			return 0;
+			return path;
 		}
 	}
-	free(path);
-	return 0;
+	return path;
 }
 
 static void prune_worktree(const char *id, const char *reason)
@@ -141,22 +145,54 @@ static void prune_worktree(const char *id, const char *reason)
 		delete_git_dir(id);
 }
 
+static int prune_cmp(const void *a, const void *b)
+{
+	const struct string_list_item *x = a;
+	const struct string_list_item *y = b;
+	int c;
+
+	if ((c = fspathcmp(x->string, y->string)))
+	    return c;
+	/* paths same; sort by .git/worktrees/<id> */
+	return strcmp(x->util, y->util);
+}
+
+static void prune_dups(struct string_list *l)
+{
+	int i;
+
+	QSORT(l->items, l->nr, prune_cmp);
+	for (i = 1; i < l->nr; i++) {
+		if (!fspathcmp(l->items[i].string, l->items[i - 1].string))
+			prune_worktree(l->items[i].util, "duplicate entry");
+	}
+}
+
 static void prune_worktrees(void)
 {
 	struct strbuf reason = STRBUF_INIT;
+	struct string_list kept = STRING_LIST_INIT_NODUP;
 	DIR *dir = opendir(git_path("worktrees"));
 	struct dirent *d;
 	if (!dir)
 		return;
 	while ((d = readdir(dir)) != NULL) {
+		char *path;
 		if (is_dot_or_dotdot(d->d_name))
 			continue;
 		strbuf_reset(&reason);
-		if (!should_prune_worktree(d->d_name, &reason))
+		path = worktree_disposition(d->d_name, &reason);
+		if (path) {
+			string_list_append(&kept, path)->util = xstrdup(d->d_name);
 			continue;
+		}
 		prune_worktree(d->d_name, reason.buf);
 	}
 	closedir(dir);
+
+	prune_dups(&kept);
+	string_list_clear(&kept, 1);
+
 	if (!show_only)
 		delete_worktrees_dir_if_empty();
 	strbuf_release(&reason);
diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh
index 9be8e97d66..7694f25a16 100755
--- a/t/t2401-worktree-prune.sh
+++ b/t/t2401-worktree-prune.sh
@@ -102,4 +102,16 @@ test_expect_success 'not prune proper checkouts' '
 	test -d .git/worktrees/nop
 '
 
+test_expect_success 'prune duplicate (linked/linked)' '
+	test_when_finished rm -fr .git/worktrees w1 w2 &&
+	git worktree add --detach w1 &&
+	git worktree add --detach w2 &&
+	sed "s/w2/w1/" .git/worktrees/w2/gitdir >.git/worktrees/w2/gitdir.new &&
+	mv .git/worktrees/w2/gitdir.new .git/worktrees/w2/gitdir &&
+	git worktree prune --verbose >actual &&
+	test_i18ngrep "duplicate entry" actual &&
+	test -d .git/worktrees/w1 &&
+	! test -d .git/worktrees/w2
+'
+
 test_done
-- 
2.27.0.290.gba653c62da

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

[Eric Sunshine]

"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(+)

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
-- 
2.27.0.290.gba653c62da

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

[Junio C Hamano]

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

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

[Eric Sunshine]

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.

[PATCH 7/8] worktree: generalize candidate worktree path validation

[Eric Sunshine]

"git worktree add" checks that the specified path is a valid location
for a new worktree by ensuring that the path does not already exist and
is not already registered to another worktree (a path can be registered
but missing, for instance, if it resides on removable media). Since "git
worktree add" is not the only command which should perform such
validation ("git worktree move" ought to also), generalize the the
validation function for use by other callers, as well.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index eebd77c46d..7c0637234e 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -274,34 +274,33 @@ static const char *worktree_basename(const char *path, int *olen)
 	return name;
 }
 
-static void validate_worktree_add(const char *path, const struct add_opts *opts)
+/* check that path is viable location for worktree */
+static void check_candidate_path(const char *path,
+				 int force,
+				 struct worktree **worktrees,
+				 const char *cmd)
 {
-	struct worktree **worktrees;
 	struct worktree *wt;
 	int locked;
 
 	if (file_exists(path) && !is_empty_dir(path))
 		die(_("'%s' already exists"), path);
 
-	worktrees = get_worktrees(0);
 	wt = find_worktree_by_path(worktrees, path);
 	if (!wt)
-		goto done;
+		return;
 
 	locked = !!worktree_lock_reason(wt);
-	if ((!locked && opts->force) || (locked && opts->force > 1)) {
+	if ((!locked && force) || (locked && force > 1)) {
 		if (delete_git_dir(wt->id))
-		    die(_("unable to re-add worktree '%s'"), path);
-		goto done;
+		    die(_("unusable worktree destination '%s'"), path);
+		return;
 	}
 
 	if (locked)
-		die(_("'%s' is a missing but locked worktree;\nuse 'add -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear"), path);
+		die(_("'%s' is a missing but locked worktree;\nuse '%s -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear"), cmd, path);
 	else
-		die(_("'%s' is a missing but already registered worktree;\nuse 'add -f' to override, or 'prune' or 'remove' to clear"), path);
-
-done:
-	free_worktrees(worktrees);
+		die(_("'%s' is a missing but already registered worktree;\nuse '%s -f' to override, or 'prune' or 'remove' to clear"), cmd, path);
 }
 
 static int add_worktree(const char *path, const char *refname,
@@ -318,8 +317,12 @@ static int add_worktree(const char *path, const char *refname,
 	struct commit *commit = NULL;
 	int is_branch = 0;
 	struct strbuf sb_name = STRBUF_INIT;
+	struct worktree **worktrees;
 
-	validate_worktree_add(path, opts);
+	worktrees = get_worktrees(0);
+	check_candidate_path(path, opts->force, worktrees, "add");
+	free_worktrees(worktrees);
+	worktrees = NULL;
 
 	/* is 'refname' a branch or commit? */
 	if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
-- 
2.27.0.290.gba653c62da

Re: [PATCH 7/8] worktree: generalize candidate worktree path validation

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> "git worktree add" checks that the specified path is a valid location
> for a new worktree by ensuring that the path does not already exist and
> is not already registered to another worktree (a path can be registered
> but missing, for instance, if it resides on removable media). Since "git
> worktree add" is not the only command which should perform such
> validation ("git worktree move" ought to also), generalize the the
> validation function for use by other callers, as well.

Makes sense.

> -	validate_worktree_add(path, opts);
> +	worktrees = get_worktrees(0);
> +	check_candidate_path(path, opts->force, worktrees, "add");
> +	free_worktrees(worktrees);
> +	worktrees = NULL;

It is somewhat unsatisfying that the libified helper still signals
its displeasure by dying, but a faithful conversion that can be
cleaned up later (if we wanted to) like this step is easier to
reason about.

[PATCH 8/8] worktree: make "move" refuse to move atop missing registered worktree

[Eric Sunshine]

"git worktree add" takes special care to avoid creating a new worktree
at a location already registered to an existing worktree even if that
worktree is missing (which can happen, for instance, if the worktree
resides on removable media). "git worktree move", however, is not so
careful when validating the destination location and will happily move
the source worktree atop the location of a missing worktree. This leads
to the anomalous situation of multiple worktrees being associated with
the same path, which is expressively forbidden by design. For example:

    $ git clone foo.git
    $ cd foo
    $ git worktree add ../bar
    $ git worktree add ../baz
    $ rm -rf ../bar
    $ git worktree move ../baz ../bar
    $ git worktree list
    .../foo beefd00f [master]
    .../bar beefd00f [bar]
    .../bar beefd00f [baz]
    $ git worktree remove ../bar
    fatal: validation failed, cannot remove working tree:
        '.../bar' does not point back to '.git/worktrees/bar'

Fix this shortcoming by enhancing "git worktree move" to perform the
same additional validation of the destination directory as done by "git
worktree add".

While at it, add a test to verify that "git worktree move" won't move a
worktree atop an existing (non-worktree) path -- a restriction which has
always been in place but was never tested.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/git-worktree.txt |  4 +++-
 builtin/worktree.c             |  3 +--
 t/t2403-worktree-move.sh       | 21 +++++++++++++++++++++
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 85d92c9761..4796c3c05e 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -126,7 +126,9 @@ OPTIONS
 	locked working tree path, specify `--force` twice.
 +
 `move` refuses to move a locked working tree unless `--force` is specified
-twice.
+twice. If the destination is already assigned to some other working tree but is
+missing (for instance, if `<new-path>` was deleted manually), then `--force`
+allows the move to proceed; use --force twice if the destination is locked.
 +
 `remove` refuses to remove an unclean working tree unless `--force` is used.
 To remove a locked working tree, specify `--force` twice.
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7c0637234e..dda7da497c 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -857,8 +857,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 		strbuf_trim_trailing_dir_sep(&dst);
 		strbuf_addstr(&dst, sep);
 	}
-	if (file_exists(dst.buf))
-		die(_("target '%s' already exists"), dst.buf);
+	check_candidate_path(dst.buf, force, worktrees, "move");
 
 	validate_no_submodules(wt);
 
diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh
index 939d18d728..7035c9d72e 100755
--- a/t/t2403-worktree-move.sh
+++ b/t/t2403-worktree-move.sh
@@ -112,6 +112,27 @@ test_expect_success 'move locked worktree (force)' '
 	git worktree move --force --force flump ploof
 '
 
+test_expect_success 'refuse to move worktree atop existing path' '
+	> bobble &&
+	git worktree add --detach beeble &&
+	test_must_fail git worktree move beeble bobble
+'
+
+test_expect_success 'move atop existing but missing worktree' '
+	git worktree add --detach gnoo &&
+	git worktree add --detach pneu &&
+	rm -fr pneu &&
+	test_must_fail git worktree move gnoo pneu &&
+	git worktree move --force gnoo pneu &&
+
+	git worktree add --detach nu &&
+	git worktree lock nu &&
+	rm -fr nu &&
+	test_must_fail git worktree move pneu nu &&
+	test_must_fail git worktree --force move pneu nu &&
+	git worktree move --force --force pneu nu
+'
+
 test_expect_success 'move a repo with uninitialized submodule' '
 	git init withsub &&
 	(
-- 
2.27.0.290.gba653c62da

Re: [PATCH 8/8] worktree: make "move" refuse to move atop missing registered worktree

[Eric Sunshine]

On Mon, Jun 8, 2020 at 2:25 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> "git worktree add" takes special care to avoid creating a new worktree
> at a location already registered to an existing worktree even if that
> worktree is missing (which can happen, for instance, if the worktree
> resides on removable media). "git worktree move", however, is not so
> careful when validating the destination location and will happily move
> the source worktree atop the location of a missing worktree. This leads
> to the anomalous situation of multiple worktrees being associated with
> the same path, which is expressively forbidden by design. For example:

s/expressively/expressly/

Re: [PATCH 8/8] worktree: make "move" refuse to move atop missing registered worktree

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> -	if (file_exists(dst.buf))
> -		die(_("target '%s' already exists"), dst.buf);
> +	check_candidate_path(dst.buf, force, worktrees, "move");

OK.  Moving to a location that is already occupied by an existing
file or a directory, even if that file or directory is not one of
the existing worktree, used to die here, but check_candidate_path()
performs that check and dies with almost the same message (it does
not say 'target'), so there is no loss of safety here.  The check
done in the check_candidate_path() helper is even better in that it
allows an existing directory as long as it is empty.

> diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh
> index 939d18d728..7035c9d72e 100755
> --- a/t/t2403-worktree-move.sh
> +++ b/t/t2403-worktree-move.sh
> @@ -112,6 +112,27 @@ test_expect_success 'move locked worktree (force)' '
>  	git worktree move --force --force flump ploof
>  '
>  
> +test_expect_success 'refuse to move worktree atop existing path' '
> +	> bobble &&

Style?

> +	git worktree add --detach beeble &&
> +	test_must_fail git worktree move beeble bobble
> +'
> +
> +test_expect_success 'move atop existing but missing worktree' '
> +	git worktree add --detach gnoo &&
> +	git worktree add --detach pneu &&
> +	rm -fr pneu &&
> +	test_must_fail git worktree move gnoo pneu &&
> +	git worktree move --force gnoo pneu &&
> +
> +	git worktree add --detach nu &&
> +	git worktree lock nu &&
> +	rm -fr nu &&
> +	test_must_fail git worktree move pneu nu &&
> +	test_must_fail git worktree --force move pneu nu &&
> +	git worktree move --force --force pneu nu
> +'
> +
>  test_expect_success 'move a repo with uninitialized submodule' '
>  	git init withsub &&
>  	(

Thanks.