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

[Thomas Gummerer]

On 02/06, Duy Nguyen wrote:
> On Tue, Feb 6, 2018 at 3:23 AM, Thomas Gummerer  wrote:
> > On 02/05, Duy Nguyen wrote:
> >> On Sun, Feb 04, 2018 at 10:13:05PM +, Thomas Gummerer wrote:
> >> > -   if (opts->new_branch)
> >> > +   if (opts->checkout_existing_branch)
> >> > +   fprintf(stderr, _(", checking out existing branch '%s'"),
> >> > +   refname);
> >> > +   else if (opts->new_branch)
> >> > fprintf(stderr, _(", creating new branch '%s'"), 
> >> > opts->new_branch);
> >>
> >> I wonder if "creating branch" and "checkout out branch" are enough.
> >
> > I thought printing the branch name might be a good idea just to show
> > more clearly what the dwim did.
> 
> No no printing branch name is definitely a good idea, especially when
> I think of one thing and type another. I shortened my example phrases
> too much. It should have been "creating branch %s" and "checkout out
> branch %s"

Ah sorry I misunderstood.  Yeah I think that makes sense then :)

> -- 
> Duy

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

[Duy Nguyen]

On Tue, Feb 6, 2018 at 3:23 AM, Thomas Gummerer  wrote:
> On 02/05, Duy Nguyen wrote:
>> On Sun, Feb 04, 2018 at 10:13:05PM +, Thomas Gummerer wrote:
>> > -   if (opts->new_branch)
>> > +   if (opts->checkout_existing_branch)
>> > +   fprintf(stderr, _(", checking out existing branch '%s'"),
>> > +   refname);
>> > +   else if (opts->new_branch)
>> > fprintf(stderr, _(", creating new branch '%s'"), 
>> > opts->new_branch);
>>
>> I wonder if "creating branch" and "checkout out branch" are enough.
>
> I thought printing the branch name might be a good idea just to show
> more clearly what the dwim did.

No no printing branch name is definitely a good idea, especially when
I think of one thing and type another. I shortened my example phrases
too much. It should have been "creating branch %s" and "checkout out
branch %s"
-- 
Duy

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

[Thomas Gummerer]

On 02/05, Duy Nguyen wrote:
> On Sun, Feb 04, 2018 at 10:13:05PM +, Thomas Gummerer wrote:
> > -   if (opts->new_branch)
> > +   if (opts->checkout_existing_branch)
> > +   fprintf(stderr, _(", checking out existing branch '%s'"),
> > +   refname);
> > +   else if (opts->new_branch)
> > fprintf(stderr, _(", creating new branch '%s'"), 
> > opts->new_branch);
> 
> I wonder if "creating branch" and "checkout out branch" are enough.

I thought printing the branch name might be a good idea just to show
more clearly what the dwim did.  Especially if we ever introduce new
heuristics, for example to cater for the case you mentioned in an
earlier email [1], I thought it might be helpful to also print the
branch name.  Especially because it makes it clear that the dwim the
reporter of the github issue hoped for didn't kick in at this point.

That said, I don't really think the branchname is of much use for my
usage of 'git worktree add', so I may be overestimating the need for
it.  I'm happy to remove it if that's preferred.

[1]: 

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

[Junio C Hamano]

Duy Nguyen  writes:

> On Sun, Feb 04, 2018 at 10:13:05PM +, Thomas Gummerer wrote:
>> -if (opts->new_branch)
>> +if (opts->checkout_existing_branch)
>> +fprintf(stderr, _(", checking out existing branch '%s'"),
>> +refname);
>> +else if (opts->new_branch)
>>  fprintf(stderr, _(", creating new branch '%s'"), 
>> opts->new_branch);
>
> I wonder if "creating branch" and "checkout out branch" are enough.

Yeah, I think new/existing are redundant.  If these were shown at
the same time, then the extra adjectives would help differenciating
them visually, but they are never shown at the same time, so...

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

[Duy Nguyen]

On Sun, Feb 04, 2018 at 10:13:05PM +, Thomas Gummerer wrote:
> - if (opts->new_branch)
> + if (opts->checkout_existing_branch)
> + fprintf(stderr, _(", checking out existing branch '%s'"),
> + refname);
> + else if (opts->new_branch)
>   fprintf(stderr, _(", creating new branch '%s'"), 
> opts->new_branch);

I wonder if "creating branch" and "checkout out branch" are enough.

> @@ -423,14 +427,25 @@ 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, );
> - opts.new_branch = xstrndup(s, n);
> - if (guess_remote) {
> - struct object_id oid;
> - const char *remote =
> - unique_tracking_name(opts.new_branch, );
> - if (remote)
> - branch = remote;
> + const char *branchname = xstrndup(s, n);
> + struct strbuf ref = STRBUF_INIT;
> +
> + if (!strbuf_check_branch_ref(, branchname) &&
> + ref_exists(ref.buf)) {
> + branch = branchname;
> + opts.checkout_existing_branch = 1;
> + UNLEAK(branch);
> + } else {
> + opts.new_branch = branchname;
> + if (guess_remote) {
> + struct object_id oid;
> + const char *remote =
> + unique_tracking_name(opts.new_branch, 
> );

Deep indentation may be a sign that it's time to move all this code to
a separate function, maybe dwim_branch() or something.

> + if (remote)
> + branch = remote;
> + }
>   }
> + strbuf_release();
>   }
>  
>   if (ac == 2 && !opts.new_branch && !opts.detach) {

[PATCH v2 3/3] worktree: teach "add" to check out existing branches

[Thomas Gummerer]

Currently 'git worktree add ' creates a new branch named after the
basename of the path by default.  If a branch with that name already
exists, the command refuses to do anything, unless the '--force' option
is given.

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.

As the current behaviour is to simply 'die()' when a branch with the name
of the basename of the path already exists, there are no backwards
compatibility worries here.

We will still 'die()' if the branch is checked out in another worktree,
unless the --force flag is passed.

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-worktree.txt |  9 +++--
 builtin/worktree.c | 31 +++
 t/t2025-worktree-add.sh| 15 ---
 3 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 41585f535d..98731b71a7 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -61,8 +61,13 @@ $ git worktree add --track -b   
/
 
 +
 If `` 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 )` was specified.
+then, as a convenience, a worktree with a branch named after
+`$(basename )` (call it ``) is created.  If ``
+doesn't exist, a new branch based on HEAD is automatically created as
+if `-b ` was given.  If `` 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.
 
 list::
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 74a853c2a3..ea420bb90b 100644
--- 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;
 };
 
 static int show_only;
@@ -320,7 +321,10 @@ static int add_worktree(const char *path, const char 
*refname,
if (ret)
goto done;
 
-   if (opts->new_branch)
+   if (opts->checkout_existing_branch)
+   fprintf(stderr, _(", checking out existing branch '%s'"),
+   refname);
+   else if (opts->new_branch)
fprintf(stderr, _(", creating new branch '%s'"), 
opts->new_branch);
 
fprintf(stderr, _(", setting HEAD to %s"),
@@ -423,14 +427,25 @@ 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, );
-   opts.new_branch = xstrndup(s, n);
-   if (guess_remote) {
-   struct object_id oid;
-   const char *remote =
-   unique_tracking_name(opts.new_branch, );
-   if (remote)
-   branch = remote;
+   const char *branchname = xstrndup(s, n);
+   struct strbuf ref = STRBUF_INIT;
+
+   if (!strbuf_check_branch_ref(, branchname) &&
+   ref_exists(ref.buf)) {
+   branch = branchname;
+   opts.checkout_existing_branch = 1;
+   UNLEAK(branch);
+   } else {
+   opts.new_branch = branchname;
+   if (guess_remote) {
+   struct object_id oid;
+   const char *remote =
+   unique_tracking_name(opts.new_branch, 
);
+   if (remote)
+   branch = remote;
+   }
}
+   strbuf_release();
}
 
if (ac == 2 && !opts.new_branch && !opts.detach) {
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  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