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

On 01/22, Duy Nguyen wrote:

On Sun, Jan 21, 2018 at 7:02 PM, Thomas Gummerer [off-list ref] wrote:

quoted

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

It starts getting a bit too magical to me. We probably should print
something "switching to branch..." or "creating new branch ..."  to
let people know what decision was taken, unless --quiet is given. Yeah
I know --quiet does not exist. You don't need to add it now either
since nobody has asked for it.

I think that's a good idea.  I'll add that, thanks.

More or less related to this, there was a question [1] whether we
could do better than $(basename <path>) for determining branch name.
Since you're doing start to check if a branch exists, people may start
asking to check for branch "foo/bar" from the path abc/foo/bar instead
of just branch "bar".

Thanks for pointing me at this.  I feel like we might get ourselves
some backwards compatibility worries when doing that.  Lets say
someone has a branch 'feature/a', using 'git worktree feature/a' would
currently create a new branch 'a', and does not die.

I'd guess most users would want 'feature/a' checked out in that case,
but we can't exactly be sure we won't break anyones workflow here.
With your suggestion I guess that would be mitigated somehow as it
shows which action is taken, but dunno.

Should we hide this behaviour behind a flag, and plan for it
eventually becoming the default?

[1] https://github.com/git-for-windows/git/issues/1390

quoted

 list::

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7cef5b120b..148a864bb9 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c

@@ -411,13 +411,21 @@ static int add(int ac, const char **av, const char *prefix)
        if (ac < 2 && !opts.new_branch && !opts.detach) {
                int n;
                const char *s = worktree_basename(path, &n);
-               opts.new_branch = xstrndup(s, n);
-               if (guess_remote) {
-                       struct object_id oid;
-                       const char *remote =
-                               unique_tracking_name(opts.new_branch, &oid);
-                       if (remote)
-                               branch = remote;
+               const char *branchname = xstrndup(s, n);
+               struct strbuf ref = STRBUF_INIT;

Perhaps a blank line after this to separate var declarations and the rest.

Will add. 

quoted

+               if (!strbuf_check_branch_ref(&ref, branchname) &&
+                   ref_exists(ref.buf)) {
+                       branch = branchname;

Hmm.. do we need UNLEAK(branch);? Maybe you should try valgrind, I'm not sure.

Good question, I'll have a look, thanks.

quoted

+                       opts.checkout = 1;
+               } else {
+                       opts.new_branch = branchname;
+                       if (guess_remote) {
+                               struct object_id oid;
+                               const char *remote =
+                                       unique_tracking_name(opts.new_branch, &oid);
+                               if (remote)
+                                       branch = remote;
+                       }
                }
        }

diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 2b95944973..721b0e4c26 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh

@@ -198,13 +198,22 @@ test_expect_success '"add" with <branch> omitted' '
        test_cmp_rev HEAD bat
 '

-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
+       )
+'
+
+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
 '

 test_expect_success '"add" no auto-vivify with --detach and <branch> omitted' '
--

2.16.0.312.g896df04e46

-- 
Duy