Re: [PATCH v5 0/6] worktree: teach "add" to check out existing branches

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 27, 2018, 08:58 UTC
Message-ID: <CAPig+cSS2T-CweGtfO1vm=cskS91hLCHBoOhDuoqHAzg=27WbA@mail.gmail.com>
In-Reply-To: <20180325134947.25828-1-t.gummerer@gmail.com>

On Sun, Mar 25, 2018 at 9:49 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:

> Thanks Eric for the review of the previous round and Duy and Junio for
> additional comments.
> This round should address all of Eric's comments from the previous round.

Thanks, it appears to cover my review comments from the previous round. I do have some additional comments on this round (which I could have raised with the previous round if I had thought of them at the time).

> As explained in more detail in a reply to the review comment directly,
> I did not add an enum to 'struct add_opts', for 'force_new_branch' and
> 'checkout_existing_branch', but instead removed 'force_new_branch'
> from the struct as it's not required.

Makes sense. In fact, I had thoughts along these lines during your previous dwim-ery series. See my comments on patch 3/6.

> The rest of the updates are mainly in the user facing messages,
> documentation and one added test.
> Interdiff below:

The interdiff looks sane. Unfortunately, due to UI regressions, I'm having second thoughts about whether this series is going in the right direction. See my comments on patch 2/6.