On 03/20, Eric Sunshine wrote:
> On Sat, Mar 17, 2018 at 6:22 PM, Thomas Gummerer <[email protected]> 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 <[email protected]>
> > ---
> > 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).
Yeah, will add.
> > 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.
I gave this a try, but I'm not sure the end result is nicer. The
problem is that 'new_branch' and 'force_new_branch' both are mutually
exclusive with 'checkout_existing_branch', but 'force_new_branch' is a
"superset" of 'new_branch'.
I can't seem to think of a nice way to encode that, especially not
without duplicating information we're already carrying in
'new_branch'. Looking at the code however I see that
'force_new_branch' is already only duplicating information that we
already have in a variable in the same function. I think just
removing that duplication and keeping the 'checkout_existing_branch'
variable in the 'add_opts' would make most sense.
> > @@ -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.
Thanks will add.
> > + 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.)
Good point. I can add an additional patch with that rename, so the
changes here stay more obvious, but the end result would still end up
less confusing.
> > +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.)
Yes, will add a test. Thanks for your review!
