From a80c4c22147bae6d9f9b907b81ab3f4d129ab690 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Mon, 24 Feb 2020 04:08:46 -0500 Subject: worktree: improve find_worktree() documentation 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 of the .git/worktrees// 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 Signed-off-by: Junio C Hamano diff --git a/worktree.h b/worktree.h index caecc7a..b8a851b 100644 --- a/worktree.h +++ b/worktree.h @@ -44,8 +44,18 @@ int submodule_uses_worktrees(const char *path); const char *get_worktree_git_dir(const struct worktree *wt); /* - * Search a worktree that can be unambiguously identified by - * "arg". "prefix" must not be NULL. + * Search for the worktree identified unambiguously by `arg` -- typically + * supplied by the user via the command-line -- which may be a pathname or some + * shorthand uniquely identifying a worktree, thus making it convenient for the + * user to specify a worktree with minimal typing. For instance, if the last + * component (say, "foo") of a worktree's pathname is unique among worktrees + * (say, "work/foo" and "work/bar"), it can be used to identify the worktree + * unambiguously. + * + * `prefix` should be the `prefix` handed to top-level Git commands along with + * `argc` and `argv`. + * + * Return the worktree identified by `arg`, or NULL if not found. */ struct worktree *find_worktree(struct worktree **list, const char *prefix, -- cgit v0.10.2-6-g49f6 From bb4995fc3fab35b56f1ca48cda9187bcf340e643 Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Mon, 24 Feb 2020 04:08:47 -0500 Subject: worktree: add utility to find worktree by pathname 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 Signed-off-by: Junio C Hamano diff --git a/worktree.c b/worktree.c index 5b4793c..43c6685 100644 --- a/worktree.c +++ b/worktree.c @@ -215,7 +215,6 @@ struct worktree *find_worktree(struct worktree **list, const char *arg) { struct worktree *wt; - char *path; char *to_free = NULL; if ((wt = find_worktree_by_suffix(list, arg))) @@ -223,11 +222,17 @@ struct worktree *find_worktree(struct worktree **list, if (prefix) arg = to_free = prefix_filename(prefix, arg); - path = real_pathdup(arg, 0); - if (!path) { - free(to_free); + wt = find_worktree_by_path(list, arg); + free(to_free); + return wt; +} + +struct worktree *find_worktree_by_path(struct worktree **list, const char *p) +{ + char *path = real_pathdup(p, 0); + + if (!path) return NULL; - } for (; *list; list++) { const char *wt_path = real_path_if_valid((*list)->path); @@ -235,7 +240,6 @@ struct worktree *find_worktree(struct worktree **list, break; } free(path); - free(to_free); return *list; } diff --git a/worktree.h b/worktree.h index b8a851b..d242a6e 100644 --- a/worktree.h +++ b/worktree.h @@ -62,6 +62,12 @@ struct worktree *find_worktree(struct worktree **list, const char *arg); /* + * Return the worktree corresponding to `path`, or NULL if no such worktree + * exists. + */ +struct worktree *find_worktree_by_path(struct worktree **, const char *path); + +/* * Return true if the given worktree is the main one. */ int is_main_worktree(const struct worktree *wt); -- cgit v0.10.2-6-g49f6 From bb69b3b009c0093417942dba136b81fbf614c4ed Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Mon, 24 Feb 2020 04:08:48 -0500 Subject: worktree: don't allow "add" validation to be fooled by suffix matching "git worktree add " performs various checks before approving as a valid location for the new worktree. Aside from ensuring that does not already exist, one of the questions it asks is whether 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 . 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 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 Signed-off-by: Eric Sunshine Signed-off-by: Junio C Hamano diff --git a/builtin/worktree.c b/builtin/worktree.c index d6bc526..24f2280 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -234,14 +234,7 @@ static void validate_worktree_add(const char *path, const struct add_opts *opts) die(_("'%s' already exists"), path); worktrees = get_worktrees(0); - /* - * find_worktree()'s suffix matching may undesirably find the main - * rather than a linked worktree (for instance, when the basenames - * of the main worktree and the one being created are the same). - * We're only interested in linked worktrees, so skip the main - * worktree with +1. - */ - wt = find_worktree(worktrees + 1, NULL, path); + wt = find_worktree_by_path(worktrees, path); if (!wt) goto done; diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh index b5ece19..5a74954 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -570,6 +570,15 @@ test_expect_success '"add" an existing locked but missing worktree' ' git worktree add --force --force --detach gnoo ' +test_expect_success '"add" not tripped up by magic worktree matching"' ' + # if worktree "sub1/bar" exists, "git worktree add bar" in distinct + # directory `sub2` should not mistakenly complain that `bar` is an + # already-registered worktree + mkdir sub1 sub2 && + git -C sub1 --git-dir=../.git worktree add --detach bozo && + git -C sub2 --git-dir=../.git worktree add --detach bozo +' + test_expect_success FUNNYNAMES 'sanitize generated worktree name' ' git worktree add --detach ". weird*..?.lock.lock" && test -d .git/worktrees/---weird-.- -- cgit v0.10.2-6-g49f6