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

[Thomas Gummerer]

On 01/22, Duy Nguyen wrote:
> On Sun, Jan 21, 2018 at 7:02 PM, Thomas Gummerer  wrote:
> > [...]
> >  +
> >  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.
> 
> 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 ) 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
> 
> >
> >  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, );
> > -   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;
> 
> Perhaps a blank line after this to separate var declarations and the rest.

Will add. 

> > +   if (!strbuf_check_branch_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.

> > +   opts.checkout = 1;
> > +   } 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;
> > +   }
> > }
> > }
> >
> > 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 &&
> > +   test_must_fail git worktree add test-branch &&
> > +   test_path_is_missing test-branch
> >  '
> >
> 

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

[Duy Nguyen]

On Sun, Jan 21, 2018 at 7:02 PM, Thomas Gummerer  wrote:
> 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 brach 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 
> ---
>
> This is a follow-up to
> https://public-inbox.org/git/20171118181345.GC32324@hank/, where this
> was first suggested, but I didn't want to do it as part of that
> series.  Now I finally got around to implementing it.
>
>  Documentation/git-worktree.txt |  9 +++--
>  builtin/worktree.c | 22 +++---
>  t/t2025-worktree-add.sh| 15 ---
>  3 files changed, 34 insertions(+), 12 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.

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.

More or less related to this, there was a question [1] whether we
could do better than $(basename ) 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".

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

>
>  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, );
> -   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;

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

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

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

> +   opts.checkout = 1;
> +   } 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;
> +   }
> }
> }
>
> 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' '
> 

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

[Robert P. J. Day]

On Sun, 21 Jan 2018, Thomas Gummerer wrote:

> 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 brach with the name
   ^

[PATCH] 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 brach 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 
---

This is a follow-up to
https://public-inbox.org/git/20171118181345.GC32324@hank/, where this
was first suggested, but I didn't want to do it as part of that
series.  Now I finally got around to implementing it.

 Documentation/git-worktree.txt |  9 +++--
 builtin/worktree.c | 22 +++---
 t/t2025-worktree-add.sh| 15 ---
 3 files changed, 34 insertions(+), 12 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 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, );
-   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 = 1;
+   } 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;
+   }
}
}
 
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 &&
+   test_must_fail git worktree add test-branch &&
+   test_path_is_missing test-branch
 '
 
 test_expect_success '"add" no auto-vivify with --detach and  omitted' '
-- 
2.16.0.312.g896df04e46