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

[Thomas Gummerer <t.gummerer@gmail.com>]

On 04/08, Eric Sunshine wrote:
> On Sat, Mar 31, 2018 at 11:18 AM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > [...]
> > However we can do a little better than that, and check the branch out if
> > it is not checked out anywhere else.  This will help users who just want
> > to check an existing branch out into a new worktree, and save a few
> > keystrokes.
> > [...]
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> > @@ -61,8 +61,13 @@ $ git worktree add --track -b <branch> <path> <remote>/<branch>
> >  If `<commit-ish>` is omitted and neither `-b` nor `-B` nor `--detach` used,
> > +then, as a convenience, a worktree with a branch named after
> > +`$(basename <path>)` (call it `<branch>`) is created.
> 
> I had a hard time digesting this. I _think_ it wants to say:
> 
>     If `<commit-ish>` is omitted and neither `-b` nor `-B` nor
>     `--detach` is used, then, as a convenience, the new worktree is
>     associated with a branch (call it `<branch>`) named after
>     `$(basename <path>)`.

Yeah, this is what it wants to say, and what you have sounds much
nicer, will change.

> >  If `<branch>`
> > +doesn't exist, a new branch based on HEAD is automatically created as
> > +if `-b <branch>` was given.  If `<branch>` exists in the repository,
> 
> Maybe: s/exists in the repository/does exist/
> Or: s/.../is a local branch/
> 
> Though, the latter may be getting too pedantic.
> 
> > +it will be checked out in the new worktree, if it's not checked out
> > +anywhere else, otherwise the command will refuse to create the
> > +worktree (unless `--force` is used).
> > diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> > @@ -198,13 +198,24 @@ test_expect_success '"add" with <branch> omitted' '
> > +test_expect_success '"add" checks out existing branch of dwimd name' '
> > +       git branch dwim HEAD~1 &&
> > +       git worktree add dwim &&
> > +       test_cmp_rev HEAD~1 dwim &&
> > +       (
> > +               cd dwim &&
> > +               test_cmp_rev dwim HEAD
> 
> Nit: Argument order of the two test_cmp_rev() invocations differs.
> 
> > +       )
> > +'
> > +
> > +test_expect_success '"add <path>" dwim fails with checked out branch' '
> > +       git checkout -b test-branch &&
> > +       test_must_fail git worktree add test-branch &&
> > +       test_path_is_missing test-branch
> > +'
> > +
> > +test_expect_success '"add --force" with existing dwimd name doesnt die' '
> > +       git worktree add --force test-branch
> >  '
> 
> Maybe make this last test slightly more robust by having it "git
> checkout test-branch" before "git worktree add ..." to protect against
> someone inserting a new test (which checks out some other branch)
> between these two. Probably not worth a re-roll.

As I'll have to re-roll anyway for the other suggestions, I'll change
this as well.

Thanks for your review!