threads / patch / 47678

patch, 7 partsnd/worktree-move reboot

Subject: [PATCH 0/7] nd/worktree-move reboot

## tl;dr

49 messages between Jan 24, 2018 and Mar 5, 2018. Diffs are folded; open one to read it.

replies: 48people: 7as markdown or json

Nguyễn Thái Ngọc Duy· Jan 24, 2018, 09:53 UTC · lore

This series adds two more commands "git worktree move" and "git worktree remove" to do those things. I think I have addressed all comments from the mail threads referenced in "What's cooking" mails. I also added the ability to remove a worktree if its worktree area is already deleted.

It's a big code change (I reorganized remove_worktree() a bit for example to keep the last/new patch clean) so I'm not going to send interdiff.

There's only one thing left that I should do, mentioned in 6/7, to print detached HEAD before we remove a worktree. But I think if that's a good idea, it could be done separately on top.

Big thanks to Junio for keeping this on 'pu' all this time. Must be hard on you to resolve conflicts over and over.

Nguyễn Thái Ngọc Duy (7):
  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
  worktree remove: allow it when $GIT_WORK_TREE is already gone
 Documentation/git-worktree.txt         |  28 +++--
 builtin/worktree.c                     | 216 +++++++++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |   5 +-
 t/t2028-worktree-move.sh               |  65 ++++++++++
 worktree.c                             |  97 +++++++++++++++
 worktree.h                             |  18 +++
 6 files changed, 418 insertions(+), 11 deletions(-)
-- 
2.16.0.47.g3d9b0fac3a
Nguyễn Thái Ngọc Duy· Jan 24, 2018, 09:53 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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 | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 worktree.h |  9 ++++++++
 2 files changed, 81 insertions(+)
Show changes to 2 files +81 −0

worktree.c, worktree.h

diff --git a/worktree.c b/worktree.c
index f5da7d286d..b238d87bf1 100644
--- a/worktree.c
+++ b/worktree.c
@@ -254,6 +254,78 @@ const char *is_worktree_locked(struct worktree *wt)
 	return wt->lock_reason;
 }
 
+/* convenient wrapper to deal with NULL strbuf */
+static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...)
+{
+	va_list params;
+
+	if (!buf)
+		return;
+
+	va_start(params, fmt);
+	strbuf_vaddf(buf, fmt, params);
+	va_end(params);
+}
+
+int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)
+{
+	struct strbuf wt_path = STRBUF_INIT;
+	char *path = NULL;
+	int err, ret = -1;
+
+	strbuf_addf(&wt_path, "%s/.git", wt->path);
+
+	if (is_main_worktree(wt)) {
+		if (is_directory(wt_path.buf)) {
+			ret = 0;
+			goto done;
+		}
+		/*
+		 * 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_gently(errmsg,
+				   _("'%s' at main working tree is not the repository directory"),
+				   wt_path.buf);
+		goto done;
+	}
+
+	/*
+	 * Make sure "gitdir" file points to a real .git file and that
+	 * file points back here.
+	 */
+	if (!is_absolute_path(wt->path)) {
+		strbuf_addf_gently(errmsg,
+				   _("'%s' file does not contain absolute path to the working tree location"),
+				   git_common_path("worktrees/%s/gitdir", wt->id));
+		goto done;
+	}
+
+	if (!file_exists(wt_path.buf)) {
+		strbuf_addf_gently(errmsg, _("'%s' does not exist"), wt_path.buf);
+		goto done;
+	}
+
+	path = xstrdup_or_null(read_gitfile_gently(wt_path.buf, &err));
+	if (!path) {
+		strbuf_addf_gently(errmsg, _("'%s' is not a .git file, error code %d"),
+				   wt_path.buf, err);
+		goto done;
+	}
+
+	ret = fspathcmp(path, real_path(git_common_path("worktrees/%s", wt->id)));
+
+	if (ret)
+		strbuf_addf_gently(errmsg, _("'%s' does not point back to '%s'"),
+				   wt->path, git_common_path("worktrees/%s", wt->id));
+done:
+	free(path);
+	strbuf_release(&wt_path);
+	return ret;
+}
+
 int is_worktree_being_rebased(const struct worktree *wt,
 			      const char *target)
 {
diff --git a/worktree.h b/worktree.h
index c28a880e18..cb577de8cd 100644
--- a/worktree.h
+++ b/worktree.h
@@ -3,6 +3,8 @@
 
 #include "refs.h"
 
+struct strbuf;
+
 struct worktree {
 	char *path;
 	char *id;
@@ -59,6 +61,13 @@ 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. Error message is
+ * returned if "errmsg" is not NULL.
+ */
+extern int validate_worktree(const struct worktree *wt,
+			     struct strbuf *errmsg);
+
 /*
  * Free up the memory for worktree(s)
  */
-- 
2.16.0.47.g3d9b0fac3a
Nguyễn Thái Ngọc Duy· Jan 24, 2018, 09:53 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 worktree.c | 18 ++++++++++++++++++
 worktree.h |  6 ++++++
 2 files changed, 24 insertions(+)
Show changes to 2 files +24 −0

worktree.c, worktree.h

diff --git a/worktree.c b/worktree.c
index b238d87bf1..98e9f32c7f 100644
--- a/worktree.c
+++ b/worktree.c
@@ -326,6 +326,24 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)
 	return ret;
 }
 
+void update_worktree_location(struct worktree *wt, const char *path_)
+{
+	struct strbuf path = STRBUF_INIT;
+
+	if (is_main_worktree(wt))
+		die("BUG: can't relocate main worktree");
+
+	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);
+	}
+	strbuf_release(&path);
+}
+
 int is_worktree_being_rebased(const struct worktree *wt,
 			      const char *target)
 {
diff --git a/worktree.h b/worktree.h
index cb577de8cd..a913428c3d 100644
--- a/worktree.h
+++ b/worktree.h
@@ -68,6 +68,12 @@ extern const char *is_worktree_locked(struct worktree *wt);
 extern int validate_worktree(const struct worktree *wt,
 			     struct strbuf *errmsg);
 
+/*
+ * Update worktrees/xxx/gitdir with the new path.
+ */
+extern void update_worktree_location(struct worktree *wt,
+				     const char *path_);
+
 /*
  * Free up the memory for worktree(s)
  */
-- 
2.16.0.47.g3d9b0fac3a
Eric Sunshine· Feb 2, 2018, 08:23 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 14 quoted lines
> diff --git a/worktree.c b/worktree.c
> @@ -326,6 +326,24 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)
> +void update_worktree_location(struct worktree *wt, const char *path_)
> +{
> +       struct strbuf path = STRBUF_INIT;
> +
> +       if (is_main_worktree(wt))
> +               die("BUG: can't relocate main worktree");
> +
> +       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));

For the path stored in 'worktrees/<id>/gitdir' (and in wt->path), this and other worktree-related code sometimes treats it only as "absolute path" and sometimes as "real path". As a reviewer, I'm having trouble understanding the logic of why, how, and when this distinction is made. Can you explain a bit to help clarify when "absolute path" is good enough and when "real path" is needed?

Show 5 quoted lines
> +               free(wt->path);
> +               wt->path = strbuf_detach(&path, NULL);
> +       }
> +       strbuf_release(&path);
> +}
Duy Nguyen· Feb 2, 2018, 09:35 UTC · re: Eric Sunshine · lore

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

On Fri, Feb 2, 2018 at 3:23 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 22 quoted lines
> On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>> diff --git a/worktree.c b/worktree.c
>> @@ -326,6 +326,24 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)
>> +void update_worktree_location(struct worktree *wt, const char *path_)
>> +{
>> +       struct strbuf path = STRBUF_INIT;
>> +
>> +       if (is_main_worktree(wt))
>> +               die("BUG: can't relocate main worktree");
>> +
>> +       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));
>
> For the path stored in 'worktrees/<id>/gitdir' (and in wt->path), this
> and other worktree-related code sometimes treats it only as "absolute
> path" and sometimes as "real path". As a reviewer, I'm having trouble
> understanding the logic of why, how, and when this distinction is
> made. Can you explain a bit to help clarify when "absolute path" is
> good enough and when "real path" is needed?

Of the top of my head, I think it's "anything but a relative path". real_path() is normally used to normalize paths before I compare them. Writing a normalized path down is nice to avoid "../" but I don't think it's strictly necessary. Well, it's also prettier when you have to print the path out. Printing "/foo/bar/abc/../../xyz" is not nice.

Hmm.. looking back in add_worktree(), we also write "gitdir" with real_path(). I think the problem here is that I should have real_path()'d "path.buf" _before_ doing fspathcmp(). Maybe that's where the confusion's from.

-- 
Duy
Nguyễn Thái Ngọc Duy· Jan 24, 2018, 09:53 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 3/7] worktree move: new command

This command allows to relocate linked worktrees. Main worktree cannot (yet) be moved.

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                     | 50 ++++++++++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |  2 +-
 t/t2028-worktree-move.sh               | 31 +++++++++++++++++++++
 4 files changed, 88 insertions(+), 2 deletions(-)
Show changes to 4 files +88 −2

Documentation/git-worktree.txt, builtin/worktree.c, contrib/completion/git-completion.bash, t/t2028-worktree-move.sh

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 41585f535d..5d115b855a 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -12,6 +12,7 @@ SYNOPSIS
 'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b <new-branch>] <path> [<commit-ish>]
 '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>
 
@@ -79,6 +80,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.
+
 prune::
 
 Prune working tree information in $GIT_DIR/worktrees.
@@ -281,7 +287,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 7cef5b120b..2faa95430a 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -17,6 +17,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
@@ -605,6 +606,53 @@ 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;
+	struct strbuf errmsg = STRBUF_INIT;
+	const char *reason;
+	char *path;
+
+	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+	if (ac != 2)
+		usage_with_options(worktree_usage, options);
+
+	path = prefix_filename(prefix, av[1]);
+	strbuf_addstr(&dst, path);
+	free(path);
+	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 tree"), av[0]);
+	if (is_main_worktree(wt))
+		die(_("'%s' is a main working tree"), av[0]);
+	reason = is_worktree_locked(wt);
+	if (reason) {
+		if (*reason)
+			die(_("cannot move a locked working tree, lock reason: %s"),
+			    reason);
+		die(_("cannot move a locked working tree"));
+	}
+	if (validate_worktree(wt, &errmsg))
+		die(_("validation failed, cannot move working tree:\n%s"),
+		    errmsg.buf);
+	strbuf_release(&errmsg);
+
+	if (rename(wt->path, dst.buf) == -1)
+		die_errno(_("failed to move '%s' to '%s'"), wt->path, dst.buf);
+
+	update_worktree_location(wt, dst.buf);
+
+	return 0;
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
 	struct option options[] = {
@@ -627,5 +675,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 3683c772c5..b87d387686 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3087,7 +3087,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.16.0.47.g3d9b0fac3a
Eric Sunshine· Feb 2, 2018, 09:15 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 16 quoted lines
> This command allows to relocate linked worktrees. Main worktree cannot
> (yet) be moved.
> [...]
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -79,6 +80,11 @@ files from being pruned automatically. This also prevents it from
> +move::
> +
> +Move a working tree to a new location. Note that the main working tree
> +cannot be moved.
> +
> @@ -281,7 +287,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

A couple other places in this document ought to be updated. Specifically, in DESCRIPTION:

    If you move a linked working tree, you need to manually update the
    administrative files so that they do not get pruned automatically.
    See section "DETAILS" for more information.
which can probably be dropped in its entirety. And, in DETAILS:
    If you move a linked working tree, you need to update the 'gitdir'
    file...
should probably be reworded to
    If you _manually_ move a linked working tree, you need to update
    the 'gitdir' file...
Show 9 quoted lines
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -605,6 +606,53 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
> +static int move_worktree(int ac, const char **av, const char *prefix)
> +{
> +       [...]
> +       worktrees = get_worktrees(0);
> +       wt = find_worktree(worktrees, prefix, av[0]);
> +       if (!wt)
> +               die(_("'%s' is not a working tree"), av[0]);

This is still leaking 'worktrees'[1]. You probably want free_worktrees() immediately after the find_worktree() invocation.

Show 12 quoted lines
> +       if (is_main_worktree(wt))
> +               die(_("'%s' is a main working tree"), av[0]);
> +       reason = is_worktree_locked(wt);
> +       if (reason) {
> +               if (*reason)
> +                       die(_("cannot move a locked working tree, lock reason: %s"),
> +                           reason);
> +               die(_("cannot move a locked working tree"));
> +       }
> +       if (validate_worktree(wt, &errmsg))
> +               die(_("validation failed, cannot move working tree:\n%s"),
> +                   errmsg.buf);
Minor: All the other error messages are presented on a single line.
Despite this error message potentially being quite long, it worries me
a bit that presenting it on two lines could complicate scripted
clients if they need to collect the error message for special
handling.
Show 7 quoted lines
> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> @@ -59,4 +59,35 @@ test_expect_success 'unlock worktree twice' '
> +test_expect_success 'move locked worktree' '
> +       git worktree lock source &&
> +       test_must_fail git worktree move source destination &&
> +       git worktree unlock source
> +'

Also from [1], wrapping 'unlock' inside a test_when_finished() invocation seems potentially desirable.

Show 11 quoted lines
> +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 &&

This seems somewhat fragile. If someone inserts a test before this one which adds another worktree, then this test would break. Perhaps the filtering of the --porcelain output could be more strict? (Not necessarily worth a re-roll, though.)

> +       git -C destination log --format=%s >actual2 &&
> +       echo init >expected2 &&
> +       test_cmp expected2 actual2
> +'
[1]: https://public-inbox.org/git/CAPig+cRqgzB4tiTb=fHuFBTqo3QEGM3m378PvOse06KdreKo2Q@mail.gmail.com/
Eric Sunshine· Feb 2, 2018, 11:23 UTC · re: Eric Sunshine · lore

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

On Fri, Feb 2, 2018 at 4:15 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 11 quoted lines
> On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>> +static int move_worktree(int ac, const char **av, const char *prefix)
>> +{
>> +       [...]
>> +       worktrees = get_worktrees(0);
>> +       wt = find_worktree(worktrees, prefix, av[0]);
>> +       if (!wt)
>> +               die(_("'%s' is not a working tree"), av[0]);
>
> This is still leaking 'worktrees'[1]. You probably want
> free_worktrees() immediately after the find_worktree() invocation.

Sorry, free_worktrees() after the last use of 'wt' since you still need to access its fields, which would be the end of the function.

Duy Nguyen· Feb 5, 2018, 13:28 UTC · re: Eric Sunshine · lore

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

On Fri, Feb 2, 2018 at 6:23 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 15 quoted lines
> On Fri, Feb 2, 2018 at 4:15 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>>> +static int move_worktree(int ac, const char **av, const char *prefix)
>>> +{
>>> +       [...]
>>> +       worktrees = get_worktrees(0);
>>> +       wt = find_worktree(worktrees, prefix, av[0]);
>>> +       if (!wt)
>>> +               die(_("'%s' is not a working tree"), av[0]);
>>
>> This is still leaking 'worktrees'[1]. You probably want
>> free_worktrees() immediately after the find_worktree() invocation.
>
> Sorry, free_worktrees() after the last use of 'wt' since you still
> need to access its fields, which would be the end of the function.
I learned SANITIZE=leak today! It not only catches this but also "dst".

Jeff is there any ongoing effort to make the test suite pass with SANITIZE=leak? My t2038 passed, so I went ahead with the full test suite and saw so many failures. I did see in your original mails that you focused on t0000 and t0001 only..

-- 
Duy
Jeff King· Feb 6, 2018, 02:13 UTC · re: Duy Nguyen · lore

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

On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote:
Show 12 quoted lines
> >> This is still leaking 'worktrees'[1]. You probably want
> >> free_worktrees() immediately after the find_worktree() invocation.
> >
> > Sorry, free_worktrees() after the last use of 'wt' since you still
> > need to access its fields, which would be the end of the function.
> 
> I learned SANITIZE=leak today! It not only catches this but also "dst".
> 
> Jeff is there any ongoing effort to make the test suite pass with
> SANITIZE=leak? My t2038 passed, so I went ahead with the full test
> suite and saw so many failures. I did see in your original mails that
> you focused on t0000 and t0001 only..

Yeah, I did those two scripts to try to prove to myself that the approach was good. But I haven't really pushed it any further.

Martin Ågren (cc'd) did some follow-up work, but I think we still have a long way to go. My hope is that people who are interested in leak-checking their new code can run some specific script they're interested in, and maybe fix up one or two nearby bits while they're there (either by fixing a leak, or just annotating via UNLEAK). Then we can slowly converge on correctness. :)

-Peff
Martin Ågren· Feb 6, 2018, 20:05 UTC · re: Jeff King · lore

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

On 6 February 2018 at 03:13, Jeff King <peff@peff.net> wrote:
Show 13 quoted lines
> On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote:
>> I learned SANITIZE=leak today! It not only catches this but also "dst".
>>
>> Jeff is there any ongoing effort to make the test suite pass with
>> SANITIZE=leak? My t2038 passed, so I went ahead with the full test
>> suite and saw so many failures. I did see in your original mails that
>> you focused on t0000 and t0001 only..
>
> Yeah, I did those two scripts to try to prove to myself that the
> approach was good. But I haven't really pushed it any further.
>
> Martin Ågren (cc'd) did some follow-up work, but I think we still have a
> long way to go.
Agreed. :-)
Show 5 quoted lines
> My hope is that people who are interested in
> leak-checking their new code can run some specific script they're
> interested in, and maybe fix up one or two nearby bits while they're
> there (either by fixing a leak, or just annotating via UNLEAK). Then we
> can slowly converge on correctness. :)

Yeah. My experience is that it's easy -- or was for me, anyway -- to get carried away trying to fix all "related" leaks to the one you started with, since there are so many dimensions to search in. Two obvious aspects are "leaks nearby" and "leaks using the same API". This is not to suggest that the situation is horrible. It's just that if you pick a leak at random and there is a fair number of "clusters" of leaks, chances are good you'll find yourself in such a cluster.

Addressing a leak without worrying too much about how it generalizes would still help. ;-)

Martin
Duy Nguyen· Feb 12, 2018, 09:56 UTC · re: Martin Ågren · lore

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

On Wed, Feb 7, 2018 at 3:05 AM, Martin Ågren <martin.agren@gmail.com> wrote:
Show 16 quoted lines
> On 6 February 2018 at 03:13, Jeff King <peff@peff.net> wrote:
>> On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote:
>>> I learned SANITIZE=leak today! It not only catches this but also "dst".
>>>
>>> Jeff is there any ongoing effort to make the test suite pass with
>>> SANITIZE=leak? My t2038 passed, so I went ahead with the full test
>>> suite and saw so many failures. I did see in your original mails that
>>> you focused on t0000 and t0001 only..
>>
>> Yeah, I did those two scripts to try to prove to myself that the
>> approach was good. But I haven't really pushed it any further.
>>
>> Martin Ågren (cc'd) did some follow-up work, but I think we still have a
>> long way to go.
>
> Agreed. :-)

Should we mark the test files that pass SANITIZE=leak and run as a sub testsuite? This way we can start growing it until it covers everything (unlikely) and make sure people don't break what's already passed.

Of course I don't expect everybody to run this new make target with SANITIZE=leak. Travis can do that for us and hopefully have some way to tell git@vger about new breakages.

-- 
Duy
Martin Ågren· Feb 12, 2018, 22:15 UTC · re: Duy Nguyen · lore

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

On 12 February 2018 at 10:56, Duy Nguyen <pclouds@gmail.com> wrote:
Show 25 quoted lines
> On Wed, Feb 7, 2018 at 3:05 AM, Martin Ågren <martin.agren@gmail.com> wrote:
>> On 6 February 2018 at 03:13, Jeff King <peff@peff.net> wrote:
>>> On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote:
>>>> I learned SANITIZE=leak today! It not only catches this but also "dst".
>>>>
>>>> Jeff is there any ongoing effort to make the test suite pass with
>>>> SANITIZE=leak? My t2038 passed, so I went ahead with the full test
>>>> suite and saw so many failures. I did see in your original mails that
>>>> you focused on t0000 and t0001 only..
>>>
>>> Yeah, I did those two scripts to try to prove to myself that the
>>> approach was good. But I haven't really pushed it any further.
>>>
>>> Martin Ågren (cc'd) did some follow-up work, but I think we still have a
>>> long way to go.
>>
>> Agreed. :-)
>
> Should we mark the test files that pass SANITIZE=leak and run as a sub
> testsuite? This way we can start growing it until it covers everything
> (unlikely) and make sure people don't break what's already passed.
>
> Of course I don't expect everybody to run this new make target with
> SANITIZE=leak. Travis can do that for us and hopefully have some way
> to tell git@vger about new breakages.

Makes sense to try to make sure that we don't regress leak-free tests. I don't know what our Travis-budget looks like, but I would volunteer to run something like this periodically using my own cycles.

My experience with the innards of our Makefiles and test-lib.sh is non-existant, but from a very cursory look it seems like something as simple as loading GIT_SKIP_TESTS from a blacklist-file might do the trick. I could try to look into it in the next couple of days.

Martin
Duy Nguyen· Feb 13, 2018, 00:27 UTC · re: Martin Ågren · lore

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

On Mon, Feb 12, 2018 at 11:15:06PM +0100, Martin Ågren wrote:
Show 35 quoted lines
> On 12 February 2018 at 10:56, Duy Nguyen <pclouds@gmail.com> wrote:
> > On Wed, Feb 7, 2018 at 3:05 AM, Martin Ågren <martin.agren@gmail.com> wrote:
> >> On 6 February 2018 at 03:13, Jeff King <peff@peff.net> wrote:
> >>> On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote:
> >>>> I learned SANITIZE=leak today! It not only catches this but also "dst".
> >>>>
> >>>> Jeff is there any ongoing effort to make the test suite pass with
> >>>> SANITIZE=leak? My t2038 passed, so I went ahead with the full test
> >>>> suite and saw so many failures. I did see in your original mails that
> >>>> you focused on t0000 and t0001 only..
> >>>
> >>> Yeah, I did those two scripts to try to prove to myself that the
> >>> approach was good. But I haven't really pushed it any further.
> >>>
> >>> Martin Ågren (cc'd) did some follow-up work, but I think we still have a
> >>> long way to go.
> >>
> >> Agreed. :-)
> >
> > Should we mark the test files that pass SANITIZE=leak and run as a sub
> > testsuite? This way we can start growing it until it covers everything
> > (unlikely) and make sure people don't break what's already passed.
> >
> > Of course I don't expect everybody to run this new make target with
> > SANITIZE=leak. Travis can do that for us and hopefully have some way
> > to tell git@vger about new breakages.
> 
> Makes sense to try to make sure that we don't regress leak-free tests. I
> don't know what our Travis-budget looks like, but I would volunteer to
> run something like this periodically using my own cycles.
> 
> My experience with the innards of our Makefiles and test-lib.sh is
> non-existant, but from a very cursory look it seems like something as
> simple as loading GIT_SKIP_TESTS from a blacklist-file might do the
> trick.

Yeah my first throught was something along that line too. But maybe it's nicer to mark a test file leak-free like this:

-- 8< --
Show changes to t/t2028-worktree-move.sh +2 −2
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 459f676683..1446f075b7 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -2,6 +2,8 @@
 
 test_description='test git worktree move, remove, lock and unlock'
 
+test_leak_free=yes
+
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 8< --

And we can run these test files with a new option --leak-check (or
something like that, we already support a similar option --valgrind).

Most of the work will be in test-lib.sh. If we detect --leak-check and
test_leak_free is not set, we skip the whole file. In the far far far
future when all files have test_leak_free=yes, we can flip the default
and delete these lines.

It would be nice if test-lib.sh can determine if 'git' binary is built
with SANITIZE=yes, but I think we can live without it.

> I could try to look into it in the next couple of days.

Have fun :) Let me know if you give up though, I'll give it a shot.
--
Duy
Jeff King· Feb 14, 2018, 03:16 UTC · re: Duy Nguyen · lore

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

On Tue, Feb 13, 2018 at 07:27:58AM +0700, Duy Nguyen wrote:
Show 27 quoted lines
> > Makes sense to try to make sure that we don't regress leak-free tests. I
> > don't know what our Travis-budget looks like, but I would volunteer to
> > run something like this periodically using my own cycles.
> > 
> > My experience with the innards of our Makefiles and test-lib.sh is
> > non-existant, but from a very cursory look it seems like something as
> > simple as loading GIT_SKIP_TESTS from a blacklist-file might do the
> > trick.
> 
> Yeah my first throught was something along that line too. But maybe
> it's nicer to mark a test file leak-free like this:
> 
> -- 8< --
> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> index 459f676683..1446f075b7 100755
> --- a/t/t2028-worktree-move.sh
> +++ b/t/t2028-worktree-move.sh
> @@ -2,6 +2,8 @@
>  
>  test_description='test git worktree move, remove, lock and unlock'
>  
> +test_leak_free=yes
> +
>  . ./test-lib.sh
>  
>  test_expect_success 'setup' '
> -- 8< --

Hmm. That is not too bad, but somehow it feels funny to me to be polluting each test script with these annotations. And to be driving it from inside the test scripts.

It seems like:
  make SANITIZE=leak test GIT_SKIP_TESTS="$(cat known-leaky)"
would be sufficient. And updating the list would just be:
  # assume we're using prove, which will keep running after failure,
  # and will record the results for us to parse (using "--state=").
  # Otherwise use "make -k" and grep in t/test-results.
  make SANITIZE=leak test
  (cd t && prove --dry --state=failed) |
  perl -lne '/^(t[0-9]{4})-.*.sh$/ and print $1' |
  sort >known-leaky

That would update both now-passing and now-failing tests. Presumably we'd keep it checked in, so "git diff" would show you the changes.

-Peff
Duy Nguyen· Feb 14, 2018, 09:07 UTC · re: Jeff King · lore

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

On Wed, Feb 14, 2018 at 10:16 AM, Jeff King <peff@peff.net> wrote:
Show 9 quoted lines
> Hmm. That is not too bad, but somehow it feels funny to me to be
> polluting each test script with these annotations. And to be driving it
> from inside the test scripts.
>
> It seems like:
>
>   make SANITIZE=leak test GIT_SKIP_TESTS="$(cat known-leaky)"
>
> would be sufficient.
And all new test files are considered leak-free by default? I like that!
Show 14 quoted lines
> And updating the list would just be:
>
>   # assume we're using prove, which will keep running after failure,
>   # and will record the results for us to parse (using "--state=").
>   # Otherwise use "make -k" and grep in t/test-results.
>   make SANITIZE=leak test
>   (cd t && prove --dry --state=failed) |
>   perl -lne '/^(t[0-9]{4})-.*.sh$/ and print $1' |
>   sort >known-leaky
>
> That would update both now-passing and now-failing tests. Presumably
> we'd keep it checked in, so "git diff" would show you the changes.
>
> -Peff
-- 
Duy
Junio C Hamano· Feb 14, 2018, 17:35 UTC · re: Duy Nguyen · lore

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

Duy Nguyen <pclouds@gmail.com> writes:
Show 12 quoted lines
> On Wed, Feb 14, 2018 at 10:16 AM, Jeff King <peff@peff.net> wrote:
>> Hmm. That is not too bad, but somehow it feels funny to me to be
>> polluting each test script with these annotations. And to be driving it
>> from inside the test scripts.
>>
>> It seems like:
>>
>>   make SANITIZE=leak test GIT_SKIP_TESTS="$(cat known-leaky)"
>>
>> would be sufficient.
>
> And all new test files are considered leak-free by default? I like that!
Sounds good ;-)
Show 13 quoted lines
>
>> And updating the list would just be:
>>
>>   # assume we're using prove, which will keep running after failure,
>>   # and will record the results for us to parse (using "--state=").
>>   # Otherwise use "make -k" and grep in t/test-results.
>>   make SANITIZE=leak test
>>   (cd t && prove --dry --state=failed) |
>>   perl -lne '/^(t[0-9]{4})-.*.sh$/ and print $1' |
>>   sort >known-leaky
>>
>> That would update both now-passing and now-failing tests. Presumably
>> we'd keep it checked in, so "git diff" would show you the changes.
Sounds good, too.
Martin Ågren· Feb 14, 2018, 21:56 UTC · re: Junio C Hamano · lore

[PATCH] t/known-leaky: add list of known-leaky test scripts

Here's what a list of known leaks might look like. It feels a bit awkward to post a known-incomplete list (I don't run all tests). Duy offered to pick up the ball if I gave up, maybe you could complete and post this as your own? :-? Even if I (or others) can't reproduce the complete list locally, regressions will be trivial to find, and newly leak-free tests fairly easy to notice.

I'm not sure if the shamelessly stolen shell snippets warrant their own scripts, or how make targets overriding various variables would be received. At least they're in the commit message.

-- >8 -- We have quite a lot of leaks in the code base. Using SANITIZE=leak, it is easy to find them, and every now and then we simply stumble upon one. Still, we can expect it to take some time to get to the point where `make SANITIZE=leak test` succeeds.

Until that happens, it would be nice if we could at least try not to regress in the sense that a test tXXXX which was at one point leak-free turns leaky. Such a regression would indicate that leak-free code turned leaky, that a new feature is leaky, or that we simply happen to trigger an existing leak as part of a newly added/modified test.

To that end, introduce a list of known-leaky tests, i.e., a list of tXXXX-values. Now this will be able to find such regressions:

    make SANITIZE=leak test GIT_SKIP_TESTS="$(cat t/known-leaky)"
The list was generated, and can be updated, as follows:
    # Assume we're using prove, which will keep running after failure,
    # and will record the results for us to parse (using "--state=").
    # Otherwise use "make -k" and grep in t/test-results.
    GIT_TEST_OPTS=-i make SANITIZE=leak test
    cd t
    prove --dry --state=failed |
    perl -lne '/^(t[0-9]{4})-.*\.sh$/ and print $1' |
    sort >known-leaky

The list added in this commit might be incomplete, since I do not run all tests (I'm missing svn, cvs, p4, Windows-only and filesystem-dependent tests, as well as "writeable /"). The majority of these do not primarily test our C code, but all of them might trigger leaks, in which case they would belong in this list.

Suggested-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/known-leaky | 539 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 539 insertions(+)
 create mode 100644 t/known-leaky
Show changes to t/known-leaky +539 −0
diff --git a/t/known-leaky b/t/known-leaky
new file mode 100644
index 0000000000..80a0af4d09
--- /dev/null
+++ b/t/known-leaky
@@ -0,0 +1,539 @@
+t0002
+t0003
+t0006
+t0008
+t0009
+t0012
+t0020
+t0021
+t0023
+t0040
+t0050
+t0056
+t0060
+t0062
+t0064
+t0070
+t0090
+t0100
+t0101
+t0110
+t0203
+t0300
+t0301
+t0302
+t1001
+t1002
+t1004
+t1005
+t1006
+t1007
+t1008
+t1011
+t1012
+t1013
+t1020
+t1021
+t1050
+t1051
+t1090
+t1301
+t1302
+t1304
+t1306
+t1308
+t1350
+t1400
+t1401
+t1402
+t1403
+t1404
+t1405
+t1406
+t1407
+t1408
+t1409
+t1410
+t1411
+t1412
+t1413
+t1414
+t1430
+t1450
+t1500
+t1502
+t1505
+t1507
+t1508
+t1510
+t1511
+t1512
+t1514
+t1700
+t2007
+t2008
+t2009
+t2010
+t2011
+t2012
+t2013
+t2014
+t2015
+t2016
+t2017
+t2018
+t2019
+t2020
+t2021
+t2022
+t2023
+t2024
+t2025
+t2026
+t2027
+t2030
+t2103
+t2106
+t2200
+t2203
+t2204
+t3001
+t3004
+t3005
+t3007
+t3010
+t3020
+t3030
+t3031
+t3032
+t3033
+t3034
+t3040
+t3050
+t3060
+t3200
+t3201
+t3202
+t3203
+t3204
+t3205
+t3210
+t3301
+t3302
+t3303
+t3304
+t3306
+t3307
+t3308
+t3309
+t3310
+t3311
+t3400
+t3402
+t3403
+t3404
+t3405
+t3406
+t3407
+t3408
+t3409
+t3410
+t3411
+t3412
+t3413
+t3414
+t3415
+t3416
+t3417
+t3418
+t3419
+t3420
+t3421
+t3425
+t3426
+t3427
+t3428
+t3429
+t3500
+t3501
+t3502
+t3503
+t3504
+t3505
+t3506
+t3507
+t3508
+t3509
+t3510
+t3511
+t3512
+t3513
+t3600
+t3700
+t3701
+t3800
+t3900
+t3901
+t3903
+t3904
+t3905
+t3906
+t4001
+t4008
+t4010
+t4013
+t4014
+t4015
+t4016
+t4017
+t4018
+t4020
+t4021
+t4022
+t4023
+t4026
+t4027
+t4028
+t4030
+t4031
+t4036
+t4038
+t4039
+t4041
+t4042
+t4043
+t4044
+t4045
+t4047
+t4048
+t4049
+t4051
+t4052
+t4053
+t4055
+t4056
+t4057
+t4059
+t4060
+t4061
+t4064
+t4065
+t4103
+t4107
+t4108
+t4111
+t4114
+t4115
+t4117
+t4118
+t4120
+t4121
+t4122
+t4124
+t4125
+t4127
+t4131
+t4135
+t4137
+t4138
+t4150
+t4151
+t4152
+t4153
+t4200
+t4201
+t4202
+t4203
+t4204
+t4205
+t4206
+t4207
+t4208
+t4209
+t4210
+t4211
+t4212
+t4213
+t4252
+t4253
+t4254
+t4255
+t4300
+t5000
+t5001
+t5002
+t5003
+t5004
+t5100
+t5150
+t5300
+t5301
+t5302
+t5303
+t5304
+t5305
+t5306
+t5310
+t5311
+t5312
+t5313
+t5314
+t5315
+t5316
+t5317
+t5400
+t5401
+t5402
+t5403
+t5404
+t5405
+t5406
+t5407
+t5408
+t5500
+t5501
+t5502
+t5503
+t5504
+t5505
+t5506
+t5509
+t5510
+t5512
+t5513
+t5514
+t5515
+t5516
+t5517
+t5518
+t5519
+t5520
+t5521
+t5522
+t5523
+t5524
+t5525
+t5526
+t5527
+t5528
+t5531
+t5532
+t5533
+t5534
+t5535
+t5536
+t5537
+t5538
+t5539
+t5540
+t5541
+t5542
+t5543
+t5544
+t5545
+t5546
+t5547
+t5550
+t5551
+t5560
+t5561
+t5570
+t5571
+t5572
+t5573
+t5600
+t5601
+t5603
+t5604
+t5605
+t5606
+t5607
+t5609
+t5610
+t5611
+t5612
+t5613
+t5614
+t5700
+t5801
+t5802
+t5810
+t5811
+t5812
+t5813
+t5814
+t5815
+t5900
+t6000
+t6002
+t6003
+t6004
+t6006
+t6007
+t6008
+t6009
+t6010
+t6011
+t6012
+t6013
+t6014
+t6015
+t6016
+t6017
+t6018
+t6019
+t6020
+t6021
+t6022
+t6024
+t6025
+t6026
+t6027
+t6028
+t6029
+t6030
+t6031
+t6032
+t6033
+t6034
+t6035
+t6036
+t6037
+t6038
+t6040
+t6041
+t6042
+t6044
+t6045
+t6050
+t6060
+t6100
+t6101
+t6110
+t6111
+t6112
+t6120
+t6130
+t6131
+t6132
+t6133
+t6134
+t6200
+t6300
+t6301
+t6302
+t6500
+t6501
+t7001
+t7003
+t7004
+t7005
+t7006
+t7007
+t7008
+t7009
+t7010
+t7011
+t7012
+t7030
+t7060
+t7061
+t7062
+t7063
+t7064
+t7102
+t7103
+t7104
+t7105
+t7106
+t7110
+t7111
+t7112
+t7201
+t7300
+t7301
+t7400
+t7401
+t7402
+t7403
+t7405
+t7406
+t7407
+t7408
+t7409
+t7410
+t7411
+t7412
+t7413
+t7414
+t7500
+t7501
+t7502
+t7503
+t7504
+t7505
+t7506
+t7507
+t7508
+t7509
+t7510
+t7511
+t7512
+t7513
+t7514
+t7515
+t7516
+t7517
+t7519
+t7520
+t7521
+t7600
+t7601
+t7602
+t7603
+t7604
+t7605
+t7606
+t7607
+t7608
+t7609
+t7610
+t7611
+t7612
+t7613
+t7614
+t7700
+t7701
+t7702
+t7800
+t7810
+t7811
+t7814
+t8001
+t8002
+t8003
+t8004
+t8005
+t8006
+t8007
+t8008
+t8009
+t8010
+t8011
+t9001
+t9002
+t9003
+t9004
+t9010
+t9020
+t9300
+t9301
+t9302
+t9303
+t9350
+t9351
+t9500
+t9502
+t9700
+t9902
+t9903
-- 
2.16.1.72.g5be1f00a9a
Jeff King· Feb 19, 2018, 21:29 UTC · re: Martin Ågren · lore

Re: [PATCH] t/known-leaky: add list of known-leaky test scripts

On Wed, Feb 14, 2018 at 10:56:37PM +0100, Martin Ågren wrote:
Show 6 quoted lines
> Here's what a list of known leaks might look like. It feels a bit
> awkward to post a known-incomplete list (I don't run all tests). Duy
> offered to pick up the ball if I gave up, maybe you could complete and
> post this as your own? :-? Even if I (or others) can't reproduce the
> complete list locally, regressions will be trivial to find, and newly
> leak-free tests fairly easy to notice.

I didn't think about that when I posted my scripts. In general, it's OK to me if you miss a script when you generate the "leaky" list. But if you skip it, you cannot say whether it is leaky or not, and should probably neither add nor remove it from the known-leaky list. So I think the second shell snippet needs to become a little more clever about skipped test scripts.

Even that isn't 100% fool-proof, as some individual tests may be skipped or not skipped on various platforms. But it may be enough in practice (and eventually we'd have no known-leaky tests, of course ;) ).

Or alternatively, we could just not bother with checking this into the repository, and it becomes a local thing for people interested in leak-testing. What's the value in having a shared known-leaky list, especially if we don't expect most people to run it.

I guess it lets us add a Travis job to do the leak-checking, which might get more coverage. So maybe if we do have an in-repo known-leaky, it should match some canonical Travis environment. That won't give us complete coverage, but at this point we're just trying to notice low-hanging fruit.

-Peff
Martin Ågren· Feb 20, 2018, 20:44 UTC · re: Jeff King · lore

Re: [PATCH] t/known-leaky: add list of known-leaky test scripts

On 19 February 2018 at 22:29, Jeff King <peff@peff.net> wrote:
Show 19 quoted lines
> On Wed, Feb 14, 2018 at 10:56:37PM +0100, Martin Ågren wrote:
>
>> Here's what a list of known leaks might look like. It feels a bit
>> awkward to post a known-incomplete list (I don't run all tests). Duy
>> offered to pick up the ball if I gave up, maybe you could complete and
>> post this as your own? :-? Even if I (or others) can't reproduce the
>> complete list locally, regressions will be trivial to find, and newly
>> leak-free tests fairly easy to notice.
>
> I didn't think about that when I posted my scripts. In general, it's OK
> to me if you miss a script when you generate the "leaky" list. But if
> you skip it, you cannot say whether it is leaky or not, and should
> probably neither add nor remove it from the known-leaky list. So I think
> the second shell snippet needs to become a little more clever about
> skipped test scripts.
>
> Even that isn't 100% fool-proof, as some individual tests may be skipped
> or not skipped on various platforms. But it may be enough in practice
> (and eventually we'd have no known-leaky tests, of course ;) ).
All good points.
> Or alternatively, we could just not bother with checking this into the
> repository, and it becomes a local thing for people interested in
> leak-testing. What's the value in having a shared known-leaky list,
> especially if we don't expect most people to run it.
This sums up my feeling about this.
Show 5 quoted lines
> I guess it lets us add a Travis job to do the leak-checking, which might
> get more coverage. So maybe if we do have an in-repo known-leaky, it
> should match some canonical Travis environment. That won't give us
> complete coverage, but at this point we're just trying to notice
> low-hanging fruit.

Right. A Travis job to get non-complete but consistent data would be "nice" (TM), but at this point it would perhaps just result in blacklist-churning. At some point, adding/removing entries in a blacklist will have some signal-value and psychology to it, but right now it might just be noise. (My patch adds an incomplete blacklist of 539 scripts...)

For finding this sort of low-hanging fruit (windfall?) any local scripting and greping will do. I sort of hesitate wasting community CPU cycles on something which might be appreciated, in theory, but that doesn't have enough attention to move beyond "every now and then, something is fixed, but just as often, tests gain leaks and the blacklist gains entries".

To sum up: I probably won't be looking into Travis-ing such a blacklist in the near future.

Martin
Jeff King· Feb 20, 2018, 21:08 UTC · re: Martin Ågren · lore

Re: [PATCH] t/known-leaky: add list of known-leaky test scripts

On Tue, Feb 20, 2018 at 09:44:51PM +0100, Martin Ågren wrote:
Show 9 quoted lines
> For finding this sort of low-hanging fruit (windfall?) any local
> scripting and greping will do. I sort of hesitate wasting community CPU
> cycles on something which might be appreciated, in theory, but that
> doesn't have enough attention to move beyond "every now and then,
> something is fixed, but just as often, tests gain leaks and the
> blacklist gains entries".
> 
> To sum up: I probably won't be looking into Travis-ing such a blacklist
> in the near future.

Yeah, I think that's the right path for now. Hopefully we can get close to zero leaks at some point, and then think about adding this kind of infrastructure. Thanks for talking it through.

-Peff
Junio C Hamano· Feb 21, 2018, 16:53 UTC · re: Martin Ågren · lore

Re: [PATCH] t/known-leaky: add list of known-leaky test scripts

Martin Ågren <martin.agren@gmail.com> writes:
Show 8 quoted lines
> On 19 February 2018 at 22:29, Jeff King <peff@peff.net> wrote:
> ...
>> Or alternatively, we could just not bother with checking this into the
>> repository, and it becomes a local thing for people interested in
>> leak-testing. What's the value in having a shared known-leaky list,
>> especially if we don't expect most people to run it.
>
> This sums up my feeling about this.

Even though keeping track of list of known-leaky tests may not be so interesting, we can still salvage useful pieces from the discussion and make them available to developers, e.g. something like

    prove --dry --state=failed |
    perl -lne '/^(t[0-9]{4})-.*\.sh$/ and print $1' | sort >$@+
    if cmp >/dev/null $@ $@+; then rm $@+; else mv $@+ $@; fi

could be made into a target to stash away the list of failing tests after a test run?

Jeff King· Feb 21, 2018, 18:25 UTC · re: Junio C Hamano · lore

Re: [PATCH] t/known-leaky: add list of known-leaky test scripts

On Wed, Feb 21, 2018 at 08:53:16AM -0800, Junio C Hamano wrote:
Show 10 quoted lines
> Even though keeping track of list of known-leaky tests may not be so
> interesting, we can still salvage useful pieces from the discussion
> and make them available to developers, e.g.  something like
> 
>     prove --dry --state=failed |
>     perl -lne '/^(t[0-9]{4})-.*\.sh$/ and print $1' | sort >$@+
>     if cmp >/dev/null $@ $@+; then rm $@+; else mv $@+ $@; fi
> 
> could be made into a target to stash away the list of failing tests
> after a test run?
Unfortunately there are some caveats in that snippet:
  1. You are using prove.
  2. You are using --state=save in the initial run.

I think we might be better off having the test scripts write to test-results/*.counts even when run under a TAP harness, and then we can have a consistent way to get the list of failed tests (we already have a "make failed" that works this way).

-Peff
Kaartic Sivaraam· Feb 25, 2018, 03:48 UTC · re: Martin Ågren · lore

Re: [PATCH] t/known-leaky: add list of known-leaky test scripts

On Wednesday 21 February 2018 02:14 AM, Martin Ågren wrote:
> To sum up: I probably won't be looking into Travis-ing such a blacklist
> in the near future.
> 

Just thinking out loud, how about having a white-list instead of a black-list and using it to run only those tests in the white list. Something like,

t/white_list ------------ t0000 t0001

To run ------

cd t/
for test in $(cat white_list)
do
    white_list_tests="$white_list_tests $test*"
done
make SANITIZE=leak $white_list_tests
-- 
Kaartic

QUOTE
---

“The most valuable person on any team is the person who makes everyone
else on the team more valuable, not the person who knows the most.”

      - Joel Spolsky
Martin Ågren· Feb 26, 2018, 21:22 UTC · re: Kaartic Sivaraam · lore

Re: [PATCH] t/known-leaky: add list of known-leaky test scripts

On 25 February 2018 at 04:48, Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote:

Show 23 quoted lines
> On Wednesday 21 February 2018 02:14 AM, Martin Ågren wrote:
>> To sum up: I probably won't be looking into Travis-ing such a blacklist
>> in the near future.
>>
>
> Just thinking out loud, how about having a white-list instead of a
> black-list and using it to run only those tests in the white list.
> Something like,
>
> t/white_list
> ------------
> t0000
> t0001
>
> To run
> ------
>
> cd t/
> for test in $(cat white_list)
> do
>     white_list_tests="$white_list_tests $test*"
> done
> make SANITIZE=leak $white_list_tests

Yeah, that would also work. An incomplete whitelist can't cause errors for those running other tests, as an incomplete blacklist could. So that's good. At some point, the whitelist would need to be turned into a blacklist. At the very latest when the whitelist is the full set of tests, in order to flip the default of new tests. ;-) Right now, I think whitelists and blacklists are about equally useful.

Let's hope we're heading for a future where a blacklist gets more and more feasible, whereas a whitelist would get longer and longer. ;-)

Martin
Nguyễn Thái Ngọc Duy· Jan 24, 2018, 09:53 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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 | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)
Show changes to builtin/worktree.c +14 −2
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 2faa95430a..89398e67e4 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -624,8 +624,6 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	path = prefix_filename(prefix, av[1]);
 	strbuf_addstr(&dst, path);
 	free(path);
-	if (file_exists(dst.buf))
-		die(_("target '%s' already exists"), av[1]);
 
 	worktrees = get_worktrees(0);
 	wt = find_worktree(worktrees, prefix, av[0]);
@@ -633,6 +631,20 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 		die(_("'%s' is not a working tree"), av[0]);
 	if (is_main_worktree(wt))
 		die(_("'%s' is a main working tree"), av[0]);
+
+	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);
+	} else if (file_exists(dst.buf)) {
+		die(_("target '%s' already exists"), av[1]);
+	}
+
 	reason = is_worktree_locked(wt);
 	if (reason) {
 		if (*reason)
-- 
2.16.0.47.g3d9b0fac3a
Eric Sunshine· Feb 2, 2018, 09:35 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 13 quoted lines
> 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>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -624,8 +624,6 @@ static int move_worktree(int ac, const char **av, const char *prefix)
>         path = prefix_filename(prefix, av[1]);
>         strbuf_addstr(&dst, path);
>         free(path);
> -       if (file_exists(dst.buf))
> -               die(_("target '%s' already exists"), av[1]);
Nit: The distance between this "removed" conditional and the code
inserted below hampered review a bit since it made it slightly more
onerous to check for unwanted logic changes. Had patch 3/7 located
this conditional below the is_main_worktree() check (where the new
code is inserted below), this 'if' would have merely mutated into an
'else if' but not otherwise moved, thus review would have been
simplified. (Not itself worth a re-roll.)
Show 14 quoted lines
>         worktrees = get_worktrees(0);
>         wt = find_worktree(worktrees, prefix, av[0]);
> @@ -633,6 +631,20 @@ static int move_worktree(int ac, const char **av, const char *prefix)
>                 die(_("'%s' is not a working tree"), av[0]);
>         if (is_main_worktree(wt))
>                 die(_("'%s' is a main working tree"), av[0]);
> +
> +       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);

Do we know at this point whether 'dst' has a terminating "/"? If it does, then this addstr() could result in "//" in the path which might be problematic on Windows.

Show 5 quoted lines
> +               if (file_exists(dst.buf))
> +                       die(_("target '%s' already exists"), dst.buf);
> +       } else if (file_exists(dst.buf)) {
> +               die(_("target '%s' already exists"), av[1]);
> +       }

I wonder if it makes sense to collapse the duplicated file_exists()/"target already exists" code?

    if (is_directory(...)) {
        ...
        strbuf_addstr(&dst, sep);
    }
    if (file_exists(dst.buf))
        die(_("target '%s' already exists"), dst.buf);

It changes the error message slightly for the non-directory case but simplifies the code a bit.

>         reason = is_worktree_locked(wt);
>         if (reason) {
>                 if (*reason)

Perhaps add a test to t2028-worktree-move.sh to show that this new behavior works?

Nguyễn Thái Ngọc Duy· Jan 24, 2018, 09:53 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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>
---
 Documentation/git-worktree.txt |  2 +-
 builtin/worktree.c             | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)
Show changes to 2 files +24 −1

Documentation/git-worktree.txt, builtin/worktree.c

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 5d115b855a..24bb69de50 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -83,7 +83,7 @@ with `--reason`.
 move::
 
 Move a working tree to a new location. Note that the main working tree
-cannot be moved.
+or linked working trees containing submodules cannot be moved.
 
 prune::
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 89398e67e4..15980a0a49 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -606,6 +606,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 = { NULL };
+	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(_("working trees containing submodules cannot be moved"));
+}
+
 static int move_worktree(int ac, const char **av, const char *prefix)
 {
 	struct option options[] = {
@@ -632,6 +653,8 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	if (is_main_worktree(wt))
 		die(_("'%s' is a main working tree"), av[0]);
 
+	validate_no_submodules(wt);
+
 	if (is_directory(dst.buf)) {
 		const char *sep = find_last_dir_sep(wt->path);
 
-- 
2.16.0.47.g3d9b0fac3a
Eric Sunshine· Feb 2, 2018, 10:06 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 30 quoted lines
> 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>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -606,6 +606,27 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
> +static void validate_no_submodules(const struct worktree *wt)
> +{
> +       struct index_state istate = { NULL };
> +       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(_("working trees containing submodules cannot be moved"));

Minor (not worth a re-roll): This could be simplified slightly by die()ing inside the loop rather than having 'found_submodules' and breaking from the loop. Doing so will leak 'istate', but it's die()ing anyhow, so should not be an issue (unless this code is someday libified).

> +}
Nguyễn Thái Ngọc Duy· Jan 24, 2018, 09:53 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 6/7] worktree remove: new command

This command allows to delete a worktree. Like 'move' you cannot remove the main worktree, or one with submodules inside [1].

For deleting $GIT_WORK_TREE, Untracked files or any staged entries are considered precious and therefore prevent removal by default. Ignored files are not precious.

When it comes to deleting $GIT_DIR, there's no "clean" check because there should not be any valuable data in there, except:

- HEAD reflog. There is nothing we can do about this until somebody
  steps up and implements the ref graveyard.
- Detached HEAD. Technically it can still be recovered. Although it
  may be nice to warn about orphan commits like 'git checkout' does.
[1] We do 'git status' with --ignore-submodules=all for safety
    anyway. But this needs a closer look by submodule people before we
    can allow deletion. For example, if a submodule is totally clean,
    but its repo not absorbed to the main .git dir, then deleting
    worktree also deletes the valuable .submodule repo too.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-worktree.txt         |  21 +++---
 builtin/worktree.c                     | 131 ++++++++++++++++++++++++++++++++-
 contrib/completion/git-completion.bash |   5 +-
 t/t2028-worktree-move.sh               |  26 +++++++
 4 files changed, 172 insertions(+), 11 deletions(-)
Show changes to 4 files +172 −11

Documentation/git-worktree.txt, builtin/worktree.c, contrib/completion/git-completion.bash, t/t2028-worktree-move.sh

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 24bb69de50..6f83723d9a 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
@@ -89,6 +90,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 or ones with submodules 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.
@@ -98,9 +106,10 @@ OPTIONS
 
 -f::
 --force::
-	By default, `add` refuses to create a new working tree when `<commit-ish>` is a branch name and
-	is already checked out by another working tree. This option overrides
-	that safeguard.
+	By default, `add` refuses to create a new working tree when
+	`<commit-ish>` is a branch name and 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>::
@@ -282,12 +291,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 15980a0a49..8b027375d7 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -19,6 +19,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
 };
@@ -624,7 +625,7 @@ static void validate_no_submodules(const struct worktree *wt)
 	discard_index(&istate);
 
 	if (found_submodules)
-		die(_("working trees containing submodules cannot be moved"));
+		die(_("working trees containing submodules cannot be moved or removed"));
 }
 
 static int move_worktree(int ac, const char **av, const char *prefix)
@@ -688,6 +689,132 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	return 0;
 }
 
+/*
+ * Note, "git status --porcelain" is used to determine if it's safe to
+ * delete a whole worktree. "git status" does not ignore user
+ * configuration, so if a normal "git status" shows "clean" for the
+ * user, then it's ok to remove it.
+ *
+ * This assumption may be a bad one. We may want to ignore
+ * (potentially bad) user settings and only delete a worktree when
+ * it's absolutely safe to do so from _our_ point of view because we
+ * know better.
+ */
+static void check_clean_worktree(struct worktree *wt,
+				 const char *original_path)
+{
+	struct argv_array child_env = ARGV_ARRAY_INIT;
+	struct child_process cp;
+	char buf[1];
+	int ret;
+
+	validate_no_submodules(wt);
+
+	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", "--ignore-submodules=none",
+			 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'"),
+			  original_path);
+	ret = xread(cp.out, buf, sizeof(buf));
+	if (ret)
+		die(_("'%s' is dirty, use --force to delete it"),
+		    original_path);
+	close(cp.out);
+	ret = finish_command(&cp);
+	if (ret)
+		die_errno(_("failed to run git-status on '%s', code %d"),
+			  original_path, ret);
+}
+
+static int delete_git_work_tree(struct worktree *wt)
+{
+	struct strbuf sb = STRBUF_INIT;
+	int ret = 0;
+
+	strbuf_addstr(&sb, wt->path);
+	if (remove_dir_recursively(&sb, 0)) {
+		error_errno(_("failed to delete '%s'"), sb.buf);
+		ret = -1;
+	}
+	strbuf_release(&sb);
+
+	return ret;
+}
+
+static int delete_git_dir(struct worktree *wt)
+{
+	struct strbuf sb = STRBUF_INIT;
+	int ret = 0;
+
+	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);
+	return ret;
+}
+
+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 errmsg = 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 tree"), av[0]);
+	if (is_main_worktree(wt))
+		die(_("'%s' is a main working tree"), av[0]);
+	reason = is_worktree_locked(wt);
+	if (reason) {
+		if (*reason)
+			die(_("cannot remove a locked working tree, lock reason: %s"),
+			    reason);
+		die(_("cannot remove a locked working tree"));
+	}
+	if (validate_worktree(wt, &errmsg))
+		die(_("validation failed, cannot remove working tree:\n%s"),
+		    errmsg.buf);
+	strbuf_release(&errmsg);
+
+	if (!force)
+		check_clean_worktree(wt, av[0]);
+
+	ret |= delete_git_work_tree(wt);
+	/*
+	 * continue on even if ret is non-zero, there's no going back
+	 * from here.
+	 */
+	ret |= delete_git_dir(wt);
+
+	free_worktrees(worktrees);
+	return ret;
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
 	struct option options[] = {
@@ -712,5 +839,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 b87d387686..ff4a39631e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3087,7 +3087,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"
@@ -3105,6 +3105,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.16.0.47.g3d9b0fac3a
Eric Sunshine· Feb 2, 2018, 11:47 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 12 quoted lines
> This command allows to delete a worktree. Like 'move' you cannot
> remove the main worktree, or one with submodules inside [1].
> [...]
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -688,6 +689,132 @@ static int move_worktree(int ac, const char **av, const char *prefix)
> +static void check_clean_worktree(struct worktree *wt,
> +                                const char *original_path)
> +{
> +       [...]
> +       validate_no_submodules(wt);

It's slightly strange seeing worktree validation in a function checking whether the worktree is clean since submodule validation isn't an issue of cleanliness. I'd have expected the caller to invoke it instead:

    int remove_worktree(...) {
        ...
        if (!force) {
            validate_no_submodules(wt);
            check_clean_worktree(wt, av[0]);
        }
        ...
    }

On the other hand, I could imagine it being called here as appropriate if submodule validation eventually also checks submodule cleanliness as hinted by the commit message.

Show 16 quoted lines
> +       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", "--ignore-submodules=none",
> +                        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'"),
> +                         original_path);
Minor: I think there was some effort recently to remove "git-foo"
style mentions from documentation and error messages. Perhaps this
could be "failed to run 'git status' on '%s'". Ditto below.
Show 25 quoted lines
> +       ret = xread(cp.out, buf, sizeof(buf));
> +       if (ret)
> +               die(_("'%s' is dirty, use --force to delete it"),
> +                   original_path);
> +       close(cp.out);
> +       ret = finish_command(&cp);
> +       if (ret)
> +               die_errno(_("failed to run git-status on '%s', code %d"),
> +                         original_path, ret);
> +}
> +
> +static int delete_git_work_tree(struct worktree *wt)
> +{
> +       struct strbuf sb = STRBUF_INIT;
> +       int ret = 0;
> +
> +       strbuf_addstr(&sb, wt->path);
> +       if (remove_dir_recursively(&sb, 0)) {
> +               error_errno(_("failed to delete '%s'"), sb.buf);
> +               ret = -1;
> +       }
> +       strbuf_release(&sb);
> +
> +       return ret;
> +}

Style nit: In the very similar delete_git_dir(), just below, there is no blank line before 'return'.

Show 27 quoted lines
> +
> +static int delete_git_dir(struct worktree *wt)
> +{
> +       struct strbuf sb = STRBUF_INIT;
> +       int ret = 0;
> +
> +       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);
> +       return ret;
> +}
> +
> +static int remove_worktree(int ac, const char **av, const char *prefix)
> +{
> +       [...]
> +       if (reason) {
> +               if (*reason)
> +                       die(_("cannot remove a locked working tree, lock reason: %s"),
> +                           reason);
> +               die(_("cannot remove a locked working tree"));
> +       }
> +       if (validate_worktree(wt, &errmsg))
> +               die(_("validation failed, cannot remove working tree:\n%s"),
> +                   errmsg.buf);
Minor: Same concern as in 3/7 about the multi-line error message
making scripted handling of error message collection more difficult.
Show 22 quoted lines
> +       strbuf_release(&errmsg);
> +
> +       if (!force)
> +               check_clean_worktree(wt, av[0]);
> +
> +       ret |= delete_git_work_tree(wt);
> +       /*
> +        * continue on even if ret is non-zero, there's no going back
> +        * from here.
> +        */
> +       ret |= delete_git_dir(wt);
> +
> +       free_worktrees(worktrees);
> +       return ret;
> +}
> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> @@ -90,4 +90,30 @@ test_expect_success 'move main worktree' '
> +test_expect_success 'remove locked worktree' '
> +       git worktree lock destination &&
> +       test_must_fail git worktree remove destination &&
> +       git worktree unlock destination
> +'
Perhaps place 'unlock' in test_when_finished()[1].
Show 7 quoted lines
> +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 &&

Reversion of 'init.t' probably belongs in the preceding test which modified it, wrapped in test_when_finished()[1].

> +       : >destination/untracked &&
> +       test_must_fail git worktree remove destination
> +'
[1]: https://public-inbox.org/git/CAPig+cSV9_6j6Nkptma3BewKW8QQcem7gwFCb42VBW4Xe0Vr2w@mail.gmail.com/
Duy Nguyen· Feb 12, 2018, 09:26 UTC · re: Eric Sunshine · lore

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

On Fri, Feb 2, 2018 at 6:47 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 31 quoted lines
> On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>> This command allows to delete a worktree. Like 'move' you cannot
>> remove the main worktree, or one with submodules inside [1].
>> [...]
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -688,6 +689,132 @@ static int move_worktree(int ac, const char **av, const char *prefix)
>> +static void check_clean_worktree(struct worktree *wt,
>> +                                const char *original_path)
>> +{
>> +       [...]
>> +       validate_no_submodules(wt);
>
> It's slightly strange seeing worktree validation in a function
> checking whether the worktree is clean since submodule validation
> isn't an issue of cleanliness. I'd have expected the caller to invoke
> it instead:
>
>     int remove_worktree(...) {
>         ...
>         if (!force) {
>             validate_no_submodules(wt);
>             check_clean_worktree(wt, av[0]);
>         }
>         ...
>     }
>
> On the other hand, I could imagine it being called here as appropriate
> if submodule validation eventually also checks submodule cleanliness
> as hinted by the commit message.

Yes I basically consider all submodules dirty until somebody sorts them out. I'll add a comment here to clarify this.

-- 
Duy
Nguyễn Thái Ngọc Duy· Jan 24, 2018, 09:53 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 7/7] worktree remove: allow it when $GIT_WORK_TREE is already gone

"git worktree remove" basically consists of two things
- delete $GIT_WORK_TREE
- delete $GIT_DIR (which is $SUPER_GIT_DIR/worktrees/something)

If $GIT_WORK_TREE is already gone for some reason, we should be able to finish the job by deleting $GIT_DIR.

Two notes:
- $GIT_WORK_TREE _can_ be missing if the worktree is locked. In that
  case we must not delete $GIT_DIR because the real $GIT_WORK_TREE may
  be in a usb stick somewhere. This is already handled because we
  check for lock first.
- validate_worktree() is still called because it may do more checks in
  future (and it already does something else, like checking main
  worktree, but that's irrelevant in this case)
Noticed-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/worktree.c       | 12 +++++++-----
 t/t2028-worktree-move.sh |  8 ++++++++
 worktree.c               |  9 ++++++++-
 worktree.h               |  5 ++++-
 4 files changed, 27 insertions(+), 7 deletions(-)
Show changes to 4 files +27 −7

builtin/worktree.c, t/t2028-worktree-move.sh, worktree.c, worktree.h

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 8b027375d7..8ce86aef0e 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -676,7 +676,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 			    reason);
 		die(_("cannot move a locked working tree"));
 	}
-	if (validate_worktree(wt, &errmsg))
+	if (validate_worktree(wt, &errmsg, 0))
 		die(_("validation failed, cannot move working tree:\n%s"),
 		    errmsg.buf);
 	strbuf_release(&errmsg);
@@ -796,15 +796,17 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
 			    reason);
 		die(_("cannot remove a locked working tree"));
 	}
-	if (validate_worktree(wt, &errmsg))
+	if (validate_worktree(wt, &errmsg, WT_VALIDATE_WORKTREE_MISSING_OK))
 		die(_("validation failed, cannot remove working tree:\n%s"),
 		    errmsg.buf);
 	strbuf_release(&errmsg);
 
-	if (!force)
-		check_clean_worktree(wt, av[0]);
+	if (file_exists(wt->path)) {
+		if (!force)
+			check_clean_worktree(wt, av[0]);
 
-	ret |= delete_git_work_tree(wt);
+		ret |= delete_git_work_tree(wt);
+	}
 	/*
 	 * continue on even if ret is non-zero, there's no going back
 	 * from here.
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index b3105eaaed..459f676683 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -116,4 +116,12 @@ test_expect_success 'force remove worktree with untracked file' '
 	test_path_is_missing destination
 '
 
+test_expect_success 'remove missing worktree' '
+	git worktree add to-be-gone &&
+	test -d .git/worktrees/to-be-gone &&
+	mv to-be-gone gone &&
+	git worktree remove to-be-gone &&
+	test_path_is_missing .git/worktrees/to-be-gone
+'
+
 test_done
diff --git a/worktree.c b/worktree.c
index 98e9f32c7f..542196f0ad 100644
--- a/worktree.c
+++ b/worktree.c
@@ -267,7 +267,8 @@ static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...)
 	va_end(params);
 }
 
-int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)
+int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
+		      unsigned flags)
 {
 	struct strbuf wt_path = STRBUF_INIT;
 	char *path = NULL;
@@ -303,6 +304,12 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)
 		goto done;
 	}
 
+	if (flags & WT_VALIDATE_WORKTREE_MISSING_OK &&
+	    !file_exists(wt->path)) {
+		ret = 0;
+		goto done;
+	}
+
 	if (!file_exists(wt_path.buf)) {
 		strbuf_addf_gently(errmsg, _("'%s' does not exist"), wt_path.buf);
 		goto done;
diff --git a/worktree.h b/worktree.h
index a913428c3d..fe38ce10c3 100644
--- a/worktree.h
+++ b/worktree.h
@@ -61,12 +61,15 @@ extern int is_main_worktree(const struct worktree *wt);
  */
 extern const char *is_worktree_locked(struct worktree *wt);
 
+#define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)
+
 /*
  * Return zero if the worktree is in good condition. Error message is
  * returned if "errmsg" is not NULL.
  */
 extern int validate_worktree(const struct worktree *wt,
-			     struct strbuf *errmsg);
+			     struct strbuf *errmsg,
+			     unsigned flags);
 
 /*
  * Update worktrees/xxx/gitdir with the new path.
-- 
2.16.0.47.g3d9b0fac3a
Eric Sunshine· Feb 2, 2018, 12:59 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 7/7] worktree remove: allow it when $GIT_WORK_TREE is already gone

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
Show 19 quoted lines
> [...]
> - $GIT_WORK_TREE _can_ be missing if the worktree is locked. In that
>   case we must not delete $GIT_DIR because the real $GIT_WORK_TREE may
>   be in a usb stick somewhere. This is already handled because we
>   check for lock first.
> [...]
>
> Noticed-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> @@ -116,4 +116,12 @@ test_expect_success 'force remove worktree with untracked file' '
> +test_expect_success 'remove missing worktree' '
> +       git worktree add to-be-gone &&
> +       test -d .git/worktrees/to-be-gone &&
> +       mv to-be-gone gone &&
> +       git worktree remove to-be-gone &&
> +       test_path_is_missing .git/worktrees/to-be-gone
> +'

Perhaps there could also be a test to verify that a missing but locked worktree is _not_ removed?

Junio C Hamano· Jan 24, 2018, 20:42 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 0/7] nd/worktree-move reboot

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 5 quoted lines
> This series adds two more commands "git worktree move" and "git
> worktree remove" to do those things. I think I have addressed all
> comments from the mail threads referenced in "What's cooking" mails. I
> also added the ability to remove a worktree if its worktree area is
> already deleted.
Thanks for moving this forward.
Nguyễn Thái Ngọc Duy· Feb 12, 2018, 09:49 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 0/7] nd/worktree-move reboot

v2 basically fixes lots of comments from Eric (many thanks!): memory leak, typos, document updates, tests, corner case fixes.

Interdiff:
Show changes to 6 files +61 −33

Documentation/git-worktree.txt, builtin/worktree.c, strbuf.c, strbuf.h, t/t2028-worktree-move.sh, worktree.c

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 6f83723d9a..d322acbc67 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -36,10 +36,6 @@ The working tree's administrative files in the repository (see
 `git worktree prune` in the main or any linked working tree to
 clean up any stale administrative files.
 
-If you move a linked working tree, you need to manually update the
-administrative files so that they do not get pruned automatically. See
-section "DETAILS" for more information.
-
 If a linked working tree is stored on a portable device or network share
 which is not always mounted, you can prevent its administrative files from
 being pruned by issuing the `git worktree lock` command, optionally
@@ -211,7 +207,7 @@ thumb is do not make any assumption about whether a path belongs to
 $GIT_DIR or $GIT_COMMON_DIR when you need to directly access something
 inside $GIT_DIR. Use `git rev-parse --git-path` to get the final path.
 
-If you move a linked working tree, you need to update the 'gitdir' file
+If you manually move a linked working tree, you need to update the 'gitdir' file
 in the entry's directory. For example, if a linked working tree is moved
 to `/newpath/test-next` and its `.git` file points to
 `/path/main/.git/worktrees/test-next`, then update
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 8ce86aef0e..f77ef994c4 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -653,21 +653,19 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 		die(_("'%s' is not a working tree"), av[0]);
 	if (is_main_worktree(wt))
 		die(_("'%s' is a main working tree"), av[0]);
-
-	validate_no_submodules(wt);
-
 	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_trim_trailing_dir_sep(&dst);
 		strbuf_addstr(&dst, sep);
-		if (file_exists(dst.buf))
-			die(_("target '%s' already exists"), dst.buf);
-	} else if (file_exists(dst.buf)) {
-		die(_("target '%s' already exists"), av[1]);
 	}
+	if (file_exists(dst.buf))
+		die(_("target '%s' already exists"), dst.buf);
+
+	validate_no_submodules(wt);
 
 	reason = is_worktree_locked(wt);
 	if (reason) {
@@ -677,7 +675,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 		die(_("cannot move a locked working tree"));
 	}
 	if (validate_worktree(wt, &errmsg, 0))
-		die(_("validation failed, cannot move working tree:\n%s"),
+		die(_("validation failed, cannot move working tree: %s"),
 		    errmsg.buf);
 	strbuf_release(&errmsg);
 
@@ -686,6 +684,8 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 
 	update_worktree_location(wt, dst.buf);
 
+	strbuf_release(&dst);
+	free_worktrees(worktrees);
 	return 0;
 }
 
@@ -708,6 +708,10 @@ static void check_clean_worktree(struct worktree *wt,
 	char buf[1];
 	int ret;
 
+	/*
+	 * Until we sort this out, all submodules are "dirty" and
+	 * will abort this function.
+	 */
 	validate_no_submodules(wt);
 
 	argv_array_pushf(&child_env, "%s=%s/.git",
@@ -724,7 +728,7 @@ static void check_clean_worktree(struct worktree *wt,
 	cp.out = -1;
 	ret = start_command(&cp);
 	if (ret)
-		die_errno(_("failed to run git-status on '%s'"),
+		die_errno(_("failed to run 'git status' on '%s'"),
 			  original_path);
 	ret = xread(cp.out, buf, sizeof(buf));
 	if (ret)
@@ -733,7 +737,7 @@ static void check_clean_worktree(struct worktree *wt,
 	close(cp.out);
 	ret = finish_command(&cp);
 	if (ret)
-		die_errno(_("failed to run git-status on '%s', code %d"),
+		die_errno(_("failed to run 'git status' on '%s', code %d"),
 			  original_path, ret);
 }
 
@@ -748,7 +752,6 @@ static int delete_git_work_tree(struct worktree *wt)
 		ret = -1;
 	}
 	strbuf_release(&sb);
-
 	return ret;
 }
 
@@ -797,7 +800,7 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
 		die(_("cannot remove a locked working tree"));
 	}
 	if (validate_worktree(wt, &errmsg, WT_VALIDATE_WORKTREE_MISSING_OK))
-		die(_("validation failed, cannot remove working tree:\n%s"),
+		die(_("validation failed, cannot remove working tree: %s"),
 		    errmsg.buf);
 	strbuf_release(&errmsg);
 
diff --git a/strbuf.c b/strbuf.c
index 1df674e919..46930ad027 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -95,6 +95,7 @@ void strbuf_trim(struct strbuf *sb)
 	strbuf_rtrim(sb);
 	strbuf_ltrim(sb);
 }
+
 void strbuf_rtrim(struct strbuf *sb)
 {
 	while (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1]))
@@ -102,6 +103,13 @@ void strbuf_rtrim(struct strbuf *sb)
 	sb->buf[sb->len] = '\0';
 }
 
+void strbuf_trim_trailing_dir_sep(struct strbuf *sb)
+{
+	while (sb->len > 0 && is_dir_sep((unsigned char)sb->buf[sb->len - 1]))
+		sb->len--;
+	sb->buf[sb->len] = '\0';
+}
+
 void strbuf_ltrim(struct strbuf *sb)
 {
 	char *b = sb->buf;
diff --git a/strbuf.h b/strbuf.h
index 14c8c10d66..e6cae5f439 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -179,6 +179,9 @@ extern void strbuf_trim(struct strbuf *);
 extern void strbuf_rtrim(struct strbuf *);
 extern void strbuf_ltrim(struct strbuf *);
 
+/* Strip trailing directory separators */
+extern void strbuf_trim_trailing_dir_sep(struct strbuf *);
+
 /**
  * Replace the contents of the strbuf with a reencoded form.  Returns -1
  * on error, 0 on success.
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 459f676683..082368d8c6 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -66,21 +66,16 @@ test_expect_success 'move non-worktree' '
 
 test_expect_success 'move locked worktree' '
 	git worktree lock source &&
-	test_must_fail git worktree move source destination &&
-	git worktree unlock source
+	test_when_finished "git worktree unlock source" &&
+	test_must_fail git worktree move source destination
 '
 
 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 worktree list --porcelain | grep "^worktree.*/destination" &&
+	! git worktree list --porcelain | grep "^worktree.*/source" >empty &&
 	git -C destination log --format=%s >actual2 &&
 	echo init >expected2 &&
 	test_cmp expected2 actual2
@@ -90,23 +85,38 @@ test_expect_success 'move main worktree' '
 	test_must_fail git worktree move . def
 '
 
+test_expect_success 'move worktree to another dir' '
+	toplevel="$(pwd)" &&
+	mkdir some-dir &&
+	git worktree move destination some-dir &&
+	test_path_is_missing source &&
+	git worktree list --porcelain | grep "^worktree.*/some-dir/destination" &&
+	git -C some-dir/destination log --format=%s >actual2 &&
+	echo init >expected2 &&
+	test_cmp expected2 actual2
+'
+
 test_expect_success 'remove main worktree' '
 	test_must_fail git worktree remove .
 '
 
+test_expect_success 'move some-dir/destination back' '
+	git worktree move some-dir/destination destination
+'
+
 test_expect_success 'remove locked worktree' '
 	git worktree lock destination &&
-	test_must_fail git worktree remove destination &&
-	git worktree unlock destination
+	test_when_finished "git worktree unlock destination" &&
+	test_must_fail git worktree remove destination
 '
 
 test_expect_success 'remove worktree with dirty tracked file' '
 	echo dirty >>destination/init.t &&
+	test_when_finished "git -C destination checkout 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
 '
@@ -124,4 +134,13 @@ test_expect_success 'remove missing worktree' '
 	test_path_is_missing .git/worktrees/to-be-gone
 '
 
+test_expect_success 'NOT remove missing-but-locked worktree' '
+	git worktree add gone-but-locked &&
+	git worktree lock gone-but-locked &&
+	test -d .git/worktrees/gone-but-locked &&
+	mv gone-but-locked really-gone-now &&
+	test_must_fail git worktree remove gone-but-locked &&
+	test_path_is_dir .git/worktrees/gone-but-locked
+'
+
 test_done
diff --git a/worktree.c b/worktree.c
index 542196f0ad..28989cf06e 100644
--- a/worktree.c
+++ b/worktree.c
@@ -340,11 +340,10 @@ void update_worktree_location(struct worktree *wt, const char *path_)
 	if (is_main_worktree(wt))
 		die("BUG: can't relocate main worktree");
 
-	strbuf_add_absolute_path(&path, path_);
+	strbuf_realpath(&path, path_, 1);
 	if (fspathcmp(wt->path, path.buf)) {
-		write_file(git_common_path("worktrees/%s/gitdir",
-					   wt->id),
-			   "%s/.git", real_path(path.buf));
+		write_file(git_common_path("worktrees/%s/gitdir", wt->id),
+			   "%s/.git", path.buf);
 		free(wt->path);
 		wt->path = strbuf_detach(&path, NULL);
 	}
Nguyễn Thái Ngọc Duy· Feb 12, 2018, 09:49 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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 | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 worktree.h |  9 +++++++
 2 files changed, 81 insertions(+)
Show changes to 2 files +81 −0

worktree.c, worktree.h

diff --git a/worktree.c b/worktree.c
index f5da7d286d..b238d87bf1 100644
--- a/worktree.c
+++ b/worktree.c
@@ -254,6 +254,78 @@ const char *is_worktree_locked(struct worktree *wt)
 	return wt->lock_reason;
 }
 
+/* convenient wrapper to deal with NULL strbuf */
+static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...)
+{
+	va_list params;
+
+	if (!buf)
+		return;
+
+	va_start(params, fmt);
+	strbuf_vaddf(buf, fmt, params);
+	va_end(params);
+}
+
+int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)
+{
+	struct strbuf wt_path = STRBUF_INIT;
+	char *path = NULL;
+	int err, ret = -1;
+
+	strbuf_addf(&wt_path, "%s/.git", wt->path);
+
+	if (is_main_worktree(wt)) {
+		if (is_directory(wt_path.buf)) {
+			ret = 0;
+			goto done;
+		}
+		/*
+		 * 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_gently(errmsg,
+				   _("'%s' at main working tree is not the repository directory"),
+				   wt_path.buf);
+		goto done;
+	}
+
+	/*
+	 * Make sure "gitdir" file points to a real .git file and that
+	 * file points back here.
+	 */
+	if (!is_absolute_path(wt->path)) {
+		strbuf_addf_gently(errmsg,
+				   _("'%s' file does not contain absolute path to the working tree location"),
+				   git_common_path("worktrees/%s/gitdir", wt->id));
+		goto done;
+	}
+
+	if (!file_exists(wt_path.buf)) {
+		strbuf_addf_gently(errmsg, _("'%s' does not exist"), wt_path.buf);
+		goto done;
+	}
+
+	path = xstrdup_or_null(read_gitfile_gently(wt_path.buf, &err));
+	if (!path) {
+		strbuf_addf_gently(errmsg, _("'%s' is not a .git file, error code %d"),
+				   wt_path.buf, err);
+		goto done;
+	}
+
+	ret = fspathcmp(path, real_path(git_common_path("worktrees/%s", wt->id)));
+
+	if (ret)
+		strbuf_addf_gently(errmsg, _("'%s' does not point back to '%s'"),
+				   wt->path, git_common_path("worktrees/%s", wt->id));
+done:
+	free(path);
+	strbuf_release(&wt_path);
+	return ret;
+}
+
 int is_worktree_being_rebased(const struct worktree *wt,
 			      const char *target)
 {
diff --git a/worktree.h b/worktree.h
index c28a880e18..cb577de8cd 100644
--- a/worktree.h
+++ b/worktree.h
@@ -3,6 +3,8 @@
 
 #include "refs.h"
 
+struct strbuf;
+
 struct worktree {
 	char *path;
 	char *id;
@@ -59,6 +61,13 @@ 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. Error message is
+ * returned if "errmsg" is not NULL.
+ */
+extern int validate_worktree(const struct worktree *wt,
+			     struct strbuf *errmsg);
+
 /*
  * Free up the memory for worktree(s)
  */
-- 
2.16.1.399.g632f88eed1
Nguyễn Thái Ngọc Duy· Feb 12, 2018, 09:49 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 worktree.c | 17 +++++++++++++++++
 worktree.h |  6 ++++++
 2 files changed, 23 insertions(+)
Show changes to 2 files +23 −0

worktree.c, worktree.h

diff --git a/worktree.c b/worktree.c
index b238d87bf1..0373faf0dc 100644
--- a/worktree.c
+++ b/worktree.c
@@ -326,6 +326,23 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)
 	return ret;
 }
 
+void update_worktree_location(struct worktree *wt, const char *path_)
+{
+	struct strbuf path = STRBUF_INIT;
+
+	if (is_main_worktree(wt))
+		die("BUG: can't relocate main worktree");
+
+	strbuf_realpath(&path, path_, 1);
+	if (fspathcmp(wt->path, path.buf)) {
+		write_file(git_common_path("worktrees/%s/gitdir", wt->id),
+			   "%s/.git", path.buf);
+		free(wt->path);
+		wt->path = strbuf_detach(&path, NULL);
+	}
+	strbuf_release(&path);
+}
+
 int is_worktree_being_rebased(const struct worktree *wt,
 			      const char *target)
 {
diff --git a/worktree.h b/worktree.h
index cb577de8cd..a913428c3d 100644
--- a/worktree.h
+++ b/worktree.h
@@ -68,6 +68,12 @@ extern const char *is_worktree_locked(struct worktree *wt);
 extern int validate_worktree(const struct worktree *wt,
 			     struct strbuf *errmsg);
 
+/*
+ * Update worktrees/xxx/gitdir with the new path.
+ */
+extern void update_worktree_location(struct worktree *wt,
+				     const char *path_);
+
 /*
  * Free up the memory for worktree(s)
  */
-- 
2.16.1.399.g632f88eed1
Nguyễn Thái Ngọc Duy· Feb 12, 2018, 09:49 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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       | 11 ++++++++++-
 strbuf.c                 |  8 ++++++++
 strbuf.h                 |  3 +++
 t/t2028-worktree-move.sh | 11 +++++++++++
 4 files changed, 32 insertions(+), 1 deletion(-)
Show changes to 4 files +32 −1

builtin/worktree.c, strbuf.c, strbuf.h, t/t2028-worktree-move.sh

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 8b9482d1e5..6fe41313c9 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -631,8 +631,17 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 		die(_("'%s' is not a working tree"), av[0]);
 	if (is_main_worktree(wt))
 		die(_("'%s' is a main working tree"), av[0]);
+	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_trim_trailing_dir_sep(&dst);
+		strbuf_addstr(&dst, sep);
+	}
 	if (file_exists(dst.buf))
-		die(_("target '%s' already exists"), av[1]);
+		die(_("target '%s' already exists"), dst.buf);
 
 	reason = is_worktree_locked(wt);
 	if (reason) {
diff --git a/strbuf.c b/strbuf.c
index 1df674e919..46930ad027 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -95,6 +95,7 @@ void strbuf_trim(struct strbuf *sb)
 	strbuf_rtrim(sb);
 	strbuf_ltrim(sb);
 }
+
 void strbuf_rtrim(struct strbuf *sb)
 {
 	while (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1]))
@@ -102,6 +103,13 @@ void strbuf_rtrim(struct strbuf *sb)
 	sb->buf[sb->len] = '\0';
 }
 
+void strbuf_trim_trailing_dir_sep(struct strbuf *sb)
+{
+	while (sb->len > 0 && is_dir_sep((unsigned char)sb->buf[sb->len - 1]))
+		sb->len--;
+	sb->buf[sb->len] = '\0';
+}
+
 void strbuf_ltrim(struct strbuf *sb)
 {
 	char *b = sb->buf;
diff --git a/strbuf.h b/strbuf.h
index 14c8c10d66..e6cae5f439 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -179,6 +179,9 @@ extern void strbuf_trim(struct strbuf *);
 extern void strbuf_rtrim(struct strbuf *);
 extern void strbuf_ltrim(struct strbuf *);
 
+/* Strip trailing directory separators */
+extern void strbuf_trim_trailing_dir_sep(struct strbuf *);
+
 /**
  * Replace the contents of the strbuf with a reencoded form.  Returns -1
  * on error, 0 on success.
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 0f8abc0854..deb486cd8e 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -85,4 +85,15 @@ test_expect_success 'move main worktree' '
 	test_must_fail git worktree move . def
 '
 
+test_expect_success 'move worktree to another dir' '
+	toplevel="$(pwd)" &&
+	mkdir some-dir &&
+	git worktree move destination some-dir &&
+	test_path_is_missing source &&
+	git worktree list --porcelain | grep "^worktree.*/some-dir/destination" &&
+	git -C some-dir/destination log --format=%s >actual2 &&
+	echo init >expected2 &&
+	test_cmp expected2 actual2
+'
+
 test_done
-- 
2.16.1.399.g632f88eed1
Nguyễn Thái Ngọc Duy· Feb 12, 2018, 09:49 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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>
---
 Documentation/git-worktree.txt |  2 +-
 builtin/worktree.c             | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)
Show changes to 2 files +24 −1

Documentation/git-worktree.txt, builtin/worktree.c

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 4fa1dd3a48..e6764ee8e0 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -79,7 +79,7 @@ with `--reason`.
 move::
 
 Move a working tree to a new location. Note that the main working tree
-cannot be moved.
+or linked working trees containing submodules cannot be moved.
 
 prune::
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 6fe41313c9..4789cebe11 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -606,6 +606,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 = { NULL };
+	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(_("working trees containing submodules cannot be moved"));
+}
+
 static int move_worktree(int ac, const char **av, const char *prefix)
 {
 	struct option options[] = {
@@ -643,6 +664,8 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	if (file_exists(dst.buf))
 		die(_("target '%s' already exists"), dst.buf);
 
+	validate_no_submodules(wt);
+
 	reason = is_worktree_locked(wt);
 	if (reason) {
 		if (*reason)
-- 
2.16.1.399.g632f88eed1
Nguyễn Thái Ngọc Duy· Feb 12, 2018, 09:49 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 6/7] worktree remove: new command

This command allows to delete a worktree. Like 'move' you cannot remove the main worktree, or one with submodules inside [1].

For deleting $GIT_WORK_TREE, Untracked files or any staged entries are considered precious and therefore prevent removal by default. Ignored files are not precious.

When it comes to deleting $GIT_DIR, there's no "clean" check because there should not be any valuable data in there, except:

- HEAD reflog. There is nothing we can do about this until somebody
  steps up and implements the ref graveyard.
- Detached HEAD. Technically it can still be recovered. Although it
  may be nice to warn about orphan commits like 'git checkout' does.
[1] We do 'git status' with --ignore-submodules=all for safety
    anyway. But this needs a closer look by submodule people before we
    can allow deletion. For example, if a submodule is totally clean,
    but its repo not absorbed to the main .git dir, then deleting
    worktree also deletes the valuable .submodule repo too.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-worktree.txt         |  21 ++--
 builtin/worktree.c                     | 134 ++++++++++++++++++++++++-
 contrib/completion/git-completion.bash |   5 +-
 t/t2028-worktree-move.sh               |  30 ++++++
 4 files changed, 179 insertions(+), 11 deletions(-)
Show changes to 4 files +179 −11

Documentation/git-worktree.txt, builtin/worktree.c, contrib/completion/git-completion.bash, t/t2028-worktree-move.sh

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index e6764ee8e0..d322acbc67 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
@@ -85,6 +86,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 or ones with submodules 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.
@@ -94,9 +102,10 @@ OPTIONS
 
 -f::
 --force::
-	By default, `add` refuses to create a new working tree when `<commit-ish>` is a branch name and
-	is already checked out by another working tree. This option overrides
-	that safeguard.
+	By default, `add` refuses to create a new working tree when
+	`<commit-ish>` is a branch name and 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>::
@@ -278,12 +287,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 4789cebe11..990e47b315 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -19,6 +19,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
 };
@@ -624,7 +625,7 @@ static void validate_no_submodules(const struct worktree *wt)
 	discard_index(&istate);
 
 	if (found_submodules)
-		die(_("working trees containing submodules cannot be moved"));
+		die(_("working trees containing submodules cannot be moved or removed"));
 }
 
 static int move_worktree(int ac, const char **av, const char *prefix)
@@ -688,6 +689,135 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	return 0;
 }
 
+/*
+ * Note, "git status --porcelain" is used to determine if it's safe to
+ * delete a whole worktree. "git status" does not ignore user
+ * configuration, so if a normal "git status" shows "clean" for the
+ * user, then it's ok to remove it.
+ *
+ * This assumption may be a bad one. We may want to ignore
+ * (potentially bad) user settings and only delete a worktree when
+ * it's absolutely safe to do so from _our_ point of view because we
+ * know better.
+ */
+static void check_clean_worktree(struct worktree *wt,
+				 const char *original_path)
+{
+	struct argv_array child_env = ARGV_ARRAY_INIT;
+	struct child_process cp;
+	char buf[1];
+	int ret;
+
+	/*
+	 * Until we sort this out, all submodules are "dirty" and
+	 * will abort this function.
+	 */
+	validate_no_submodules(wt);
+
+	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", "--ignore-submodules=none",
+			 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'"),
+			  original_path);
+	ret = xread(cp.out, buf, sizeof(buf));
+	if (ret)
+		die(_("'%s' is dirty, use --force to delete it"),
+		    original_path);
+	close(cp.out);
+	ret = finish_command(&cp);
+	if (ret)
+		die_errno(_("failed to run 'git status' on '%s', code %d"),
+			  original_path, ret);
+}
+
+static int delete_git_work_tree(struct worktree *wt)
+{
+	struct strbuf sb = STRBUF_INIT;
+	int ret = 0;
+
+	strbuf_addstr(&sb, wt->path);
+	if (remove_dir_recursively(&sb, 0)) {
+		error_errno(_("failed to delete '%s'"), sb.buf);
+		ret = -1;
+	}
+	strbuf_release(&sb);
+	return ret;
+}
+
+static int delete_git_dir(struct worktree *wt)
+{
+	struct strbuf sb = STRBUF_INIT;
+	int ret = 0;
+
+	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);
+	return ret;
+}
+
+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 errmsg = 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 tree"), av[0]);
+	if (is_main_worktree(wt))
+		die(_("'%s' is a main working tree"), av[0]);
+	reason = is_worktree_locked(wt);
+	if (reason) {
+		if (*reason)
+			die(_("cannot remove a locked working tree, lock reason: %s"),
+			    reason);
+		die(_("cannot remove a locked working tree"));
+	}
+	if (validate_worktree(wt, &errmsg))
+		die(_("validation failed, cannot remove working tree: %s"),
+		    errmsg.buf);
+	strbuf_release(&errmsg);
+
+	if (!force)
+		check_clean_worktree(wt, av[0]);
+
+	ret |= delete_git_work_tree(wt);
+	/*
+	 * continue on even if ret is non-zero, there's no going back
+	 * from here.
+	 */
+	ret |= delete_git_dir(wt);
+
+	free_worktrees(worktrees);
+	return ret;
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
 	struct option options[] = {
@@ -712,5 +842,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 b87d387686..ff4a39631e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3087,7 +3087,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"
@@ -3105,6 +3105,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 deb486cd8e..4718c4552f 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -96,4 +96,34 @@ test_expect_success 'move worktree to another dir' '
 	test_cmp expected2 actual2
 '
 
+test_expect_success 'remove main worktree' '
+	test_must_fail git worktree remove .
+'
+
+test_expect_success 'move some-dir/destination back' '
+	git worktree move some-dir/destination destination
+'
+
+test_expect_success 'remove locked worktree' '
+	git worktree lock destination &&
+	test_when_finished "git worktree unlock destination" &&
+	test_must_fail git worktree remove destination
+'
+
+test_expect_success 'remove worktree with dirty tracked file' '
+	echo dirty >>destination/init.t &&
+	test_when_finished "git -C destination checkout init.t" &&
+	test_must_fail git worktree remove destination
+'
+
+test_expect_success 'remove worktree with untracked file' '
+	: >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.16.1.399.g632f88eed1
Nguyễn Thái Ngọc Duy· Feb 12, 2018, 09:49 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 7/7] worktree remove: allow it when $GIT_WORK_TREE is already gone

"git worktree remove" basically consists of two things
- delete $GIT_WORK_TREE
- delete $GIT_DIR (which is $SUPER_GIT_DIR/worktrees/something)

If $GIT_WORK_TREE is already gone for some reason, we should be able to finish the job by deleting $GIT_DIR.

Two notes:
- $GIT_WORK_TREE _can_ be missing if the worktree is locked. In that
  case we must not delete $GIT_DIR because the real $GIT_WORK_TREE may
  be in a usb stick somewhere. This is already handled because we
  check for lock first.
- validate_worktree() is still called because it may do more checks in
  future (and it already does something else, like checking main
  worktree, but that's irrelevant in this case)
Noticed-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/worktree.c       | 12 +++++++-----
 t/t2028-worktree-move.sh | 17 +++++++++++++++++
 worktree.c               |  9 ++++++++-
 worktree.h               |  5 ++++-
 4 files changed, 36 insertions(+), 7 deletions(-)
Show changes to 4 files +36 −7

builtin/worktree.c, t/t2028-worktree-move.sh, worktree.c, worktree.h

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 990e47b315..f77ef994c4 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -674,7 +674,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 			    reason);
 		die(_("cannot move a locked working tree"));
 	}
-	if (validate_worktree(wt, &errmsg))
+	if (validate_worktree(wt, &errmsg, 0))
 		die(_("validation failed, cannot move working tree: %s"),
 		    errmsg.buf);
 	strbuf_release(&errmsg);
@@ -799,15 +799,17 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
 			    reason);
 		die(_("cannot remove a locked working tree"));
 	}
-	if (validate_worktree(wt, &errmsg))
+	if (validate_worktree(wt, &errmsg, WT_VALIDATE_WORKTREE_MISSING_OK))
 		die(_("validation failed, cannot remove working tree: %s"),
 		    errmsg.buf);
 	strbuf_release(&errmsg);
 
-	if (!force)
-		check_clean_worktree(wt, av[0]);
+	if (file_exists(wt->path)) {
+		if (!force)
+			check_clean_worktree(wt, av[0]);
 
-	ret |= delete_git_work_tree(wt);
+		ret |= delete_git_work_tree(wt);
+	}
 	/*
 	 * continue on even if ret is non-zero, there's no going back
 	 * from here.
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 4718c4552f..082368d8c6 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -126,4 +126,21 @@ test_expect_success 'force remove worktree with untracked file' '
 	test_path_is_missing destination
 '
 
+test_expect_success 'remove missing worktree' '
+	git worktree add to-be-gone &&
+	test -d .git/worktrees/to-be-gone &&
+	mv to-be-gone gone &&
+	git worktree remove to-be-gone &&
+	test_path_is_missing .git/worktrees/to-be-gone
+'
+
+test_expect_success 'NOT remove missing-but-locked worktree' '
+	git worktree add gone-but-locked &&
+	git worktree lock gone-but-locked &&
+	test -d .git/worktrees/gone-but-locked &&
+	mv gone-but-locked really-gone-now &&
+	test_must_fail git worktree remove gone-but-locked &&
+	test_path_is_dir .git/worktrees/gone-but-locked
+'
+
 test_done
diff --git a/worktree.c b/worktree.c
index 0373faf0dc..28989cf06e 100644
--- a/worktree.c
+++ b/worktree.c
@@ -267,7 +267,8 @@ static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...)
 	va_end(params);
 }
 
-int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)
+int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
+		      unsigned flags)
 {
 	struct strbuf wt_path = STRBUF_INIT;
 	char *path = NULL;
@@ -303,6 +304,12 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)
 		goto done;
 	}
 
+	if (flags & WT_VALIDATE_WORKTREE_MISSING_OK &&
+	    !file_exists(wt->path)) {
+		ret = 0;
+		goto done;
+	}
+
 	if (!file_exists(wt_path.buf)) {
 		strbuf_addf_gently(errmsg, _("'%s' does not exist"), wt_path.buf);
 		goto done;
diff --git a/worktree.h b/worktree.h
index a913428c3d..fe38ce10c3 100644
--- a/worktree.h
+++ b/worktree.h
@@ -61,12 +61,15 @@ extern int is_main_worktree(const struct worktree *wt);
  */
 extern const char *is_worktree_locked(struct worktree *wt);
 
+#define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)
+
 /*
  * Return zero if the worktree is in good condition. Error message is
  * returned if "errmsg" is not NULL.
  */
 extern int validate_worktree(const struct worktree *wt,
-			     struct strbuf *errmsg);
+			     struct strbuf *errmsg,
+			     unsigned flags);
 
 /*
  * Update worktrees/xxx/gitdir with the new path.
-- 
2.16.1.399.g632f88eed1
Nguyễn Thái Ngọc Duy· Feb 12, 2018, 09:49 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 3/7] worktree move: new command

This command allows to relocate linked worktrees. Main worktree cannot (yet) be moved.

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         | 13 ++++---
 builtin/worktree.c                     | 53 ++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |  2 +-
 t/t2028-worktree-move.sh               | 26 +++++++++++++
 4 files changed, 87 insertions(+), 7 deletions(-)
Show changes to 4 files +87 −7

Documentation/git-worktree.txt, builtin/worktree.c, contrib/completion/git-completion.bash, t/t2028-worktree-move.sh

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 41585f535d..4fa1dd3a48 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -12,6 +12,7 @@ SYNOPSIS
 'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b <new-branch>] <path> [<commit-ish>]
 '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>
 
@@ -34,10 +35,6 @@ The working tree's administrative files in the repository (see
 `git worktree prune` in the main or any linked working tree to
 clean up any stale administrative files.
 
-If you move a linked working tree, you need to manually update the
-administrative files so that they do not get pruned automatically. See
-section "DETAILS" for more information.
-
 If a linked working tree is stored on a portable device or network share
 which is not always mounted, you can prevent its administrative files from
 being pruned by issuing the `git worktree lock` command, optionally
@@ -79,6 +76,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.
+
 prune::
 
 Prune working tree information in $GIT_DIR/worktrees.
@@ -196,7 +198,7 @@ thumb is do not make any assumption about whether a path belongs to
 $GIT_DIR or $GIT_COMMON_DIR when you need to directly access something
 inside $GIT_DIR. Use `git rev-parse --git-path` to get the final path.
 
-If you move a linked working tree, you need to update the 'gitdir' file
+If you manually move a linked working tree, you need to update the 'gitdir' file
 in the entry's directory. For example, if a linked working tree is moved
 to `/newpath/test-next` and its `.git` file points to
 `/path/main/.git/worktrees/test-next`, then update
@@ -281,7 +283,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 7cef5b120b..8b9482d1e5 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -17,6 +17,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
@@ -605,6 +606,56 @@ 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;
+	struct strbuf errmsg = STRBUF_INIT;
+	const char *reason;
+	char *path;
+
+	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+	if (ac != 2)
+		usage_with_options(worktree_usage, options);
+
+	path = prefix_filename(prefix, av[1]);
+	strbuf_addstr(&dst, path);
+	free(path);
+
+	worktrees = get_worktrees(0);
+	wt = find_worktree(worktrees, prefix, av[0]);
+	if (!wt)
+		die(_("'%s' is not a working tree"), av[0]);
+	if (is_main_worktree(wt))
+		die(_("'%s' is a main working tree"), av[0]);
+	if (file_exists(dst.buf))
+		die(_("target '%s' already exists"), av[1]);
+
+	reason = is_worktree_locked(wt);
+	if (reason) {
+		if (*reason)
+			die(_("cannot move a locked working tree, lock reason: %s"),
+			    reason);
+		die(_("cannot move a locked working tree"));
+	}
+	if (validate_worktree(wt, &errmsg))
+		die(_("validation failed, cannot move working tree: %s"),
+		    errmsg.buf);
+	strbuf_release(&errmsg);
+
+	if (rename(wt->path, dst.buf) == -1)
+		die_errno(_("failed to move '%s' to '%s'"), wt->path, dst.buf);
+
+	update_worktree_location(wt, dst.buf);
+
+	strbuf_release(&dst);
+	free_worktrees(worktrees);
+	return 0;
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
 	struct option options[] = {
@@ -627,5 +678,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 3683c772c5..b87d387686 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3087,7 +3087,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..0f8abc0854 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -59,4 +59,30 @@ 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_when_finished "git worktree unlock source" &&
+	test_must_fail git worktree move source destination
+'
+
+test_expect_success 'move worktree' '
+	toplevel="$(pwd)" &&
+	git worktree move source destination &&
+	test_path_is_missing source &&
+	git worktree list --porcelain | grep "^worktree.*/destination" &&
+	! git worktree list --porcelain | grep "^worktree.*/source" >empty &&
+	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.16.1.399.g632f88eed1
Eric Sunshine· Mar 4, 2018, 05:26 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH] t2028: fix minor error and issues in newly-added "worktree move" tests

Recently-added "git worktree move" tests include a minor error and a few small issues. Specifically:

* checking non-existence of wrong file ("source" instead of
  "destination")
* unneeded redirect (">empty")
* unused variable ("toplevel")
* restoring a worktree location by means of a separate test somewhat
  distant from the test which moved it rather than using
  test_when_finished() to restore it in a self-contained fashion
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
This patch is built atop nd/worktree-move-reboot in 'next'.

I didn't get around to doing a proper review of nd/worktree-move-reboot v2 [1] until after it had graduated to 'next'. Although v2 fixed all the issues identified in my review of v1 [2], it introduced a few minor issues of its own. This patch addresses those issues.

[1]: https://public-inbox.org/git/20180212094940.23834-1-pclouds@gmail.com/ [2]: https://public-inbox.org/git/20180124095357.19645-1-pclouds@gmail.com/

 t/t2028-worktree-move.sh | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)
Show changes to t/t2028-worktree-move.sh +3 −7
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 082368d8c6..d70d13dabe 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -75,7 +75,7 @@ test_expect_success 'move worktree' '
 	git worktree move source destination &&
 	test_path_is_missing source &&
 	git worktree list --porcelain | grep "^worktree.*/destination" &&
-	! git worktree list --porcelain | grep "^worktree.*/source" >empty &&
+	! git worktree list --porcelain | grep "^worktree.*/source" &&
 	git -C destination log --format=%s >actual2 &&
 	echo init >expected2 &&
 	test_cmp expected2 actual2
@@ -86,10 +86,10 @@ test_expect_success 'move main worktree' '
 '
 
 test_expect_success 'move worktree to another dir' '
-	toplevel="$(pwd)" &&
 	mkdir some-dir &&
 	git worktree move destination some-dir &&
-	test_path_is_missing source &&
+	test_when_finished "git worktree move some-dir/destination destination" &&
+	test_path_is_missing destination &&
 	git worktree list --porcelain | grep "^worktree.*/some-dir/destination" &&
 	git -C some-dir/destination log --format=%s >actual2 &&
 	echo init >expected2 &&
@@ -100,10 +100,6 @@ test_expect_success 'remove main worktree' '
 	test_must_fail git worktree remove .
 '
 
-test_expect_success 'move some-dir/destination back' '
-	git worktree move some-dir/destination destination
-'
-
 test_expect_success 'remove locked worktree' '
 	git worktree lock destination &&
 	test_when_finished "git worktree unlock destination" &&
-- 
2.16.2.660.g709887971b
Duy Nguyen· Mar 5, 2018, 09:32 UTC · re: Eric Sunshine · lore

Re: [PATCH] t2028: fix minor error and issues in newly-added "worktree move" tests

On Sun, Mar 4, 2018 at 12:26 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 13 quoted lines
> Recently-added "git worktree move" tests include a minor error and a few
> small issues. Specifically:
>
> * checking non-existence of wrong file ("source" instead of
>   "destination")
>
> * unneeded redirect (">empty")
>
> * unused variable ("toplevel")
>
> * restoring a worktree location by means of a separate test somewhat
>   distant from the test which moved it rather than using
>   test_when_finished() to restore it in a self-contained fashion
Argh... You're right again :) This looks good.
Show 57 quoted lines
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>
> This patch is built atop nd/worktree-move-reboot in 'next'.
>
> I didn't get around to doing a proper review of nd/worktree-move-reboot
> v2 [1] until after it had graduated to 'next'. Although v2 fixed all the
> issues identified in my review of v1 [2], it introduced a few minor
> issues of its own. This patch addresses those issues.
>
> [1]: https://public-inbox.org/git/20180212094940.23834-1-pclouds@gmail.com/
> [2]: https://public-inbox.org/git/20180124095357.19645-1-pclouds@gmail.com/
>
>  t/t2028-worktree-move.sh | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> index 082368d8c6..d70d13dabe 100755
> --- a/t/t2028-worktree-move.sh
> +++ b/t/t2028-worktree-move.sh
> @@ -75,7 +75,7 @@ test_expect_success 'move worktree' '
>         git worktree move source destination &&
>         test_path_is_missing source &&
>         git worktree list --porcelain | grep "^worktree.*/destination" &&
> -       ! git worktree list --porcelain | grep "^worktree.*/source" >empty &&
> +       ! git worktree list --porcelain | grep "^worktree.*/source" &&
>         git -C destination log --format=%s >actual2 &&
>         echo init >expected2 &&
>         test_cmp expected2 actual2
> @@ -86,10 +86,10 @@ test_expect_success 'move main worktree' '
>  '
>
>  test_expect_success 'move worktree to another dir' '
> -       toplevel="$(pwd)" &&
>         mkdir some-dir &&
>         git worktree move destination some-dir &&
> -       test_path_is_missing source &&
> +       test_when_finished "git worktree move some-dir/destination destination" &&
> +       test_path_is_missing destination &&
>         git worktree list --porcelain | grep "^worktree.*/some-dir/destination" &&
>         git -C some-dir/destination log --format=%s >actual2 &&
>         echo init >expected2 &&
> @@ -100,10 +100,6 @@ test_expect_success 'remove main worktree' '
>         test_must_fail git worktree remove .
>  '
>
> -test_expect_success 'move some-dir/destination back' '
> -       git worktree move some-dir/destination destination
> -'
> -
>  test_expect_success 'remove locked worktree' '
>         git worktree lock destination &&
>         test_when_finished "git worktree unlock destination" &&
> --
> 2.16.2.660.g709887971b
>
-- 
Duy
SZEDER Gábor· Mar 5, 2018, 12:48 UTC · re: Eric Sunshine · lore

Re: [PATCH] t2028: fix minor error and issues in newly-added "worktree move" tests

Show 13 quoted lines
> Recently-added "git worktree move" tests include a minor error and a few
> small issues. Specifically:
> 
> * checking non-existence of wrong file ("source" instead of
>   "destination")
> 
> * unneeded redirect (">empty")
> 
> * unused variable ("toplevel")
> 
> * restoring a worktree location by means of a separate test somewhat
>   distant from the test which moved it rather than using
>   test_when_finished() to restore it in a self-contained fashion
There is one more issue in these tests.
 
Show 10 quoted lines
> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> index 082368d8c6..d70d13dabe 100755
> --- a/t/t2028-worktree-move.sh
> +++ b/t/t2028-worktree-move.sh
> @@ -75,7 +75,7 @@ test_expect_success 'move worktree' '
>  	git worktree move source destination &&
>  	test_path_is_missing source &&
>  	git worktree list --porcelain | grep "^worktree.*/destination" &&
> -	! git worktree list --porcelain | grep "^worktree.*/source" >empty &&
> +	! git worktree list --porcelain | grep "^worktree.*/source" &&

The main purpose of this test script is to test the 'git worktree' command, but these pipes hide its exit code. Could you please save 'git worktree's output into an intermediate file, and run 'grep' on the file's contents?

This also applies to two other tests in this test script.
Show 31 quoted lines
>  	git -C destination log --format=%s >actual2 &&
>  	echo init >expected2 &&
>  	test_cmp expected2 actual2
> @@ -86,10 +86,10 @@ test_expect_success 'move main worktree' '
>  '
>  
>  test_expect_success 'move worktree to another dir' '
> -	toplevel="$(pwd)" &&
>  	mkdir some-dir &&
>  	git worktree move destination some-dir &&
> -	test_path_is_missing source &&
> +	test_when_finished "git worktree move some-dir/destination destination" &&
> +	test_path_is_missing destination &&
>  	git worktree list --porcelain | grep "^worktree.*/some-dir/destination" &&
>  	git -C some-dir/destination log --format=%s >actual2 &&
>  	echo init >expected2 &&
> @@ -100,10 +100,6 @@ test_expect_success 'remove main worktree' '
>  	test_must_fail git worktree remove .
>  '
>  
> -test_expect_success 'move some-dir/destination back' '
> -	git worktree move some-dir/destination destination
> -'
> -
>  test_expect_success 'remove locked worktree' '
>  	git worktree lock destination &&
>  	test_when_finished "git worktree unlock destination" &&
> -- 
> 2.16.2.660.g709887971b
> 
> 
Junio C Hamano· Mar 5, 2018, 18:37 UTC · re: SZEDER Gábor · lore

Re: [PATCH] t2028: fix minor error and issues in newly-added "worktree move" tests

SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 6 quoted lines
> There is one more issue in these tests.
> ...
> The main purpose of this test script is to test the 'git worktree'
> command, but these pipes hide its exit code.
> Could you please save 'git worktree's output into an intermediate
> file, and run 'grep' on the file's contents?

Here is what I tentatively came up with, while deciding what should be queued based on Eric's patch, as a possible squash/fixup.

 t/t2028-worktree-move.sh | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
Show changes to t/t2028-worktree-move.sh +8 −4
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index d70d13dabe..1c391f370e 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -7,7 +7,8 @@ test_description='test git worktree move, remove, lock and unlock'
 test_expect_success 'setup' '
 	test_commit init &&
 	git worktree add source &&
-	git worktree list --porcelain | grep "^worktree" >actual &&
+	git worktree list --porcelain >out &&
+	grep "^worktree" out >actual &&
 	cat <<-EOF >expected &&
 	worktree $(pwd)
 	worktree $(pwd)/source
@@ -74,8 +75,10 @@ test_expect_success 'move worktree' '
 	toplevel="$(pwd)" &&
 	git worktree move source destination &&
 	test_path_is_missing source &&
-	git worktree list --porcelain | grep "^worktree.*/destination" &&
-	! git worktree list --porcelain | grep "^worktree.*/source" &&
+	git worktree list --porcelain >out &&
+	grep "^worktree.*/destination" out &&
+	git worktree list --porcelain >out &&
+	! grep "^worktree.*/source" out &&
 	git -C destination log --format=%s >actual2 &&
 	echo init >expected2 &&
 	test_cmp expected2 actual2
@@ -90,7 +93,8 @@ test_expect_success 'move worktree to another dir' '
 	git worktree move destination some-dir &&
 	test_when_finished "git worktree move some-dir/destination destination" &&
 	test_path_is_missing destination &&
-	git worktree list --porcelain | grep "^worktree.*/some-dir/destination" &&
+	git worktree list --porcelain >out &&
+	grep "^worktree.*/some-dir/destination" out &&
 	git -C some-dir/destination log --format=%s >actual2 &&
 	echo init >expected2 &&
 	test_cmp expected2 actual2
Eric Sunshine· Mar 5, 2018, 18:44 UTC · re: Junio C Hamano · lore

Re: [PATCH] t2028: fix minor error and issues in newly-added "worktree move" tests

On Mon, Mar 5, 2018 at 1:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> Could you please save 'git worktree's output into an intermediate
>> file, and run 'grep' on the file's contents?
>
> Here is what I tentatively came up with, while deciding what should
> be queued based on Eric's patch, as a possible squash/fixup.
Thanks for saving a round-trip. One comment below...
Show 11 quoted lines
> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> @@ -74,8 +75,10 @@ test_expect_success 'move worktree' '
>         toplevel="$(pwd)" &&
>         git worktree move source destination &&
>         test_path_is_missing source &&
> -       git worktree list --porcelain | grep "^worktree.*/destination" &&
> -       ! git worktree list --porcelain | grep "^worktree.*/source" &&
> +       git worktree list --porcelain >out &&
> +       grep "^worktree.*/destination" out &&
> +       git worktree list --porcelain >out &&
> +       ! grep "^worktree.*/source" out &&

The second "git worktree list --porcelain >out" can be dropped, leaving the two grep's back-to-back since it they can test the same 'out' file

>         git -C destination log --format=%s >actual2 &&
>         echo init >expected2 &&
>         test_cmp expected2 actual2
Eric Sunshine· Mar 4, 2018, 05:36 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v2 0/7] nd/worktree-move reboot

On Mon, Feb 12, 2018 at 4:49 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> v2 basically fixes lots of comments from Eric (many thanks!): memory
> leak, typos, document updates, tests, corner case fixes.
> Interdiff:

Thanks, I finally got around to doing a full re-read of the entire series. v2 appears to address all issues raised in my review[1] of v1.

I did find a few minor issues in tests newly added and revised in v2. However, since v2 already migrated to 'next', rather than pointing out the issues as review comments, I instead sent a patch[2] (built atop nd/worktree-move-reboot) to address them.

[1]: https://public-inbox.org/git/20180124095357.19645-1-pclouds@gmail.com/ [2]: https://public-inbox.org/git/20180304052647.26614-1-sunshine@sunshineco.com/

← back to recent threads