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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 30, 2018, 08:22 UTC
Message-ID: <CAPig+cSbUVcs+eg1V_7xPY4ZMUpGZ5dQQ8iD8DWomZ-K=0mkug@mail.gmail.com>
In-Reply-To: <20180830072808.GB11944@sigill.intra.peff.net>

On Thu, Aug 30, 2018 at 3:28 AM Jeff King <peff@peff.net> wrote:

Show 11 quoted lines

> On Tue, Aug 28, 2018 at 05:20:22PM -0400, Eric Sunshine wrote:
> > +     /*
> > +      * 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 tried repeatedly to write the comment as a one-liner but simply couldn't do it in a way which conveyed the necessary information.

Show 10 quoted lines

> > +     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
> 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 couldn't come up with an improvement, so I went with this.

Show 6 quoted lines

> 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'm somewhat hesitant to sidestep die() here because die() brings its own "special" behaviors and cleanups. (Even if they aren't needed today, perhaps they will be in the future.)

> I wonder if just manually writing "hint:" would be so bad.

That would lose coloring of "hint", though.

Another alternative would be to make it just a long one-liner error message. I don't feel strongly one way or the other.

Show 13 quoted lines

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

I did suspect that a reviewer would comment on this, but, yes, the logic does get more complicated in subsequent patches, and the 'goto' makes a cleaner result in the end.