BUG: git worktree add directory-path fails due to over-aggressive disallowing of worktrees with same suffix

The bug:
  git init worktree-test-repo
  git --git-dir=worktree-test-repo/.git commit --allow-empty -m "first commit"
  git --git-dir=worktree-test-repo/.git branch branch1
  git --git-dir=worktree-test-repo/.git branch branch2
  mkdir unique-path-1 unique-path-2
  cd unique-path-1
  git --git-dir=../worktree-test-repo/.git worktree add subdir branch1
  cd ../unique-path-2
  git --git-dir=../worktree-test-repo/.git worktree add subdir branch2
  # FAILS WITH: fatal: 'subdir' is a missing but already registered worktree; use 'add -f' to override, or 'prune' or 'remove' to clear

Note: the following will succeed, implying that it is the suffix matching that is going awry:
  cd ..
  git --git-dir=worktree-test-repo/.git worktree add unique-path-2/subdir branch2

This appears to have been introduced by the following commit:
  commit cb56f55c16c128e18449c9417dc045f787c1b663
  Author: Eric Sunshine <sunshine@sunshineco.com>
  Date:   Tue Aug 28 17:20:22 2018 -0400

      worktree: disallow adding same path multiple times

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

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

Thank you for your time,
- Cameron Gunnin

On Mon, Feb 11, 2019 at 6:50 PM Cameron Gunnin <cameron.gunnin@synopsys.com> wrote:

When crafting cb56f55c16 (worktree: disallow adding same path multiple times, 2018-08-28), I flip-flopped between two implementations: (1) using find_worktree(), and (2) manually scanning the worktree list with absolute path comparison. The latter approach, would not have suffered from this problem. The one ugly bit of the manual scan was that it was a bit too cozy with the underlying worktree implementation, so I eventually went with the find_worktree() approach despite its obvious drawback of doing suffix matching (though, I didn't feel particularly comfortable with the decision).

A likely reasonable approach to fixing it would probably be to introduce new worktree API to find a worktree without magic suffix matching (i.e. literal absolute path matching) which would avoid having to imbue the higher-level "git worktree" command with that low-level knowledge, and then take advantage of that new API.

"git worktree add <path>" performs various checks before approving <path> as a valid location for the new worktree. One such check is asking whether <path> is already a registered worktree. Unfortunately, this check can be fooled by the magic suffix matching performed by find_worktree() as a convenience to minimize typing when identifying worktrees on the command-line.

That the check could be fooled was a known issue, and a special-case work-around for accidental matching against the main worktree was already in place even when the validation check was first implemented[1]. Since that time, an additional case of accidental suffix matching has been reported[2], which was not covered by the existing special case.

Rather than adding more band-aids to the validation to work around magic matching performed by find_worktree(), this patch series fixes the problem once and for all by locating a worktree deterministically based only on pathname.

[1]: https://lore.kernel.org/git/CAPig+cQh8hxeoVjLHDKhAcZVQPpPT5v0AUY8gsL9=qfJ7z-L2A@mail.gmail.com/ [2]: https://lore.kernel.org/git/0308570E-AAA3-43B8-A592-F4DA9760DBED@synopsys.com/

Eric Sunshine (3):
  worktree: improve find_worktree() documentation
  worktree: add utility to find worktree by pathname
  worktree: don't allow "add" validation to be fooled by suffix matching

 builtin/worktree.c      |  9 +--------
 t/t2400-worktree-add.sh |  9 +++++++++
 worktree.c              | 16 ++++++++++------
 worktree.h              | 20 ++++++++++++++++++--
 4 files changed, 38 insertions(+), 16 deletions(-)

find_worktree() employs heuristics to match user provided input -- which may be a pathname or some sort of shorthand -- with an actual worktree. Although this convenience allows a user to identify a worktree with minimal typing, the black-box nature of these heuristics makes it potentially difficult for callers which already know the exact path of a worktree to be confident that the correct worktree will be returned for any specific pathname (particularly a relative one), especially as the heuristics are enhanced and updated.

Therefore, add a companion function, find_worktree_by_path(), which deterministically identifies a worktree strictly by pathname with no interpretation and no magic matching.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 worktree.c | 16 ++++++++++------
 worktree.h |  6 ++++++
 2 files changed, 16 insertions(+), 6 deletions(-)

"git worktree add <path>" performs various checks before approving <path> as a valid location for the new worktree. Aside from ensuring that <path> does not already exist, one of the questions it asks is whether <path> is already a registered worktree. To perform this check, it queries find_worktree() and disallows the "add" operation if find_worktree() finds a match for <path>. As a convenience, however, find_worktree() casts an overly wide net to allow users to identify worktrees by shorthand in order to keep typing to a minimum. For instance, it performs suffix matching which, given subtrees "foo/bar" and "foo/baz", can correctly select the latter when asked only for "baz".

"add" validation knows the exact path it is interrogating, so this sort of heuristic-based matching is, at best, questionable for this use-case and, at worst, may may accidentally interpret <path> as matching an existing worktree and incorrectly report it as already registered even when it isn't. (In fact, validate_worktree_add() already contains a special case to avoid accidentally matching against the main worktree, precisely due to this problem.)

Avoid the problem of potential accidental matching against an existing worktree by instead taking advantage of find_worktree_by_path() which matches paths deterministically, without applying any sort of magic shorthand matching performed by find_worktree().

Reported-by: Cameron Gunnin <cameron.gunnin@synopsys.com>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 builtin/worktree.c      | 9 +--------
 t/t2400-worktree-add.sh | 9 +++++++++
 2 files changed, 10 insertions(+), 8 deletions(-)

Eric Sunshine <sunshine@sunshineco.com> writes:

Combination of 2/3 and 3/3 makes perfect sense as "fix once and for all" solution.

Thanks.

Do a better job of explaining that find_worktree()'s main purpose is to locate a worktree based upon input from a user which may be some sort of shorthand for identifying a worktree rather than an actual path. For instance, one shorthand a user can use to identify a worktree is by unique path suffix (i.e. given worktrees at paths "foo/bar" and "foo/baz", the latter can be identified simply as "baz"). The actual heuristics find_worktree() uses to select a worktree may be expanded in the future (for instance, one day it may allow worktree selection by <id> of the .git/worktrees/<id>/ administrative directory), thus the documentation does not provide a precise description of how matching is performed, instead leaving it open-ended to allow for future enhancement.

While at it, drop mention of the non-NULL requirement of `prefix` since NULL has long been allowed. For instance, prefix_filename() has explicitly allowed NULL since 116fb64e43 (prefix_filename: drop length parameter, 2017-03-20), and find_worktree() itself since e4da43b1f0 (prefix_filename: return newly allocated string, 2017-03-20).

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 worktree.h | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Eric Sunshine <sunshine@sunshineco.com> writes:

Makes sense. I wonder if strange synonyms that make more than one paths string refer to the same filesystem entity (read: case insensitive or unicode smashing filesystems Windows and macintoshes use) have similar effect on the codepath, and if so, if we want to do anything about it (my personal answers are "I don't know but I would not be surprised if it is the case" and "No, just tell users not to hurt themselves and move on").

Will queue.  Thanks.