[PATCH 4/6] worktree move: accept destination as directory

[PATCH 4/6] worktree move: accept destination as directory

[Nguyễn Thái Ngọc Duy]

Similar to "mv a b/", which is actually "mv a b/a", we extract basename
of source worktree and create a directory of the same name at
destination if dst path is a directory.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/worktree.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 0d8b57c..900b68b 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -541,7 +541,13 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	strbuf_addstr(&dst, prefix_filename(prefix,
 					    strlen(prefix),
 					    av[1]));
-	if (file_exists(dst.buf))
+	if (is_directory(dst.buf))
+		/*
+		 * keep going, dst will be appended after we get the
+		 * source's absolute path
+		 */
+		;
+	else if (file_exists(dst.buf))
 		die(_("target '%s' already exists"), av[1]);
 
 	worktrees = get_worktrees(0);
@@ -559,6 +565,17 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	if (validate_worktree(wt, 0))
 		return -1;
 
+	if (is_directory(dst.buf)) {
+		const char *sep = find_last_dir_sep(wt->path);
+
+		if (!sep)
+			die(_("could not figure out destination name from '%s'"),
+			    wt->path);
+		strbuf_addstr(&dst, sep);
+		if (file_exists(dst.buf))
+			die(_("target '%s' already exists"), dst.buf);
+	}
+
 	if (rename(wt->path, dst.buf) == -1)
 		die_errno(_("failed to move '%s' to '%s'"), wt->path, dst.buf);
 
-- 
2.8.2.524.g6ff3d78

[PATCH 0/6] nd/worktree-move update

[Nguyễn Thái Ngọc Duy]

This

 - squashes in Johannes' fix (that's already on jch/nd/worktree-move)
 - fixes the compile problem on latest master (because prefix_filename
   takes one argument less)
 - fixes the test failure because real_path() is called twice (the
   first one hidden in read_gitfile_gently) but the output is not
   duplicated.

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 74fc8578fc..b5afba1646 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -561,9 +561,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	if (ac != 2)
 		usage_with_options(worktree_usage, options);
 
-	strbuf_addstr(&dst, prefix_filename(prefix,
-					    strlen(prefix),
-					    av[1]));
+	strbuf_addstr(&dst, prefix_filename(prefix, av[1]));
 	if (is_directory(dst.buf))
 		/*
 		 * keep going, dst will be appended after we get the
diff --git a/worktree.c b/worktree.c
index 85bf481cec..c695dcf982 100644
--- a/worktree.c
+++ b/worktree.c
@@ -311,8 +311,8 @@ static int report(int quiet, const char *fmt, ...)
 int validate_worktree(const struct worktree *wt, int quiet)
 {
 	struct strbuf sb = STRBUF_INIT;
-	const char *path;
-	int err;
+	char *path;
+	int err, ret;
 
 	if (is_main_worktree(wt)) {
 		/*
@@ -344,14 +344,17 @@ int validate_worktree(const struct worktree *wt, int quiet)
 		return report(quiet, _("'%s/.git' does not exist"), wt->path);
 	}
 
-	path = read_gitfile_gently(sb.buf, &err);
+	path = xstrdup_or_null(read_gitfile_gently(sb.buf, &err));
 	strbuf_release(&sb);
 	if (!path)
 		return report(quiet, _("'%s/.git' is not a .git file, error code %d"),
 			      wt->path, err);
 
-	if (fspathcmp(path, real_path(git_common_path("worktrees/%s", wt->id))))
-		return report(quiet, _("'%s' does not point back to"),
+	ret = fspathcmp(path, real_path(git_common_path("worktrees/%s", wt->id)));
+	free(path);
+
+	if (ret)
+		return report(quiet, _("'%s' does not point back to '%s'"),
 			      wt->path, git_common_path("worktrees/%s", wt->id));
 
 	return 0;

Nguyễn Thái Ngọc Duy (6):
  worktree.c: add validate_worktree()
  worktree.c: add update_worktree_location()
  worktree move: new command
  worktree move: accept destination as directory
  worktree move: refuse to move worktrees with submodules
  worktree remove: new command

 Documentation/git-worktree.txt         |  28 +++---
 builtin/worktree.c                     | 160 +++++++++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |   5 +-
 t/t2028-worktree-move.sh               |  57 ++++++++++++
 worktree.c                             |  87 ++++++++++++++++++
 worktree.h                             |  11 +++
 6 files changed, 337 insertions(+), 11 deletions(-)

-- 
2.11.0.157.gd943d85

[PATCH 1/6] worktree.c: add validate_worktree()

[Nguyễn Thái Ngọc Duy]

This function is later used by "worktree move" and "worktree remove"
to ensure that we have a good connection between the repository and
the worktree. For example, if a worktree is moved manually, the
worktree location recorded in $GIT_DIR/worktrees/.../gitdir is
incorrect and we should not move that one.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 worktree.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 worktree.h |  5 +++++
 2 files changed, 71 insertions(+)

diff --git a/worktree.c b/worktree.c
index bae787cf8d..40cc031ac9 100644
--- a/worktree.c
+++ b/worktree.c
@@ -294,6 +294,72 @@ const char *is_worktree_locked(struct worktree *wt)
 	return wt->lock_reason;
 }
 
+static int report(int quiet, const char *fmt, ...)
+{
+	va_list params;
+
+	if (quiet)
+		return -1;
+
+	va_start(params, fmt);
+	vfprintf(stderr, fmt, params);
+	fputc('\n', stderr);
+	va_end(params);
+	return -1;
+}
+
+int validate_worktree(const struct worktree *wt, int quiet)
+{
+	struct strbuf sb = STRBUF_INIT;
+	char *path;
+	int err, ret;
+
+	if (is_main_worktree(wt)) {
+		/*
+		 * Main worktree using .git file to point to the
+		 * repository would make it impossible to know where
+		 * the actual worktree is if this function is executed
+		 * from another worktree. No .git file support for now.
+		 */
+		strbuf_addf(&sb, "%s/.git", wt->path);
+		if (!is_directory(sb.buf)) {
+			strbuf_release(&sb);
+			return report(quiet, _("'%s/.git' at main worktree is not the repository directory"),
+				      wt->path);
+		}
+		return 0;
+	}
+
+	/*
+	 * Make sure "gitdir" file points to a real .git file and that
+	 * file points back here.
+	 */
+	if (!is_absolute_path(wt->path))
+		return report(quiet, _("'%s' file does not contain absolute path to the worktree location"),
+			      git_common_path("worktrees/%s/gitdir", wt->id));
+
+	strbuf_addf(&sb, "%s/.git", wt->path);
+	if (!file_exists(sb.buf)) {
+		strbuf_release(&sb);
+		return report(quiet, _("'%s/.git' does not exist"), wt->path);
+	}
+
+	path = xstrdup_or_null(read_gitfile_gently(sb.buf, &err));
+	strbuf_release(&sb);
+	if (!path)
+		return report(quiet, _("'%s/.git' is not a .git file, error code %d"),
+			      wt->path, err);
+
+	ret = fspathcmp(path, real_path(git_common_path("worktrees/%s", wt->id)));
+	free(path);
+
+	if (ret)
+		return report(quiet, _("'%s' does not point back to '%s'"),
+			      wt->path, git_common_path("worktrees/%s", wt->id));
+
+	return 0;
+}
+
 int is_worktree_being_rebased(const struct worktree *wt,
 			      const char *target)
 {
diff --git a/worktree.h b/worktree.h
index 6bfb985203..33f7405e33 100644
--- a/worktree.h
+++ b/worktree.h
@@ -58,6 +58,11 @@ extern int is_main_worktree(const struct worktree *wt);
 extern const char *is_worktree_locked(struct worktree *wt);
 
 /*
+ * Return zero if the worktree is in good condition.
+ */
+extern int validate_worktree(const struct worktree *wt, int quiet);
+
+/*
  * Free up the memory for worktree(s)
  */
 extern void free_worktrees(struct worktree **);
-- 
2.11.0.157.gd943d85

[PATCH 2/6] worktree.c: add update_worktree_location()

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 worktree.c | 21 +++++++++++++++++++++
 worktree.h |  6 ++++++
 2 files changed, 27 insertions(+)

diff --git a/worktree.c b/worktree.c
index 40cc031ac9..c695dcf982 100644
--- a/worktree.c
+++ b/worktree.c
@@ -360,6 +360,27 @@ int validate_worktree(const struct worktree *wt, int quiet)
 	return 0;
 }
 
+int update_worktree_location(struct worktree *wt, const char *path_)
+{
+	struct strbuf path = STRBUF_INIT;
+	int ret = 0;
+
+	if (is_main_worktree(wt))
+		return 0;
+
+	strbuf_add_absolute_path(&path, path_);
+	if (fspathcmp(wt->path, path.buf)) {
+		write_file(git_common_path("worktrees/%s/gitdir",
+					   wt->id),
+			   "%s/.git", real_path(path.buf));
+		free(wt->path);
+		wt->path = strbuf_detach(&path, NULL);
+		ret = 0;
+	}
+	strbuf_release(&path);
+	return ret;
+}
+
 int is_worktree_being_rebased(const struct worktree *wt,
 			      const char *target)
 {
diff --git a/worktree.h b/worktree.h
index 33f7405e33..b896bdec55 100644
--- a/worktree.h
+++ b/worktree.h
@@ -63,6 +63,12 @@ extern const char *is_worktree_locked(struct worktree *wt);
 extern int validate_worktree(const struct worktree *wt, int quiet);
 
 /*
+ * Update worktrees/xxx/gitdir with the new path.
+ */
+extern int update_worktree_location(struct worktree *wt,
+				    const char *path_);
+
+/*
  * Free up the memory for worktree(s)
  */
 extern void free_worktrees(struct worktree **);
-- 
2.11.0.157.gd943d85

[PATCH 3/6] worktree move: new command

[Nguyễn Thái Ngọc Duy]

There are two options to move the main worktree, but both have
complications, so it's not implemented yet. Anyway the options are:

 - convert the main worktree to a linked one and move it away, leave the
   git repository where it is. The repo essentially becomes bare after
   this move.

 - move the repository with the main worktree. The tricky part is make
   sure all file descriptors to the repository are closed, or it may
   fail on Windows.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-worktree.txt         |  7 +++++-
 builtin/worktree.c                     | 41 ++++++++++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |  2 +-
 t/t2028-worktree-move.sh               | 31 +++++++++++++++++++++++++
 4 files changed, 79 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 553cf8413f..b47a3247bb 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -12,6 +12,7 @@ SYNOPSIS
 'git worktree add' [-f] [--detach] [--checkout] [-b <new-branch>] <path> [<branch>]
 'git worktree list' [--porcelain]
 'git worktree lock' [--reason <string>] <worktree>
+'git worktree move' <worktree> <new-path>
 'git worktree prune' [-n] [-v] [--expire <expire>]
 'git worktree unlock' <worktree>
 
@@ -71,6 +72,11 @@ files from being pruned automatically. This also prevents it from
 being moved or deleted. Optionally, specify a reason for the lock
 with `--reason`.
 
+move::
+
+Move a working tree to a new location. Note that the main working tree
+cannot be moved yet.
+
 prune::
 
 Prune working tree information in $GIT_DIR/worktrees.
@@ -252,7 +258,6 @@ performed manually, such as:
 
 - `remove` to remove a linked working tree and its administrative files (and
   warn if the working tree is dirty)
-- `mv` to move or rename a working tree and update its administrative files
 
 GIT
 ---
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9993ded41a..e3fbfe2a71 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -15,6 +15,7 @@ static const char * const worktree_usage[] = {
 	N_("git worktree add [<options>] <path> [<branch>]"),
 	N_("git worktree list [<options>]"),
 	N_("git worktree lock [<options>] <path>"),
+	N_("git worktree move <worktree> <new-path>"),
 	N_("git worktree prune [<options>]"),
 	N_("git worktree unlock <path>"),
 	NULL
@@ -525,6 +526,44 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
 	return ret;
 }
 
+static int move_worktree(int ac, const char **av, const char *prefix)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+	struct worktree **worktrees, *wt;
+	struct strbuf dst = STRBUF_INIT;
+	const char *reason;
+
+	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+	if (ac != 2)
+		usage_with_options(worktree_usage, options);
+
+	strbuf_addstr(&dst, prefix_filename(prefix, av[1]));
+	if (file_exists(dst.buf))
+		die(_("target '%s' already exists"), av[1]);
+
+	worktrees = get_worktrees(0);
+	wt = find_worktree(worktrees, prefix, av[0]);
+	if (!wt)
+		die(_("'%s' is not a working directory"), av[0]);
+	if (is_main_worktree(wt))
+		die(_("'%s' is a main working directory"), av[0]);
+	reason = is_worktree_locked(wt);
+	if (reason) {
+		if (*reason)
+			die(_("already locked, reason: %s"), reason);
+		die(_("already locked, no reason"));
+	}
+	if (validate_worktree(wt, 0))
+		return -1;
+
+	if (rename(wt->path, dst.buf) == -1)
+		die_errno(_("failed to move '%s' to '%s'"), wt->path, dst.buf);
+
+	return update_worktree_location(wt, dst.buf);
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
 	struct option options[] = {
@@ -547,5 +586,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
 		return lock_worktree(ac - 1, av + 1, prefix);
 	if (!strcmp(av[1], "unlock"))
 		return unlock_worktree(ac - 1, av + 1, prefix);
+	if (!strcmp(av[1], "move"))
+		return move_worktree(ac - 1, av + 1, prefix);
 	usage_with_options(worktree_usage, options);
 }
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1150164d5c..651809c52f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3036,7 +3036,7 @@ _git_whatchanged ()
 
 _git_worktree ()
 {
-	local subcommands="add list lock prune unlock"
+	local subcommands="add list lock move prune unlock"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
 	if [ -z "$subcommand" ]; then
 		__gitcomp "$subcommands"
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 8298aaf97f..bef420a086 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -59,4 +59,35 @@ test_expect_success 'unlock worktree twice' '
 	test_path_is_missing .git/worktrees/source/locked
 '
 
+test_expect_success 'move non-worktree' '
+	mkdir abc &&
+	test_must_fail git worktree move abc def
+'
+
+test_expect_success 'move locked worktree' '
+	git worktree lock source &&
+	test_must_fail git worktree move source destination &&
+	git worktree unlock source
+'
+
+test_expect_success 'move worktree' '
+	toplevel="$(pwd)" &&
+	git worktree move source destination &&
+	test_path_is_missing source &&
+	git worktree list --porcelain | grep "^worktree" >actual &&
+	cat <<-EOF >expected &&
+	worktree $toplevel
+	worktree $toplevel/destination
+	worktree $toplevel/elsewhere
+	EOF
+	test_cmp expected actual &&
+	git -C destination log --format=%s >actual2 &&
+	echo init >expected2 &&
+	test_cmp expected2 actual2
+'
+
+test_expect_success 'move main worktree' '
+	test_must_fail git worktree move . def
+'
+
 test_done
-- 
2.11.0.157.gd943d85

[PATCH 4/6] worktree move: accept destination as directory

[Nguyễn Thái Ngọc Duy]

Similar to "mv a b/", which is actually "mv a b/a", we extract basename
of source worktree and create a directory of the same name at
destination if dst path is a directory.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/worktree.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index e3fbfe2a71..116507e47e 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -540,7 +540,13 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 		usage_with_options(worktree_usage, options);
 
 	strbuf_addstr(&dst, prefix_filename(prefix, av[1]));
-	if (file_exists(dst.buf))
+	if (is_directory(dst.buf))
+		/*
+		 * keep going, dst will be appended after we get the
+		 * source's absolute path
+		 */
+		;
+	else if (file_exists(dst.buf))
 		die(_("target '%s' already exists"), av[1]);
 
 	worktrees = get_worktrees(0);
@@ -558,6 +564,17 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	if (validate_worktree(wt, 0))
 		return -1;
 
+	if (is_directory(dst.buf)) {
+		const char *sep = find_last_dir_sep(wt->path);
+
+		if (!sep)
+			die(_("could not figure out destination name from '%s'"),
+			    wt->path);
+		strbuf_addstr(&dst, sep);
+		if (file_exists(dst.buf))
+			die(_("target '%s' already exists"), dst.buf);
+	}
+
 	if (rename(wt->path, dst.buf) == -1)
 		die_errno(_("failed to move '%s' to '%s'"), wt->path, dst.buf);
 
-- 
2.11.0.157.gd943d85

[PATCH 5/6] worktree move: refuse to move worktrees with submodules

[Nguyễn Thái Ngọc Duy]

Submodules contains .git files with relative paths. After a worktree
move, these files need to be updated or they may point to nowhere.

This is a bandage patch to make sure "worktree move" don't break
people's worktrees by accident. When .git file update code is in
place, this validate_no_submodules() could be removed.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/worktree.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 116507e47e..825b3e9d9a 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -526,6 +526,27 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
 	return ret;
 }
 
+static void validate_no_submodules(const struct worktree *wt)
+{
+	struct index_state istate = {0};
+	int i, found_submodules = 0;
+
+	if (read_index_from(&istate, worktree_git_path(wt, "index")) > 0) {
+		for (i = 0; i < istate.cache_nr; i++) {
+			struct cache_entry *ce = istate.cache[i];
+
+			if (S_ISGITLINK(ce->ce_mode)) {
+				found_submodules = 1;
+				break;
+			}
+		}
+	}
+	discard_index(&istate);
+
+	if (found_submodules)
+		die(_("This working tree contains submodules and cannot be moved yet"));
+}
+
 static int move_worktree(int ac, const char **av, const char *prefix)
 {
 	struct option options[] = {
@@ -564,6 +585,8 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	if (validate_worktree(wt, 0))
 		return -1;
 
+	validate_no_submodules(wt);
+
 	if (is_directory(dst.buf)) {
 		const char *sep = find_last_dir_sep(wt->path);
 
-- 
2.11.0.157.gd943d85

[PATCH 6/6] worktree remove: new command

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-worktree.txt         | 21 +++++----
 builtin/worktree.c                     | 79 ++++++++++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |  5 ++-
 t/t2028-worktree-move.sh               | 26 +++++++++++
 4 files changed, 121 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index b47a3247bb..020eeb157f 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -14,6 +14,7 @@ SYNOPSIS
 'git worktree lock' [--reason <string>] <worktree>
 'git worktree move' <worktree> <new-path>
 'git worktree prune' [-n] [-v] [--expire <expire>]
+'git worktree remove' [--force] <worktree>
 'git worktree unlock' <worktree>
 
 DESCRIPTION
@@ -81,6 +82,13 @@ prune::
 
 Prune working tree information in $GIT_DIR/worktrees.
 
+remove::
+
+Remove a working tree. Only clean working trees (no untracked files
+and no modification in tracked files) can be removed. Unclean working
+trees can be removed with `--force`. The main working tree cannot be
+removed.
+
 unlock::
 
 Unlock a working tree, allowing it to be pruned, moved or deleted.
@@ -90,9 +98,10 @@ OPTIONS
 
 -f::
 --force::
-	By default, `add` refuses to create a new working tree when `<branch>`
-	is already checked out by another working tree. This option overrides
-	that safeguard.
+	By default, `add` refuses to create a new working tree when
+	`<branch>` is already checked out by another working tree and
+	`remove` refuses to remove an unclean working tree. This option
+	overrides that safeguard.
 
 -b <new-branch>::
 -B <new-branch>::
@@ -253,12 +262,6 @@ Multiple checkout in general is still experimental, and the support
 for submodules is incomplete. It is NOT recommended to make multiple
 checkouts of a superproject.
 
-git-worktree could provide more automation for tasks currently
-performed manually, such as:
-
-- `remove` to remove a linked working tree and its administrative files (and
-  warn if the working tree is dirty)
-
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 825b3e9d9a..b5afba1646 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -17,6 +17,7 @@ static const char * const worktree_usage[] = {
 	N_("git worktree lock [<options>] <path>"),
 	N_("git worktree move <worktree> <new-path>"),
 	N_("git worktree prune [<options>]"),
+	N_("git worktree remove [<options>] <worktree>"),
 	N_("git worktree unlock <path>"),
 	NULL
 };
@@ -604,6 +605,82 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	return update_worktree_location(wt, dst.buf);
 }
 
+static int remove_worktree(int ac, const char **av, const char *prefix)
+{
+	int force = 0;
+	struct option options[] = {
+		OPT_BOOL(0, "force", &force,
+			 N_("force removing even if the worktree is dirty")),
+		OPT_END()
+	};
+	struct worktree **worktrees, *wt;
+	struct strbuf sb = STRBUF_INIT;
+	const char *reason;
+	int ret = 0;
+
+	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+	if (ac != 1)
+		usage_with_options(worktree_usage, options);
+
+	worktrees = get_worktrees(0);
+	wt = find_worktree(worktrees, prefix, av[0]);
+	if (!wt)
+		die(_("'%s' is not a working directory"), av[0]);
+	if (is_main_worktree(wt))
+		die(_("'%s' is a main working directory"), av[0]);
+	reason = is_worktree_locked(wt);
+	if (reason) {
+		if (*reason)
+			die(_("already locked, reason: %s"), reason);
+		die(_("already locked, no reason"));
+	}
+	if (validate_worktree(wt, 0))
+		return -1;
+
+	if (!force) {
+		struct argv_array child_env = ARGV_ARRAY_INIT;
+		struct child_process cp;
+		char buf[1];
+
+		argv_array_pushf(&child_env, "%s=%s/.git",
+				 GIT_DIR_ENVIRONMENT, wt->path);
+		argv_array_pushf(&child_env, "%s=%s",
+				 GIT_WORK_TREE_ENVIRONMENT, wt->path);
+		memset(&cp, 0, sizeof(cp));
+		argv_array_pushl(&cp.args, "status", "--porcelain", NULL);
+		cp.env = child_env.argv;
+		cp.git_cmd = 1;
+		cp.dir = wt->path;
+		cp.out = -1;
+		ret = start_command(&cp);
+		if (ret)
+			die_errno(_("failed to run git-status on '%s', code %d"),
+				  av[0], ret);
+		ret = xread(cp.out, buf, sizeof(buf));
+		if (ret)
+			die(_("'%s' is dirty, use --force to delete it"), av[0]);
+		close(cp.out);
+		ret = finish_command(&cp);
+		if (ret)
+			die_errno(_("failed to run git-status on '%s', code %d"),
+				  av[0], ret);
+	}
+	strbuf_addstr(&sb, wt->path);
+	if (remove_dir_recursively(&sb, 0)) {
+		error_errno(_("failed to delete '%s'"), sb.buf);
+		ret = -1;
+	}
+	strbuf_reset(&sb);
+	strbuf_addstr(&sb, git_common_path("worktrees/%s", wt->id));
+	if (remove_dir_recursively(&sb, 0)) {
+		error_errno(_("failed to delete '%s'"), sb.buf);
+		ret = -1;
+	}
+	strbuf_release(&sb);
+	free_worktrees(worktrees);
+	return ret;
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
 	struct option options[] = {
@@ -628,5 +705,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
 		return unlock_worktree(ac - 1, av + 1, prefix);
 	if (!strcmp(av[1], "move"))
 		return move_worktree(ac - 1, av + 1, prefix);
+	if (!strcmp(av[1], "remove"))
+		return remove_worktree(ac - 1, av + 1, prefix);
 	usage_with_options(worktree_usage, options);
 }
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 651809c52f..eb8cb7ecee 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3036,7 +3036,7 @@ _git_whatchanged ()
 
 _git_worktree ()
 {
-	local subcommands="add list lock move prune unlock"
+	local subcommands="add list lock move prune remove unlock"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
 	if [ -z "$subcommand" ]; then
 		__gitcomp "$subcommands"
@@ -3054,6 +3054,9 @@ _git_worktree ()
 		prune,--*)
 			__gitcomp "--dry-run --expire --verbose"
 			;;
+		remove,--*)
+			__gitcomp "--force"
+			;;
 		*)
 			;;
 		esac
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index bef420a086..b3105eaaed 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -90,4 +90,30 @@ test_expect_success 'move main worktree' '
 	test_must_fail git worktree move . def
 '
 
+test_expect_success 'remove main worktree' '
+	test_must_fail git worktree remove .
+'
+
+test_expect_success 'remove locked worktree' '
+	git worktree lock destination &&
+	test_must_fail git worktree remove destination &&
+	git worktree unlock destination
+'
+
+test_expect_success 'remove worktree with dirty tracked file' '
+	echo dirty >>destination/init.t &&
+	test_must_fail git worktree remove destination
+'
+
+test_expect_success 'remove worktree with untracked file' '
+	git -C destination checkout init.t &&
+	: >destination/untracked &&
+	test_must_fail git worktree remove destination
+'
+
+test_expect_success 'force remove worktree with untracked file' '
+	git worktree remove --force destination &&
+	test_path_is_missing destination
+'
+
 test_done
-- 
2.11.0.157.gd943d85

Re: [PATCH 1/6] worktree.c: add validate_worktree()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> This function is later used by "worktree move" and "worktree remove"
> to ensure that we have a good connection between the repository and
> the worktree. For example, if a worktree is moved manually, the
> worktree location recorded in $GIT_DIR/worktrees/.../gitdir is
> incorrect and we should not move that one.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  worktree.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  worktree.h |  5 +++++
>  2 files changed, 71 insertions(+)
>
> diff --git a/worktree.c b/worktree.c
> index bae787cf8d..40cc031ac9 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -294,6 +294,72 @@ const char *is_worktree_locked(struct worktree *wt)
>  	return wt->lock_reason;
>  }
>  
> +static int report(int quiet, const char *fmt, ...)
> +{
> +	va_list params;
> +
> +	if (quiet)
> +		return -1;
> +
> +	va_start(params, fmt);
> +	vfprintf(stderr, fmt, params);
> +	fputc('\n', stderr);
> +	va_end(params);
> +	return -1;
> +}
> +
> +int validate_worktree(const struct worktree *wt, int quiet)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	char *path;
> +	int err, ret;
> +
> +	if (is_main_worktree(wt)) {
> +		/*
> +		 * Main worktree using .git file to point to the
> +		 * repository would make it impossible to know where
> +		 * the actual worktree is if this function is executed
> +		 * from another worktree. No .git file support for now.
> +		 */
> +		strbuf_addf(&sb, "%s/.git", wt->path);
> +		if (!is_directory(sb.buf)) {
> +			strbuf_release(&sb);
> +			return report(quiet, _("'%s/.git' at main worktree is not the repository directory"),
> +				      wt->path);

These messages come without telling what they are.  Should they say
that these are errors?  Or does the severity depend on the caller,
i.e. why they want to know if a particular worktree is valid, and
sometimes these are errors and other times they are mere warnings?

> +		}
> +		return 0;
> +	}
> +
> +	/*
> +	 * Make sure "gitdir" file points to a real .git file and that
> +	 * file points back here.
> +	 */
> +	if (!is_absolute_path(wt->path))
> +		return report(quiet, _("'%s' file does not contain absolute path to the worktree location"),
> +			      git_common_path("worktrees/%s/gitdir", wt->id));

It makes me wonder if this kind of error reporting belongs to the
place where these are read (and a new wt is written out to the
filesystem, perhaps).  The programmer who wrote this code may have
known that wt->path is prepared by reading "worktrees/%s/gitdir" and
without doing real_path() or absolute_path() on the result when this
code was written, but nothing guarantees that to stay true over time
as the code evolves.

> +	strbuf_addf(&sb, "%s/.git", wt->path);
> +	if (!file_exists(sb.buf)) {
> +		strbuf_release(&sb);
> +		return report(quiet, _("'%s/.git' does not exist"), wt->path);
> +	}
> +
> +	path = xstrdup_or_null(read_gitfile_gently(sb.buf, &err));
> +	strbuf_release(&sb);
> +	if (!path)
> +		return report(quiet, _("'%s/.git' is not a .git file, error code %d"),
> +			      wt->path, err);

The above should do, at least for now.  It is unfortunate that no
existing code needs to turn READ_GITFILE_ERR_* into human readble
error message.

> +	ret = fspathcmp(path, real_path(git_common_path("worktrees/%s", wt->id)));
> +	free(path);
> +
> +	if (ret)
> +		return report(quiet, _("'%s' does not point back to '%s'"),
> +			      wt->path, git_common_path("worktrees/%s", wt->id));
> +
> +	return 0;
> +}
> +
>  int is_worktree_being_rebased(const struct worktree *wt,
>  			      const char *target)
>  {
> diff --git a/worktree.h b/worktree.h
> index 6bfb985203..33f7405e33 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -58,6 +58,11 @@ extern int is_main_worktree(const struct worktree *wt);
>  extern const char *is_worktree_locked(struct worktree *wt);
>  
>  /*
> + * Return zero if the worktree is in good condition.
> + */
> +extern int validate_worktree(const struct worktree *wt, int quiet);
> +
> +/*
>   * Free up the memory for worktree(s)
>   */
>  extern void free_worktrees(struct worktree **);

Re: [PATCH 2/6] worktree.c: add update_worktree_location()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  worktree.c | 21 +++++++++++++++++++++
>  worktree.h |  6 ++++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/worktree.c b/worktree.c
> index 40cc031ac9..c695dcf982 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -360,6 +360,27 @@ int validate_worktree(const struct worktree *wt, int quiet)
>  	return 0;
>  }
>  
> +int update_worktree_location(struct worktree *wt, const char *path_)
> +{
> +	struct strbuf path = STRBUF_INIT;
> +	int ret = 0;
> +
> +	if (is_main_worktree(wt))
> +		return 0;

There is no "refusing to move main worktree location" error message
issued?  Perhaps it is done elsewhere and this is merely for an
added safety (in which case it is OK)?

> +
> +	strbuf_add_absolute_path(&path, path_);
> +	if (fspathcmp(wt->path, path.buf)) {
> +		write_file(git_common_path("worktrees/%s/gitdir",
> +					   wt->id),
> +			   "%s/.git", real_path(path.buf));
> +		free(wt->path);
> +		wt->path = strbuf_detach(&path, NULL);
> +		ret = 0;
> +	}
> +	strbuf_release(&path);
> +	return ret;
> +}

Does anybody set "ret" to anything but 0 in this function?  It is
unclear what the return value from this function means in the first
place, but I am assuming that this is meant to follow the usual
convention of 0 for success and negative for error, in which case
the "return early if this is the primary one" would want to return
-1, I guess.

>  int is_worktree_being_rebased(const struct worktree *wt,
>  			      const char *target)
>  {
> diff --git a/worktree.h b/worktree.h
> index 33f7405e33..b896bdec55 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -63,6 +63,12 @@ extern const char *is_worktree_locked(struct worktree *wt);
>  extern int validate_worktree(const struct worktree *wt, int quiet);
>  
>  /*
> + * Update worktrees/xxx/gitdir with the new path.
> + */
> +extern int update_worktree_location(struct worktree *wt,
> +				    const char *path_);
> +
> +/*
>   * Free up the memory for worktree(s)
>   */
>  extern void free_worktrees(struct worktree **);

Re: [PATCH 3/6] worktree move: new command

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> There are two options to move the main worktree, but both have
> complications, so it's not implemented yet. Anyway the options are:
>
>  - convert the main worktree to a linked one and move it away, leave the
>    git repository where it is. The repo essentially becomes bare after
>    this move.
>
>  - move the repository with the main worktree. The tricky part is make
>    sure all file descriptors to the repository are closed, or it may
>    fail on Windows.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/git-worktree.txt         |  7 +++++-
>  builtin/worktree.c                     | 41 ++++++++++++++++++++++++++++++++++
>  contrib/completion/git-completion.bash |  2 +-
>  t/t2028-worktree-move.sh               | 31 +++++++++++++++++++++++++
>  4 files changed, 79 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 553cf8413f..b47a3247bb 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -12,6 +12,7 @@ SYNOPSIS
>  'git worktree add' [-f] [--detach] [--checkout] [-b <new-branch>] <path> [<branch>]
>  'git worktree list' [--porcelain]
>  'git worktree lock' [--reason <string>] <worktree>
> +'git worktree move' <worktree> <new-path>
>  'git worktree prune' [-n] [-v] [--expire <expire>]
>  'git worktree unlock' <worktree>
>  
> @@ -71,6 +72,11 @@ files from being pruned automatically. This also prevents it from
>  being moved or deleted. Optionally, specify a reason for the lock
>  with `--reason`.
>  
> +move::
> +
> +Move a working tree to a new location. Note that the main working tree
> +cannot be moved yet.
> +

You do not need to say "yet" here.  It may come, or it may never
come, and it does not matter to the readers an iota when they read
this.  The only thing that matters to them that they need to know is
that they cannot move the primary one.

> +static int move_worktree(int ac, const char **av, const char *prefix)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	struct worktree **worktrees, *wt;
> +	struct strbuf dst = STRBUF_INIT;
> +	const char *reason;
> +
> +	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
> +	if (ac != 2)
> +		usage_with_options(worktree_usage, options);
> +
> +	strbuf_addstr(&dst, prefix_filename(prefix, av[1]));
> +	if (file_exists(dst.buf))
> +		die(_("target '%s' already exists"), av[1]);
> +
> +	worktrees = get_worktrees(0);
> +	wt = find_worktree(worktrees, prefix, av[0]);
> +	if (!wt)
> +		die(_("'%s' is not a working directory"), av[0]);
> +	if (is_main_worktree(wt))
> +		die(_("'%s' is a main working directory"), av[0]);

s/directory/tree/ perhaps, as Documentation/git-worktree.txt
advertises these as "working trees"?

The user _may_ be well aware that av[0] is the primary one, and this
message would solicit a "Huh--so what?" from such a user, unless it
says that moving the primary one is not supported.

> +	reason = is_worktree_locked(wt);
> +	if (reason) {
> +		if (*reason)
> +			die(_("already locked, reason: %s"), reason);

Good.

> +		die(_("already locked, no reason"));

I would suggest s/, no reason// here.  To somebody who reads these
two lines of calls to die(), it is clear what you wanted to mean by
that (i.e. we would have given the reason string if it were
avalable, but there isn't, so we are stressing the fact that we got
nothing), but to an end user who only sees the latter, without
necessarily knowing that some other times the former message may
have been given, it is confusing.

For that matter, I am not sure "already" is a good phrase to use
here.  It's not like the end-user is asking to lock the worktree.
If we refuse to move a locked worktree, perhaps we should say so.
i.e. "cannot move a locked working tree (reason for locking: %s)"
or something.

Re: [PATCH 4/6] worktree move: accept destination as directory

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Similar to "mv a b/", which is actually "mv a b/a", we extract basename
> of source worktree and create a directory of the same name at
> destination if dst path is a directory.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/worktree.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index e3fbfe2a71..116507e47e 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -540,7 +540,13 @@ static int move_worktree(int ac, const char **av, const char *prefix)
>  		usage_with_options(worktree_usage, options);
>  
>  	strbuf_addstr(&dst, prefix_filename(prefix, av[1]));
> -	if (file_exists(dst.buf))
> +	if (is_directory(dst.buf))
> +		/*
> +		 * keep going, dst will be appended after we get the
> +		 * source's absolute path
> +		 */
> +		;

Ugly.  Why not do that "infer the real destination b/a when existing
directory b/ was given" here, without "else if" so that we can catch
"hey, b/a exists already" with the existing code?  That way you do
not have to do is_directory() twice.

For that matter, perhaps we should go back to 3/6 and move the "if
dst.buf exists error out" to after wt is validated.  That would make
it stand out why having these is_directory() on the same thing twice
is ugly, I would think.

> +	else if (file_exists(dst.buf))
>  		die(_("target '%s' already exists"), av[1]);
>  
>  	worktrees = get_worktrees(0);
> @@ -558,6 +564,17 @@ static int move_worktree(int ac, const char **av, const char *prefix)
>  	if (validate_worktree(wt, 0))
>  		return -1;
>  
> +	if (is_directory(dst.buf)) {
> +		const char *sep = find_last_dir_sep(wt->path);
> +
> +		if (!sep)
> +			die(_("could not figure out destination name from '%s'"),
> +			    wt->path);
> +		strbuf_addstr(&dst, sep);
> +		if (file_exists(dst.buf))
> +			die(_("target '%s' already exists"), dst.buf);
> +	}
> +
>  	if (rename(wt->path, dst.buf) == -1)
>  		die_errno(_("failed to move '%s' to '%s'"), wt->path, dst.buf);

Re: [PATCH 5/6] worktree move: refuse to move worktrees with submodules

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Submodules contains .git files with relative paths. After a worktree
> move, these files need to be updated or they may point to nowhere.
>
> This is a bandage patch to make sure "worktree move" don't break
> people's worktrees by accident. When .git file update code is in
> place, this validate_no_submodules() could be removed.

Right.  I am OK with "small steps first" approach like this.  We'd
need to document the limitation, though, i.e. "Note that you cannot
move the primary working tree.  Also you cannot move a working tree
that has a submodule."

Again drop "yet" from the error message.  Our aspiration does not
ease the pain the end users get when they are given the error
message.

Re: [PATCH 6/6] worktree remove: new command

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> +	worktrees = get_worktrees(0);
> +	wt = find_worktree(worktrees, prefix, av[0]);
> +	if (!wt)
> +		die(_("'%s' is not a working directory"), av[0]);
> +	if (is_main_worktree(wt))
> +		die(_("'%s' is a main working directory"), av[0]);

The same comment as 3/6 applies here.

> +	reason = is_worktree_locked(wt);
> +	if (reason) {
> +		if (*reason)
> +			die(_("already locked, reason: %s"), reason);
> +		die(_("already locked, no reason"));
> +	}

The same comment as 3/6 applies here, too.

This is shared with 3/6 but I wonder if "--force" should be usable
as a way to bust this refusal.  There is an "unlock" operation, so
probably such a short-cut is not necessary---if you want to repair
your repository by moving or removing a working tree and if you
cannot do so due to an outstanding lock, you can do a two-step dance
"unlock followed by move or remove".  So I am OK with "--force" that
does not bust the lock.

> +	if (validate_worktree(wt, 0))
> +		return -1;
> +
> +	if (!force) {
> +		struct argv_array child_env = ARGV_ARRAY_INIT;
> +		struct child_process cp;
> +		char buf[1];
> +
> +		argv_array_pushf(&child_env, "%s=%s/.git",
> +				 GIT_DIR_ENVIRONMENT, wt->path);
> +		argv_array_pushf(&child_env, "%s=%s",
> +				 GIT_WORK_TREE_ENVIRONMENT, wt->path);
> +		memset(&cp, 0, sizeof(cp));
> +		argv_array_pushl(&cp.args, "status", "--porcelain", NULL);
> +		cp.env = child_env.argv;
> +		cp.git_cmd = 1;
> +		cp.dir = wt->path;
> +		cp.out = -1;
> +		ret = start_command(&cp);
> +		if (ret)
> +			die_errno(_("failed to run git-status on '%s', code %d"),
> +				  av[0], ret);

Do we return "code" from start_command() that is usable like this?

Is this "git status --porcelain" call affected by settings like
"submodule.*.ignore"?  If so, is that a good thing?

Oh, submodules.  Unlike "move" that may make their .git files
pointing at strange places after the operation finishes, "remove"
does not have to worry about them, because they are going to
disappear--I think that is OK, but I could be missing some cases
where a working tree that is not dirty may still want to be kept.
I dunno.

> +		ret = xread(cp.out, buf, sizeof(buf));
> +		if (ret)
> +			die(_("'%s' is dirty, use --force to delete it"), av[0]);
> +		close(cp.out);
> +		ret = finish_command(&cp);
> +		if (ret)
> +			die_errno(_("failed to run git-status on '%s', code %d"),
> +				  av[0], ret);
> +	}
> +	strbuf_addstr(&sb, wt->path);
> +	if (remove_dir_recursively(&sb, 0)) {

Oh, submodules.  If this working tree has submodules that are not
yet absorbed, wouldn't this go into their ".git" recursively and
end up losing everything?

> +		error_errno(_("failed to delete '%s'"), sb.buf);
> +		ret = -1;
> +	}
> +	strbuf_reset(&sb);
> +	strbuf_addstr(&sb, git_common_path("worktrees/%s", wt->id));
> +	if (remove_dir_recursively(&sb, 0)) {
> +		error_errno(_("failed to delete '%s'"), sb.buf);
> +		ret = -1;
> +	}

Re: [PATCH 0/6] nd/worktree-move update

[Jeff King]

On Thu, Apr 20, 2017 at 05:10:18PM +0700, Nguyễn Thái Ngọc Duy wrote:

>  - fixes the compile problem on latest master (because prefix_filename
>    takes one argument less)

It also now returns an allocated buffer.

So:

> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -561,9 +561,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)
>  	if (ac != 2)
>  		usage_with_options(worktree_usage, options);
>  
> -	strbuf_addstr(&dst, prefix_filename(prefix,
> -					    strlen(prefix),
> -					    av[1]));
> +	strbuf_addstr(&dst, prefix_filename(prefix, av[1]));

...this is now a leak. Probably:

  const char *filename = prefix_filename(prefix, av[1]);
  strbuf_attach(&dst, filename, strlen(filename), strlen(filename));

is what you want. That would be less awkward if we had a
strbuf_attach_str().

Or if we had a strbuf variant of prefix_filename(), you could do:

  prefix_filename_buf(&dst, prefix, av[1]);

I almost added that when I did the prefix_filename() work, since it uses
a strbuf internally. But there were no callers that would have used it.
Maybe it's worth doing now.

-- >8 --
From d0b933fc023023a017df9268360aa327c28b90f0 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Fri, 21 Apr 2017 10:53:07 -0400
Subject: [PATCH] prefix_filename: add strbuf variant

Now that prefix_filename() always allocates, it's awkward to
put its value directly into a strbuf. You have to either
free:

  const char *filename = prefix_filename(prefix, arg);
  strbuf_addstr(&buf, filename);
  free(filename);

or you have to attach:

  const char *filename = prefix_filename(prefix, arg);
  strbuf_attach(&buf, filename, strlen(filename), strlen(filename));

Since we're already using a strbuf internally, it's easy to
provide a variant that lets you write directly into one:

  prefix_filename_buf(&buf, prefix, arg);

For consistency with git_path_buf(), the function overwrites
the strbuf rather than appending.

Signed-off-by: Jeff King <peff@peff.net>
---
 abspath.c | 17 ++++++++++++-----
 cache.h   |  6 ++++++
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/abspath.c b/abspath.c
index 7f1cfe979..7594f62a5 100644
--- a/abspath.c
+++ b/abspath.c
@@ -246,21 +246,28 @@ char *absolute_pathdup(const char *path)
 	return strbuf_detach(&sb, NULL);
 }
 
-char *prefix_filename(const char *pfx, const char *arg)
+void prefix_filename_buf(struct strbuf *path, const char *pfx, const char *arg)
 {
-	struct strbuf path = STRBUF_INIT;
 	size_t pfx_len = pfx ? strlen(pfx) : 0;
 
+	strbuf_reset(path);
+
 	if (!pfx_len)
 		; /* nothing to prefix */
 	else if (is_absolute_path(arg))
 		pfx_len = 0;
 	else
-		strbuf_add(&path, pfx, pfx_len);
+		strbuf_add(path, pfx, pfx_len);
 
-	strbuf_addstr(&path, arg);
+	strbuf_addstr(path, arg);
 #ifdef GIT_WINDOWS_NATIVE
-	convert_slashes(path.buf + pfx_len);
+	convert_slashes(path->buf + pfx_len);
 #endif
+}
+
+char *prefix_filename(const char *pfx, const char *arg)
+{
+	struct strbuf path = STRBUF_INIT;
+	prefix_filename_buf(&path, pfx, arg);
 	return strbuf_detach(&path, NULL);
 }
diff --git a/cache.h b/cache.h
index ba27595d5..209039a98 100644
--- a/cache.h
+++ b/cache.h
@@ -548,6 +548,12 @@ extern char *prefix_path_gently(const char *prefix, int len, int *remaining, con
  */
 extern char *prefix_filename(const char *prefix, const char *path);
 
+/*
+ * Like prefix_filename, but write into "buf", overwriting any
+ * previous contents of the strbuf.
+ */
+extern void prefix_filename_buf(struct strbuf *out, const char *prefix, const char *path);
+
 extern int check_filename(const char *prefix, const char *name);
 extern void verify_filename(const char *prefix,
 			    const char *name,
-- 
2.13.0.rc0.364.g36b4d8031

Re: [PATCH 1/6] worktree.c: add validate_worktree()

[Duy Nguyen]

On Fri, Apr 21, 2017 at 9:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> +int validate_worktree(const struct worktree *wt, int quiet)
>> +{
>> +     struct strbuf sb = STRBUF_INIT;
>> +     char *path;
>> +     int err, ret;
>> +
>> +     if (is_main_worktree(wt)) {
>> +             /*
>> +              * Main worktree using .git file to point to the
>> +              * repository would make it impossible to know where
>> +              * the actual worktree is if this function is executed
>> +              * from another worktree. No .git file support for now.
>> +              */
>> +             strbuf_addf(&sb, "%s/.git", wt->path);
>> +             if (!is_directory(sb.buf)) {
>> +                     strbuf_release(&sb);
>> +                     return report(quiet, _("'%s/.git' at main worktree is not the repository directory"),
>> +                                   wt->path);
>
> These messages come without telling what they are.  Should they say
> that these are errors?  Or does the severity depend on the caller,
> i.e. why they want to know if a particular worktree is valid, and
> sometimes these are errors and other times they are mere warnings?

I'll save the error in a strbuf and let the caller decide how to
present them, which gives better context (e.g. "unable to move
worktree because ...")

>> +             }
>> +             return 0;
>> +     }
>> +
>> +     /*
>> +      * Make sure "gitdir" file points to a real .git file and that
>> +      * file points back here.
>> +      */
>> +     if (!is_absolute_path(wt->path))
>> +             return report(quiet, _("'%s' file does not contain absolute path to the worktree location"),
>> +                           git_common_path("worktrees/%s/gitdir", wt->id));
>
> It makes me wonder if this kind of error reporting belongs to the
> place where these are read (and a new wt is written out to the
> filesystem, perhaps).  The programmer who wrote this code may have
> known that wt->path is prepared by reading "worktrees/%s/gitdir" and
> without doing real_path() or absolute_path() on the result when this
> code was written, but nothing guarantees that to stay true over time
> as the code evolves.

This is almost like fsck for worktrees and for now only be checked
before we do destructive things to worktrees (moving, removing..).

Yeah we probably should do this at read time too (after checking if a
worktree is locked, and skip the next check because wt->path may not
exist). But we probably want to either make this function cheaper, or
we cache the worktree list. Probably the latter. It's on my todo list.
-- 
Duy