Re: [PATCH 5/9] worktree: disallow adding same path multiple times

From: Jeff King <peff@peff.net>
Date: Aug 30, 2018, 07:28 UTC
Message-ID: <20180830072808.GB11944@sigill.intra.peff.net>
In-Reply-To: <20180828212026.21989-6-sunshine@sunshineco.com>

On Tue, Aug 28, 2018 at 05:20:22PM -0400, Eric Sunshine wrote:

Show 11 quoted lines

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

Show 24 quoted lines

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

Show 8 quoted lines

> +	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