[PATCH v2 0/7] worktree: tighten duplicate path detection

[PATCH v2 0/7] worktree: tighten duplicate path detection

[Eric Sunshine]

This is a re-roll of [1] which teaches "git worktree prune" to detect
multiple worktrees referencing the same path, and makes "git worktree
move" disallow moving a worktree atop another registered worktree when
the destination worktree is missing (for instance, if it resides on
removable media).

Changes in v2:

* drop patch 2/8, which made 'prune' remove corrupt locked worktree
  entries, since it was difficult to justify the change

* fix a couple typos and a style violation

* fix a Sparse warning reported by Ramsay[2]

* fix a -Werror=main complaint noticed by Junio

[1]: https://lore.kernel.org/git/20200608062356.40264-1-sunshine@sunshineco.com/T/
[2]: https://lore.kernel.org/git/CAPig+cTF+pwBasVCzmucXmMZcm1K0ctkGOavj7bMcGsw2MvoKw@mail.gmail.com/T/

Eric Sunshine (7):
  worktree: factor out repeated string literal
  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             | 128 ++++++++++++++++++++++++---------
 t/t2401-worktree-prune.sh      |  24 +++++++
 t/t2403-worktree-move.sh       |  21 ++++++
 4 files changed, 141 insertions(+), 36 deletions(-)

Interdiff against v1:
diff --git a/builtin/worktree.c b/builtin/worktree.c
index dda7da497c..1238b6bab1 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -68,11 +68,11 @@ static void delete_worktrees_dir_if_empty(void)
 }
 
 /*
- * 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.
+ * Return true if worktree entry should be pruned, along with the reason for
+ * pruning. Otherwise, return false and the worktree's path, or NULL if it
+ * cannot be determined. Caller is responsible for freeing returned path.
  */
-static char *worktree_disposition(const char *id, struct strbuf *reason)
+static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)
 {
 	struct stat st;
 	char *path;
@@ -80,19 +80,22 @@ static char *worktree_disposition(const char *id, struct strbuf *reason)
 	size_t len;
 	ssize_t read_result;
 
+	*wtpath = NULL;
 	if (!is_directory(git_path("worktrees/%s", id))) {
 		strbuf_addstr(reason, _("not a valid directory"));
-		return NULL;
+		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 NULL;
+		return 1;
 	}
 	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 NULL;
+		return 1;
 	}
 	len = xsize_t(st.st_size);
 	path = xmallocz(len);
@@ -103,7 +106,7 @@ static char *worktree_disposition(const char *id, struct strbuf *reason)
 			    strerror(errno));
 		close(fd);
 		free(path);
-		return NULL;
+		return 1;
 	}
 	close(fd);
 
@@ -112,29 +115,29 @@ static char *worktree_disposition(const char *id, struct strbuf *reason)
 			    _("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"),
 			    (uintmax_t)len, (uintmax_t)read_result);
 		free(path);
-		return NULL;
+		return 1;
 	}
 	while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
 		len--;
 	if (!len) {
 		strbuf_addstr(reason, _("invalid gitdir file"));
 		free(path);
-		return NULL;
+		return 1;
 	}
 	path[len] = '\0';
 	if (!file_exists(path)) {
-		if (file_exists(git_path("worktrees/%s/locked", id)))
-			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"));
 			free(path);
-			return NULL;
+			return 1;
 		} else {
-			return path;
+			*wtpath = path;
+			return 0;
 		}
 	}
-	return path;
+	*wtpath = path;
+	return 0;
 }
 
 static void prune_worktree(const char *id, const char *reason)
@@ -153,7 +156,12 @@ 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 */
+	/*
+	 * paths same; prune_dupes() removes all but the first worktree entry
+	 * having the same path, so sort main worktree ('util' is NULL) above
+	 * linked worktrees ('util' not NULL) since main worktree can't be
+	 * removed
+	 */
 	if (!x->util)
 		return -1;
 	if (!y->util)
@@ -176,7 +184,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 strbuf main_path = STRBUF_INIT;
 	struct string_list kept = STRING_LIST_INIT_NODUP;
 	DIR *dir = opendir(git_path("worktrees"));
 	struct dirent *d;
@@ -187,19 +195,17 @@ static void prune_worktrees(void)
 		if (is_dot_or_dotdot(d->d_name))
 			continue;
 		strbuf_reset(&reason);
-		path = worktree_disposition(d->d_name, &reason);
-		if (path) {
+		if (should_prune_worktree(d->d_name, &reason, &path))
+			prune_worktree(d->d_name, reason.buf);
+		else if (path)
 			string_list_append(&kept, path)->util = xstrdup(d->d_name);
-			continue;
-		}
-		prune_worktree(d->d_name, reason.buf);
 	}
 	closedir(dir);
 
-	strbuf_add_absolute_path(&main, get_git_common_dir());
+	strbuf_add_absolute_path(&main_path, 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));
+	strbuf_strip_suffix(&main_path, "/.");
+	string_list_append(&kept, strbuf_detach(&main_path, NULL));
 	prune_dups(&kept);
 	string_list_clear(&kept, 1);
 
diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh
index 5f3db93b31..a6ce7f590b 100755
--- a/t/t2401-worktree-prune.sh
+++ b/t/t2401-worktree-prune.sh
@@ -69,21 +69,11 @@ test_expect_success 'prune directories with gitdir pointing to nowhere' '
 '
 
 test_expect_success 'not prune locked checkout' '
-	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 &&
+	test_when_finished rm -r .git/worktrees &&
 	mkdir -p .git/worktrees/ghi &&
-	: >.git/worktrees/ghi/gitdir &&
 	: >.git/worktrees/ghi/locked &&
 	git worktree prune &&
-	! test -d .git/worktrees/ghi
+	test -d .git/worktrees/ghi
 '
 
 test_expect_success 'not prune recent checkouts' '
diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh
index 7035c9d72e..a4e1a178e0 100755
--- a/t/t2403-worktree-move.sh
+++ b/t/t2403-worktree-move.sh
@@ -113,7 +113,7 @@ test_expect_success 'move locked worktree (force)' '
 '
 
 test_expect_success 'refuse to move worktree atop existing path' '
-	> bobble &&
+	>bobble &&
 	git worktree add --detach beeble &&
 	test_must_fail git worktree move beeble bobble
 '

Range-diff against v1:
1:  75e2f832bf = 1:  75e2f832bf worktree: factor out repeated string literal
2:  24662000d2 < -:  ---------- worktree: prune corrupted worktree even if locked
3:  4fb95b3eea = 2:  0e458b3da5 worktree: give "should be pruned?" function more meaningful name
4:  d16d993aa2 = 3:  7cf5b6ca40 worktree: make high-level pruning re-usable
5:  f6bf2f0e72 ! 4:  e28790761f worktree: prune duplicate entries referencing same worktree path
    @@ builtin/worktree.c: static void delete_worktrees_dir_if_empty(void)
      
     -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.
    ++ * Return true if worktree entry should be pruned, along with the reason for
    ++ * pruning. Otherwise, return false and the worktree's path, or NULL if it
    ++ * cannot be determined. Caller is responsible for freeing returned path.
     + */
    -+static char *worktree_disposition(const char *id, struct strbuf *reason)
    ++static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)
      {
      	struct stat st;
      	char *path;
     @@ builtin/worktree.c: static int should_prune_worktree(const char *id, struct strbuf *reason)
    + 	size_t len;
    + 	ssize_t read_result;
      
    ++	*wtpath = NULL;
      	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);
    -@@ builtin/worktree.c: static int should_prune_worktree(const char *id, struct strbuf *reason)
    - 			    strerror(errno));
    - 		close(fd);
    - 		free(path);
    --		return 1;
    -+		return NULL;
    - 	}
    - 	close(fd);
    - 
    + 		return 1;
     @@ builtin/worktree.c: 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;
    + 			return 1;
      		} else {
    --			return 0;
    -+			return path;
    ++			*wtpath = path;
    + 			return 0;
      		}
      	}
     -	free(path);
    --	return 0;
    -+	return path;
    ++	*wtpath = path;
    + 	return 0;
      }
      
    - static void prune_worktree(const char *id, const char *reason)
     @@ builtin/worktree.c: static void prune_worktree(const char *id, const char *reason)
      		delete_git_dir(id);
      }
    @@ builtin/worktree.c: static void prune_worktree(const char *id, const char *reaso
      			continue;
      		strbuf_reset(&reason);
     -		if (!should_prune_worktree(d->d_name, &reason))
    -+		path = worktree_disposition(d->d_name, &reason);
    -+		if (path) {
    +-			continue;
    +-		prune_worktree(d->d_name, reason.buf);
    ++		if (should_prune_worktree(d->d_name, &reason, &path))
    ++			prune_worktree(d->d_name, reason.buf);
    ++		else if (path)
     +			string_list_append(&kept, path)->util = xstrdup(d->d_name);
    - 			continue;
    -+		}
    - 		prune_worktree(d->d_name, reason.buf);
      	}
      	closedir(dir);
     +
6:  6244cbb689 ! 5:  ded0632001 worktree: prune linked worktree referencing main worktree path
    @@ builtin/worktree.c: 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 */
    ++	/*
    ++	 * paths same; prune_dupes() removes all but the first worktree entry
    ++	 * having the same path, so sort main worktree ('util' is NULL) above
    ++	 * linked worktrees ('util' not NULL) since main worktree can't be
    ++	 * removed
    ++	 */
     +	if (!x->util)
     +		return -1;
     +	if (!y->util)
    @@ builtin/worktree.c: static void prune_dups(struct string_list *l)
      static void prune_worktrees(void)
      {
      	struct strbuf reason = STRBUF_INIT;
    -+	struct strbuf main = STRBUF_INIT;
    ++	struct strbuf main_path = STRBUF_INIT;
      	struct string_list kept = STRING_LIST_INIT_NODUP;
      	DIR *dir = opendir(git_path("worktrees"));
      	struct dirent *d;
    @@ builtin/worktree.c: static void prune_worktrees(void)
      	}
      	closedir(dir);
      
    -+	strbuf_add_absolute_path(&main, get_git_common_dir());
    ++	strbuf_add_absolute_path(&main_path, 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));
    ++	strbuf_strip_suffix(&main_path, "/.");
    ++	string_list_append(&kept, strbuf_detach(&main_path, NULL));
      	prune_dups(&kept);
      	string_list_clear(&kept, 1);
      
7:  1355b373d3 = 6:  2ca210fa73 worktree: generalize candidate worktree path validation
8:  8bfe91bfd2 ! 7:  74dd7d1ac0 worktree: make "move" refuse to move atop missing registered worktree
    @@ Commit message
         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:
    +    the same path, which is expressly forbidden by design. For example:
     
             $ git clone foo.git
             $ cd foo
    @@ t/t2403-worktree-move.sh: test_expect_success 'move locked worktree (force)' '
      '
      
     +test_expect_success 'refuse to move worktree atop existing path' '
    -+	> bobble &&
    ++	>bobble &&
     +	git worktree add --detach beeble &&
     +	test_must_fail git worktree move beeble bobble
     +'
-- 
2.27.0.90.gabb59f83a2

[PATCH v2 1/7] 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.90.gabb59f83a2

[PATCH v2 2/7] 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 9b15f19fc5..35d38607e7 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.90.gabb59f83a2

[PATCH v2 3/7] 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 35d38607e7..706ac68751 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.90.gabb59f83a2

[PATCH v2 4/7] 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        | 49 ++++++++++++++++++++++++++++++++++-----
 t/t2401-worktree-prune.sh | 12 ++++++++++
 2 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 706ac68751..65492752a7 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 true if worktree entry should be pruned, along with the reason for
+ * pruning. Otherwise, return false and the worktree's path, or NULL if it
+ * cannot be determined. Caller is responsible for freeing returned path.
+ */
+static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)
 {
 	struct stat st;
 	char *path;
@@ -75,6 +80,7 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)
 	size_t len;
 	ssize_t read_result;
 
+	*wtpath = NULL;
 	if (!is_directory(git_path("worktrees/%s", id))) {
 		strbuf_addstr(reason, _("not a valid directory"));
 		return 1;
@@ -120,16 +126,17 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)
 	}
 	path[len] = '\0';
 	if (!file_exists(path)) {
-		free(path);
 		if (stat(git_path("worktrees/%s/index", id), &st) ||
 		    st.st_mtime <= expire) {
 			strbuf_addstr(reason, _("gitdir file points to non-existent location"));
+			free(path);
 			return 1;
 		} else {
+			*wtpath = path;
 			return 0;
 		}
 	}
-	free(path);
+	*wtpath = path;
 	return 0;
 }
 
@@ -141,22 +148,52 @@ 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))
-			continue;
-		prune_worktree(d->d_name, reason.buf);
+		if (should_prune_worktree(d->d_name, &reason, &path))
+			prune_worktree(d->d_name, reason.buf);
+		else if (path)
+			string_list_append(&kept, path)->util = xstrdup(d->d_name);
 	}
 	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 b7d6d5d45a..fd3916fee0 100755
--- a/t/t2401-worktree-prune.sh
+++ b/t/t2401-worktree-prune.sh
@@ -92,4 +92,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.90.gabb59f83a2

[PATCH v2 5/7] 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        | 15 +++++++++++++++
 t/t2401-worktree-prune.sh | 12 ++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 65492752a7..350108eba0 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -156,6 +156,16 @@ static int prune_cmp(const void *a, const void *b)
 
 	if ((c = fspathcmp(x->string, y->string)))
 	    return c;
+	/*
+	 * paths same; prune_dupes() removes all but the first worktree entry
+	 * having the same path, so sort main worktree ('util' is NULL) above
+	 * linked worktrees ('util' not NULL) since main worktree can't be
+	 * removed
+	 */
+	if (!x->util)
+		return -1;
+	if (!y->util)
+		return 1;
 	/* paths same; sort by .git/worktrees/<id> */
 	return strcmp(x->util, y->util);
 }
@@ -174,6 +184,7 @@ static void prune_dups(struct string_list *l)
 static void prune_worktrees(void)
 {
 	struct strbuf reason = STRBUF_INIT;
+	struct strbuf main_path = STRBUF_INIT;
 	struct string_list kept = STRING_LIST_INIT_NODUP;
 	DIR *dir = opendir(git_path("worktrees"));
 	struct dirent *d;
@@ -191,6 +202,10 @@ static void prune_worktrees(void)
 	}
 	closedir(dir);
 
+	strbuf_add_absolute_path(&main_path, get_git_common_dir());
+	/* massage main worktree absolute path to match 'gitdir' content */
+	strbuf_strip_suffix(&main_path, "/.");
+	string_list_append(&kept, strbuf_detach(&main_path, NULL));
 	prune_dups(&kept);
 	string_list_clear(&kept, 1);
 
diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh
index fd3916fee0..a6ce7f590b 100755
--- a/t/t2401-worktree-prune.sh
+++ b/t/t2401-worktree-prune.sh
@@ -104,4 +104,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.90.gabb59f83a2

[PATCH v2 6/7] 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 350108eba0..8fcf3f38fb 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -280,34 +280,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,
@@ -324,8 +323,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.90.gabb59f83a2

[PATCH v2 7/7] 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 expressly 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 8fcf3f38fb..1238b6bab1 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -863,8 +863,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..a4e1a178e0 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.90.gabb59f83a2

Re: [PATCH v2 4/7] worktree: prune duplicate entries referencing same worktree path

[Shourya Shukla]

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 706ac68751..65492752a7 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 true if worktree entry should be pruned, along with the reason for
> + * pruning. Otherwise, return false and the worktree's path, or NULL if it
> + * cannot be determined. Caller is responsible for freeing returned path.
> + */
> +static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)
>  {
>  	struct stat st;
>  	char *path;
> @@ -75,6 +80,7 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)
>  	size_t len;
>  	ssize_t read_result;
>  
> +	*wtpath = NULL;
>  	if (!is_directory(git_path("worktrees/%s", id))) {
>  		strbuf_addstr(reason, _("not a valid directory"));
>  		return 1;
> @@ -120,16 +126,17 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)
>  	}
>  	path[len] = '\0';
>  	if (!file_exists(path)) {
> -		free(path);
>  		if (stat(git_path("worktrees/%s/index", id), &st) ||
>  		    st.st_mtime <= expire) {
>  			strbuf_addstr(reason, _("gitdir file points to non-existent location"));
> +			free(path);
>  			return 1;
>  		} else {
> +			*wtpath = path;
>  			return 0;
>  		}
>  	}
> -	free(path);
> +	*wtpath = path;
>  	return 0;
>  }

What exactly is the role of 'wtpath' in here? Maybe this is explained in
the later patches. 

> @@ -141,22 +148,52 @@ 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);
> +}
> 

Can we rename the function arguments better? 'a' and 'b' sound very
naive to me. Maybe change these to something more like: 'firstwt' and
'secondwt'? Yeah even this sounds kiddish but you get the idea right? Or
like 'wt' and 'wt_dup'?

> diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh
> index b7d6d5d45a..fd3916fee0 100755
> --- a/t/t2401-worktree-prune.sh
> +++ b/t/t2401-worktree-prune.sh
> @@ -92,4 +92,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 &&

Nit: maybe make it 'rm -rf' as that's the popular way of doing it?

Otherwise this looks good to me. But again, we need comments from others
too.

Nicely done! :)

Re: [PATCH v2 4/7] worktree: prune duplicate entries referencing same worktree path

[Eric Sunshine]

On Wed, Jun 10, 2020 at 7:50 AM Shourya Shukla
<shouryashukla.oo@gmail.com> wrote:
> > +static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)
>
> What exactly is the role of 'wtpath' in here? Maybe this is explained in
> the later patches.

'wtpath' holds the location of the worktree. It's used in this patch
by prune_worktrees() to collect a list of paths which haven't been
marked for pruning. Once it has the full list, it passes it to
prune_dups() for pruning duplicate entries.

> > +static int prune_cmp(const void *a, const void *b)
>
> Can we rename the function arguments better? 'a' and 'b' sound very
> naive to me. Maybe change these to something more like: 'firstwt' and
> 'secondwt'? Yeah even this sounds kiddish but you get the idea right? Or
> like 'wt' and 'wt_dup'?

As with any language, C has its idioms, as does Git itself. Sticking
to idioms makes it easier for others to understand the code
at-a-glance. Short variable names, such as "i" and "j" for indexes,
"n" for counters, "s" and "t" for strings, are very common among
experienced programmers. Likewise, "a" and "b" are well-understood as
the arguments of a "comparison" function. There are many such examples
in the Git source code itself. Here are just a few:

    cmp_uint32(const void *a_, const void *b_)
    maildir_filename_cmp(const char *a, const char *b)
    tipcmp(const void *a_, const void *b_)
    cmp_by_tag_and_age(const void *a_, const void *b_)
    cmp_remaining_objects(const void *a, const void *b)
    version_cmp(const char *a, const char *b)
    diffnamecmp(const void *a_, const void *b_)
    spanhash_cmp(const void *a_, const void *b_)
    void_hashcmp(const void *a, const void *b)
    pathspec_item_cmp(const void *a_, const void *b_)

> > +test_expect_success 'prune duplicate (linked/linked)' '
> > +   test_when_finished rm -fr .git/worktrees w1 w2 &&
>
> Nit: maybe make it 'rm -rf' as that's the popular way of doing it?

It's true that "-rf" has wider usage in Git tests than "-fr", though
the latter is heavily used, as well. Ordinarily, matching the style
already present in a particular script would be a good way to choose
between the two, however, the worktree-related test scripts which this
series touches already have a mix of the two. This is the first use of
combined "-r" and "-f" in this particular script, so I could go with
the more common "-rf", however, it's not worth re-rolling just for
that.

Thanks for the review.

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

[Shourya Shukla]

On 10/06 02:30, Eric Sunshine wrote:
> "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.

There is an extra 'the' after generalize.

> -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;

Should we do a 'return 1' on failure instead of just a blank 'return' so
that we can denote failure of finding a worktree?

>  	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);

Let's wrap this to 72 characters at maximum per line maybe? Meaning that
the error message gets split into 2 lines.

> -		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);

Similarly here.

>  
>  static int add_worktree(const char *path, const char *refname,
> @@ -324,8 +323,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) &&

It is necessary to call 'free_worktrees(worktrees)' at the end right?
The 'get_worktrees()' function states that
    The caller is responsible for freeing the memory from the returned
    worktree(s).

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

[Eric Sunshine]

On Wed, Jun 10, 2020 at 1:12 PM Shourya Shukla
<shouryashukla.oo@gmail.com> wrote:
> On 10/06 02:30, Eric Sunshine wrote:
> > "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.
>
> There is an extra 'the' after generalize.

Thanks for noticing. I'll fix it if I re-roll, otherwise it can stay
(unless Junio happens to fix it when queuing).

> >    if (!wt)
> > -       goto done;
> > +       return;
>
> Should we do a 'return 1' on failure instead of just a blank 'return' so
> that we can denote failure of finding a worktree?

This function is declared as returning 'void', so we can't "return 1".
The function instead signals a problem by die()'ing.

Changing it to return a success or failure result rather than dying is
a different matter which can be done later if someone wants to do so,
but is outside the scope of this patch series which is only making the
minimal necessary changes to adapt the function for wider use.

> > -       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);
>
> Let's wrap this to 72 characters at maximum per line maybe? Meaning that
> the error message gets split into 2 lines.

I'm not sure what you want to see wrapped; the warning message itself
or the source code line? As for the warning message, it already is
wrapped (see the embedded "\n").

At any rate, this patch makes the minimal change necessary to meet the
goal of making the function re-usable. Anything beyond that (such as
wrapping long lines) is outside the scope of the patch and would make
it harder to reason about the changes. Wrapping the line is certainly
something that someone can do later as a follow-up, but is not the
goal of this series.

> > -   validate_worktree_add(path, opts);
> > +   worktrees = get_worktrees(0);
> > +   check_candidate_path(path, opts->force, worktrees, "add");
> > +   free_worktrees(worktrees);
> > +   worktrees = NULL;
>
> It is necessary to call 'free_worktrees(worktrees)' at the end? The
> 'get_worktrees()' states that
>   The caller is responsible for freeing the memory from the returned
>   worktree(s).

This code _does_ call free_worktrees() as it should, so I'm not sure
what you're asking or saying. (Perhaps you're confusing "caller" and
"callee"?)

Re: [PATCH v2 4/7] worktree: prune duplicate entries referencing same worktree path

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Jun 10, 2020 at 7:50 AM Shourya Shukla
> <shouryashukla.oo@gmail.com> wrote:
>> > +static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)
>>
>> What exactly is the role of 'wtpath' in here? Maybe this is explained in
>> the later patches.
>
> 'wtpath' holds the location of the worktree. It's used in this patch
> by prune_worktrees() to collect a list of paths which haven't been
> marked for pruning. Once it has the full list, it passes it to
> prune_dups() for pruning duplicate entries.

"wt" being a fairly common abbreviation of "work(ing) tree" in the
codebase may escape new readers of the code.  The comment before the
function can explicitly mention the variable name in the description
to help them, I would think.  For example ...

> +/*
> + * Return true if worktree entry should be pruned, along with the reason for
> + * pruning. Otherwise, return false and the worktree's path, or NULL if it

... the first sentence makes it clear that the function returns two
things (i.e. "true"---presumably is returned as its return value, and
"the reason"---the readers are supposed to guess it is stuffed in
the strbuf), and the second sentence also says the function returns
two things (i.e. "false", and "the worktree's path"---it is not
immediately obvious where NULL goes, though).

> + * cannot be determined. Caller is responsible for freeing returned path.
> + */

	Determine if the worktree entry specified by its "id" should
	be pruned.

	When returning 'true', the caller-supplied strbuf "reason"
	is filled with the reason why it should be pruned.  "wtpath"
	is left intact.

	When returning 'false', the string variable pointed by
	"wtpath" receives the absolute path of the worktree; or NULL
	if the location of the worktree cannot be determined.
	"reason" is left intact.

perhaps?  I didn't check what you do to *wtpath when you return
true, so "left intact" may not be what you are doing, and I am *not*
suggesting to leave it intact in that case---I am only suggesting
that we should describe what happens.

>> > +static int prune_cmp(const void *a, const void *b)
>>
>> Can we rename the function arguments better? 'a' and 'b' sound very
>> naive to me. Maybe change these to something more like: 'firstwt' and
>> 'secondwt'? Yeah even this sounds kiddish but you get the idea right? Or
>> like 'wt' and 'wt_dup'?
>
> As with any language, C has its idioms, as does Git itself. Sticking
> to idioms makes it easier for others to understand the code
> at-a-glance. Short variable names, such as "i" and "j" for indexes,
> "n" for counters, "s" and "t" for strings, are very common among
> experienced programmers. Likewise, "a" and "b" are well-understood as
> the arguments of a "comparison" function. There are many such examples
> in the Git source code itself. Here are just a few:
>
>     cmp_uint32(const void *a_, const void *b_)
>     maildir_filename_cmp(const char *a, const char *b)
>     tipcmp(const void *a_, const void *b_)
>     cmp_by_tag_and_age(const void *a_, const void *b_)
>     cmp_remaining_objects(const void *a, const void *b)
>     version_cmp(const char *a, const char *b)
>     diffnamecmp(const void *a_, const void *b_)
>     spanhash_cmp(const void *a_, const void *b_)
>     void_hashcmp(const void *a, const void *b)
>     pathspec_item_cmp(const void *a_, const void *b_)

While that is true, what happens in the funcion is a bit unusual.

> +static int prune_cmp(const void *a, const void *b)
> +{
> +	const struct string_list_item *x = a;
> +	const struct string_list_item *y = b;

Usually we do

	static int foo_cmp(const void *a_, const void *b_)
	{
		const true_type *a = a_;
		const true_type *b = b_;

		/* use a and b for comparison */

without involving 'x' and 'y'.

>> > +test_expect_success 'prune duplicate (linked/linked)' '
>> > +   test_when_finished rm -fr .git/worktrees w1 w2 &&
>>
>> Nit: maybe make it 'rm -rf' as that's the popular way of doing it?
>
> It's true that "-rf" has wider usage in Git tests than "-fr", though
> the latter is heavily used, as well.

I myself write "rm -rf" more often when I am casually programming
out of inertia, but when consciously making a decision on how to
spell the combination, I tend to go alphabetical, because there is
no logical reason to write 'r' first, other than the fact that "-rf"
visually looks prettier than "-fr".  

If recursiveness of the removal is more important than forcedness,
then favouring "-rf" over "-fr" would be justifiable, but I do not
think that is the case here.

And as you said, it is not something that justifies a reroll (or
updating existing uses of "-rf") alone.