Re: [PATCH] worktree: teach "add" to check out existing branches
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Jan 22, 2018, 11:18 UTC
- Message-ID
- <CACsJy8D9LS7e=cVE3Fq2qOnxK5++nFg2vjuhkNtRO-Bx0X1j6w@mail.gmail.com>
- In-Reply-To
- <20180121120208.12760-1-t.gummerer@gmail.com>
On Sun, Jan 21, 2018 at 7:02 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
Show 47 quoted lines
> 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
Show 20 quoted lines
>
> 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.
Show 46 quoted lines
> + 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