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

[Duy Nguyen <pclouds@gmail.com>]

On Sun, Jan 21, 2018 at 7:02 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> Currently 'git worktree add <path>' 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 <t.gummerer@gmail.com>
> ---
>
> 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 <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.

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

[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, &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.

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

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