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

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

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

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

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

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

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

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

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

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

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

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

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

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

[Eric Sunshine]

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?

> +               free(wt->path);
> +               wt->path = strbuf_detach(&path, NULL);
> +       }
> +       strbuf_release(&path);
> +}

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

[Duy Nguyen]

On Fri, Feb 2, 2018 at 3:23 PM, 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:
>> 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

[PATCH 3/7] worktree move: new command

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

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

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

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

[Eric Sunshine]

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> 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...

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

> +       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.

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

> +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/

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

[Eric Sunshine]

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.

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

[Duy Nguyen]

On Fri, Feb 2, 2018 at 6:23 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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

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

[Jeff King]

On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote:

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

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

[Martin Ågren]

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. :-)

> 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

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

[Duy Nguyen]

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.
-- 
Duy

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

[Martin Ågren]

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. I could try to look into it in the next couple of days.

Martin

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

[Duy Nguyen]

On Mon, Feb 12, 2018 at 11:15:06PM +0100, Martin Ågren wrote:
> 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< --
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

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

[Jeff King]

On Tue, Feb 13, 2018 at 07:27:58AM +0700, Duy Nguyen wrote:

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

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

[Duy Nguyen]

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!

> 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

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

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

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

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

[Martin Ågren]

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

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

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

[Jeff King]

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

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

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

[Martin Ågren]

On 19 February 2018 at 22:29, Jeff King <peff@peff.net> wrote:
> 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.

> 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

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

[Jeff King]

On Tue, Feb 20, 2018 at 09:44:51PM +0100, Martin Ågren wrote:

> 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

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

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

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

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

[Jeff King]

On Wed, Feb 21, 2018 at 08:53:16AM -0800, Junio C Hamano wrote:

> 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

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

[Kaartic Sivaraam]

[-- Attachment #1.1: Type: text/plain, Size: 725 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

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

[Martin Ågren]

On 25 February 2018 at 04:48, Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> wrote:
> 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

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

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

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

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

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

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

[Eric Sunshine]

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> 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.)

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

> +               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?

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

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

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

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

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

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

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

[Eric Sunshine]

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> 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).

> +}

[PATCH 6/7] worktree remove: new command

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

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

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

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

[Eric Sunshine]

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.

> +       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.

> +       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'.

> +
> +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.

> +       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].

> +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/

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

[Duy Nguyen]

On Fri, Feb 2, 2018 at 6:47 PM, 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:
>> 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

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

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

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

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

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

[Eric Sunshine]

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> [...]
> - $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?

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

[Junio C Hamano]

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

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

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

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

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

Interdiff:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/worktree.c       | 11 ++++++++++-
 strbuf.c                 |  8 ++++++++
 strbuf.h                 |  3 +++
 t/t2028-worktree-move.sh | 11 +++++++++++
 4 files changed, 32 insertions(+), 1 deletion(-)

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

[Eric Sunshine]

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

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

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

[Duy Nguyen]

On Sun, Mar 4, 2018 at 12:26 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 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.

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

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

[SZEDER Gábor]

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

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

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

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

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

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

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

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

[Eric Sunshine]

On Mon, Mar 5, 2018 at 1:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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...

> 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

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

[Eric Sunshine]

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/