Re: [PATCH v5 0/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:
> > Thanks Eric for the review of the previous round and Duy and Junio for
> > additional comments.
> > This round should address all of Eric's comments from the previous round.
> 
> Thanks, it appears to cover my review comments from the previous
> round. I do have some additional comments on this round (which I could
> have raised with the previous round if I had thought of them at the
> time).
> 
> > As explained in more detail in a reply to the review comment directly,
> > I did not add an enum to 'struct add_opts', for 'force_new_branch' and
> > 'checkout_existing_branch', but instead removed 'force_new_branch'
> > from the struct as it's not required.
> 
> Makes sense. In fact, I had thoughts along these lines during your
> previous dwim-ery series. See my comments on patch 3/6.
> 
> > The rest of the updates are mainly in the user facing messages,
> > documentation and one added test.
> > Interdiff below:
> 
> The interdiff looks sane. Unfortunately, due to UI regressions, I'm
> having second thoughts about whether this series is going in the right
> direction. See my comments on patch 2/6.

Thanks for your reviews of this series!  As I mentioned in the reply
there I'm going to see whether or not I can fix those regressions
(hopefully I can :)), and send a re-roll.

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

[Eric Sunshine]

On Sun, Mar 25, 2018 at 9:49 AM, Thomas Gummerer  wrote:
> Thanks Eric for the review of the previous round and Duy and Junio for
> additional comments.
> This round should address all of Eric's comments from the previous round.

Thanks, it appears to cover my review comments from the previous
round. I do have some additional comments on this round (which I could
have raised with the previous round if I had thought of them at the
time).

> As explained in more detail in a reply to the review comment directly,
> I did not add an enum to 'struct add_opts', for 'force_new_branch' and
> 'checkout_existing_branch', but instead removed 'force_new_branch'
> from the struct as it's not required.

Makes sense. In fact, I had thoughts along these lines during your
previous dwim-ery series. See my comments on patch 3/6.

> The rest of the updates are mainly in the user facing messages,
> documentation and one added test.
> Interdiff below:

The interdiff looks sane. Unfortunately, due to UI regressions, I'm
having second thoughts about whether this series is going in the right
direction. See my comments on patch 2/6.

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

[Thomas Gummerer]

Thanks Eric for the review of the previous round and Duy and Junio for
additional comments.

Previous rounds are at <20180121120208.12760-1-t.gumme...@gmail.com>,
<20180204221305.28300-1-t.gumme...@gmail.com>,
<20180317220830.30963-1-t.gumme...@gmail.com> and
<2018031719.4940-1-t.gumme...@gmail.com>.

This round should address all of Eric's comments from the previous round.

As explained in more detail in a reply to the review comment directly,
I did not add an enum to 'struct add_opts', for 'force_new_branch' and
'checkout_existing_branch', but instead removed 'force_new_branch'
from the struct as it's not required.

The rest of the updates are mainly in the user facing messages,
documentation and one added test.

Interdiff below:

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 98731b71a7..eaa6bf713f 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -67,7 +67,7 @@ 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.
+worktree (unless `--force` is used).
 
 list::
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index df5c0427ba..895838b943 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -28,7 +28,6 @@ struct add_opts {
int checkout;
int keep_locked;
const char *new_branch;
-   int force_new_branch;
int checkout_existing_branch;
 };
 
@@ -320,12 +319,12 @@ static int add_worktree(const char *path, const char 
*refname,
goto done;
 
if (opts->checkout_existing_branch)
-   fprintf(stderr, _("checking out branch '%s'"),
-   refname);
+   fprintf_ln(stderr, _("checking out branch '%s'"),
+  refname);
else if (opts->new_branch)
-   fprintf(stderr, _("creating branch '%s'"), opts->new_branch);
+   fprintf_ln(stderr, _("creating branch '%s'"), opts->new_branch);
 
-   fprintf(stderr, _("worktree HEAD is now at %s"),
+   fprintf(stderr, _("new worktree HEAD is now at %s"),
find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
 
strbuf_reset();
@@ -434,8 +433,7 @@ static int add(int ac, const char **av, const char *prefix)
if (!strcmp(branch, "-"))
branch = "@{-1}";
 
-   opts.force_new_branch = !!new_branch_force;
-   if (opts.force_new_branch) {
+   if (new_branch_force) {
struct strbuf symref = STRBUF_INIT;
 
opts.new_branch = new_branch_force;
@@ -472,7 +470,7 @@ static int add(int ac, const char **av, const char *prefix)
struct child_process cp = CHILD_PROCESS_INIT;
cp.git_cmd = 1;
argv_array_push(, "branch");
-   if (opts.force_new_branch)
+   if (new_branch_force)
argv_array_push(, "--force");
argv_array_push(, opts.new_branch);
argv_array_push(, branch);
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 721b0e4c26..fb99f4c46f 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -198,15 +198,15 @@ test_expect_success '"add" with  omitted' '
test_cmp_rev HEAD bat
 '
 
-test_expect_success '"add" auto-vivify checks out existing branch' '
+test_expect_success '"add" checks out existing branch of dwimd name' '
test_commit c1 &&
test_commit c2 &&
-   git branch precious HEAD~1 &&
-   git worktree add precious &&
-   test_cmp_rev HEAD~1 precious &&
+   git branch dwim HEAD~1 &&
+   git worktree add dwim &&
+   test_cmp_rev HEAD~1 dwim &&
(
-   cd precious &&
-   test_cmp_rev precious HEAD
+   cd dwim &&
+   test_cmp_rev dwim HEAD
)
 '
 
@@ -216,6 +216,10 @@ test_expect_success '"add" auto-vivify fails with checked 
out branch' '
test_path_is_missing test-branch
 '
 
+test_expect_success '"add --force" with existing dwimd name doesnt die' '
+   git worktree add --force test-branch
+'
+
 test_expect_success '"add" no auto-vivify with --detach and  omitted' '
git worktree add --detach mish/mash &&
test_must_fail git rev-parse mash -- &&

Thomas Gummerer (6):
  worktree: improve message when creating a new worktree
  worktree: be clearer when "add" dwim-ery kicks in
  worktree: remove force_new_branch from struct add_opts
  worktree: factor out dwim_branch function
  worktree: teach "add" to check out existing branches
  t2025: rename now outdated branch name

 Documentation/git-worktree.txt |  9 --
 builtin/worktree.c | 64 +++---
 t/t2025-worktree-add.sh| 23 +++
 3 files changed, 72