On 03/20, Eric Sunshine wrote:

On Sat, Mar 17, 2018 at 6:22 PM, Thomas Gummerer [off-list ref] wrote:

quoted

[...]
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 <redacted>
---

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.

quoted

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.

quoted

@@ -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.

quoted

+       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.

quoted

+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!