Re: [PATCH v5 5/6] worktree: teach "add" to check out existing branches

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Mar 25, 2018 at 9:49 AM, Thomas Gummerer <t.gummerer@xxxxxxxxx> 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.
> [...]
> Signed-off-by: Thomas Gummerer <t.gummerer@xxxxxxxxx>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -317,7 +318,10 @@ static int add_worktree(const char *path, const char *refname,
> -       if (opts->new_branch)
> +       if (opts->checkout_existing_branch)
> +               fprintf_ln(stderr, _("checking out branch '%s'"),
> +                          refname);

This fprintf_ln() can easily fit on one line; no need to wrap
'refname' to the next one.

Not worth a re-roll, though.

> +       else if (opts->new_branch)
>                 fprintf_ln(stderr, _("creating branch '%s'"), opts->new_branch);
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> @@ -198,13 +198,26 @@ test_expect_success '"add" with <branch> omitted' '
> -test_expect_success '"add" auto-vivify does not clobber existing branch' '
> +test_expect_success '"add" checks out existing branch of dwimd name' '
>         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
> +       )
> +'

Looking at this more closely, once it stops being a "don't clobber
precious branch" test, it's doing more than it needs to do, thus is
confusing for future readers. The setup -- creating two commits and
making "precious" point one commit back -- was very intentional and
allowed the test to verify not only that the worktree wasn't created
but that "precious" didn't change to reference a different commit.
However, _none_ of this matters once it becomes a dwim'ing test; the
unnecessary code is confusing since it doesn't make sense in the
context of dwim'ing. I _think_ the entire test can collapse to:

    git branch existing &&
    git worktree add existing &&
    (
        cd existing &&
        test_cmp_rev existing HEAD
    )

In other words, this patch should drop the "precious" test altogether
and just introduce a new dwim'ing test (and drop patch 6/6).

I do think that with the potential confusion to future readers, this
does deserve a re-roll.

> +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 --force" with existing dwimd name doesnt die' '
> +       git worktree add --force test-branch
>  '



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux