[PATCH 0/9] worktree: fix bugs and broaden --force applicability

[PATCH 0/9] worktree: fix bugs and broaden --force applicability

[Eric Sunshine]

This series started as a fix for a bug reported by Peff[1] in which the
"database" of worktrees could be corrupted (or at least become
internally inconsistent) by having multiple worktree entries associated
with the same path.

Peff's particular use-case for git-worktree is Documentation/doc-diff
which wants to re-use a worktree if it exists or create it anew if it
doesn't. Unfortunately, this is a bit more difficult than it should be
if the worktree directory is deleted manually (without pruning the
worktree entry) between script invocations. To simplify this use-case
for tools, it was suggested[2] that "git worktree add --force" could
deal with the problem of a registered-but-missing worktree (much as a
tool might rely upon "mkdir -p" to create or re-use an existing
directory). This series implements that proposal, as well.

Fixing the original bug revealed another existing bug, and after several
additional "while we're here" changes, the series ended up a bit longer
than expected.

[1]: https://public-inbox.org/git/20180821193556.GA859@sigill.intra.peff.net/
[2]: https://public-inbox.org/git/CAPig+cTghgbBo5VfZN+VP2VM00nPkhUqm0dOUqO37arxraxBKw@mail.gmail.com/

Eric Sunshine (9):
  worktree: don't die() in library function find_worktree()
  worktree: move delete_git_dir() earlier in file for upcoming new
    callers
  worktree: generalize delete_git_dir() to reduce code duplication
  worktree: prepare for more checks of whether path can become worktree
  worktree: disallow adding same path multiple times
  worktree: teach 'add' to respect --force for registered but missing
    path
  worktree: teach 'move' to override lock when --force given twice
  worktree: teach 'remove' to override lock when --force given twice
  worktree: delete .git/worktrees if empty after 'remove'

 Documentation/git-worktree.txt |  12 +++-
 builtin/worktree.c             | 113 ++++++++++++++++++++++-----------
 t/t2025-worktree-add.sh        |  18 ++++++
 t/t2028-worktree-move.sh       |  44 +++++++++++++
 worktree.c                     |   6 +-
 5 files changed, 154 insertions(+), 39 deletions(-)

-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH 1/9] worktree: don't die() in library function find_worktree()

[Eric Sunshine]

Callers don't expect library function find_worktree() to die(); they
expect it to return the named worktree if found, or NULL if not.
Although find_worktree() itself never invokes die(), it calls
real_pathdup() with 'die_on_error' incorrectly set to 'true', thus will
die() indirectly if the user-provided path is not to real_pathdup()'s
liking. This can be observed, for instance, with any git-worktree
command which searches for an existing worktree:

    $ git worktree unlock foo
    fatal: 'foo' is not a working tree
    $ git worktree unlock foo/bar
    fatal: Invalid path '.../foo': No such file or directory

The first error message is the expected one from "git worktree unlock"
not finding the specified worktree; the second is from find_worktree()
invoking real_pathdup() incorrectly and die()ing prematurely.

Aside from the inconsistent error message between the two cases, this
bug hasn't otherwise been a serious problem since existing callers all
die() anyhow when the worktree can't be found. However, that may not be
true of callers added in the future, so fix find_worktree() to avoid
die()ing.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 t/t2028-worktree-move.sh | 8 ++++++++
 worktree.c               | 6 +++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 5f7d45b7b7..60aba7c41a 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -141,4 +141,12 @@ test_expect_success 'NOT remove missing-but-locked worktree' '
 	test_path_is_dir .git/worktrees/gone-but-locked
 '
 
+test_expect_success 'proper error when worktree not found' '
+	for i in noodle noodle/bork
+	do
+		test_must_fail git worktree lock $i 2>err &&
+		test_i18ngrep "not a working tree" err || return 1
+	done
+'
+
 test_done
diff --git a/worktree.c b/worktree.c
index 97cda5f97b..b0d0b5426d 100644
--- a/worktree.c
+++ b/worktree.c
@@ -217,7 +217,11 @@ struct worktree *find_worktree(struct worktree **list,
 
 	if (prefix)
 		arg = to_free = prefix_filename(prefix, arg);
-	path = real_pathdup(arg, 1);
+	path = real_pathdup(arg, 0);
+	if (!path) {
+		free(to_free);
+		return NULL;
+	}
 	for (; *list; list++)
 		if (!fspathcmp(path, real_path((*list)->path)))
 			break;
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH 2/9] worktree: move delete_git_dir() earlier in file for upcoming new callers

[Eric Sunshine]

This is a pure code movement to avoid having to forward-declare the
function when new callers are subsequently added.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 41e7714396..a8128289cc 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -47,6 +47,20 @@ static int git_worktree_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
+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 prune_worktree(const char *id, struct strbuf *reason)
 {
 	struct stat st;
@@ -822,20 +836,6 @@ static int delete_git_work_tree(struct worktree *wt)
 	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;
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH 3/9] worktree: generalize delete_git_dir() to reduce code duplication

[Eric Sunshine]

prune_worktrees() and delete_git_dir() both remove worktree
administrative entries from .git/worktrees, and their implementations
are nearly identical. The only difference is that prune_worktrees() is
also capable of removing a bogus non-worktree-related file from
.git/worktrees.

Simplify by extending delete_git_dir() to handle the little bit of
extra functionality needed by prune_worktrees(), and drop the
effectively duplicate code from the latter.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index a8128289cc..0affcb476c 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -47,16 +47,17 @@ static int git_worktree_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
-static int delete_git_dir(struct worktree *wt)
+static int delete_git_dir(const char *id)
 {
 	struct strbuf sb = STRBUF_INIT;
-	int ret = 0;
+	int ret;
 
-	strbuf_addstr(&sb, git_common_path("worktrees/%s", wt->id));
-	if (remove_dir_recursively(&sb, 0)) {
+	strbuf_addstr(&sb, git_common_path("worktrees/%s", id));
+	ret = remove_dir_recursively(&sb, 0);
+	if (ret < 0 && errno == ENOTDIR)
+		ret = unlink(sb.buf);
+	if (ret)
 		error_errno(_("failed to delete '%s'"), sb.buf);
-		ret = -1;
-	}
 	strbuf_release(&sb);
 	return ret;
 }
@@ -130,10 +131,8 @@ static int prune_worktree(const char *id, struct strbuf *reason)
 static void prune_worktrees(void)
 {
 	struct strbuf reason = STRBUF_INIT;
-	struct strbuf path = STRBUF_INIT;
 	DIR *dir = opendir(git_path("worktrees"));
 	struct dirent *d;
-	int ret;
 	if (!dir)
 		return;
 	while ((d = readdir(dir)) != NULL) {
@@ -146,18 +145,12 @@ static void prune_worktrees(void)
 			printf("%s\n", reason.buf);
 		if (show_only)
 			continue;
-		git_path_buf(&path, "worktrees/%s", d->d_name);
-		ret = remove_dir_recursively(&path, 0);
-		if (ret < 0 && errno == ENOTDIR)
-			ret = unlink(path.buf);
-		if (ret)
-			error_errno(_("failed to remove '%s'"), path.buf);
+		delete_git_dir(d->d_name);
 	}
 	closedir(dir);
 	if (!show_only)
 		rmdir(git_path("worktrees"));
 	strbuf_release(&reason);
-	strbuf_release(&path);
 }
 
 static int prune(int ac, const char **av, const char *prefix)
@@ -882,7 +875,7 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
 	 * continue on even if ret is non-zero, there's no going back
 	 * from here.
 	 */
-	ret |= delete_git_dir(wt);
+	ret |= delete_git_dir(wt->id);
 
 	free_worktrees(worktrees);
 	return ret;
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH 4/9] worktree: prepare for more checks of whether path can become worktree

[Eric Sunshine]

Certain conditions must be met for a path to be a valid candidate as the
location of a new worktree; for instance, the path must not exist or
must be an empty directory. Although the number of conditions is small,
new conditions will soon be added so factor out the existing checks into
a separate function to avoid further bloating add_worktree().

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 0affcb476c..46c93771e8 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -219,6 +219,12 @@ static const char *worktree_basename(const char *path, int *olen)
 	return name;
 }
 
+static void validate_worktree_add(const char *path, const struct add_opts *opts)
+{
+	if (file_exists(path) && !is_empty_dir(path))
+		die(_("'%s' already exists"), path);
+}
+
 static int add_worktree(const char *path, const char *refname,
 			const struct add_opts *opts)
 {
@@ -233,8 +239,7 @@ static int add_worktree(const char *path, const char *refname,
 	struct commit *commit = NULL;
 	int is_branch = 0;
 
-	if (file_exists(path) && !is_empty_dir(path))
-		die(_("'%s' already exists"), path);
+	validate_worktree_add(path, opts);
 
 	/* is 'refname' a branch or commit? */
 	if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH 5/9] worktree: disallow adding same path multiple times

[Eric Sunshine]

A given path should only ever be associated with a single registered
worktree. This invariant is enforced by refusing to create a new
worktree at a given path if that path already exists. For example:

    $ git worktree add -q --detach foo
    $ git worktree add -q --detach foo
    fatal: 'foo' already exists

However, the check can be fooled, and the invariant broken, if the
path is missing. Continuing the example:

    $ rm -fr foo
    $ git worktree add -q --detach foo
    $ git worktree list
    ...      eadebfe [master]
    .../foo  eadebfe (detached HEAD)
    .../foo  eadebfe (detached HEAD)

This "corruption" leads to the unfortunate situation in which the
worktree can not be removed:

    $ git worktree remove foo
    fatal: validation failed, cannot remove working tree: '.../foo'
    does not point back to '.git/worktrees/foo'

Nor can the bogus entry be pruned:

    $ git worktree prune -v
    $ git worktree list
    ...      eadebfe [master]
    .../foo  eadebfe (detached HEAD)
    .../foo  eadebfe (detached HEAD)

without first deleting the worktree directory manually:

    $ rm -fr foo
    $ git worktree prune -v
    Removing .../foo: gitdir file points to non-existent location
    Removing .../foo1: gitdir file points to non-existent location
    $ git worktree list
    ...  eadebfe [master]

or by manually deleting the worktree entry in .git/worktrees.

To address this problem, upgrade "git worktree add" validation to
allow worktree creation only if the given path is not already
associated with an existing worktree (even if the path itself is
non-existent), thus preventing such bogus worktree entries from being
created in the first place.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c      | 25 +++++++++++++++++++++++++
 t/t2025-worktree-add.sh |  7 +++++++
 2 files changed, 32 insertions(+)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 46c93771e8..1122f27b5f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -221,8 +221,33 @@ static const char *worktree_basename(const char *path, int *olen)
 
 static void validate_worktree_add(const char *path, const struct add_opts *opts)
 {
+	struct worktree **worktrees;
+	struct worktree *wt;
+	int locked;
+
 	if (file_exists(path) && !is_empty_dir(path))
 		die(_("'%s' already exists"), path);
+
+	worktrees = get_worktrees(0);
+	/*
+	 * find_worktree()'s suffix matching may undesirably find the main
+	 * rather than a linked worktree (for instance, when the basenames
+	 * of the main worktree and the one being created are the same).
+	 * We're only interested in linked worktrees, so skip the main
+	 * worktree with +1.
+	 */
+	wt = find_worktree(worktrees + 1, NULL, path);
+	if (!wt)
+		goto done;
+
+	locked = !!is_worktree_locked(wt);
+	if (locked)
+		die(_("'%s' is a missing but locked worktree;\nuse 'unlock' and 'prune' or 'remove' to clear"), path);
+	else
+		die(_("'%s' is a missing but already registered worktree;\nuse 'prune' or 'remove' to clear"), path);
+
+done:
+	free_worktrees(worktrees);
 }
 
 static int add_worktree(const char *path, const char *refname,
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 07d292317c..67fccc6591 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -552,4 +552,11 @@ test_expect_success '"add" in bare repo invokes post-checkout hook' '
 	test_cmp hook.expect goozy/hook.actual
 '
 
+test_expect_success '"add" an existing but missing worktree' '
+	git worktree add --detach pneu &&
+	test_must_fail git worktree add --detach pneu &&
+	rm -fr pneu &&
+	test_must_fail git worktree add --detach pneu
+'
+
 test_done
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH 6/9] worktree: teach 'add' to respect --force for registered but missing path

[Eric Sunshine]

For safety, "git worktree add <path>" will refuse to add a new
worktree at <path> if <path> is already associated with a worktree
entry, even if <path> is missing (for instance, has been deleted or
resides on non-mounted removable media or network share). The typical
way to re-create a worktree at <path> in such a situation is either to
prune all "broken" entries ("git worktree prune") or to selectively
remove the worktree entry manually ("git worktree remove <path>").

However, neither of these approaches ("prune" nor "remove") is
especially convenient, and they may be unsuitable for scripting when a
tool merely wants to re-use a worktree if it exists or create it from
scratch if it doesn't (much as a tool might use "mkdir -p" to re-use
or create a directory).

Therefore, teach 'add' to respect --force as a convenient way to
re-use a path already associated with a worktree entry if the path is
non-existent. For a locked worktree, require --force to be specified
twice.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/git-worktree.txt |  8 ++++++--
 builtin/worktree.c             | 10 ++++++++--
 t/t2025-worktree-add.sh        | 13 ++++++++++++-
 3 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 29a5b7e252..8537692f05 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -120,8 +120,12 @@ OPTIONS
 --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 and `remove` refuses to remove an unclean
-	working tree. This option overrides these safeguards.
+	another working tree, or if `<path>` is already assigned to some
+	working tree but is missing (for instance, if `<path>` was deleted
+	manually). This option overrides these safeguards. To add a missing but
+	locked working tree path, specify `--force` twice.
++
+`remove` refuses to remove an unclean working tree unless `--force` is used.
 
 -b <new-branch>::
 -B <new-branch>::
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 1122f27b5f..3eb2f89b0f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -241,10 +241,16 @@ static void validate_worktree_add(const char *path, const struct add_opts *opts)
 		goto done;
 
 	locked = !!is_worktree_locked(wt);
+	if ((!locked && opts->force) || (locked && opts->force > 1)) {
+		if (delete_git_dir(wt->id))
+		    die(_("unable to re-add worktree '%s'"), path);
+		goto done;
+	}
+
 	if (locked)
-		die(_("'%s' is a missing but locked worktree;\nuse 'unlock' and 'prune' or 'remove' to clear"), path);
+		die(_("'%s' is a missing but locked worktree;\nuse 'add -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear"), path);
 	else
-		die(_("'%s' is a missing but already registered worktree;\nuse 'prune' or 'remove' to clear"), path);
+		die(_("'%s' is a missing but already registered worktree;\nuse 'add -f' to override, or 'prune' or 'remove' to clear"), path);
 
 done:
 	free_worktrees(worktrees);
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 67fccc6591..286bba35d8 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -556,7 +556,18 @@ test_expect_success '"add" an existing but missing worktree' '
 	git worktree add --detach pneu &&
 	test_must_fail git worktree add --detach pneu &&
 	rm -fr pneu &&
-	test_must_fail git worktree add --detach pneu
+	test_must_fail git worktree add --detach pneu &&
+	git worktree add --force --detach pneu
+'
+
+test_expect_success '"add" an existing locked but missing worktree' '
+	git worktree add --detach gnoo &&
+	git worktree lock gnoo &&
+	test_when_finished "git worktree unlock gnoo || :" &&
+	rm -fr gnoo &&
+	test_must_fail git worktree add --detach gnoo &&
+	test_must_fail git worktree add --force --detach gnoo &&
+	git worktree add --force --force --detach gnoo
 '
 
 test_done
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH 7/9] worktree: teach 'move' to override lock when --force given twice

[Eric Sunshine]

For consistency with "add -f -f", which allows a missing but locked
worktree path to be re-used, allow "move -f -f" to override a lock,
as well, as a convenience.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/git-worktree.txt |  3 +++
 builtin/worktree.c             | 13 +++++++++----
 t/t2028-worktree-move.sh       | 14 ++++++++++++++
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 8537692f05..d08b8d8e4f 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -125,6 +125,9 @@ OPTIONS
 	manually). This option overrides these safeguards. To add a missing but
 	locked working tree path, specify `--force` twice.
 +
+`move` refuses to move a locked working tree unless `--force` is specified
+twice.
++
 `remove` refuses to remove an unclean working tree unless `--force` is used.
 
 -b <new-branch>::
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 3eb2f89b0f..354a6c0eb5 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -740,13 +740,17 @@ static void validate_no_submodules(const struct worktree *wt)
 
 static int move_worktree(int ac, const char **av, const char *prefix)
 {
+	int force = 0;
 	struct option options[] = {
+		OPT__FORCE(&force,
+			 N_("force move even if worktree is dirty or locked"),
+			 PARSE_OPT_NOCOMPLETE),
 		OPT_END()
 	};
 	struct worktree **worktrees, *wt;
 	struct strbuf dst = STRBUF_INIT;
 	struct strbuf errmsg = STRBUF_INIT;
-	const char *reason;
+	const char *reason = NULL;
 	char *path;
 
 	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
@@ -777,12 +781,13 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 
 	validate_no_submodules(wt);
 
-	reason = is_worktree_locked(wt);
+	if (force < 2)
+		reason = is_worktree_locked(wt);
 	if (reason) {
 		if (*reason)
-			die(_("cannot move a locked working tree, lock reason: %s"),
+			die(_("cannot move a locked working tree, lock reason: %s\nuse 'move -f -f' to override or unlock first"),
 			    reason);
-		die(_("cannot move a locked working tree"));
+		die(_("cannot move a locked working tree;\nuse 'move -f -f' to override or unlock first"));
 	}
 	if (validate_worktree(wt, &errmsg, 0))
 		die(_("validation failed, cannot move working tree: %s"),
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 60aba7c41a..9756ede8f1 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -98,6 +98,20 @@ test_expect_success 'move worktree to another dir' '
 	test_cmp expected2 actual2
 '
 
+test_expect_success 'move locked worktree (force)' '
+	test_when_finished "
+		git worktree unlock flump || :
+		git worktree remove flump || :
+		git worktree unlock ploof || :
+		git worktree remove ploof || :
+		" &&
+	git worktree add --detach flump &&
+	git worktree lock flump &&
+	test_must_fail git worktree move flump ploof" &&
+	test_must_fail git worktree move --force flump ploof" &&
+	git worktree move --force --force flump ploof
+'
+
 test_expect_success 'remove main worktree' '
 	test_must_fail git worktree remove .
 '
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH 8/9] worktree: teach 'remove' to override lock when --force given twice

[Eric Sunshine]

For consistency with "add -f -f" and "move -f -f" which override
the lock on a worktree, allow "remove -f -f" to do so, as well, as a
convenience.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/git-worktree.txt |  1 +
 builtin/worktree.c             | 11 ++++++-----
 t/t2028-worktree-move.sh       | 10 ++++++++++
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index d08b8d8e4f..e2ee9fc21b 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -129,6 +129,7 @@ OPTIONS
 twice.
 +
 `remove` refuses to remove an unclean working tree unless `--force` is used.
+To remove a locked working tree, specify `--force` twice.
 
 -b <new-branch>::
 -B <new-branch>::
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 354a6c0eb5..a95fe67da6 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -875,13 +875,13 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
 	int force = 0;
 	struct option options[] = {
 		OPT__FORCE(&force,
-			 N_("force removing even if the worktree is dirty"),
+			 N_("force removal even if worktree is dirty or locked"),
 			 PARSE_OPT_NOCOMPLETE),
 		OPT_END()
 	};
 	struct worktree **worktrees, *wt;
 	struct strbuf errmsg = STRBUF_INIT;
-	const char *reason;
+	const char *reason = NULL;
 	int ret = 0;
 
 	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
@@ -894,12 +894,13 @@ static int remove_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]);
-	reason = is_worktree_locked(wt);
+	if (force < 2)
+		reason = is_worktree_locked(wt);
 	if (reason) {
 		if (*reason)
-			die(_("cannot remove a locked working tree, lock reason: %s"),
+			die(_("cannot remove a locked working tree, lock reason: %s\nuse 'remove -f -f' to override or unlock first"),
 			    reason);
-		die(_("cannot remove a locked working tree"));
+		die(_("cannot remove a locked working tree;\nuse 'remove -f -f' to override or unlock first"));
 	}
 	if (validate_worktree(wt, &errmsg, WT_VALIDATE_WORKTREE_MISSING_OK))
 		die(_("validation failed, cannot remove working tree: %s"),
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 9756ede8f1..1b5079e8fa 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -163,4 +163,14 @@ test_expect_success 'proper error when worktree not found' '
 	done
 '
 
+test_expect_success 'remove locked worktree (force)' '
+	git worktree add --detach gumby &&
+	test_when_finished "git worktree remove gumby || :" &&
+	git worktree lock gumby &&
+	test_when_finished "git worktree unlock gumby || :" &&
+	test_must_fail git worktree remove gumby &&
+	test_must_fail git worktree remove --force gumby &&
+	git worktree remove --force --force gumby
+'
+
 test_done
-- 
2.19.0.rc1.350.ge57e33dbd1

[PATCH 9/9] worktree: delete .git/worktrees if empty after 'remove'

[Eric Sunshine]

For cleanliness, "git worktree prune" deletes the .git/worktrees
directory if it is empty after pruning is complete.

For consistency, make "git worktree remove <path>" likewise delete
.git/worktrees if it is empty after the removal.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c       |  8 +++++++-
 t/t2028-worktree-move.sh | 12 ++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index a95fe67da6..c4abbde2b8 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -62,6 +62,11 @@ static int delete_git_dir(const char *id)
 	return ret;
 }
 
+static void delete_worktrees_dir_if_empty(void)
+{
+	rmdir(git_path("worktrees")); /* ignore failed removal */
+}
+
 static int prune_worktree(const char *id, struct strbuf *reason)
 {
 	struct stat st;
@@ -149,7 +154,7 @@ static void prune_worktrees(void)
 	}
 	closedir(dir);
 	if (!show_only)
-		rmdir(git_path("worktrees"));
+		delete_worktrees_dir_if_empty();
 	strbuf_release(&reason);
 }
 
@@ -918,6 +923,7 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
 	 * from here.
 	 */
 	ret |= delete_git_dir(wt->id);
+	delete_worktrees_dir_if_empty();
 
 	free_worktrees(worktrees);
 	return ret;
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 1b5079e8fa..33c0337733 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -173,4 +173,16 @@ test_expect_success 'remove locked worktree (force)' '
 	git worktree remove --force --force gumby
 '
 
+test_expect_success 'remove cleans up .git/worktrees when empty' '
+	git init moog &&
+	(
+		cd moog &&
+		test_commit bim &&
+		git worktree add --detach goom &&
+		test_path_exists .git/worktrees &&
+		git worktree remove goom &&
+		test_path_is_missing .git/worktrees
+	)
+'
+
 test_done
-- 
2.19.0.rc1.350.ge57e33dbd1

Re: [PATCH 3/9] worktree: generalize delete_git_dir() to reduce code duplication

[Jeff King]

On Tue, Aug 28, 2018 at 05:20:20PM -0400, Eric Sunshine wrote:

> prune_worktrees() and delete_git_dir() both remove worktree
> administrative entries from .git/worktrees, and their implementations
> are nearly identical. The only difference is that prune_worktrees() is
> also capable of removing a bogus non-worktree-related file from
> .git/worktrees.
> 
> Simplify by extending delete_git_dir() to handle the little bit of
> extra functionality needed by prune_worktrees(), and drop the
> effectively duplicate code from the latter.

Makes sense. The name "delete_git_dir()" is a little funny (I assume it
means "the git dir", not a worktree's git-dir), but that is not new (and
it's static in worktree.c, which helps).

Your patch maybe stretches that a little by deleting non-directories.
Maybe delete_from_worktrees() would be a better name. Probably not worth
a re-roll, though.

-Peff

Re: [PATCH 5/9] worktree: disallow adding same path multiple times

[Jeff King]

On Tue, Aug 28, 2018 at 05:20:22PM -0400, Eric Sunshine wrote:

> A given path should only ever be associated with a single registered
> worktree. This invariant is enforced by refusing to create a new
> worktree at a given path if that path already exists. For example:
> 
>     $ git worktree add -q --detach foo
>     $ git worktree add -q --detach foo
>     fatal: 'foo' already exists
> 
> However, the check can be fooled, and the invariant broken, if the
> path is missing. Continuing the example:
> [...]

Nicely explained.

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 46c93771e8..1122f27b5f 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -221,8 +221,33 @@ static const char *worktree_basename(const char *path, int *olen)
>  
>  static void validate_worktree_add(const char *path, const struct add_opts *opts)
>  {
> +	struct worktree **worktrees;
> +	struct worktree *wt;
> +	int locked;
> +
>  	if (file_exists(path) && !is_empty_dir(path))
>  		die(_("'%s' already exists"), path);
> +
> +	worktrees = get_worktrees(0);
> +	/*
> +	 * find_worktree()'s suffix matching may undesirably find the main
> +	 * rather than a linked worktree (for instance, when the basenames
> +	 * of the main worktree and the one being created are the same).
> +	 * We're only interested in linked worktrees, so skip the main
> +	 * worktree with +1.
> +	 */
> +	wt = find_worktree(worktrees + 1, NULL, path);

Very appropriate use of a comment. :)

I wondered whether find_worktree() would discover a collision like this:

  git worktree add --detach foo
  rm -rf foo
  git worktree add --detach bar/foo

but it is smart enough to actually compare the gitdirs and so we
(correctly) create "foo" and "foo1" in $GIT_DIR/worktrees.

> +	if (!wt)
> +		goto done;
> +
> +	locked = !!is_worktree_locked(wt);
> +	if (locked)
> +		die(_("'%s' is a missing but locked worktree;\nuse 'unlock' and 'prune' or 'remove' to clear"), path);
> +	else
> +		die(_("'%s' is a missing but already registered worktree;\nuse 'prune' or 'remove' to clear"), path);

Nice, I like giving separate messages for the locked and unlocked cases
(which I imagine will become even more obvious as you add --force
support).

The formatting of the message itself is a little funny:

  $ git worktree add --detach foo
  fatal: 'foo' is a missing but already registered worktree;
  use 'prune' or 'remove' to clear

I'd say that the second line would ideally be advise(), since we're
dying. You could do:

  error(%s is missing...)
  advise(use prune...)
  exit(128);

but that loses the "fatal" in the first message. I wonder if just
manually writing "hint:" would be so bad.

> +done:
> +	free_worktrees(worktrees);

You could easily avoid this goto with:

  if (wt) {
     /* check wt or die */
  }

  free_worktrees(worktrees);

but it may not be worth it if the logic gets more complicated in future
patches.

-Peff

Re: [PATCH 6/9] worktree: teach 'add' to respect --force for registered but missing path

[Jeff King]

On Tue, Aug 28, 2018 at 05:20:23PM -0400, Eric Sunshine wrote:

> For safety, "git worktree add <path>" will refuse to add a new
> worktree at <path> if <path> is already associated with a worktree
> entry, even if <path> is missing (for instance, has been deleted or
> resides on non-mounted removable media or network share). The typical
> way to re-create a worktree at <path> in such a situation is either to
> prune all "broken" entries ("git worktree prune") or to selectively
> remove the worktree entry manually ("git worktree remove <path>").
> 
> However, neither of these approaches ("prune" nor "remove") is
> especially convenient, and they may be unsuitable for scripting when a
> tool merely wants to re-use a worktree if it exists or create it from
> scratch if it doesn't (much as a tool might use "mkdir -p" to re-use
> or create a directory).
> 
> Therefore, teach 'add' to respect --force as a convenient way to
> re-use a path already associated with a worktree entry if the path is
> non-existent. For a locked worktree, require --force to be specified
> twice.

This makes sense to me, and...

> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  Documentation/git-worktree.txt |  8 ++++++--
>  builtin/worktree.c             | 10 ++++++++--
>  t/t2025-worktree-add.sh        | 13 ++++++++++++-
>  3 files changed, 26 insertions(+), 5 deletions(-)

The patch looks quite good. One minor comment:

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 1122f27b5f..3eb2f89b0f 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -241,10 +241,16 @@ static void validate_worktree_add(const char *path, const struct add_opts *opts)
>  		goto done;
>  
>  	locked = !!is_worktree_locked(wt);
> +	if ((!locked && opts->force) || (locked && opts->force > 1)) {
> +		if (delete_git_dir(wt->id))
> +		    die(_("unable to re-add worktree '%s'"), path);
> +		goto done;
> +	}

This "unable to re-add" seemed funny to me at first, since the failure
is in deletion. I guess we're relying on delete_git_dir() to already
have said "I had trouble deleting $GIT_DIR/worktrees/foo", and this is
just the follow-up to tell that the whole operation is cancelled. So
that makes sense.

I wonder if we should volunteer the information that we're overwriting
an existing worktree. I guess the user would generally know that
already, though, since they just specified "-f", so it's probably just
being overly chatty to do so.

-Peff

Re: [PATCH 7/9] worktree: teach 'move' to override lock when --force given twice

[Jeff King]

On Tue, Aug 28, 2018 at 05:20:24PM -0400, Eric Sunshine wrote:

> For consistency with "add -f -f", which allows a missing but locked
> worktree path to be re-used, allow "move -f -f" to override a lock,
> as well, as a convenience.

I don't have a strong opinion on this one, as I have never used
"worktree mv" myself. :)

But anytime I see "-f -f", I have to wonder what "-f" does. In this
case, nothing. Is there some future lesser forcing we might use it for?

-Peff

Re: [PATCH 8/9] worktree: teach 'remove' to override lock when --force given twice

[Jeff King]

On Tue, Aug 28, 2018 at 05:20:25PM -0400, Eric Sunshine wrote:

> For consistency with "add -f -f" and "move -f -f" which override
> the lock on a worktree, allow "remove -f -f" to do so, as well, as a
> convenience.

This one makes more sense to me than the last, since "remove -f" already
does something useful.

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 354a6c0eb5..a95fe67da6 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -875,13 +875,13 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
>  	int force = 0;
>  	struct option options[] = {
>  		OPT__FORCE(&force,
> -			 N_("force removing even if the worktree is dirty"),
> +			 N_("force removal even if worktree is dirty or locked"),

I wonder if somebody might assume from this that a single "-f" would
override a lock. Perhaps not the end of the world, and the manpage does
make it clear. And also I don't really know how to be more specific here
without an overly long line.

I'm guessing all those thoughts went through your head before ending up
here, too. :)

-Peff

Re: [PATCH 0/9] worktree: fix bugs and broaden --force applicability

[Jeff King]

On Tue, Aug 28, 2018 at 05:20:17PM -0400, Eric Sunshine wrote:

> This series started as a fix for a bug reported by Peff[1] in which the
> "database" of worktrees could be corrupted (or at least become
> internally inconsistent) by having multiple worktree entries associated
> with the same path.
> 
> Peff's particular use-case for git-worktree is Documentation/doc-diff
> which wants to re-use a worktree if it exists or create it anew if it
> doesn't. Unfortunately, this is a bit more difficult than it should be
> if the worktree directory is deleted manually (without pruning the
> worktree entry) between script invocations. To simplify this use-case
> for tools, it was suggested[2] that "git worktree add --force" could
> deal with the problem of a registered-but-missing worktree (much as a
> tool might rely upon "mkdir -p" to create or re-use an existing
> directory). This series implements that proposal, as well.
> 
> Fixing the original bug revealed another existing bug, and after several
> additional "while we're here" changes, the series ended up a bit longer
> than expected.

Thanks for working on this. I think this nicely solves the problem I was
having.  I had a few minor comments which I sent in reply to individual
patches, but overall it looks very good. I'd be happy to see it picked
up with or without any changes based on my comments.

I'd want to do this on top. :)

-- >8 --
Subject: [PATCH] doc-diff: force worktree add

We avoid re-creating our temporary worktree if it's already
there. But we may run into a situation where the worktree
has been deleted, but an entry still exists in
$GIT_DIR/worktrees.

Older versions of git-worktree would annoyingly create a
series of duplicate entries. Recent versions now detect and
prevent this, allowing you to override with "-f". Since we
know that the worktree in question was just our temporary
workspace, it's safe for us to always pass "-f".

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/doc-diff | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/doc-diff b/Documentation/doc-diff
index f483fe427c..19d841ddeb 100755
--- a/Documentation/doc-diff
+++ b/Documentation/doc-diff
@@ -54,7 +54,7 @@ fi
 # results that don't differ between the two trees.
 if ! test -d "$tmp/worktree"
 then
-	git worktree add --detach "$tmp/worktree" "$from" &&
+	git worktree add -f --detach "$tmp/worktree" "$from" &&
 	dots=$(echo "$tmp/worktree" | sed 's#[^/]*#..#g') &&
 	ln -s "$dots/config.mak" "$tmp/worktree/config.mak"
 fi
-- 
2.19.0.rc1.539.g3876d0831e

Re: [PATCH 5/9] worktree: disallow adding same path multiple times

[Eric Sunshine]

On Thu, Aug 30, 2018 at 3:28 AM Jeff King <peff@peff.net> wrote:
> On Tue, Aug 28, 2018 at 05:20:22PM -0400, Eric Sunshine wrote:
> > +     /*
> > +      * find_worktree()'s suffix matching may undesirably find the main
> > +      * rather than a linked worktree (for instance, when the basenames
> > +      * of the main worktree and the one being created are the same).
> > +      * We're only interested in linked worktrees, so skip the main
> > +      * worktree with +1.
> > +      */
> > +     wt = find_worktree(worktrees + 1, NULL, path);
>
> Very appropriate use of a comment. :)

I tried repeatedly to write the comment as a one-liner but simply
couldn't do it in a way which conveyed the necessary information.

> > +     if (locked)
> > +             die(_("'%s' is a missing but locked worktree;\nuse 'unlock' and 'prune' or 'remove' to clear"), path);
> > +     else
> > +             die(_("'%s' is a missing but already registered worktree;\nuse 'prune' or 'remove' to clear"), path);
>
> Nice, I like giving separate messages for the locked and unlocked cases
> The formatting of the message itself is a little funny:
>   $ git worktree add --detach foo
>   fatal: 'foo' is a missing but already registered worktree;
>   use 'prune' or 'remove' to clear

I couldn't come up with an improvement, so I went with this.

> I'd say that the second line would ideally be advise(), since we're
> dying. You could do:
>   error(%s is missing...)
>   advise(use prune...)
>   exit(128);
> but that loses the "fatal" in the first message.

I'm somewhat hesitant to sidestep die() here because die() brings its
own "special" behaviors and cleanups. (Even if they aren't needed
today, perhaps they will be in the future.)

> I wonder if just manually writing "hint:" would be so bad.

That would lose coloring of "hint", though.

Another alternative would be to make it just a long one-liner error
message. I don't feel strongly one way or the other.

> > +done:
> > +     free_worktrees(worktrees);
>
> You could easily avoid this goto with:
>
>   if (wt) {
>      /* check wt or die */
>   }
>
>   free_worktrees(worktrees);
>
> but it may not be worth it if the logic gets more complicated in future
> patches.

I did suspect that a reviewer would comment on this, but, yes, the
logic does get more complicated in subsequent patches, and the 'goto'
makes a cleaner result in the end.

Re: [PATCH 6/9] worktree: teach 'add' to respect --force for registered but missing path

[Eric Sunshine]

On Thu, Aug 30, 2018 at 3:36 AM Jeff King <peff@peff.net> wrote:
> On Tue, Aug 28, 2018 at 05:20:23PM -0400, Eric Sunshine wrote:
> > +     if ((!locked && opts->force) || (locked && opts->force > 1)) {
> > +             if (delete_git_dir(wt->id))
> > +                 die(_("unable to re-add worktree '%s'"), path);
> > +             goto done;
> > +     }
>
> This "unable to re-add" seemed funny to me at first, since the failure
> is in deletion. I guess we're relying on delete_git_dir() to already
> have said "I had trouble deleting $GIT_DIR/worktrees/foo", and this is
> just the follow-up to tell that the whole operation is cancelled. So
> that makes sense.

Correct, delete_git_dir() has already used error() to explain that it
couldn't delete .git/worktrees/<id>, so this finalizing die() doesn't
need to repeat that.

> I wonder if we should volunteer the information that we're overwriting
> an existing worktree. I guess the user would generally know that
> already, though, since they just specified "-f", so it's probably just
> being overly chatty to do so.

That's my conclusion, as well. An explicit use of --force shouldn't
need such chattiness.

Re: [PATCH 7/9] worktree: teach 'move' to override lock when --force given twice

[Eric Sunshine]

On Thu, Aug 30, 2018 at 3:38 AM Jeff King <peff@peff.net> wrote:
> On Tue, Aug 28, 2018 at 05:20:24PM -0400, Eric Sunshine wrote:
> > For consistency with "add -f -f", which allows a missing but locked
> > worktree path to be re-used, allow "move -f -f" to override a lock,
> > as well, as a convenience.
>
> I don't have a strong opinion on this one, as I have never used
> "worktree mv" myself. :)

I don't have strong feelings about this either (nor about "remove -f -f").

> But anytime I see "-f -f", I have to wonder what "-f" does. In this
> case, nothing. Is there some future lesser forcing we might use it for?

I had the same concern. A single --force probably ought to be
sufficient (given that there is no other meaning presently for a
single --force), but it somehow seemed wrong to override a lock with a
single --force when the other commands demand specifying it twice.

The strictness could always be downgraded later to require only a
single --force if it becomes obvious that that makes more sense.

Re: [PATCH 8/9] worktree: teach 'remove' to override lock when --force given twice

[Eric Sunshine]

On Thu, Aug 30, 2018 at 3:40 AM Jeff King <peff@peff.net> wrote:
> On Tue, Aug 28, 2018 at 05:20:25PM -0400, Eric Sunshine wrote:
> > -                      N_("force removing even if the worktree is dirty"),
> > +                      N_("force removal even if worktree is dirty or locked"),
>
> I wonder if somebody might assume from this that a single "-f" would
> override a lock. Perhaps not the end of the world, and the manpage does
> make it clear. And also I don't really know how to be more specific here
> without an overly long line.

Precisely, on all counts. Plus, if they try a single --force and it
fails, the hint printed when it fails says explicitly to use --force
twice, so the "solution" is easily discoverable.

> I'm guessing all those thoughts went through your head before ending up
> here, too. :)

You guess correctly and you came to the same conclusion as I did.
These -h summary lines are just to short for any real discussion.

Re: [PATCH 3/9] worktree: generalize delete_git_dir() to reduce code duplication

[Eric Sunshine]

On Thu, Aug 30, 2018 at 2:57 AM Jeff King <peff@peff.net> wrote:
> On Tue, Aug 28, 2018 at 05:20:20PM -0400, Eric Sunshine wrote:
> > Simplify by extending delete_git_dir() to handle the little bit of
> > extra functionality needed by prune_worktrees(), and drop the
> > effectively duplicate code from the latter.
>
> Makes sense. The name "delete_git_dir()" is a little funny (I assume it
> means "the git dir", not a worktree's git-dir), but that is not new (and
> it's static in worktree.c, which helps).

It's not necessarily the best name, but, as you say, it's not a new
issue, and it is local to that file.

Also, it's such a small function, and it's quite clear from the
implementation that it's deleting stuff from .git/worktrees that it's
probably okay to leave the name as-is for now.

> Your patch maybe stretches that a little by deleting non-directories.
> Maybe delete_from_worktrees() would be a better name. Probably not worth
> a re-roll, though.

That's perhaps a bit better, though still somewhat ambiguous, as I can
also read it as deleting something from every worktree directory, as
opposed to from .git/worktrees.

Re: [PATCH 0/9] worktree: fix bugs and broaden --force applicability

[Eric Sunshine]

On Thu, Aug 30, 2018 at 3:54 AM Jeff King <peff@peff.net> wrote:
> On Tue, Aug 28, 2018 at 05:20:17PM -0400, Eric Sunshine wrote:
> > This series started as a fix for a bug reported by Peff[1] in which the
> > "database" of worktrees could be corrupted (or at least become
> > internally inconsistent) by having multiple worktree entries associated
> > with the same path.
>
> Thanks for working on this. I think this nicely solves the problem I was
> having.  I had a few minor comments which I sent in reply to individual
> patches, but overall it looks very good. I'd be happy to see it picked
> up with or without any changes based on my comments.

Thanks for the review comments. Hopefully, the series can land as-is
without a re-roll.

It would be nice to find a better way to format the hint attached to
the fatal message, but that can always be tweaked with a follow-on
patch if I don't have to re-roll for some other reason.

Re: [PATCH 0/9] worktree: fix bugs and broaden --force applicability

[Eric Sunshine]

On Thu, Aug 30, 2018 at 3:54 AM Jeff King <peff@peff.net> wrote:
> Subject: [PATCH] doc-diff: force worktree add
>
> We avoid re-creating our temporary worktree if it's already
> there. But we may run into a situation where the worktree
> has been deleted, but an entry still exists in
> $GIT_DIR/worktrees.

Can "clean" or "distclean" also be augmented to remove this worktree
(and directory)? I guess that "distclean" would make more sense than
"clean"(?).

> Older versions of git-worktree would annoyingly create a
> series of duplicate entries. Recent versions now detect and
> prevent this, allowing you to override with "-f". Since we
> know that the worktree in question was just our temporary
> workspace, it's safe for us to always pass "-f".

Makes sense, and the patch looks correct.

> Signed-off-by: Jeff King <peff@peff.net>

Re: [PATCH 0/9] worktree: fix bugs and broaden --force applicability

[Jeff King]

On Thu, Aug 30, 2018 at 05:04:32AM -0400, Eric Sunshine wrote:

> On Thu, Aug 30, 2018 at 3:54 AM Jeff King <peff@peff.net> wrote:
> > Subject: [PATCH] doc-diff: force worktree add
> >
> > We avoid re-creating our temporary worktree if it's already
> > there. But we may run into a situation where the worktree
> > has been deleted, but an entry still exists in
> > $GIT_DIR/worktrees.
> 
> Can "clean" or "distclean" also be augmented to remove this worktree
> (and directory)? I guess that "distclean" would make more sense than
> "clean"(?).

I suppose so. I don't think I've _ever_ used distclean, and I only
rarely use "clean" (a testament to our Makefile's efforts to accurately
track dependencies). I'd usually use "git clean" when I want something
pristine (because I don't want to trust the Makefile at all).

-Peff

Re: [PATCH 0/9] worktree: fix bugs and broaden --force applicability

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I suppose so. I don't think I've _ever_ used distclean, and I only
> rarely use "clean" (a testament to our Makefile's efforts to accurately
> track dependencies). I'd usually use "git clean" when I want something
> pristine (because I don't want to trust the Makefile at all).

I do not trust "git clean" all that much, and pre-cleaning with
"make distclean" and then running "git clean -x" has become my bad
habit.  I jump around quite a bit during the day, which would end up
littering the working tree with *.o files that are only known to one
but not both of {maint,pu}/Makefile's distclean rules.  I even do
"for i in pu maint master next; do git checkout $i; make distclean; done"
sometimes before running "git clean -x" ;-)

Re: [PATCH 0/9] worktree: fix bugs and broaden --force applicability

[Ramsay Jones]

On 30/08/18 21:14, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> I suppose so. I don't think I've _ever_ used distclean, and I only
>> rarely use "clean" (a testament to our Makefile's efforts to accurately
>> track dependencies). I'd usually use "git clean" when I want something
>> pristine (because I don't want to trust the Makefile at all).
> 
> I do not trust "git clean" all that much, and pre-cleaning with
> "make distclean" and then running "git clean -x" has become my bad
> habit.  I jump around quite a bit during the day, which would end up
> littering the working tree with *.o files that are only known to one
> but not both of {maint,pu}/Makefile's distclean rules.  I even do
> "for i in pu maint master next; do git checkout $i; make distclean; done"
> sometimes before running "git clean -x" ;-)
> 

'git clean -x' always removes _way_ more than I want it
to - in particular, I lost my config.mak more than once.

So, no I don't trust it. ;-)

ATB,
Ramsay Jones

Re: [PATCH 0/9] worktree: fix bugs and broaden --force applicability

[Jeff King]

On Fri, Aug 31, 2018 at 12:49:39AM +0100, Ramsay Jones wrote:

> On 30/08/18 21:14, Junio C Hamano wrote:
> > Jeff King <peff@peff.net> writes:
> > 
> >> I suppose so. I don't think I've _ever_ used distclean, and I only
> >> rarely use "clean" (a testament to our Makefile's efforts to accurately
> >> track dependencies). I'd usually use "git clean" when I want something
> >> pristine (because I don't want to trust the Makefile at all).
> > 
> > I do not trust "git clean" all that much, and pre-cleaning with
> > "make distclean" and then running "git clean -x" has become my bad
> > habit.  I jump around quite a bit during the day, which would end up
> > littering the working tree with *.o files that are only known to one
> > but not both of {maint,pu}/Makefile's distclean rules.  I even do
> > "for i in pu maint master next; do git checkout $i; make distclean; done"
> > sometimes before running "git clean -x" ;-)
> > 
> 
> 'git clean -x' always removes _way_ more than I want it
> to - in particular, I lost my config.mak more than once.

Heh. I have done that, too, but fortunately mine is a symlink to a copy
that is held in a git repository. ;)

-Peff

Re: [PATCH 0/9] worktree: fix bugs and broaden --force applicability

[Ramsay Jones]

On 31/08/18 01:54, Jeff King wrote:
> On Fri, Aug 31, 2018 at 12:49:39AM +0100, Ramsay Jones wrote:
> 
>> On 30/08/18 21:14, Junio C Hamano wrote:
>>> Jeff King <peff@peff.net> writes:
>>>
>>>> I suppose so. I don't think I've _ever_ used distclean, and I only
>>>> rarely use "clean" (a testament to our Makefile's efforts to accurately
>>>> track dependencies). I'd usually use "git clean" when I want something
>>>> pristine (because I don't want to trust the Makefile at all).
>>>
>>> I do not trust "git clean" all that much, and pre-cleaning with
>>> "make distclean" and then running "git clean -x" has become my bad
>>> habit.  I jump around quite a bit during the day, which would end up
>>> littering the working tree with *.o files that are only known to one
>>> but not both of {maint,pu}/Makefile's distclean rules.  I even do
>>> "for i in pu maint master next; do git checkout $i; make distclean; done"
>>> sometimes before running "git clean -x" ;-)
>>>
>>
>> 'git clean -x' always removes _way_ more than I want it
>> to - in particular, I lost my config.mak more than once.
> 
> Heh. I have done that, too, but fortunately mine is a symlink to a copy
> that is held in a git repository. ;)

:-D

Now, why didn't I think of that! ;-)

ATB,
Ramsay Jones