Re: [PATCH v4 4/4] worktree: teach "add" to check out existing branches

> On Sat, Mar 17, 2018 at 6:22 PM, 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.
> > [...]
> > We will still 'die()' if the branch is checked out in another worktree,
> > unless the --force flag is passed.
> >
> > 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 new branch based at HEAD is created automatically,
> > -as if `-b $(basename <path>)` was specified.
> > +then, as a convenience, a worktree with a branch named after
> > +`$(basename <path>)` (call it `<branch>`) is created.  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,
> > +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.
> 
> Should this mention --force?
> 
>     ... refuse to create the worktree (unless --force is used).

> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > @@ -29,6 +29,7 @@ struct add_opts {
> >         int keep_locked;
> >         const char *new_branch;
> >         int force_new_branch;
> > +       int checkout_existing_branch;
> 
> As 'force_new_branch' and 'checkout_existing_branch' are mutually
> exclusive, I wonder if they should be combined into an enum to make it
> easier to reason about.

> > @@ -318,8 +319,11 @@ static int add_worktree(const char *path, const char *refname,
> > -       if (opts->new_branch)
> > -               fprintf(stderr, _("creating new branch '%s'"), opts->new_branch);
> > +       if (opts->checkout_existing_branch)
> > +               fprintf(stderr, _("checking out branch '%s'"),
> > +                       refname);
> 
> As with "creating branch" in 2/4, "checking out branch..." here is
> missing a newline.

> > +       else if (opts->new_branch)
> > +               fprintf(stderr, _("creating branch '%s'"), opts->new_branch);
> >
> >         fprintf(stderr, _("worktree HEAD is now at %s"),
> >                 find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
> > diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> > @@ -198,13 +198,22 @@ test_expect_success '"add" with <branch> omitted' '
> > -test_expect_success '"add" auto-vivify does not clobber existing branch' '
> > +test_expect_success '"add" auto-vivify checks out existing branch' '
> >         test_commit c1 &&
> >         test_commit c2 &&
> >         git branch precious HEAD~1 &&
> > -       test_must_fail git worktree add precious &&
> > +       git worktree add precious &&
> >         test_cmp_rev HEAD~1 precious &&
> > -       test_path_is_missing precious
> > +       (
> > +               cd precious &&
> > +               test_cmp_rev precious HEAD
> > +       )
> > +'
> 
> This test is no longer checking auto-vivification ("bringing a new
> branch to life automatically"), but rather branch name inference, so
> the title is now misleading. Perhaps retitle it to '"add" checks out
> existing branch of dwim'd name' or something.
> 
> (The name "precious" is also now misleading since it's no longer
> checking that a precious branch does not get clobbered, however,
> changing the name would make the diff unnecessarily noisy, so it's
> probably okay as is.)

> > +test_expect_success '"add" auto-vivify fails with checked out branch' '
> > +       git checkout -b test-branch &&
> > +       test_must_fail git worktree add test-branch &&
> > +       test_path_is_missing test-branch
> >  '
> 
> Should we also have a corresponding test that this "fail" can be
> overridden by --force? (I realize that --force is tested elsewhere,
> but only with an explicitly-provided branch name, whereas this dwims
> the branch name.)