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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxx/T/
[2]: https://lore.kernel.org/git/CAPig+cTF+pwBasVCzmucXmMZcm1K0ctkGOavj7bMcGsw2MvoKw@xxxxxxxxxxxxxx/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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux