On Thu, Aug 30, 2018 at 3:28 AM Jeff King <peff@xxxxxxxx> wrote:
> 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.
> > + 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.
> 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.
> > +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.
