{"thread":{"id":"53634","subject":"[PATCH 1/8] worktree: factor out repeated string literal","startedAt":"2020-06-08T06:25:02Z","lastAt":"2020-06-09T17:38:25Z","messageCount":20,"participants":["Eric Sunshine","Shourya Shukla","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"399272","messageId":"20200608062356.40264-2-sunshine@sunshineco.com","threadId":"53634","inReplyTo":"20200608062356.40264-1-sunshine@sunshineco.com","subject":"[PATCH 1/8] worktree: factor out repeated string literal","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-08T06:23:49Z","receivedAt":"2020-06-08T06:25:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"For each worktree removed by \"git worktree prune\", it reports the reason\nfor the removal. All reasons share the common prefix \"Removing\nworktrees/%s:\". As new removal reasons are added, this prefix needs to\nbe duplicated, which is error-prone and potentially cumbersome.\nTherefore, factor out the common prefix.\n\nAlthough this change seems to increase the \"sentence lego quotient\", it\nshould be reasonably safe, as the reason for removal is a distinct\nclause, not strictly related to the prefix. Moreover, the \"worktrees\" in\n\"Removing worktrees/%s:\" is a path literal which ought not be localized,\nso by factoring it out, we can more easily avoid exposing that path\nfragment to translators.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n builtin/worktree.c | 23 ++++++++++++-----------\n 1 file changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex d99db35668..9b15f19fc5 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -76,19 +76,19 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n \tssize_t read_result;\n \n \tif (!is_directory(git_path(\"worktrees/%s\", id))) {\n-\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: not a valid directory\"), id);\n+\t\tstrbuf_addstr(reason, _(\"not a valid directory\"));\n \t\treturn 1;\n \t}\n \tif (file_exists(git_path(\"worktrees/%s/locked\", id)))\n \t\treturn 0;\n \tif (stat(git_path(\"worktrees/%s/gitdir\", id), &st)) {\n-\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: gitdir file does not exist\"), id);\n+\t\tstrbuf_addstr(reason, _(\"gitdir file does not exist\"));\n \t\treturn 1;\n \t}\n \tfd = open(git_path(\"worktrees/%s/gitdir\", id), O_RDONLY);\n \tif (fd < 0) {\n-\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: unable to read gitdir file (%s)\"),\n-\t\t\t    id, strerror(errno));\n+\t\tstrbuf_addf(reason, _(\"unable to read gitdir file (%s)\"),\n+\t\t\t    strerror(errno));\n \t\treturn 1;\n \t}\n \tlen = xsize_t(st.st_size);\n@@ -96,8 +96,8 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n \n \tread_result = read_in_full(fd, path, len);\n \tif (read_result < 0) {\n-\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: unable to read gitdir file (%s)\"),\n-\t\t\t    id, strerror(errno));\n+\t\tstrbuf_addf(reason, _(\"unable to read gitdir file (%s)\"),\n+\t\t\t    strerror(errno));\n \t\tclose(fd);\n \t\tfree(path);\n \t\treturn 1;\n@@ -106,15 +106,15 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n \n \tif (read_result != len) {\n \t\tstrbuf_addf(reason,\n-\t\t\t    _(\"Removing worktrees/%s: short read (expected %\"PRIuMAX\" bytes, read %\"PRIuMAX\")\"),\n-\t\t\t    id, (uintmax_t)len, (uintmax_t)read_result);\n+\t\t\t    _(\"short read (expected %\"PRIuMAX\" bytes, read %\"PRIuMAX\")\"),\n+\t\t\t    (uintmax_t)len, (uintmax_t)read_result);\n \t\tfree(path);\n \t\treturn 1;\n \t}\n \twhile (len && (path[len - 1] == '\\n' || path[len - 1] == '\\r'))\n \t\tlen--;\n \tif (!len) {\n-\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: invalid gitdir file\"), id);\n+\t\tstrbuf_addstr(reason, _(\"invalid gitdir file\"));\n \t\tfree(path);\n \t\treturn 1;\n \t}\n@@ -123,7 +123,7 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n \t\tfree(path);\n \t\tif (stat(git_path(\"worktrees/%s/index\", id), &st) ||\n \t\t    st.st_mtime <= expire) {\n-\t\t\tstrbuf_addf(reason, _(\"Removing worktrees/%s: gitdir file points to non-existent location\"), id);\n+\t\t\tstrbuf_addstr(reason, _(\"gitdir file points to non-existent location\"));\n \t\t\treturn 1;\n \t\t} else {\n \t\t\treturn 0;\n@@ -147,7 +147,8 @@ static void prune_worktrees(void)\n \t\tif (!prune_worktree(d->d_name, &reason))\n \t\t\tcontinue;\n \t\tif (show_only || verbose)\n-\t\t\tprintf(\"%s\\n\", reason.buf);\n+\t\t\tprintf_ln(_(\"Removing %s/%s: %s\"),\n+\t\t\t\t  \"worktrees\", d->d_name, reason.buf);\n \t\tif (show_only)\n \t\t\tcontinue;\n \t\tdelete_git_dir(d->d_name);\n-- \n2.27.0.290.gba653c62da\n\n"},{"id":"399273","messageId":"20200608062356.40264-1-sunshine@sunshineco.com","threadId":"53634","inReplyTo":null,"subject":"[PATCH 0/8] worktree: tighten duplicate path detection","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-08T06:23:48Z","receivedAt":"2020-06-08T06:25:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This patch series started as a (presumably) simple enhancement to make\n\"git worktree prune\" more helpful to users who shoot themselves in the\nfoot[1] by causing a linked worktree to point at the main worktree, a\nsituation which \"git worktree add\" tries hard to disallow but which\nusers can accomplish merely be deleting and renaming directories\n(without even mucking around inside the .git/worktrees/<id> directory).\nThe series grew as I realized that \"git worktree move\" does not employ\nthe same protections as \"git worktree add\" and happily allows multiple\nlinked worktrees to references the same location, thus that also had to\nbe fixed.\n\n[1]: https://lore.kernel.org/git/CAPig+cTU8+N6Chimpoa2_T-TcXxw-3B9-9pjCLz7WeOh472P_A@mail.gmail.com/\n\nEric Sunshine (8):\n  worktree: factor out repeated string literal\n  worktree: prune corrupted worktree even if locked\n  worktree: give \"should be pruned?\" function more meaningful name\n  worktree: make high-level pruning re-usable\n  worktree: prune duplicate entries referencing same worktree path\n  worktree: prune linked worktree referencing main worktree path\n  worktree: generalize candidate worktree path validation\n  worktree: make \"move\" refuse to move atop missing registered worktree\n\n Documentation/git-worktree.txt |   4 +-\n builtin/worktree.c             | 142 ++++++++++++++++++++++-----------\n t/t2401-worktree-prune.sh      |  38 ++++++++-\n t/t2403-worktree-move.sh       |  21 +++++\n 4 files changed, 157 insertions(+), 48 deletions(-)\n\n-- \n2.27.0.290.gba653c62da\n\n"},{"id":"399274","messageId":"20200608062356.40264-4-sunshine@sunshineco.com","threadId":"53634","inReplyTo":"20200608062356.40264-1-sunshine@sunshineco.com","subject":"[PATCH 3/8] worktree: give \"should be pruned?\" function more meaningful name","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-08T06:23:51Z","receivedAt":"2020-06-08T06:25:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Readers of the name prune_worktree() are likely to expect the function\nto actually prune a worktree, however, it only answers the question\n\"should this worktree be pruned?\". Give it a name more reflective of its\ntrue purpose to avoid such confusion.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n builtin/worktree.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex f7351413af..27681a1396 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -67,7 +67,7 @@ static void delete_worktrees_dir_if_empty(void)\n \trmdir(git_path(\"worktrees\")); /* ignore failed removal */\n }\n \n-static int prune_worktree(const char *id, struct strbuf *reason)\n+static int should_prune_worktree(const char *id, struct strbuf *reason)\n {\n \tstruct stat st;\n \tchar *path;\n@@ -144,7 +144,7 @@ static void prune_worktrees(void)\n \t\tif (is_dot_or_dotdot(d->d_name))\n \t\t\tcontinue;\n \t\tstrbuf_reset(&reason);\n-\t\tif (!prune_worktree(d->d_name, &reason))\n+\t\tif (!should_prune_worktree(d->d_name, &reason))\n \t\t\tcontinue;\n \t\tif (show_only || verbose)\n \t\t\tprintf_ln(_(\"Removing %s/%s: %s\"),\n-- \n2.27.0.290.gba653c62da\n\n"},{"id":"399275","messageId":"20200608062356.40264-3-sunshine@sunshineco.com","threadId":"53634","inReplyTo":"20200608062356.40264-1-sunshine@sunshineco.com","subject":"[PATCH 2/8] worktree: prune corrupted worktree even if locked","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-08T06:23:50Z","receivedAt":"2020-06-08T06:25:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The .git/worktrees/<id>/locked file created by \"git worktree lock\" is\nintended to prevent a missing worktree -- which might reside on a\nremovable device or network share -- from being pruned. It is not meant\nto prevent a corrupt worktree from being pruned, yet it short-circuits\nalmost all \"git worktree prune\" corruption checks. This can make it\nimpossible[1] to prune a worktree which becomes corrupt after the lock\nis placed since \"git worktree prune\" won't prune it, and it may not even\nbe possible to unlock it with \"git worktree unlock\", depending upon the\nnature of the corruption.\n\nTherefore, delay the check for .git/worktrees/<id>/locked until after\nall forms of corruption have been checked so that it behaves as\noriginally intended (to wit: preventing pruning of a missing worktree\nonly).\n\n[1]: Impossible, that is, without manually mucking around with\n     .git/worktrees/<id>/ administrative files.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n builtin/worktree.c        |  4 ++--\n t/t2401-worktree-prune.sh | 14 ++++++++++++--\n 2 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 9b15f19fc5..f7351413af 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -79,8 +79,6 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n \t\tstrbuf_addstr(reason, _(\"not a valid directory\"));\n \t\treturn 1;\n \t}\n-\tif (file_exists(git_path(\"worktrees/%s/locked\", id)))\n-\t\treturn 0;\n \tif (stat(git_path(\"worktrees/%s/gitdir\", id), &st)) {\n \t\tstrbuf_addstr(reason, _(\"gitdir file does not exist\"));\n \t\treturn 1;\n@@ -121,6 +119,8 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n \tpath[len] = '\\0';\n \tif (!file_exists(path)) {\n \t\tfree(path);\n+\t\tif (file_exists(git_path(\"worktrees/%s/locked\", id)))\n+\t\t\treturn 0;\n \t\tif (stat(git_path(\"worktrees/%s/index\", id), &st) ||\n \t\t    st.st_mtime <= expire) {\n \t\t\tstrbuf_addstr(reason, _(\"gitdir file points to non-existent location\"));\ndiff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh\nindex b7d6d5d45a..9be8e97d66 100755\n--- a/t/t2401-worktree-prune.sh\n+++ b/t/t2401-worktree-prune.sh\n@@ -69,13 +69,23 @@ test_expect_success 'prune directories with gitdir pointing to nowhere' '\n '\n \n test_expect_success 'not prune locked checkout' '\n-\ttest_when_finished rm -r .git/worktrees &&\n-\tmkdir -p .git/worktrees/ghi &&\n+\ttest_when_finished rm -fr .git/worktrees ghi &&\n+\tgit worktree add ghi &&\n \t: >.git/worktrees/ghi/locked &&\n+\trm -r ghi &&\n \tgit worktree prune &&\n \ttest -d .git/worktrees/ghi\n '\n \n+test_expect_success 'prune corrupt despite lock' '\n+\ttest_when_finished rm -fr .git/worktrees ghi &&\n+\tmkdir -p .git/worktrees/ghi &&\n+\t: >.git/worktrees/ghi/gitdir &&\n+\t: >.git/worktrees/ghi/locked &&\n+\tgit worktree prune &&\n+\t! test -d .git/worktrees/ghi\n+'\n+\n test_expect_success 'not prune recent checkouts' '\n \ttest_when_finished rm -r .git/worktrees &&\n \tgit worktree add jlm HEAD &&\n-- \n2.27.0.290.gba653c62da\n\n"},{"id":"399276","messageId":"20200608062356.40264-5-sunshine@sunshineco.com","threadId":"53634","inReplyTo":"20200608062356.40264-1-sunshine@sunshineco.com","subject":"[PATCH 4/8] worktree: make high-level pruning re-usable","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-08T06:23:52Z","receivedAt":"2020-06-08T06:25:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The low-level logic for removing a worktree is well encapsulated in\ndelete_git_dir(). However, high-level details related to pruning a\nworktree -- such as dealing with verbosity and dry-run mode -- are not\nencapsulated. Factor out this high-level logic into its own function so\nit can be re-used as new worktree corruption detectors are added.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n builtin/worktree.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 27681a1396..d0c046e885 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -133,6 +133,14 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)\n \treturn 0;\n }\n \n+static void prune_worktree(const char *id, const char *reason)\n+{\n+\tif (show_only || verbose)\n+\t\tprintf_ln(_(\"Removing %s/%s: %s\"), \"worktrees\", id, reason);\n+\tif (!show_only)\n+\t\tdelete_git_dir(id);\n+}\n+\n static void prune_worktrees(void)\n {\n \tstruct strbuf reason = STRBUF_INIT;\n@@ -146,12 +154,7 @@ static void prune_worktrees(void)\n \t\tstrbuf_reset(&reason);\n \t\tif (!should_prune_worktree(d->d_name, &reason))\n \t\t\tcontinue;\n-\t\tif (show_only || verbose)\n-\t\t\tprintf_ln(_(\"Removing %s/%s: %s\"),\n-\t\t\t\t  \"worktrees\", d->d_name, reason.buf);\n-\t\tif (show_only)\n-\t\t\tcontinue;\n-\t\tdelete_git_dir(d->d_name);\n+\t\tprune_worktree(d->d_name, reason.buf);\n \t}\n \tclosedir(dir);\n \tif (!show_only)\n-- \n2.27.0.290.gba653c62da\n\n"},{"id":"399277","messageId":"20200608062356.40264-7-sunshine@sunshineco.com","threadId":"53634","inReplyTo":"20200608062356.40264-1-sunshine@sunshineco.com","subject":"[PATCH 6/8] worktree: prune linked worktree referencing main worktree path","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-08T06:23:54Z","receivedAt":"2020-06-08T06:25:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"\"git worktree prune\" detects when multiple entries are associated with\nthe same path and prunes the duplicates, however, it does not detect\nwhen a linked worktree points at the path of the main worktree.\nAlthough \"git worktree add\" disallows creating a new worktree with the\nsame path as the main worktree, such a case can arise outside the\ncontrol of Git even without the user mucking with .git/worktree/<id>/\nadministrative files. For instance:\n\n    $ git clone foo.git\n    $ git -C foo worktree add ../bar\n    $ rm -rf bar\n    $ mv foo bar\n    $ git -C bar worktree list\n    .../bar deadfeeb [master]\n    .../bar deadfeeb [bar]\n\nHelp the user recover from such corruption by extending \"git worktree\nprune\" to also detect when a linked worktree is associated with the path\nof the main worktree.\n\nReported-by: Jonathan Müller <jonathanmueller.dev@gmail.com>\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n builtin/worktree.c        | 10 ++++++++++\n t/t2401-worktree-prune.sh | 12 ++++++++++++\n 2 files changed, 22 insertions(+)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 2cb95f16d4..eebd77c46d 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -153,6 +153,11 @@ static int prune_cmp(const void *a, const void *b)\n \n \tif ((c = fspathcmp(x->string, y->string)))\n \t    return c;\n+\t/* paths same; main worktee (util==0) sorts above all others */\n+\tif (!x->util)\n+\t\treturn -1;\n+\tif (!y->util)\n+\t\treturn 1;\n \t/* paths same; sort by .git/worktrees/<id> */\n \treturn strcmp(x->util, y->util);\n }\n@@ -171,6 +176,7 @@ static void prune_dups(struct string_list *l)\n static void prune_worktrees(void)\n {\n \tstruct strbuf reason = STRBUF_INIT;\n+\tstruct strbuf main = STRBUF_INIT;\n \tstruct string_list kept = STRING_LIST_INIT_NODUP;\n \tDIR *dir = opendir(git_path(\"worktrees\"));\n \tstruct dirent *d;\n@@ -190,6 +196,10 @@ static void prune_worktrees(void)\n \t}\n \tclosedir(dir);\n \n+\tstrbuf_add_absolute_path(&main, get_git_common_dir());\n+\t/* massage main worktree absolute path to match 'gitdir' content */\n+\tstrbuf_strip_suffix(&main, \"/.\");\n+\tstring_list_append(&kept, strbuf_detach(&main, 0));\n \tprune_dups(&kept);\n \tstring_list_clear(&kept, 1);\n \ndiff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh\nindex 7694f25a16..5f3db93b31 100755\n--- a/t/t2401-worktree-prune.sh\n+++ b/t/t2401-worktree-prune.sh\n@@ -114,4 +114,16 @@ test_expect_success 'prune duplicate (linked/linked)' '\n \t! test -d .git/worktrees/w2\n '\n \n+test_expect_success 'prune duplicate (main/linked)' '\n+\ttest_when_finished rm -fr repo wt &&\n+\ttest_create_repo repo &&\n+\ttest_commit -C repo x &&\n+\tgit -C repo worktree add --detach ../wt &&\n+\trm -fr wt &&\n+\tmv repo wt &&\n+\tgit -C wt worktree prune --verbose >actual &&\n+\ttest_i18ngrep \"duplicate entry\" actual &&\n+\t! test -d .git/worktrees/wt\n+'\n+\n test_done\n-- \n2.27.0.290.gba653c62da\n\n"},{"id":"399278","messageId":"20200608062356.40264-8-sunshine@sunshineco.com","threadId":"53634","inReplyTo":"20200608062356.40264-1-sunshine@sunshineco.com","subject":"[PATCH 7/8] worktree: generalize candidate worktree path validation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-08T06:23:55Z","receivedAt":"2020-06-08T06:25:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"\"git worktree add\" checks that the specified path is a valid location\nfor a new worktree by ensuring that the path does not already exist and\nis not already registered to another worktree (a path can be registered\nbut missing, for instance, if it resides on removable media). Since \"git\nworktree add\" is not the only command which should perform such\nvalidation (\"git worktree move\" ought to also), generalize the the\nvalidation function for use by other callers, as well.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n builtin/worktree.c | 29 ++++++++++++++++-------------\n 1 file changed, 16 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex eebd77c46d..7c0637234e 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -274,34 +274,33 @@ static const char *worktree_basename(const char *path, int *olen)\n \treturn name;\n }\n \n-static void validate_worktree_add(const char *path, const struct add_opts *opts)\n+/* check that path is viable location for worktree */\n+static void check_candidate_path(const char *path,\n+\t\t\t\t int force,\n+\t\t\t\t struct worktree **worktrees,\n+\t\t\t\t const char *cmd)\n {\n-\tstruct worktree **worktrees;\n \tstruct worktree *wt;\n \tint locked;\n \n \tif (file_exists(path) && !is_empty_dir(path))\n \t\tdie(_(\"'%s' already exists\"), path);\n \n-\tworktrees = get_worktrees(0);\n \twt = find_worktree_by_path(worktrees, path);\n \tif (!wt)\n-\t\tgoto done;\n+\t\treturn;\n \n \tlocked = !!worktree_lock_reason(wt);\n-\tif ((!locked && opts->force) || (locked && opts->force > 1)) {\n+\tif ((!locked && force) || (locked && force > 1)) {\n \t\tif (delete_git_dir(wt->id))\n-\t\t    die(_(\"unable to re-add worktree '%s'\"), path);\n-\t\tgoto done;\n+\t\t    die(_(\"unusable worktree destination '%s'\"), path);\n+\t\treturn;\n \t}\n \n \tif (locked)\n-\t\tdie(_(\"'%s' is a missing but locked worktree;\\nuse 'add -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear\"), path);\n+\t\tdie(_(\"'%s' is a missing but locked worktree;\\nuse '%s -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear\"), cmd, path);\n \telse\n-\t\tdie(_(\"'%s' is a missing but already registered worktree;\\nuse 'add -f' to override, or 'prune' or 'remove' to clear\"), path);\n-\n-done:\n-\tfree_worktrees(worktrees);\n+\t\tdie(_(\"'%s' is a missing but already registered worktree;\\nuse '%s -f' to override, or 'prune' or 'remove' to clear\"), cmd, path);\n }\n \n static int add_worktree(const char *path, const char *refname,\n@@ -318,8 +317,12 @@ static int add_worktree(const char *path, const char *refname,\n \tstruct commit *commit = NULL;\n \tint is_branch = 0;\n \tstruct strbuf sb_name = STRBUF_INIT;\n+\tstruct worktree **worktrees;\n \n-\tvalidate_worktree_add(path, opts);\n+\tworktrees = get_worktrees(0);\n+\tcheck_candidate_path(path, opts->force, worktrees, \"add\");\n+\tfree_worktrees(worktrees);\n+\tworktrees = NULL;\n \n \t/* is 'refname' a branch or commit? */\n \tif (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&\n-- \n2.27.0.290.gba653c62da\n\n"},{"id":"399279","messageId":"20200608062356.40264-9-sunshine@sunshineco.com","threadId":"53634","inReplyTo":"20200608062356.40264-1-sunshine@sunshineco.com","subject":"[PATCH 8/8] worktree: make \"move\" refuse to move atop missing registered worktree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-08T06:23:56Z","receivedAt":"2020-06-08T06:25:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"\"git worktree add\" takes special care to avoid creating a new worktree\nat a location already registered to an existing worktree even if that\nworktree is missing (which can happen, for instance, if the worktree\nresides on removable media). \"git worktree move\", however, is not so\ncareful when validating the destination location and will happily move\nthe source worktree atop the location of a missing worktree. This leads\nto the anomalous situation of multiple worktrees being associated with\nthe same path, which is expressively forbidden by design. For example:\n\n    $ git clone foo.git\n    $ cd foo\n    $ git worktree add ../bar\n    $ git worktree add ../baz\n    $ rm -rf ../bar\n    $ git worktree move ../baz ../bar\n    $ git worktree list\n    .../foo beefd00f [master]\n    .../bar beefd00f [bar]\n    .../bar beefd00f [baz]\n    $ git worktree remove ../bar\n    fatal: validation failed, cannot remove working tree:\n        '.../bar' does not point back to '.git/worktrees/bar'\n\nFix this shortcoming by enhancing \"git worktree move\" to perform the\nsame additional validation of the destination directory as done by \"git\nworktree add\".\n\nWhile at it, add a test to verify that \"git worktree move\" won't move a\nworktree atop an existing (non-worktree) path -- a restriction which has\nalways been in place but was never tested.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n Documentation/git-worktree.txt |  4 +++-\n builtin/worktree.c             |  3 +--\n t/t2403-worktree-move.sh       | 21 +++++++++++++++++++++\n 3 files changed, 25 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex 85d92c9761..4796c3c05e 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -126,7 +126,9 @@ OPTIONS\n \tlocked working tree path, specify `--force` twice.\n +\n `move` refuses to move a locked working tree unless `--force` is specified\n-twice.\n+twice. If the destination is already assigned to some other working tree but is\n+missing (for instance, if `<new-path>` was deleted manually), then `--force`\n+allows the move to proceed; use --force twice if the destination is locked.\n +\n `remove` refuses to remove an unclean working tree unless `--force` is used.\n To remove a locked working tree, specify `--force` twice.\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 7c0637234e..dda7da497c 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -857,8 +857,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \t\tstrbuf_trim_trailing_dir_sep(&dst);\n \t\tstrbuf_addstr(&dst, sep);\n \t}\n-\tif (file_exists(dst.buf))\n-\t\tdie(_(\"target '%s' already exists\"), dst.buf);\n+\tcheck_candidate_path(dst.buf, force, worktrees, \"move\");\n \n \tvalidate_no_submodules(wt);\n \ndiff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh\nindex 939d18d728..7035c9d72e 100755\n--- a/t/t2403-worktree-move.sh\n+++ b/t/t2403-worktree-move.sh\n@@ -112,6 +112,27 @@ test_expect_success 'move locked worktree (force)' '\n \tgit worktree move --force --force flump ploof\n '\n \n+test_expect_success 'refuse to move worktree atop existing path' '\n+\t> bobble &&\n+\tgit worktree add --detach beeble &&\n+\ttest_must_fail git worktree move beeble bobble\n+'\n+\n+test_expect_success 'move atop existing but missing worktree' '\n+\tgit worktree add --detach gnoo &&\n+\tgit worktree add --detach pneu &&\n+\trm -fr pneu &&\n+\ttest_must_fail git worktree move gnoo pneu &&\n+\tgit worktree move --force gnoo pneu &&\n+\n+\tgit worktree add --detach nu &&\n+\tgit worktree lock nu &&\n+\trm -fr nu &&\n+\ttest_must_fail git worktree move pneu nu &&\n+\ttest_must_fail git worktree --force move pneu nu &&\n+\tgit worktree move --force --force pneu nu\n+'\n+\n test_expect_success 'move a repo with uninitialized submodule' '\n \tgit init withsub &&\n \t(\n-- \n2.27.0.290.gba653c62da\n\n"},{"id":"399280","messageId":"20200608062356.40264-6-sunshine@sunshineco.com","threadId":"53634","inReplyTo":"20200608062356.40264-1-sunshine@sunshineco.com","subject":"[PATCH 5/8] worktree: prune duplicate entries referencing same worktree path","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-08T06:23:53Z","receivedAt":"2020-06-08T06:25:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"A fundamental restriction of linked working trees is that there must\nonly ever be a single worktree associated with a particular path, thus\n\"git worktree add\" explicitly disallows creation of a new worktree at\nthe same location as an existing registered worktree. Nevertheless,\nusers can still \"shoot themselves in the foot\" by mucking with\nadministrative files in .git/worktree/<id>/. Worse, \"git worktree move\"\nis careless[1] and allows a worktree to be moved atop a registered but\nmissing worktree (which can happen, for instance, if the worktree is on\nremovable media). For instance:\n\n    $ git clone foo.git\n    $ cd foo\n    $ git worktree add ../bar\n    $ git worktree add ../baz\n    $ rm -rf ../bar\n    $ git worktree move ../baz ../bar\n    $ git worktree list\n    .../foo beefd00f [master]\n    .../bar beefd00f [bar]\n    .../bar beefd00f [baz]\n\nHelp users recover from this form of corruption by teaching \"git\nworktree prune\" to detect when multiple worktrees are associated with\nthe same path.\n\n[1]: A subsequent commit will fix \"git worktree move\" validation to be\n     more strict.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n builtin/worktree.c        | 64 ++++++++++++++++++++++++++++++---------\n t/t2401-worktree-prune.sh | 12 ++++++++\n 2 files changed, 62 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex d0c046e885..2cb95f16d4 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -67,7 +67,12 @@ static void delete_worktrees_dir_if_empty(void)\n \trmdir(git_path(\"worktrees\")); /* ignore failed removal */\n }\n \n-static int should_prune_worktree(const char *id, struct strbuf *reason)\n+/*\n+ * Return NULL if worktree entry should be pruned (along with reason for\n+ * pruning), otherwise return the path of the worktree itself. Caller is\n+ * responsible for freeing return value.\n+ */\n+static char *worktree_disposition(const char *id, struct strbuf *reason)\n {\n \tstruct stat st;\n \tchar *path;\n@@ -77,17 +82,17 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)\n \n \tif (!is_directory(git_path(\"worktrees/%s\", id))) {\n \t\tstrbuf_addstr(reason, _(\"not a valid directory\"));\n-\t\treturn 1;\n+\t\treturn NULL;\n \t}\n \tif (stat(git_path(\"worktrees/%s/gitdir\", id), &st)) {\n \t\tstrbuf_addstr(reason, _(\"gitdir file does not exist\"));\n-\t\treturn 1;\n+\t\treturn NULL;\n \t}\n \tfd = open(git_path(\"worktrees/%s/gitdir\", id), O_RDONLY);\n \tif (fd < 0) {\n \t\tstrbuf_addf(reason, _(\"unable to read gitdir file (%s)\"),\n \t\t\t    strerror(errno));\n-\t\treturn 1;\n+\t\treturn NULL;\n \t}\n \tlen = xsize_t(st.st_size);\n \tpath = xmallocz(len);\n@@ -98,7 +103,7 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)\n \t\t\t    strerror(errno));\n \t\tclose(fd);\n \t\tfree(path);\n-\t\treturn 1;\n+\t\treturn NULL;\n \t}\n \tclose(fd);\n \n@@ -107,30 +112,29 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)\n \t\t\t    _(\"short read (expected %\"PRIuMAX\" bytes, read %\"PRIuMAX\")\"),\n \t\t\t    (uintmax_t)len, (uintmax_t)read_result);\n \t\tfree(path);\n-\t\treturn 1;\n+\t\treturn NULL;\n \t}\n \twhile (len && (path[len - 1] == '\\n' || path[len - 1] == '\\r'))\n \t\tlen--;\n \tif (!len) {\n \t\tstrbuf_addstr(reason, _(\"invalid gitdir file\"));\n \t\tfree(path);\n-\t\treturn 1;\n+\t\treturn NULL;\n \t}\n \tpath[len] = '\\0';\n \tif (!file_exists(path)) {\n-\t\tfree(path);\n \t\tif (file_exists(git_path(\"worktrees/%s/locked\", id)))\n-\t\t\treturn 0;\n+\t\t\treturn path;\n \t\tif (stat(git_path(\"worktrees/%s/index\", id), &st) ||\n \t\t    st.st_mtime <= expire) {\n \t\t\tstrbuf_addstr(reason, _(\"gitdir file points to non-existent location\"));\n-\t\t\treturn 1;\n+\t\t\tfree(path);\n+\t\t\treturn NULL;\n \t\t} else {\n-\t\t\treturn 0;\n+\t\t\treturn path;\n \t\t}\n \t}\n-\tfree(path);\n-\treturn 0;\n+\treturn path;\n }\n \n static void prune_worktree(const char *id, const char *reason)\n@@ -141,22 +145,54 @@ static void prune_worktree(const char *id, const char *reason)\n \t\tdelete_git_dir(id);\n }\n \n+static int prune_cmp(const void *a, const void *b)\n+{\n+\tconst struct string_list_item *x = a;\n+\tconst struct string_list_item *y = b;\n+\tint c;\n+\n+\tif ((c = fspathcmp(x->string, y->string)))\n+\t    return c;\n+\t/* paths same; sort by .git/worktrees/<id> */\n+\treturn strcmp(x->util, y->util);\n+}\n+\n+static void prune_dups(struct string_list *l)\n+{\n+\tint i;\n+\n+\tQSORT(l->items, l->nr, prune_cmp);\n+\tfor (i = 1; i < l->nr; i++) {\n+\t\tif (!fspathcmp(l->items[i].string, l->items[i - 1].string))\n+\t\t\tprune_worktree(l->items[i].util, \"duplicate entry\");\n+\t}\n+}\n+\n static void prune_worktrees(void)\n {\n \tstruct strbuf reason = STRBUF_INIT;\n+\tstruct string_list kept = STRING_LIST_INIT_NODUP;\n \tDIR *dir = opendir(git_path(\"worktrees\"));\n \tstruct dirent *d;\n \tif (!dir)\n \t\treturn;\n \twhile ((d = readdir(dir)) != NULL) {\n+\t\tchar *path;\n \t\tif (is_dot_or_dotdot(d->d_name))\n \t\t\tcontinue;\n \t\tstrbuf_reset(&reason);\n-\t\tif (!should_prune_worktree(d->d_name, &reason))\n+\t\tpath = worktree_disposition(d->d_name, &reason);\n+\t\tif (path) {\n+\t\t\tstring_list_append(&kept, path)->util = xstrdup(d->d_name);\n \t\t\tcontinue;\n+\t\t}\n \t\tprune_worktree(d->d_name, reason.buf);\n \t}\n \tclosedir(dir);\n+\n+\tprune_dups(&kept);\n+\tstring_list_clear(&kept, 1);\n+\n \tif (!show_only)\n \t\tdelete_worktrees_dir_if_empty();\n \tstrbuf_release(&reason);\ndiff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh\nindex 9be8e97d66..7694f25a16 100755\n--- a/t/t2401-worktree-prune.sh\n+++ b/t/t2401-worktree-prune.sh\n@@ -102,4 +102,16 @@ test_expect_success 'not prune proper checkouts' '\n \ttest -d .git/worktrees/nop\n '\n \n+test_expect_success 'prune duplicate (linked/linked)' '\n+\ttest_when_finished rm -fr .git/worktrees w1 w2 &&\n+\tgit worktree add --detach w1 &&\n+\tgit worktree add --detach w2 &&\n+\tsed \"s/w2/w1/\" .git/worktrees/w2/gitdir >.git/worktrees/w2/gitdir.new &&\n+\tmv .git/worktrees/w2/gitdir.new .git/worktrees/w2/gitdir &&\n+\tgit worktree prune --verbose >actual &&\n+\ttest_i18ngrep \"duplicate entry\" actual &&\n+\ttest -d .git/worktrees/w1 &&\n+\t! test -d .git/worktrees/w2\n+'\n+\n test_done\n-- \n2.27.0.290.gba653c62da\n\n"},{"id":"399297","messageId":"CAPig+cSzam0fTBrs=3MP6jieD4DBNCgx9kkrBm1+ahhmPWtrjA@mail.gmail.com","threadId":"53634","inReplyTo":"20200608062356.40264-9-sunshine@sunshineco.com","subject":"Re: [PATCH 8/8] worktree: make \"move\" refuse to move atop missing registered worktree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-08T15:19:48Z","receivedAt":"2020-06-08T15:20:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 8, 2020 at 2:25 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> \"git worktree add\" takes special care to avoid creating a new worktree\n> at a location already registered to an existing worktree even if that\n> worktree is missing (which can happen, for instance, if the worktree\n> resides on removable media). \"git worktree move\", however, is not so\n> careful when validating the destination location and will happily move\n> the source worktree atop the location of a missing worktree. This leads\n> to the anomalous situation of multiple worktrees being associated with\n> the same path, which is expressively forbidden by design. For example:\n\ns/expressively/expressly/\n"},{"id":"399311","messageId":"20200608193834.GA4196@konoha","threadId":"53634","inReplyTo":"20200608062356.40264-2-sunshine@sunshineco.com","subject":"Re: [PATCH 1/8] worktree: factor out repeated string literal","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-06-08T19:38:34Z","receivedAt":"2020-06-08T19:38:43Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 08/06 02:23, Eric Sunshine wrote:\nHello Eric!\n\n> Although this change seems to increase the \"sentence lego quotient\", it\n> should be reasonably safe, as the reason for removal is a distinct\n> clause, not strictly related to the prefix. Moreover, the \"worktrees\" in\n> \"Removing worktrees/%s:\" is a path literal which ought not be localized,\n> so by factoring it out, we can more easily avoid exposing that path\n> fragment to translators.\n\nCould you explain the above paragraph just a bit more? The English is\nconfusing me a bit.\n\n"},{"id":"399317","messageId":"xmqqzh9djlpb.fsf@gitster.c.googlers.com","threadId":"53634","inReplyTo":"20200608062356.40264-3-sunshine@sunshineco.com","subject":"Re: [PATCH 2/8] worktree: prune corrupted worktree even if locked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-08T21:23:44Z","receivedAt":"2020-06-08T21:23:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> The .git/worktrees/<id>/locked file created by \"git worktree lock\" is\n> intended to prevent a missing worktree -- which might reside on a\n> removable device or network share -- from being pruned. It is not meant\n> to prevent a corrupt worktree from being pruned, yet it short-circuits\n> almost all \"git worktree prune\" corruption checks.\n\nThe '.git/worktrees/<id>/locked' file is what 'It' in \"It is not\nmeant to\" refers to, but the 'it' in \"yet it short-circuits\" cannot\nrefer to the same thing---my reading hiccuped there.\n\n\"Its presence causes most of the corruption checks skipped by 'git\nworktree prune'\", perhaps.\n\n> This can make it\n> impossible[1] to prune a worktree which becomes corrupt after the lock\n> is placed since \"git worktree prune\" won't prune it, and it may not even\n> be possible to unlock it with \"git worktree unlock\", depending upon the\n> nature of the corruption.\n\nThe latter is because... \"worktree unlock\" does not skip corruption\ncheck and refuses to unlock a corrupted worktree, or something?\n\n> Therefore, delay the check for .git/worktrees/<id>/locked until after\n> all forms of corruption have been checked so that it behaves as\n> originally intended (to wit: preventing pruning of a missing worktree\n> only).\n\nAn alternative could be to allow unlocking a worktree even if it is\ncorrupt, and with that, such an unprunable corrupt worktree can\nfirst be unlocked and then pruned?  A naive first thought is that\nmight make it slightly safer, but the reason why this approach was\ntaken is because the end user already said 'prune' so that should\ntrump whatever \".git/worktrees/<id>/\" has?\n\nBut the intent of locking a worktree is \"make sure that the end user\nis aware of the fact that it is locked before allowing the worktree\nto be pruned\", isn't it?  Unless there is a way for a corruption to\nadd the \"locked\" file the end-user did not intend to have, if we\nsense the \"locked\" file given to a worktree, shouldn't we honor that\nexisting \"locked\" file's intent?\n\nI am growing skeptical about the approach taken by this step.  There\nmust be something missing that I may become aware of after reading\nthe remainder of the series.\n\n\t... goes back and digs a bit ...\n\nThis came from 23af91d1 (prune: strategies for linked checkouts,\n2014-11-30) which explains:\n\n    To prevent `git prune --worktrees` from deleting a\n    $GIT_DIR/worktrees entry (which can be useful in some\n    situations, such as when the entry's working tree is stored on a\n    portable device), add a file named 'locked' to the entry's\n    directory.\n\nNotice that \"in some situations, such as\" gives just one example,\nand it is clear that it is the only envisioned use case.\n\nIt therefore feels more sensible to honor the \"locked\" file whether\nthe actual worktree (or just a part of it) is accessible or not when\n\"prune\" gets exercised.  After all, if some parts of the actual\nworktree gets moved to removal media when not in use, such a partial\nremoval may make the worktree appear as if it is \"corrupt\".  We do\nnot want to declare that it is corrupt and we ignore the locked\nstate, or do we?\n\nThanks.\n\n> [1]: Impossible, that is, without manually mucking around with\n>      .git/worktrees/<id>/ administrative files.\n>\n> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n> ---\n>  builtin/worktree.c        |  4 ++--\n>  t/t2401-worktree-prune.sh | 14 ++++++++++++--\n>  2 files changed, 14 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 9b15f19fc5..f7351413af 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -79,8 +79,6 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n>  \t\tstrbuf_addstr(reason, _(\"not a valid directory\"));\n>  \t\treturn 1;\n>  \t}\n> -\tif (file_exists(git_path(\"worktrees/%s/locked\", id)))\n> -\t\treturn 0;\n>  \tif (stat(git_path(\"worktrees/%s/gitdir\", id), &st)) {\n>  \t\tstrbuf_addstr(reason, _(\"gitdir file does not exist\"));\n>  \t\treturn 1;\n> @@ -121,6 +119,8 @@ static int prune_worktree(const char *id, struct strbuf *reason)\n>  \tpath[len] = '\\0';\n>  \tif (!file_exists(path)) {\n>  \t\tfree(path);\n> +\t\tif (file_exists(git_path(\"worktrees/%s/locked\", id)))\n> +\t\t\treturn 0;\n>  \t\tif (stat(git_path(\"worktrees/%s/index\", id), &st) ||\n>  \t\t    st.st_mtime <= expire) {\n>  \t\t\tstrbuf_addstr(reason, _(\"gitdir file points to non-existent location\"));\n> diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh\n> index b7d6d5d45a..9be8e97d66 100755\n> --- a/t/t2401-worktree-prune.sh\n> +++ b/t/t2401-worktree-prune.sh\n> @@ -69,13 +69,23 @@ test_expect_success 'prune directories with gitdir pointing to nowhere' '\n>  '\n>  \n>  test_expect_success 'not prune locked checkout' '\n> -\ttest_when_finished rm -r .git/worktrees &&\n> -\tmkdir -p .git/worktrees/ghi &&\n> +\ttest_when_finished rm -fr .git/worktrees ghi &&\n> +\tgit worktree add ghi &&\n>  \t: >.git/worktrees/ghi/locked &&\n> +\trm -r ghi &&\n>  \tgit worktree prune &&\n>  \ttest -d .git/worktrees/ghi\n>  '\n>  \n> +test_expect_success 'prune corrupt despite lock' '\n> +\ttest_when_finished rm -fr .git/worktrees ghi &&\n> +\tmkdir -p .git/worktrees/ghi &&\n> +\t: >.git/worktrees/ghi/gitdir &&\n> +\t: >.git/worktrees/ghi/locked &&\n> +\tgit worktree prune &&\n> +\t! test -d .git/worktrees/ghi\n> +'\n> +\n>  test_expect_success 'not prune recent checkouts' '\n>  \ttest_when_finished rm -r .git/worktrees &&\n>  \tgit worktree add jlm HEAD &&\n"},{"id":"399319","messageId":"xmqqv9k1jlmc.fsf@gitster.c.googlers.com","threadId":"53634","inReplyTo":"20200608062356.40264-4-sunshine@sunshineco.com","subject":"Re: [PATCH 3/8] worktree: give \"should be pruned?\" function more meaningful name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-08T21:25:31Z","receivedAt":"2020-06-08T21:25:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> Readers of the name prune_worktree() are likely to expect the function\n> to actually prune a worktree, however, it only answers the question\n> \"should this worktree be pruned?\". Give it a name more reflective of its\n> true purpose to avoid such confusion.\n\n100% agreed.\n"},{"id":"399320","messageId":"xmqqr1upjlfa.fsf@gitster.c.googlers.com","threadId":"53634","inReplyTo":"20200608062356.40264-5-sunshine@sunshineco.com","subject":"Re: [PATCH 4/8] worktree: make high-level pruning re-usable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-08T21:29:45Z","receivedAt":"2020-06-08T21:29:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> The low-level logic for removing a worktree is well encapsulated in\n> delete_git_dir(). However, high-level details related to pruning a\n> worktree -- such as dealing with verbosity and dry-run mode -- are not\n> encapsulated. Factor out this high-level logic into its own function so\n> it can be re-used as new worktree corruption detectors are added.\n>\n> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n> ---\n>  builtin/worktree.c | 15 +++++++++------\n>  1 file changed, 9 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 27681a1396..d0c046e885 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -133,6 +133,14 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)\n>  \treturn 0;\n>  }\n>  \n> +static void prune_worktree(const char *id, const char *reason)\n> +{\n> +\tif (show_only || verbose)\n> +\t\tprintf_ln(_(\"Removing %s/%s: %s\"), \"worktrees\", id, reason);\n> +\tif (!show_only)\n> +\t\tdelete_git_dir(id);\n> +}\n\nMakes sense, and this sensible name can be used only because we've\ncleaned up the other one in the previous step.\n\nGood so far (except that I still do not know why 2/8 is a good move\nafter reading the series up to this point).\n\n>  static void prune_worktrees(void)\n>  {\n>  \tstruct strbuf reason = STRBUF_INIT;\n> @@ -146,12 +154,7 @@ static void prune_worktrees(void)\n>  \t\tstrbuf_reset(&reason);\n>  \t\tif (!should_prune_worktree(d->d_name, &reason))\n>  \t\t\tcontinue;\n> -\t\tif (show_only || verbose)\n> -\t\t\tprintf_ln(_(\"Removing %s/%s: %s\"),\n> -\t\t\t\t  \"worktrees\", d->d_name, reason.buf);\n> -\t\tif (show_only)\n> -\t\t\tcontinue;\n> -\t\tdelete_git_dir(d->d_name);\n> +\t\tprune_worktree(d->d_name, reason.buf);\n>  \t}\n>  \tclosedir(dir);\n>  \tif (!show_only)\n"},{"id":"399322","messageId":"CAPig+cTRsyPWGb1J+28dWESKqK84RQr-yhZEmTf1=mtkmO4Y0w@mail.gmail.com","threadId":"53634","inReplyTo":"20200608193834.GA4196@konoha","subject":"Re: [PATCH 1/8] worktree: factor out repeated string literal","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-08T21:41:46Z","receivedAt":"2020-06-08T21:42:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 8, 2020 at 3:38 PM Shourya Shukla\n<shouryashukla.oo@gmail.com> wrote:\n> On 08/06 02:23, Eric Sunshine wrote:\n> > Although this change seems to increase the \"sentence lego quotient\", it\n> > should be reasonably safe, as the reason for removal is a distinct\n> > clause, not strictly related to the prefix. Moreover, the \"worktrees\" in\n> > \"Removing worktrees/%s:\" is a path literal which ought not be localized,\n> > so by factoring it out, we can more easily avoid exposing that path\n> > fragment to translators.\n>\n> Could you explain the above paragraph just a bit more? The English is\n> confusing me a bit.\n\nWhich part is confusing you? The \"sentence lego\" bit or the bit about\nnot wanting translators touching a path literal? Or something else?\n\nThe path element bit is easy: A literal path, such as\n\".git/worktrees/foo\" should not be translated to a user's local\nlanguage.\n\nSentence lego refers to the process of composing text from several\npieces. Doing so can make it difficult for a translator to localize it\nto a particular language because the standalone pieces don't\nnecessarily provide sufficient context for a proper translation. For\ninstance, this example is from [1]:\n\n    Translatable strings should be entire sentences. It is often not\n    possible to translate single verbs or adjectives in a\n    substitutable way.\n\n    printf (\"File %s is %s protected\", filename, rw ? \"write\" : \"read\");\n\n    Most translators will not look at the source and will thus only see\n    the string \"File %s is %s protected\", which is\n    unintelligible. Change this to\n\n    printf (rw ? \"File %s is write protected\" :\n             \"File %s is read protected\", filename);\n\n    This way the translator will not only understand the message, she\n    will also be able to find the appropriate grammatical\n    construction. A French translator for example translates \"write\n    protected\" like \"protected against writing\".\n\n[1]: https://www.gnu.org/software/gettext/manual/html_node/Preparing-Strings.html\n"},{"id":"399323","messageId":"xmqqmu5djk1s.fsf@gitster.c.googlers.com","threadId":"53634","inReplyTo":"20200608062356.40264-7-sunshine@sunshineco.com","subject":"Re: [PATCH 6/8] worktree: prune linked worktree referencing main worktree path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-08T21:59:27Z","receivedAt":"2020-06-08T21:59:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 2cb95f16d4..eebd77c46d 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -153,6 +153,11 @@ static int prune_cmp(const void *a, const void *b)\n>  \n>  \tif ((c = fspathcmp(x->string, y->string)))\n>  \t    return c;\n> +\t/* paths same; main worktee (util==0) sorts above all others */\n\nmissing 'r'.  \"util ==NULL\" or \"!util\"?  Is the \"main worktree has\nNULL in util\" an invariant enforced by prune_worktrees()?\n\n> +\tif (!x->util)\n> +\t\treturn -1;\n\nIf x->util and y->util are both NULL, shouldn't they compare equal?\n\nOr is there something that enforces an invariant that there is only\none such x whose util is NULL?  Yes, that is the case...\n\n> +\tif (!y->util)\n> +\t\treturn 1;\n>  \t/* paths same; sort by .git/worktrees/<id> */\n>  \treturn strcmp(x->util, y->util);\n>  }\n> @@ -171,6 +176,7 @@ static void prune_dups(struct string_list *l)\n>  static void prune_worktrees(void)\n>  {\n>  \tstruct strbuf reason = STRBUF_INIT;\n> +\tstruct strbuf main = STRBUF_INIT;\n>  \tstruct string_list kept = STRING_LIST_INIT_NODUP;\n>  \tDIR *dir = opendir(git_path(\"worktrees\"));\n>  \tstruct dirent *d;\n> @@ -190,6 +196,10 @@ static void prune_worktrees(void)\n>  \t}\n>  \tclosedir(dir);\n>  \n> +\tstrbuf_add_absolute_path(&main, get_git_common_dir());\n> +\t/* massage main worktree absolute path to match 'gitdir' content */\n> +\tstrbuf_strip_suffix(&main, \"/.\");\n> +\tstring_list_append(&kept, strbuf_detach(&main, 0));\n\n... of course, it is done here.\n\nThe existing loop picks up all <id> from .git/worktrees/<id>/ and\nthey always have xstrdup(<id>) that cannot be NULL.  But the string\nlist item created here, whose util is NULL, is thrown into &kept and\nthere is nobody who adds a string list item with NULL in util.  So\nyes, there is only one \"main\" prune_cmp() needs to worry about.\n\nAnd the reason why prune_cmp() tiebreaks entries with the same .string\n(i.e. \"path\") so that the primary worktree comes early is because ...\n\n>  \tprune_dups(&kept);\n\n... deduping is done by later entries with the same path as an\nearlier one, keeping the earliest one among the ones with the same\npath.  We want the primary worktree to survive, or we'd be in deep\nyoghurt.\n\nThat reason is more important to comment in prune_cmp() than the\nhint that !util is for primary worktree.  IOW, \"why do we sort the\nprimary one early?\" is more important than \"by inspecting .util\nfield, we sort primary one early\" (the latter lacks \"why\").\n\n>  \tstring_list_clear(&kept, 1);\n>  \n> diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh\n> index 7694f25a16..5f3db93b31 100755\n> --- a/t/t2401-worktree-prune.sh\n> +++ b/t/t2401-worktree-prune.sh\n> @@ -114,4 +114,16 @@ test_expect_success 'prune duplicate (linked/linked)' '\n>  \t! test -d .git/worktrees/w2\n>  '\n>  \n> +test_expect_success 'prune duplicate (main/linked)' '\n> +\ttest_when_finished rm -fr repo wt &&\n> +\ttest_create_repo repo &&\n> +\ttest_commit -C repo x &&\n> +\tgit -C repo worktree add --detach ../wt &&\n> +\trm -fr wt &&\n> +\tmv repo wt &&\n> +\tgit -C wt worktree prune --verbose >actual &&\n> +\ttest_i18ngrep \"duplicate entry\" actual &&\n> +\t! test -d .git/worktrees/wt\n> +'\n> +\n>  test_done\n"},{"id":"399324","messageId":"xmqqimg1jjx8.fsf@gitster.c.googlers.com","threadId":"53634","inReplyTo":"20200608062356.40264-8-sunshine@sunshineco.com","subject":"Re: [PATCH 7/8] worktree: generalize candidate worktree path validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-08T22:02:11Z","receivedAt":"2020-06-08T22:02:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> \"git worktree add\" checks that the specified path is a valid location\n> for a new worktree by ensuring that the path does not already exist and\n> is not already registered to another worktree (a path can be registered\n> but missing, for instance, if it resides on removable media). Since \"git\n> worktree add\" is not the only command which should perform such\n> validation (\"git worktree move\" ought to also), generalize the the\n> validation function for use by other callers, as well.\n\nMakes sense.\n\n> -\tvalidate_worktree_add(path, opts);\n> +\tworktrees = get_worktrees(0);\n> +\tcheck_candidate_path(path, opts->force, worktrees, \"add\");\n> +\tfree_worktrees(worktrees);\n> +\tworktrees = NULL;\n\nIt is somewhat unsatisfying that the libified helper still signals\nits displeasure by dying, but a faithful conversion that can be\ncleaned up later (if we wanted to) like this step is easier to\nreason about.\n\n"},{"id":"399325","messageId":"xmqqeeqpjjpe.fsf@gitster.c.googlers.com","threadId":"53634","inReplyTo":"20200608062356.40264-9-sunshine@sunshineco.com","subject":"Re: [PATCH 8/8] worktree: make \"move\" refuse to move atop missing registered worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-08T22:06:53Z","receivedAt":"2020-06-08T22:07:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> -\tif (file_exists(dst.buf))\n> -\t\tdie(_(\"target '%s' already exists\"), dst.buf);\n> +\tcheck_candidate_path(dst.buf, force, worktrees, \"move\");\n\nOK.  Moving to a location that is already occupied by an existing\nfile or a directory, even if that file or directory is not one of\nthe existing worktree, used to die here, but check_candidate_path()\nperforms that check and dies with almost the same message (it does\nnot say 'target'), so there is no loss of safety here.  The check\ndone in the check_candidate_path() helper is even better in that it\nallows an existing directory as long as it is empty.\n\n> diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh\n> index 939d18d728..7035c9d72e 100755\n> --- a/t/t2403-worktree-move.sh\n> +++ b/t/t2403-worktree-move.sh\n> @@ -112,6 +112,27 @@ test_expect_success 'move locked worktree (force)' '\n>  \tgit worktree move --force --force flump ploof\n>  '\n>  \n> +test_expect_success 'refuse to move worktree atop existing path' '\n> +\t> bobble &&\n\nStyle?\n\n> +\tgit worktree add --detach beeble &&\n> +\ttest_must_fail git worktree move beeble bobble\n> +'\n> +\n> +test_expect_success 'move atop existing but missing worktree' '\n> +\tgit worktree add --detach gnoo &&\n> +\tgit worktree add --detach pneu &&\n> +\trm -fr pneu &&\n> +\ttest_must_fail git worktree move gnoo pneu &&\n> +\tgit worktree move --force gnoo pneu &&\n> +\n> +\tgit worktree add --detach nu &&\n> +\tgit worktree lock nu &&\n> +\trm -fr nu &&\n> +\ttest_must_fail git worktree move pneu nu &&\n> +\ttest_must_fail git worktree --force move pneu nu &&\n> +\tgit worktree move --force --force pneu nu\n> +'\n> +\n>  test_expect_success 'move a repo with uninitialized submodule' '\n>  \tgit init withsub &&\n>  \t(\n\nThanks.\n"},{"id":"399353","messageId":"CAPig+cRd1_U7MZ41LBcFSTaFGq_ENdY9JufV_jvwSX=AMV0NOQ@mail.gmail.com","threadId":"53634","inReplyTo":"xmqqzh9djlpb.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/8] worktree: prune corrupted worktree even if locked","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-09T17:34:55Z","receivedAt":"2020-06-09T17:35:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 8, 2020 at 5:24 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> > The .git/worktrees/<id>/locked file created by \"git worktree lock\" is\n> > intended to prevent a missing worktree -- which might reside on a\n> > removable device or network share -- from being pruned. It is not meant\n> > to prevent a corrupt worktree from being pruned, yet it short-circuits\n> > almost all \"git worktree prune\" corruption checks.\n>\n> The '.git/worktrees/<id>/locked' file is what 'It' in \"It is not\n> meant to\" refers to, but the 'it' in \"yet it short-circuits\" cannot\n> refer to the same thing---my reading hiccuped there.\n>\n> \"Its presence causes most of the corruption checks skipped by 'git\n> worktree prune'\", perhaps.\n\nI can adopt that wording, but see below...\n\n> > This can make it\n> > impossible[1] to prune a worktree which becomes corrupt after the lock\n> > is placed since \"git worktree prune\" won't prune it, and it may not even\n> > be possible to unlock it with \"git worktree unlock\", depending upon the\n> > nature of the corruption.\n>\n> The latter is because... \"worktree unlock\" does not skip corruption\n> check and refuses to unlock a corrupted worktree, or something?\n\nThat bit of the commit message was a late addition and somewhat\nintentionally hand-wavy. I don't think \"git worktree unlock\" will\ncurrently die or misbehave due to corruption, but was thinking that it\nsomeday might if additional checks are ever added. But, it's not\nworth pursuing since...\n\n> But the intent of locking a worktree is \"make sure that the end user\n> is aware of the fact that it is locked before allowing the worktree\n> to be pruned\", isn't it? Unless there is a way for a corruption to\n> add the \"locked\" file the end-user did not intend to have, if we\n> sense the \"locked\" file given to a worktree, shouldn't we honor that\n> existing \"locked\" file's intent?\n>\n> I am growing skeptical about the approach taken by this step. There\n> must be something missing that I may become aware of after reading\n> the remainder of the series.\n\nYou're not the only person skeptical about this patch. I flip-flopped\non it multiple times, first convincing myself it was the right thing\nto do, then convincing myself that the original code was correct, and\nso forth. That's a good indication that such a change overall is\nquestionable.\n\nAside from that, this patch is unrelated to the intent of this series.\nSo, I'll drop it when I re-roll.\n"},{"id":"399354","messageId":"CAPig+cSbwRXeuuk1nOTbVtbQ+cf3Z3+ouCDVBONSMq0htxS48w@mail.gmail.com","threadId":"53634","inReplyTo":"xmqqmu5djk1s.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 6/8] worktree: prune linked worktree referencing main worktree path","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-09T17:38:09Z","receivedAt":"2020-06-09T17:38:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 8, 2020 at 5:59 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> > +     /* paths same; main worktee (util==0) sorts above all others */\n>\n> And the reason why prune_cmp() tiebreaks entries with the same .string\n> (i.e. \"path\") so that the primary worktree comes early is because ...\n>\n> >       prune_dups(&kept);\n>\n> ... deduping is done by later entries with the same path as an\n> earlier one, keeping the earliest one among the ones with the same\n> path.  We want the primary worktree to survive, or we'd be in deep\n> yoghurt.\n>\n> That reason is more important to comment in prune_cmp() than the\n> hint that !util is for primary worktree.  IOW, \"why do we sort the\n> primary one early?\" is more important than \"by inspecting .util\n> field, we sort primary one early\" (the latter lacks \"why\").\n\nI'll update the comment to explain this.\n\nThanks for the review.\n"}]}