{"thread":{"id":"50472","subject":"BUG: git worktree add directory-path fails due to over-aggressive disallowing of worktrees with same suffix","startedAt":"2019-02-11T23:50:36Z","lastAt":"2020-02-24T21:06:24Z","messageCount":8,"participants":["Cameron Gunnin","Eric Sunshine","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"369055","messageId":"0308570E-AAA3-43B8-A592-F4DA9760DBED@synopsys.com","threadId":"50472","inReplyTo":null,"subject":"BUG: git worktree add directory-path fails due to over-aggressive disallowing of worktrees with same suffix","fromName":"Cameron Gunnin","fromEmail":"cameron.gunnin@synopsys.com","sentAt":"2019-02-11T23:50:34Z","receivedAt":"2019-02-11T23:50:36Z","isPatch":false,"sender":{"key":"cameron.gunnin@synopsys.com","avatar":null},"body":"The bug:\n git init worktree-test-repo\n git --git-dir=worktree-test-repo/.git commit --allow-empty -m \"first commit\"\n git --git-dir=worktree-test-repo/.git branch branch1\n git --git-dir=worktree-test-repo/.git branch branch2\n mkdir unique-path-1 unique-path-2\n cd unique-path-1\n git --git-dir=../worktree-test-repo/.git worktree add subdir branch1\n cd ../unique-path-2\n git --git-dir=../worktree-test-repo/.git worktree add subdir branch2\n # FAILS WITH: fatal: 'subdir' is a missing but already registered worktree; use 'add -f' to override, or 'prune' or 'remove' to clear\n\nNote: the following will succeed, implying that it is the suffix matching that is going awry:\n cd ..\n git --git-dir=worktree-test-repo/.git worktree add unique-path-2/subdir branch2\n\nThis appears to have been introduced by the following commit:\n commit cb56f55c16c128e18449c9417dc045f787c1b663\n Author: Eric Sunshine \n Date: Tue Aug 28 17:20:22 2018 -0400\n\n worktree: disallow adding same path multiple times\n\nThe intention of the above commit was to avoid a bad case where a worktree at a given path was removed manually then re-created via 'git worktree add', leading to two entries in 'git worktree list' for the same directory. However, the bug shown here is a valid use case, as we're technically using two different paths.\n\nThe fix, I think, should be applied to builtin/worktree.c to the validate_worktree_add method. After finding a worktree that matches the suffix (via find_worktree), it should check that the absolute path of the found worktree is the same as the absolute path of the worktree being added, and allow the add when they are different. Or, perhaps there should be a way to invoke 'find_worktree' such that it only finds absolute path matches.\n\nThank you for your time,\n- Cameron Gunnin\n\n"},{"id":"369197","messageId":"CAPig+cQh8hxeoVjLHDKhAcZVQPpPT5v0AUY8gsL9=qfJ7z-L2A@mail.gmail.com","threadId":"50472","inReplyTo":"0308570E-AAA3-43B8-A592-F4DA9760DBED@synopsys.com","subject":"Re: BUG: git worktree add directory-path fails due to over-aggressive disallowing of worktrees with same suffix","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-12T22:44:41Z","receivedAt":"2019-02-12T22:44:56Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 11, 2019 at 6:50 PM Cameron Gunnin\n wrote:\n> The bug:\n> cd unique-path-1\n> git --git-dir=../worktree-test-repo/.git worktree add subdir branch1\n> cd ../unique-path-2\n> git --git-dir=../worktree-test-repo/.git worktree add subdir branch2\n> # FAILS WITH: fatal: 'subdir' is a missing but already registered worktree; use 'add -f' to override, or 'prune' or 'remove' to clear\n>\n> This appears to have been introduced by the following commit:\n> worktree: disallow adding same path multiple times\n>\n> The fix, I think, should be applied to builtin/worktree.c to the\n> validate_worktree_add method. After finding a worktree that matches\n> the suffix (via find_worktree), it should check that the absolute\n> path of the found worktree is the same as the absolute path of the\n> worktree being added, and allow the add when they are different. Or,\n> perhaps there should be a way to invoke 'find_worktree' such that it\n> only finds absolute path matches.\n\nWhen crafting cb56f55c16 (worktree: disallow adding same path multiple\ntimes, 2018-08-28), I flip-flopped between two implementations: (1)\nusing find_worktree(), and (2) manually scanning the worktree list\nwith absolute path comparison. The latter approach, would not have\nsuffered from this problem. The one ugly bit of the manual scan was\nthat it was a bit too cozy with the underlying worktree\nimplementation, so I eventually went with the find_worktree() approach\ndespite its obvious drawback of doing suffix matching (though, I\ndidn't feel particularly comfortable with the decision).\n\nA likely reasonable approach to fixing it would probably be to\nintroduce new worktree API to find a worktree without magic suffix\nmatching (i.e. literal absolute path matching) which would avoid\nhaving to imbue the higher-level \"git worktree\" command with that\nlow-level knowledge, and then take advantage of that new API.\n"},{"id":"392397","messageId":"20200224090848.54321-3-sunshine@sunshineco.com","threadId":"50472","inReplyTo":"20200224090848.54321-1-sunshine@sunshineco.com","subject":"[PATCH 2/3] worktree: add utility to find worktree by pathname","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-24T09:08:47Z","receivedAt":"2020-02-24T09:12:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"find_worktree() employs heuristics to match user provided input -- which\nmay be a pathname or some sort of shorthand -- with an actual worktree.\nAlthough this convenience allows a user to identify a worktree with\nminimal typing, the black-box nature of these heuristics makes it\npotentially difficult for callers which already know the exact path of a\nworktree to be confident that the correct worktree will be returned for\nany specific pathname (particularly a relative one), especially as the\nheuristics are enhanced and updated.\n\nTherefore, add a companion function, find_worktree_by_path(), which\ndeterministically identifies a worktree strictly by pathname with no\ninterpretation and no magic matching.\n\nSigned-off-by: Eric Sunshine \n---\n worktree.c | 16 ++++++++++------\n worktree.h | 6 ++++++\n 2 files changed, 16 insertions(+), 6 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex 5b4793caa3..43c6685d4e 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -215,7 +215,6 @@ struct worktree *find_worktree(struct worktree **list,\n \t\t\t const char *arg)\n {\n \tstruct worktree *wt;\n-\tchar *path;\n \tchar *to_free = NULL;\n \n \tif ((wt = find_worktree_by_suffix(list, arg)))\n@@ -223,11 +222,17 @@ struct worktree *find_worktree(struct worktree **list,\n \n \tif (prefix)\n \t\targ = to_free = prefix_filename(prefix, arg);\n-\tpath = real_pathdup(arg, 0);\n-\tif (!path) {\n-\t\tfree(to_free);\n+\twt = find_worktree_by_path(list, arg);\n+\tfree(to_free);\n+\treturn wt;\n+}\n+\n+struct worktree *find_worktree_by_path(struct worktree **list, const char *p)\n+{\n+\tchar *path = real_pathdup(p, 0);\n+\n+\tif (!path)\n \t\treturn NULL;\n-\t}\n \tfor (; *list; list++) {\n \t\tconst char *wt_path = real_path_if_valid((*list)->path);\n \n@@ -235,7 +240,6 @@ struct worktree *find_worktree(struct worktree **list,\n \t\t\tbreak;\n \t}\n \tfree(path);\n-\tfree(to_free);\n \treturn *list;\n }\n \ndiff --git a/worktree.h b/worktree.h\nindex b8a851b92b..d242a6e71c 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -61,6 +61,12 @@ struct worktree *find_worktree(struct worktree **list,\n \t\t\t const char *prefix,\n \t\t\t const char *arg);\n \n+/*\n+ * Return the worktree corresponding to `path`, or NULL if no such worktree\n+ * exists.\n+ */\n+struct worktree *find_worktree_by_path(struct worktree **, const char *path);\n+\n /*\n * Return true if the given worktree is the main one.\n */\n-- \n2.25.1.526.gf05a752211\n\n"},{"id":"392398","messageId":"20200224090848.54321-4-sunshine@sunshineco.com","threadId":"50472","inReplyTo":"20200224090848.54321-1-sunshine@sunshineco.com","subject":"[PATCH 3/3] worktree: don't allow \"add\" validation to be fooled by suffix matching","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-24T09:08:48Z","receivedAt":"2020-02-24T09:12:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"\"git worktree add \" performs various checks before approving\n as a valid location for the new worktree. Aside from ensuring\nthat does not already exist, one of the questions it asks is\nwhether is already a registered worktree. To perform this check,\nit queries find_worktree() and disallows the \"add\" operation if\nfind_worktree() finds a match for . As a convenience, however,\nfind_worktree() casts an overly wide net to allow users to identify\nworktrees by shorthand in order to keep typing to a minimum. For\ninstance, it performs suffix matching which, given subtrees \"foo/bar\"\nand \"foo/baz\", can correctly select the latter when asked only for\n\"baz\".\n\n\"add\" validation knows the exact path it is interrogating, so this sort\nof heuristic-based matching is, at best, questionable for this use-case\nand, at worst, may may accidentally interpret as matching an\nexisting worktree and incorrectly report it as already registered even\nwhen it isn't. (In fact, validate_worktree_add() already contains a\nspecial case to avoid accidentally matching against the main worktree,\nprecisely due to this problem.)\n\nAvoid the problem of potential accidental matching against an existing\nworktree by instead taking advantage of find_worktree_by_path() which\nmatches paths deterministically, without applying any sort of magic\nshorthand matching performed by find_worktree().\n\nReported-by: Cameron Gunnin \nSigned-off-by: Eric Sunshine \n---\n builtin/worktree.c | 9 +--------\n t/t2400-worktree-add.sh | 9 +++++++++\n 2 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex d6bc5263f1..24f22800f3 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -234,14 +234,7 @@ static void validate_worktree_add(const char *path, const struct add_opts *opts)\n \t\tdie(_(\"'%s' already exists\"), path);\n \n \tworktrees = get_worktrees(0);\n-\t/*\n-\t * find_worktree()'s suffix matching may undesirably find the main\n-\t * rather than a linked worktree (for instance, when the basenames\n-\t * of the main worktree and the one being created are the same).\n-\t * We're only interested in linked worktrees, so skip the main\n-\t * worktree with +1.\n-\t */\n-\twt = find_worktree(worktrees + 1, NULL, path);\n+\twt = find_worktree_by_path(worktrees, path);\n \tif (!wt)\n \t\tgoto done;\n \ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex b5ece19460..5a7495474a 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -570,6 +570,15 @@ test_expect_success '\"add\" an existing locked but missing worktree' '\n \tgit worktree add --force --force --detach gnoo\n '\n \n+test_expect_success '\"add\" not tripped up by magic worktree matching\"' '\n+\t# if worktree \"sub1/bar\" exists, \"git worktree add bar\" in distinct\n+\t# directory `sub2` should not mistakenly complain that `bar` is an\n+\t# already-registered worktree\n+\tmkdir sub1 sub2 &&\n+\tgit -C sub1 --git-dir=../.git worktree add --detach bozo &&\n+\tgit -C sub2 --git-dir=../.git worktree add --detach bozo\n+'\n+\n test_expect_success FUNNYNAMES 'sanitize generated worktree name' '\n \tgit worktree add --detach \". weird*..?.lock.lock\" &&\n \ttest -d .git/worktrees/---weird-.-\n-- \n2.25.1.526.gf05a752211\n\n"},{"id":"392400","messageId":"20200224090848.54321-2-sunshine@sunshineco.com","threadId":"50472","inReplyTo":"20200224090848.54321-1-sunshine@sunshineco.com","subject":"[PATCH 1/3] worktree: improve find_worktree() documentation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-24T09:08:46Z","receivedAt":"2020-02-24T09:13:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Do a better job of explaining that find_worktree()'s main purpose is to\nlocate a worktree based upon input from a user which may be some sort of\nshorthand for identifying a worktree rather than an actual path. For\ninstance, one shorthand a user can use to identify a worktree is by\nunique path suffix (i.e. given worktrees at paths \"foo/bar\" and\n\"foo/baz\", the latter can be identified simply as \"baz\"). The actual\nheuristics find_worktree() uses to select a worktree may be expanded in\nthe future (for instance, one day it may allow worktree selection by\n of the .git/worktrees// administrative directory), thus the\ndocumentation does not provide a precise description of how matching is\nperformed, instead leaving it open-ended to allow for future\nenhancement.\n\nWhile at it, drop mention of the non-NULL requirement of `prefix` since\nNULL has long been allowed. For instance, prefix_filename() has\nexplicitly allowed NULL since 116fb64e43 (prefix_filename: drop length\nparameter, 2017-03-20), and find_worktree() itself since e4da43b1f0\n(prefix_filename: return newly allocated string, 2017-03-20).\n\nSigned-off-by: Eric Sunshine \n---\n worktree.h | 14 ++++++++++++--\n 1 file changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/worktree.h b/worktree.h\nindex caecc7a281..b8a851b92b 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -44,8 +44,18 @@ int submodule_uses_worktrees(const char *path);\n const char *get_worktree_git_dir(const struct worktree *wt);\n \n /*\n- * Search a worktree that can be unambiguously identified by\n- * \"arg\". \"prefix\" must not be NULL.\n+ * Search for the worktree identified unambiguously by `arg` -- typically\n+ * supplied by the user via the command-line -- which may be a pathname or some\n+ * shorthand uniquely identifying a worktree, thus making it convenient for the\n+ * user to specify a worktree with minimal typing. For instance, if the last\n+ * component (say, \"foo\") of a worktree's pathname is unique among worktrees\n+ * (say, \"work/foo\" and \"work/bar\"), it can be used to identify the worktree\n+ * unambiguously.\n+ *\n+ * `prefix` should be the `prefix` handed to top-level Git commands along with\n+ * `argc` and `argv`.\n+ *\n+ * Return the worktree identified by `arg`, or NULL if not found.\n */\n struct worktree *find_worktree(struct worktree **list,\n \t\t\t const char *prefix,\n-- \n2.25.1.526.gf05a752211\n\n"},{"id":"392399","messageId":"20200224090848.54321-1-sunshine@sunshineco.com","threadId":"50472","inReplyTo":"CAPig+cQh8hxeoVjLHDKhAcZVQPpPT5v0AUY8gsL9=qfJ7z-L2A@mail.gmail.com","subject":"[PATCH 0/3] git-worktree: fix \"add\" being fooled by suffix matching","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-24T09:08:45Z","receivedAt":"2020-02-24T09:13:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"\"git worktree add \" performs various checks before approving\n as a valid location for the new worktree. One such check is\nasking whether is already a registered worktree. Unfortunately,\nthis check can be fooled by the magic suffix matching performed by\nfind_worktree() as a convenience to minimize typing when identifying\nworktrees on the command-line.\n\nThat the check could be fooled was a known issue, and a special-case\nwork-around for accidental matching against the main worktree was\nalready in place even when the validation check was first\nimplemented[1]. Since that time, an additional case of accidental suffix\nmatching has been reported[2], which was not covered by the existing\nspecial case.\n\nRather than adding more band-aids to the validation to work around magic\nmatching performed by find_worktree(), this patch series fixes the\nproblem once and for all by locating a worktree deterministically based\nonly on pathname.\n\n[1]: https://lore.kernel.org/git/CAPig+cQh8hxeoVjLHDKhAcZVQPpPT5v0AUY8gsL9=qfJ7z-L2A@mail.gmail.com/\n[2]: https://lore.kernel.org/git/0308570E-AAA3-43B8-A592-F4DA9760DBED@synopsys.com/\n\nEric Sunshine (3):\n worktree: improve find_worktree() documentation\n worktree: add utility to find worktree by pathname\n worktree: don't allow \"add\" validation to be fooled by suffix matching\n\n builtin/worktree.c | 9 +--------\n t/t2400-worktree-add.sh | 9 +++++++++\n worktree.c | 16 ++++++++++------\n worktree.h | 20 ++++++++++++++++++--\n 4 files changed, 38 insertions(+), 16 deletions(-)\n\n-- \n2.25.1.526.gf05a752211\n\n"},{"id":"392439","messageId":"xmqqzhd7og06.fsf@gitster-ct.c.googlers.com","threadId":"50472","inReplyTo":"20200224090848.54321-1-sunshine@sunshineco.com","subject":"Re: [PATCH 0/3] git-worktree: fix \"add\" being fooled by suffix matching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-24T21:02:49Z","receivedAt":"2020-02-24T21:02:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Rather than adding more band-aids to the validation to work around magic\n> matching performed by find_worktree(), this patch series fixes the\n> problem once and for all by locating a worktree deterministically based\n> only on pathname.\n\nMakes sense. I wonder if strange synonyms that make more than one\npaths string refer to the same filesystem entity (read: case\ninsensitive or unicode smashing filesystems Windows and macintoshes\nuse) have similar effect on the codepath, and if so, if we want to\ndo anything about it (my personal answers are \"I don't know but I\nwould not be surprised if it is the case\" and \"No, just tell users\nnot to hurt themselves and move on\").\n\nWill queue. Thanks.\n"},{"id":"392440","messageId":"xmqqv9nvofuc.fsf@gitster-ct.c.googlers.com","threadId":"50472","inReplyTo":"20200224090848.54321-4-sunshine@sunshineco.com","subject":"Re: [PATCH 3/3] worktree: don't allow \"add\" validation to be fooled by suffix matching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-24T21:06:19Z","receivedAt":"2020-02-24T21:06:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Avoid the problem of potential accidental matching against an existing\n> worktree by instead taking advantage of find_worktree_by_path() which\n> matches paths deterministically, without applying any sort of magic\n> shorthand matching performed by find_worktree().\n\nCombination of 2/3 and 3/3 makes perfect sense as \"fix once and for\nall\" solution.\n\nThanks.\n"}]}