On Tue, Aug 28, 2018 at 05:20:22PM -0400, Eric Sunshine wrote: > A given path should only ever be associated with a single registered > worktree. This invariant is enforced by refusing to create a new > worktree at a given path if that path already exists. For example: > > $ git worktree add -q --detach foo > $ git worktree add -q --detach foo > fatal: 'foo' already exists > > However, the check can be fooled, and the invariant broken, if the > path is missing. Continuing the example: > [...] Nicely explained. > diff --git a/builtin/worktree.c b/builtin/worktree.c > index 46c93771e8..1122f27b5f 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -221,8 +221,33 @@ static const char *worktree_basename(const char *path, int *olen) > > static void validate_worktree_add(const char *path, const struct add_opts *opts) > { > + struct worktree **worktrees; > + struct worktree *wt; > + int locked; > + > if (file_exists(path) && !is_empty_dir(path)) > 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); Very appropriate use of a comment. :) I wondered whether find_worktree() would discover a collision like this: git worktree add --detach foo rm -rf foo git worktree add --detach bar/foo but it is smart enough to actually compare the gitdirs and so we (correctly) create "foo" and "foo1" in $GIT_DIR/worktrees. > + if (!wt) > + goto done; > + > + locked = !!is_worktree_locked(wt); > + if (locked) > + die(_("'%s' is a missing but locked worktree;\nuse 'unlock' and 'prune' or 'remove' to clear"), path); > + else > + die(_("'%s' is a missing but already registered worktree;\nuse 'prune' or 'remove' to clear"), path); Nice, I like giving separate messages for the locked and unlocked cases (which I imagine will become even more obvious as you add --force support). The formatting of the message itself is a little funny: $ git worktree add --detach foo fatal: 'foo' is a missing but already registered worktree; use 'prune' or 'remove' to clear I'd say that the second line would ideally be advise(), since we're dying. You could do: error(%s is missing...) advise(use prune...) exit(128); but that loses the "fatal" in the first message. I wonder if just manually writing "hint:" would be so bad. > +done: > + free_worktrees(worktrees); You could easily avoid this goto with: if (wt) { /* check wt or die */ } free_worktrees(worktrees); but it may not be worth it if the logic gets more complicated in future patches. -Peff