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

[Thomas Gummerer]

On 03/27, Eric Sunshine wrote:
> On Sun, Mar 25, 2018 at 9:49 AM, 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.
> > [...]
> > Signed-off-by: Thomas Gummerer 
> > ---
> > 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  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.

Hmm I do think that it's important to have the branch we create the
new working tree from different from the branch of the main working
tree, to make sure we don't accidentally overwrite an existing branch,
and to show that the new branch is checked out.

Maybe I'm overly paranoid though?

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

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

[Eric Sunshine]

On Sun, Mar 25, 2018 at 9:49 AM, 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.
> [...]
> Signed-off-by: Thomas Gummerer 
> ---
> 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  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
>  '