{"thread":{"id":"53646","subject":"[PATCH v2 1/7] worktree: factor out repeated string literal","startedAt":"2020-06-10T06:32:00Z","lastAt":"2020-06-10T17:34:41Z","messageCount":13,"participants":["Eric Sunshine","Shourya Shukla","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":7},"messages":[{"id":"399390","messageId":"20200610063049.74666-2-sunshine@sunshineco.com","threadId":"53646","inReplyTo":"20200610063049.74666-1-sunshine@sunshineco.com","subject":"[PATCH v2 1/7] worktree: factor out repeated string literal","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-10T06:30:43Z","receivedAt":"2020-06-10T06:32:00Z","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.90.gabb59f83a2\n\n"},{"id":"399391","messageId":"20200610063049.74666-1-sunshine@sunshineco.com","threadId":"53646","inReplyTo":null,"subject":"[PATCH v2 0/7] worktree: tighten duplicate path detection","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-10T06:30:42Z","receivedAt":"2020-06-10T06:32:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This is a re-roll of [1] which teaches \"git worktree prune\" to detect\nmultiple worktrees referencing the same path, and makes \"git worktree\nmove\" disallow moving a worktree atop another registered worktree when\nthe destination worktree is missing (for instance, if it resides on\nremovable media).\n\nChanges in v2:\n\n* drop patch 2/8, which made 'prune' remove corrupt locked worktree\n  entries, since it was difficult to justify the change\n\n* fix a couple typos and a style violation\n\n* fix a Sparse warning reported by Ramsay[2]\n\n* fix a -Werror=main complaint noticed by Junio\n\n[1]: https://lore.kernel.org/git/20200608062356.40264-1-sunshine@sunshineco.com/T/\n[2]: https://lore.kernel.org/git/CAPig+cTF+pwBasVCzmucXmMZcm1K0ctkGOavj7bMcGsw2MvoKw@mail.gmail.com/T/\n\nEric Sunshine (7):\n  worktree: factor out repeated string literal\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             | 128 ++++++++++++++++++++++++---------\n t/t2401-worktree-prune.sh      |  24 +++++++\n t/t2403-worktree-move.sh       |  21 ++++++\n 4 files changed, 141 insertions(+), 36 deletions(-)\n\nInterdiff against v1:\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex dda7da497c..1238b6bab1 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -68,11 +68,11 @@ static void delete_worktrees_dir_if_empty(void)\n }\n \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+ * Return true if worktree entry should be pruned, along with the reason for\n+ * pruning. Otherwise, return false and the worktree's path, or NULL if it\n+ * cannot be determined. Caller is responsible for freeing returned path.\n  */\n-static char *worktree_disposition(const char *id, struct strbuf *reason)\n+static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)\n {\n \tstruct stat st;\n \tchar *path;\n@@ -80,19 +80,22 @@ static char *worktree_disposition(const char *id, struct strbuf *reason)\n \tsize_t len;\n \tssize_t read_result;\n \n+\t*wtpath = NULL;\n \tif (!is_directory(git_path(\"worktrees/%s\", id))) {\n \t\tstrbuf_addstr(reason, _(\"not a valid directory\"));\n-\t\treturn NULL;\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 NULL;\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, _(\"unable to read gitdir file (%s)\"),\n \t\t\t    strerror(errno));\n-\t\treturn NULL;\n+\t\treturn 1;\n \t}\n \tlen = xsize_t(st.st_size);\n \tpath = xmallocz(len);\n@@ -103,7 +106,7 @@ static char *worktree_disposition(const char *id, struct strbuf *reason)\n \t\t\t    strerror(errno));\n \t\tclose(fd);\n \t\tfree(path);\n-\t\treturn NULL;\n+\t\treturn 1;\n \t}\n \tclose(fd);\n \n@@ -112,29 +115,29 @@ static char *worktree_disposition(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 NULL;\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_addstr(reason, _(\"invalid gitdir file\"));\n \t\tfree(path);\n-\t\treturn NULL;\n+\t\treturn 1;\n \t}\n \tpath[len] = '\\0';\n \tif (!file_exists(path)) {\n-\t\tif (file_exists(git_path(\"worktrees/%s/locked\", id)))\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\tfree(path);\n-\t\t\treturn NULL;\n+\t\t\treturn 1;\n \t\t} else {\n-\t\t\treturn path;\n+\t\t\t*wtpath = path;\n+\t\t\treturn 0;\n \t\t}\n \t}\n-\treturn path;\n+\t*wtpath = path;\n+\treturn 0;\n }\n \n static void prune_worktree(const char *id, const char *reason)\n@@ -153,7 +156,12 @@ 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+\t/*\n+\t * paths same; prune_dupes() removes all but the first worktree entry\n+\t * having the same path, so sort main worktree ('util' is NULL) above\n+\t * linked worktrees ('util' not NULL) since main worktree can't be\n+\t * removed\n+\t */\n \tif (!x->util)\n \t\treturn -1;\n \tif (!y->util)\n@@ -176,7 +184,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 strbuf main_path = STRBUF_INIT;\n \tstruct string_list kept = STRING_LIST_INIT_NODUP;\n \tDIR *dir = opendir(git_path(\"worktrees\"));\n \tstruct dirent *d;\n@@ -187,19 +195,17 @@ 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\tpath = worktree_disposition(d->d_name, &reason);\n-\t\tif (path) {\n+\t\tif (should_prune_worktree(d->d_name, &reason, &path))\n+\t\t\tprune_worktree(d->d_name, reason.buf);\n+\t\telse if (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-\tstrbuf_add_absolute_path(&main, get_git_common_dir());\n+\tstrbuf_add_absolute_path(&main_path, 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+\tstrbuf_strip_suffix(&main_path, \"/.\");\n+\tstring_list_append(&kept, strbuf_detach(&main_path, NULL));\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 5f3db93b31..a6ce7f590b 100755\n--- a/t/t2401-worktree-prune.sh\n+++ b/t/t2401-worktree-prune.sh\n@@ -69,21 +69,11 @@ 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 -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+\ttest_when_finished rm -r .git/worktrees &&\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+\ttest -d .git/worktrees/ghi\n '\n \n test_expect_success 'not prune recent checkouts' '\ndiff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh\nindex 7035c9d72e..a4e1a178e0 100755\n--- a/t/t2403-worktree-move.sh\n+++ b/t/t2403-worktree-move.sh\n@@ -113,7 +113,7 @@ test_expect_success 'move locked worktree (force)' '\n '\n \n test_expect_success 'refuse to move worktree atop existing path' '\n-\t> bobble &&\n+\t>bobble &&\n \tgit worktree add --detach beeble &&\n \ttest_must_fail git worktree move beeble bobble\n '\n\nRange-diff against v1:\n1:  75e2f832bf = 1:  75e2f832bf worktree: factor out repeated string literal\n2:  24662000d2 < -:  ---------- worktree: prune corrupted worktree even if locked\n3:  4fb95b3eea = 2:  0e458b3da5 worktree: give \"should be pruned?\" function more meaningful name\n4:  d16d993aa2 = 3:  7cf5b6ca40 worktree: make high-level pruning re-usable\n5:  f6bf2f0e72 ! 4:  e28790761f worktree: prune duplicate entries referencing same worktree path\n    @@ builtin/worktree.c: static void delete_worktrees_dir_if_empty(void)\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    ++ * Return true if worktree entry should be pruned, along with the reason for\n    ++ * pruning. Otherwise, return false and the worktree's path, or NULL if it\n    ++ * cannot be determined. Caller is responsible for freeing returned path.\n     + */\n    -+static char *worktree_disposition(const char *id, struct strbuf *reason)\n    ++static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)\n      {\n      \tstruct stat st;\n      \tchar *path;\n     @@ builtin/worktree.c: static int should_prune_worktree(const char *id, struct strbuf *reason)\n    + \tsize_t len;\n    + \tssize_t read_result;\n      \n    ++\t*wtpath = NULL;\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    -@@ builtin/worktree.c: 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    + \t\treturn 1;\n     @@ builtin/worktree.c: 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\treturn 1;\n      \t\t} else {\n    --\t\t\treturn 0;\n    -+\t\t\treturn path;\n    ++\t\t\t*wtpath = path;\n    + \t\t\treturn 0;\n      \t\t}\n      \t}\n     -\tfree(path);\n    --\treturn 0;\n    -+\treturn path;\n    ++\t*wtpath = path;\n    + \treturn 0;\n      }\n      \n    - static void prune_worktree(const char *id, const char *reason)\n     @@ builtin/worktree.c: static void prune_worktree(const char *id, const char *reason)\n      \t\tdelete_git_dir(id);\n      }\n    @@ builtin/worktree.c: static void prune_worktree(const char *id, const char *reaso\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\tcontinue;\n    +-\t\tprune_worktree(d->d_name, reason.buf);\n    ++\t\tif (should_prune_worktree(d->d_name, &reason, &path))\n    ++\t\t\tprune_worktree(d->d_name, reason.buf);\n    ++\t\telse if (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     +\n6:  6244cbb689 ! 5:  ded0632001 worktree: prune linked worktree referencing main worktree path\n    @@ builtin/worktree.c: 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    ++\t/*\n    ++\t * paths same; prune_dupes() removes all but the first worktree entry\n    ++\t * having the same path, so sort main worktree ('util' is NULL) above\n    ++\t * linked worktrees ('util' not NULL) since main worktree can't be\n    ++\t * removed\n    ++\t */\n     +\tif (!x->util)\n     +\t\treturn -1;\n     +\tif (!y->util)\n    @@ builtin/worktree.c: 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 strbuf main_path = STRBUF_INIT;\n      \tstruct string_list kept = STRING_LIST_INIT_NODUP;\n      \tDIR *dir = opendir(git_path(\"worktrees\"));\n      \tstruct dirent *d;\n    @@ builtin/worktree.c: static void prune_worktrees(void)\n      \t}\n      \tclosedir(dir);\n      \n    -+\tstrbuf_add_absolute_path(&main, get_git_common_dir());\n    ++\tstrbuf_add_absolute_path(&main_path, 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    ++\tstrbuf_strip_suffix(&main_path, \"/.\");\n    ++\tstring_list_append(&kept, strbuf_detach(&main_path, NULL));\n      \tprune_dups(&kept);\n      \tstring_list_clear(&kept, 1);\n      \n7:  1355b373d3 = 6:  2ca210fa73 worktree: generalize candidate worktree path validation\n8:  8bfe91bfd2 ! 7:  74dd7d1ac0 worktree: make \"move\" refuse to move atop missing registered worktree\n    @@ Commit message\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    +    the same path, which is expressly forbidden by design. For example:\n     \n             $ git clone foo.git\n             $ cd foo\n    @@ t/t2403-worktree-move.sh: test_expect_success 'move locked worktree (force)' '\n      '\n      \n     +test_expect_success 'refuse to move worktree atop existing path' '\n    -+\t> bobble &&\n    ++\t>bobble &&\n     +\tgit worktree add --detach beeble &&\n     +\ttest_must_fail git worktree move beeble bobble\n     +'\n-- \n2.27.0.90.gabb59f83a2\n\n"},{"id":"399392","messageId":"20200610063049.74666-3-sunshine@sunshineco.com","threadId":"53646","inReplyTo":"20200610063049.74666-1-sunshine@sunshineco.com","subject":"[PATCH v2 2/7] worktree: give \"should be pruned?\" function more meaningful name","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-10T06:30:44Z","receivedAt":"2020-06-10T06:32:09Z","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 9b15f19fc5..35d38607e7 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.90.gabb59f83a2\n\n"},{"id":"399393","messageId":"20200610063049.74666-7-sunshine@sunshineco.com","threadId":"53646","inReplyTo":"20200610063049.74666-1-sunshine@sunshineco.com","subject":"[PATCH v2 6/7] worktree: generalize candidate worktree path validation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-10T06:30:48Z","receivedAt":"2020-06-10T06:32:11Z","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 350108eba0..8fcf3f38fb 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -280,34 +280,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@@ -324,8 +323,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.90.gabb59f83a2\n\n"},{"id":"399394","messageId":"20200610063049.74666-4-sunshine@sunshineco.com","threadId":"53646","inReplyTo":"20200610063049.74666-1-sunshine@sunshineco.com","subject":"[PATCH v2 3/7] worktree: make high-level pruning re-usable","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-10T06:30:45Z","receivedAt":"2020-06-10T06:32:11Z","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 35d38607e7..706ac68751 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.90.gabb59f83a2\n\n"},{"id":"399395","messageId":"20200610063049.74666-6-sunshine@sunshineco.com","threadId":"53646","inReplyTo":"20200610063049.74666-1-sunshine@sunshineco.com","subject":"[PATCH v2 5/7] worktree: prune linked worktree referencing main worktree path","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-10T06:30:47Z","receivedAt":"2020-06-10T06:32:13Z","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        | 15 +++++++++++++++\n t/t2401-worktree-prune.sh | 12 ++++++++++++\n 2 files changed, 27 insertions(+)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 65492752a7..350108eba0 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -156,6 +156,16 @@ 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/*\n+\t * paths same; prune_dupes() removes all but the first worktree entry\n+\t * having the same path, so sort main worktree ('util' is NULL) above\n+\t * linked worktrees ('util' not NULL) since main worktree can't be\n+\t * removed\n+\t */\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@@ -174,6 +184,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_path = STRBUF_INIT;\n \tstruct string_list kept = STRING_LIST_INIT_NODUP;\n \tDIR *dir = opendir(git_path(\"worktrees\"));\n \tstruct dirent *d;\n@@ -191,6 +202,10 @@ static void prune_worktrees(void)\n \t}\n \tclosedir(dir);\n \n+\tstrbuf_add_absolute_path(&main_path, get_git_common_dir());\n+\t/* massage main worktree absolute path to match 'gitdir' content */\n+\tstrbuf_strip_suffix(&main_path, \"/.\");\n+\tstring_list_append(&kept, strbuf_detach(&main_path, NULL));\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 fd3916fee0..a6ce7f590b 100755\n--- a/t/t2401-worktree-prune.sh\n+++ b/t/t2401-worktree-prune.sh\n@@ -104,4 +104,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.90.gabb59f83a2\n\n"},{"id":"399396","messageId":"20200610063049.74666-8-sunshine@sunshineco.com","threadId":"53646","inReplyTo":"20200610063049.74666-1-sunshine@sunshineco.com","subject":"[PATCH v2 7/7] worktree: make \"move\" refuse to move atop missing registered worktree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-10T06:30:49Z","receivedAt":"2020-06-10T06:32:14Z","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 expressly 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 8fcf3f38fb..1238b6bab1 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -863,8 +863,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..a4e1a178e0 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.90.gabb59f83a2\n\n"},{"id":"399397","messageId":"20200610063049.74666-5-sunshine@sunshineco.com","threadId":"53646","inReplyTo":"20200610063049.74666-1-sunshine@sunshineco.com","subject":"[PATCH v2 4/7] worktree: prune duplicate entries referencing same worktree path","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-10T06:30:46Z","receivedAt":"2020-06-10T06:32:17Z","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        | 49 ++++++++++++++++++++++++++++++++++-----\n t/t2401-worktree-prune.sh | 12 ++++++++++\n 2 files changed, 55 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 706ac68751..65492752a7 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 true if worktree entry should be pruned, along with the reason for\n+ * pruning. Otherwise, return false and the worktree's path, or NULL if it\n+ * cannot be determined. Caller is responsible for freeing returned path.\n+ */\n+static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)\n {\n \tstruct stat st;\n \tchar *path;\n@@ -75,6 +80,7 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)\n \tsize_t len;\n \tssize_t read_result;\n \n+\t*wtpath = NULL;\n \tif (!is_directory(git_path(\"worktrees/%s\", id))) {\n \t\tstrbuf_addstr(reason, _(\"not a valid directory\"));\n \t\treturn 1;\n@@ -120,16 +126,17 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)\n \t}\n \tpath[len] = '\\0';\n \tif (!file_exists(path)) {\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_addstr(reason, _(\"gitdir file points to non-existent location\"));\n+\t\t\tfree(path);\n \t\t\treturn 1;\n \t\t} else {\n+\t\t\t*wtpath = path;\n \t\t\treturn 0;\n \t\t}\n \t}\n-\tfree(path);\n+\t*wtpath = path;\n \treturn 0;\n }\n \n@@ -141,22 +148,52 @@ 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\t\tcontinue;\n-\t\tprune_worktree(d->d_name, reason.buf);\n+\t\tif (should_prune_worktree(d->d_name, &reason, &path))\n+\t\t\tprune_worktree(d->d_name, reason.buf);\n+\t\telse if (path)\n+\t\t\tstring_list_append(&kept, path)->util = xstrdup(d->d_name);\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 b7d6d5d45a..fd3916fee0 100755\n--- a/t/t2401-worktree-prune.sh\n+++ b/t/t2401-worktree-prune.sh\n@@ -92,4 +92,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.90.gabb59f83a2\n\n"},{"id":"399405","messageId":"20200610115028.GA11750@konoha","threadId":"53646","inReplyTo":"20200610063049.74666-5-sunshine@sunshineco.com","subject":"Re: [PATCH v2 4/7] worktree: prune duplicate entries referencing same worktree path","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-06-10T11:50:28Z","receivedAt":"2020-06-10T11:50:44Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 706ac68751..65492752a7 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 true if worktree entry should be pruned, along with the reason for\n> + * pruning. Otherwise, return false and the worktree's path, or NULL if it\n> + * cannot be determined. Caller is responsible for freeing returned path.\n> + */\n> +static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)\n>  {\n>  \tstruct stat st;\n>  \tchar *path;\n> @@ -75,6 +80,7 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)\n>  \tsize_t len;\n>  \tssize_t read_result;\n>  \n> +\t*wtpath = NULL;\n>  \tif (!is_directory(git_path(\"worktrees/%s\", id))) {\n>  \t\tstrbuf_addstr(reason, _(\"not a valid directory\"));\n>  \t\treturn 1;\n> @@ -120,16 +126,17 @@ static int should_prune_worktree(const char *id, struct strbuf *reason)\n>  \t}\n>  \tpath[len] = '\\0';\n>  \tif (!file_exists(path)) {\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_addstr(reason, _(\"gitdir file points to non-existent location\"));\n> +\t\t\tfree(path);\n>  \t\t\treturn 1;\n>  \t\t} else {\n> +\t\t\t*wtpath = path;\n>  \t\t\treturn 0;\n>  \t\t}\n>  \t}\n> -\tfree(path);\n> +\t*wtpath = path;\n>  \treturn 0;\n>  }\n\nWhat exactly is the role of 'wtpath' in here? Maybe this is explained in\nthe later patches. \n\n> @@ -141,22 +148,52 @@ 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\nCan we rename the function arguments better? 'a' and 'b' sound very\nnaive to me. Maybe change these to something more like: 'firstwt' and\n'secondwt'? Yeah even this sounds kiddish but you get the idea right? Or\nlike 'wt' and 'wt_dup'?\n\n> diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh\n> index b7d6d5d45a..fd3916fee0 100755\n> --- a/t/t2401-worktree-prune.sh\n> +++ b/t/t2401-worktree-prune.sh\n> @@ -92,4 +92,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\nNit: maybe make it 'rm -rf' as that's the popular way of doing it?\n\nOtherwise this looks good to me. But again, we need comments from others\ntoo.\n\nNicely done! :)\n"},{"id":"399412","messageId":"CAPig+cQjR78+wanyRXhB33qsVwHqjSAc_c1O+CG6ZkJi6W3mDA@mail.gmail.com","threadId":"53646","inReplyTo":"20200610115028.GA11750@konoha","subject":"Re: [PATCH v2 4/7] worktree: prune duplicate entries referencing same worktree path","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-10T15:21:11Z","receivedAt":"2020-06-10T15:21:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 10, 2020 at 7:50 AM Shourya Shukla\n<shouryashukla.oo@gmail.com> wrote:\n> > +static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)\n>\n> What exactly is the role of 'wtpath' in here? Maybe this is explained in\n> the later patches.\n\n'wtpath' holds the location of the worktree. It's used in this patch\nby prune_worktrees() to collect a list of paths which haven't been\nmarked for pruning. Once it has the full list, it passes it to\nprune_dups() for pruning duplicate entries.\n\n> > +static int prune_cmp(const void *a, const void *b)\n>\n> Can we rename the function arguments better? 'a' and 'b' sound very\n> naive to me. Maybe change these to something more like: 'firstwt' and\n> 'secondwt'? Yeah even this sounds kiddish but you get the idea right? Or\n> like 'wt' and 'wt_dup'?\n\nAs with any language, C has its idioms, as does Git itself. Sticking\nto idioms makes it easier for others to understand the code\nat-a-glance. Short variable names, such as \"i\" and \"j\" for indexes,\n\"n\" for counters, \"s\" and \"t\" for strings, are very common among\nexperienced programmers. Likewise, \"a\" and \"b\" are well-understood as\nthe arguments of a \"comparison\" function. There are many such examples\nin the Git source code itself. Here are just a few:\n\n    cmp_uint32(const void *a_, const void *b_)\n    maildir_filename_cmp(const char *a, const char *b)\n    tipcmp(const void *a_, const void *b_)\n    cmp_by_tag_and_age(const void *a_, const void *b_)\n    cmp_remaining_objects(const void *a, const void *b)\n    version_cmp(const char *a, const char *b)\n    diffnamecmp(const void *a_, const void *b_)\n    spanhash_cmp(const void *a_, const void *b_)\n    void_hashcmp(const void *a, const void *b)\n    pathspec_item_cmp(const void *a_, const void *b_)\n\n> > +test_expect_success 'prune duplicate (linked/linked)' '\n> > +   test_when_finished rm -fr .git/worktrees w1 w2 &&\n>\n> Nit: maybe make it 'rm -rf' as that's the popular way of doing it?\n\nIt's true that \"-rf\" has wider usage in Git tests than \"-fr\", though\nthe latter is heavily used, as well. Ordinarily, matching the style\nalready present in a particular script would be a good way to choose\nbetween the two, however, the worktree-related test scripts which this\nseries touches already have a mix of the two. This is the first use of\ncombined \"-r\" and \"-f\" in this particular script, so I could go with\nthe more common \"-rf\", however, it's not worth re-rolling just for\nthat.\n\nThanks for the review.\n"},{"id":"399422","messageId":"20200610171153.GA39055@konoha","threadId":"53646","inReplyTo":"20200610063049.74666-7-sunshine@sunshineco.com","subject":"Re: [PATCH v2 6/7] worktree: generalize candidate worktree path validation","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-06-10T17:11:53Z","receivedAt":"2020-06-10T17:12:03Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"On 10/06 02:30, Eric Sunshine wrote:\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\nThere is an extra 'the' after generalize.\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\nShould we do a 'return 1' on failure instead of just a blank 'return' so\nthat we can denote failure of finding a worktree?\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\nLet's wrap this to 72 characters at maximum per line maybe? Meaning that\nthe error message gets split into 2 lines.\n\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\nSimilarly here.\n\n>  \n>  static int add_worktree(const char *path, const char *refname,\n> @@ -324,8 +323,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\nIt is necessary to call 'free_worktrees(worktrees)' at the end right?\nThe 'get_worktrees()' function states that\n    The caller is responsible for freeing the memory from the returned\n    worktree(s).\n"},{"id":"399424","messageId":"CAPig+cSLwPmDbk8Wjqb06OrNGHfmTLDOHjs88C+q1FhLGqLrrQ@mail.gmail.com","threadId":"53646","inReplyTo":"20200610171153.GA39055@konoha","subject":"Re: [PATCH v2 6/7] worktree: generalize candidate worktree path validation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-06-10T17:18:34Z","receivedAt":"2020-06-10T17:18:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 10, 2020 at 1:12 PM Shourya Shukla\n<shouryashukla.oo@gmail.com> wrote:\n> On 10/06 02:30, Eric Sunshine wrote:\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>\n> There is an extra 'the' after generalize.\n\nThanks for noticing. I'll fix it if I re-roll, otherwise it can stay\n(unless Junio happens to fix it when queuing).\n\n> >    if (!wt)\n> > -       goto done;\n> > +       return;\n>\n> Should we do a 'return 1' on failure instead of just a blank 'return' so\n> that we can denote failure of finding a worktree?\n\nThis function is declared as returning 'void', so we can't \"return 1\".\nThe function instead signals a problem by die()'ing.\n\nChanging it to return a success or failure result rather than dying is\na different matter which can be done later if someone wants to do so,\nbut is outside the scope of this patch series which is only making the\nminimal necessary changes to adapt the function for wider use.\n\n> > -       die(_(\"'%s' is a missing but locked worktree;\\nuse 'add -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear\"), path);\n> > +       die(_(\"'%s' is a missing but locked worktree;\\nuse '%s -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear\"), cmd, path);\n>\n> Let's wrap this to 72 characters at maximum per line maybe? Meaning that\n> the error message gets split into 2 lines.\n\nI'm not sure what you want to see wrapped; the warning message itself\nor the source code line? As for the warning message, it already is\nwrapped (see the embedded \"\\n\").\n\nAt any rate, this patch makes the minimal change necessary to meet the\ngoal of making the function re-usable. Anything beyond that (such as\nwrapping long lines) is outside the scope of the patch and would make\nit harder to reason about the changes. Wrapping the line is certainly\nsomething that someone can do later as a follow-up, but is not the\ngoal of this series.\n\n> > -   validate_worktree_add(path, opts);\n> > +   worktrees = get_worktrees(0);\n> > +   check_candidate_path(path, opts->force, worktrees, \"add\");\n> > +   free_worktrees(worktrees);\n> > +   worktrees = NULL;\n>\n> It is necessary to call 'free_worktrees(worktrees)' at the end? The\n> 'get_worktrees()' states that\n>   The caller is responsible for freeing the memory from the returned\n>   worktree(s).\n\nThis code _does_ call free_worktrees() as it should, so I'm not sure\nwhat you're asking or saying. (Perhaps you're confusing \"caller\" and\n\"callee\"?)\n"},{"id":"399425","messageId":"xmqqa71ag6zb.fsf@gitster.c.googlers.com","threadId":"53646","inReplyTo":"CAPig+cQjR78+wanyRXhB33qsVwHqjSAc_c1O+CG6ZkJi6W3mDA@mail.gmail.com","subject":"Re: [PATCH v2 4/7] worktree: prune duplicate entries referencing same worktree path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-10T17:34:32Z","receivedAt":"2020-06-10T17:34:41Z","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> On Wed, Jun 10, 2020 at 7:50 AM Shourya Shukla\n> <shouryashukla.oo@gmail.com> wrote:\n>> > +static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)\n>>\n>> What exactly is the role of 'wtpath' in here? Maybe this is explained in\n>> the later patches.\n>\n> 'wtpath' holds the location of the worktree. It's used in this patch\n> by prune_worktrees() to collect a list of paths which haven't been\n> marked for pruning. Once it has the full list, it passes it to\n> prune_dups() for pruning duplicate entries.\n\n\"wt\" being a fairly common abbreviation of \"work(ing) tree\" in the\ncodebase may escape new readers of the code.  The comment before the\nfunction can explicitly mention the variable name in the description\nto help them, I would think.  For example ...\n\n> +/*\n> + * Return true if worktree entry should be pruned, along with the reason for\n> + * pruning. Otherwise, return false and the worktree's path, or NULL if it\n\n... the first sentence makes it clear that the function returns two\nthings (i.e. \"true\"---presumably is returned as its return value, and\n\"the reason\"---the readers are supposed to guess it is stuffed in\nthe strbuf), and the second sentence also says the function returns\ntwo things (i.e. \"false\", and \"the worktree's path\"---it is not\nimmediately obvious where NULL goes, though).\n\n> + * cannot be determined. Caller is responsible for freeing returned path.\n> + */\n\n\tDetermine if the worktree entry specified by its \"id\" should\n\tbe pruned.\n\n\tWhen returning 'true', the caller-supplied strbuf \"reason\"\n\tis filled with the reason why it should be pruned.  \"wtpath\"\n\tis left intact.\n\n\tWhen returning 'false', the string variable pointed by\n\t\"wtpath\" receives the absolute path of the worktree; or NULL\n\tif the location of the worktree cannot be determined.\n\t\"reason\" is left intact.\n\nperhaps?  I didn't check what you do to *wtpath when you return\ntrue, so \"left intact\" may not be what you are doing, and I am *not*\nsuggesting to leave it intact in that case---I am only suggesting\nthat we should describe what happens.\n\n>> > +static int prune_cmp(const void *a, const void *b)\n>>\n>> Can we rename the function arguments better? 'a' and 'b' sound very\n>> naive to me. Maybe change these to something more like: 'firstwt' and\n>> 'secondwt'? Yeah even this sounds kiddish but you get the idea right? Or\n>> like 'wt' and 'wt_dup'?\n>\n> As with any language, C has its idioms, as does Git itself. Sticking\n> to idioms makes it easier for others to understand the code\n> at-a-glance. Short variable names, such as \"i\" and \"j\" for indexes,\n> \"n\" for counters, \"s\" and \"t\" for strings, are very common among\n> experienced programmers. Likewise, \"a\" and \"b\" are well-understood as\n> the arguments of a \"comparison\" function. There are many such examples\n> in the Git source code itself. Here are just a few:\n>\n>     cmp_uint32(const void *a_, const void *b_)\n>     maildir_filename_cmp(const char *a, const char *b)\n>     tipcmp(const void *a_, const void *b_)\n>     cmp_by_tag_and_age(const void *a_, const void *b_)\n>     cmp_remaining_objects(const void *a, const void *b)\n>     version_cmp(const char *a, const char *b)\n>     diffnamecmp(const void *a_, const void *b_)\n>     spanhash_cmp(const void *a_, const void *b_)\n>     void_hashcmp(const void *a, const void *b)\n>     pathspec_item_cmp(const void *a_, const void *b_)\n\nWhile that is true, what happens in the funcion is a bit unusual.\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\nUsually we do\n\n\tstatic int foo_cmp(const void *a_, const void *b_)\n\t{\n\t\tconst true_type *a = a_;\n\t\tconst true_type *b = b_;\n\n\t\t/* use a and b for comparison */\n\nwithout involving 'x' and 'y'.\n\n>> > +test_expect_success 'prune duplicate (linked/linked)' '\n>> > +   test_when_finished rm -fr .git/worktrees w1 w2 &&\n>>\n>> Nit: maybe make it 'rm -rf' as that's the popular way of doing it?\n>\n> It's true that \"-rf\" has wider usage in Git tests than \"-fr\", though\n> the latter is heavily used, as well.\n\nI myself write \"rm -rf\" more often when I am casually programming\nout of inertia, but when consciously making a decision on how to\nspell the combination, I tend to go alphabetical, because there is\nno logical reason to write 'r' first, other than the fact that \"-rf\"\nvisually looks prettier than \"-fr\".  \n\nIf recursiveness of the removal is more important than forcedness,\nthen favouring \"-rf\" over \"-fr\" would be justifiable, but I do not\nthink that is the case here.\n\nAnd as you said, it is not something that justifies a reroll (or\nupdating existing uses of \"-rf\") alone.\n"}]}