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
