[PATCH v7 0/4] worktree: teach "add" to check out existing branches

[Thomas Gummerer <t.gummerer@gmail.com>]

Thanks Eric for the review and all of the suggestions in the last
round.

Previous rounds are at <20180121120208.12760-1-t.gummerer@gmail.com>,
<20180204221305.28300-1-t.gummerer@gmail.com>,
<20180317220830.30963-1-t.gummerer@gmail.com>,
<20180317222219.4940-1-t.gummerer@gmail.com>,
<20180325134947.25828-1-t.gummerer@gmail.com> and
<20180331151804.30380-1-t.gummerer@gmail.com>.

The main change once again in this series is the user interface.  It
feels like we went in a complete circle here now, as this iteration is
bringing the "Preparing ..." line back (although in a slightly
different form than the original), and is moving away from printing
it's own "HEAD is now at..." line again.  This also means we don't
need the new hidden option to 'git reset' anymore, which is nice.

I do like the new UI more than what we had in the last round (which I
already liked more than the original UI) :)

Other than those changes, it also includes Eric's suggestion for a
better wording in the documentation, fixes the argument order to
test_cmd_rev in a test, and makes a test more robust.

To demonstrate the UI updates, let's compare the new UI and the old UI
again.  This is the same commands as used for the demonstration in the
last iteration, so please have a look at <20180331151804.30380-1-t.gummerer@gmail.com> 
for an example of what it looked like after the last round.

New UI:

 - guess-remote mode

    $ git worktree add --guess-remote ../next
    Preparing worktree (new branch 'next')
    Branch 'next' set up to track remote branch 'next' from 'origin'.
    HEAD is now at caa68db14 Merge branch 'sb/packfiles-in-repository' into next

 - original dwim (create a branch based on the current HEAD)

    $ git worktree add ../test
    Preparing worktree (new branch 'test')
    HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'

 - new dwim (check out existing branch)

    $ git worktree add ../test
    Preparing worktree (checking out 'test')
    HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'

 - detached HEAD

    $ git worktree add ../test2 origin/master
    Preparing worktree (detached HEAD c2a499e6c)
    HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'

 - resetting existing branch

    $ git worktree add -B next ../test3 origin/master
    Preparing worktree (resetting branch 'next' (was at caa68db14))
    Branch 'next' set up to track remote branch 'master' from 'origin'.
    HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'

    This output is new in this round and wasn't previously discussed.
    While working on the "Preparing ..." line I noticed this, and
    thought it would be a good idea.  I feel like this fits in the
    scope of the series quite well, as it's improving the UI, but I'm
    happy to split it out if that's preferred.

    It may also be worth displaying the title of the commit here, but
    at that point the line would get a bit long, so dunno.

 - large repository (with original dwim)

    $ git worktree add ../test
    Preparing worktree (new branch 'test')
    Checking out files: 100% (62915/62915), done.
    HEAD is now at c2a9838452a4 Merge tag 'for-4.16/dm-fixes-4' of git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm

 - error when directory already exists

    $ git worktree add ../test
    Preparing worktree (checking out 'test')
    fatal: '../test' already exists

Compare this to the old UI (new dwim omitted, as there's no old
version of that):

 - guess-remote mode

    $ git worktree add --guess-remote ../next
    Branch 'next' set up to track remote branch 'next' from 'origin'.
    Preparing ../next (identifier next)
    HEAD is now at caa68db14 Merge branch 'sb/packfiles-in-repository' into next

 - original dwim (create a branch based on the current HEAD)

    $ git worktree add ../test
    Preparing ../test (identifier test)
    HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'

 - no new branch created

    $ git worktree add ../test2 origin/master
    Preparing ../test2 (identifier test2)
    HEAD is now at c2a499e6c Merge branch 'jh/partial-clone'

 - large repository

    $ git worktree add ../test
    Preparing ../test (identifier test)
    Checking out files: 100% (62915/62915), done.
    HEAD is now at c2a9838452a4 Merge tag 'for-4.16/dm-fixes-4' of git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm

 - error when directory already exists

    $ git worktree add ../test
    fatal: '../test' already exists

Interdiff below:

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index eaa6bf713f..5d54f36a71 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -61,13 +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 worktree with a branch named after
-`$(basename <path>)` (call it `<branch>`) is created.  If `<branch>`
+then, as a convenience, the new worktree is associated with a branch
+(call it `<branch>`) named after `$(basename <path>)`.  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 (unless `--force` is used).
+if `-b <branch>` was given.  If `<branch>` does exist, 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 (unless
+`--force` is used).
 
 list::
 
diff --git a/builtin/reset.c b/builtin/reset.c
index 54b2576449..e15f595799 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -288,7 +288,6 @@ static int git_reset_config(const char *var, const char *value, void *cb)
 int cmd_reset(int argc, const char **argv, const char *prefix)
 {
 	int reset_type = NONE, update_ref_status = 0, quiet = 0;
-	int show_new_head_line = 1;
 	int patch_mode = 0, unborn;
 	const char *rev;
 	struct object_id oid;
@@ -311,7 +310,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		OPT_BOOL('p', "patch", &patch_mode, N_("select hunks interactively")),
 		OPT_BOOL('N', "intent-to-add", &intent_to_add,
 				N_("record only the fact that removed paths will be added later")),
-		OPT_HIDDEN_BOOL(0, "show-new-head-line", &show_new_head_line, N_("show information on the new HEAD")),
 		OPT_END()
 	};
 
@@ -405,8 +403,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		 * switched to, saving the previous head in ORIG_HEAD before. */
 		update_ref_status = reset_refs(rev, &oid);
 
-		if (reset_type == HARD && !update_ref_status && !quiet &&
-		    show_new_head_line)
+		if (reset_type == HARD && !update_ref_status && !quiet)
 			print_new_head_line(lookup_commit_reference(&oid));
 	}
 	if (!pathspec.nr)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index ccc2e63e0f..f5a5283b39 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -27,7 +27,6 @@ struct add_opts {
 	int detach;
 	int checkout;
 	int keep_locked;
-	int checkout_existing_branch;
 };
 
 static int show_only;
@@ -317,28 +316,16 @@ static int add_worktree(const char *path, const char *refname,
 	if (ret)
 		goto done;
 
-	if (opts->checkout_existing_branch)
-		  fprintf_ln(stderr, _("Checking out branch '%s'"), refname);
 	if (opts->checkout) {
 		cp.argv = NULL;
 		argv_array_clear(&cp.args);
 		argv_array_pushl(&cp.args, "reset", "--hard", NULL);
-		argv_array_push(&cp.args, "--no-show-new-head-line");
 		cp.env = child_env.argv;
 		ret = run_command(&cp);
 		if (ret)
 			goto done;
 	}
 
-	fprintf(stderr, _("New worktree HEAD is now at %s"),
-		find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
-
-	strbuf_reset(&sb);
-	pp_commit_easy(CMIT_FMT_ONELINE, commit, &sb);
-	if (sb.len > 0)
-		fprintf(stderr, " %s", sb.buf);
-	fputc('\n', stderr);
-
 	is_junk = 0;
 	FREE_AND_NULL(junk_work_tree);
 	FREE_AND_NULL(junk_git_dir);
@@ -366,6 +353,34 @@ static int add_worktree(const char *path, const char *refname,
 	return ret;
 }
 
+static void print_preparing_worktree_line(const char *branch,
+					  const char *new_branch,
+					  const char *new_branch_force,
+					  int checkout_existing_branch)
+{
+	if (checkout_existing_branch) {
+		printf_ln(_("Preparing worktree (checking out '%s')"), branch);
+	} else if (new_branch_force) {
+		struct commit *commit = lookup_commit_reference_by_name(new_branch_force);
+		if (!commit)
+			printf_ln(_("Preparing worktree (new branch '%s')"), new_branch_force);
+		else
+			printf_ln(_("Preparing worktree (resetting branch '%s' (was at %s))"),
+				  new_branch_force,
+				  find_unique_abbrev(commit->object.oid.hash,
+						     DEFAULT_ABBREV));
+	} else if (new_branch) {
+		printf_ln(_("Preparing worktree (new branch '%s')"), new_branch);
+	} else {
+		struct commit *commit = lookup_commit_reference_by_name(branch);
+		if (!commit)
+			die(_("invalid reference: %s"), branch);
+		printf_ln(_("Preparing worktree (detached HEAD %s)"),
+			  find_unique_abbrev(commit->object.oid.hash,
+					     DEFAULT_ABBREV));
+	}
+}
+
 static const char *dwim_branch(const char *path, const char **new_branch,
 			       int *checkout_existing_branch)
 {
@@ -397,6 +412,7 @@ static int add(int ac, const char **av, const char *prefix)
 	struct add_opts opts;
 	const char *new_branch_force = NULL;
 	char *path;
+	int checkout_existing_branch = 0;
 	const char *branch;
 	const char *new_branch = NULL;
 	const char *opt_track = NULL;
@@ -445,7 +461,7 @@ static int add(int ac, const char **av, const char *prefix)
 
 	if (ac < 2 && !new_branch && !opts.detach) {
 		const char *s = dwim_branch(path, &new_branch,
-					    &opts.checkout_existing_branch);
+					    &checkout_existing_branch);
 		if (s)
 			branch = s;
 	}
@@ -465,10 +481,11 @@ static int add(int ac, const char **av, const char *prefix)
 		}
 	}
 
+	print_preparing_worktree_line(branch, new_branch, new_branch_force,
+				      checkout_existing_branch);
+
 	if (new_branch) {
 		struct child_process cp = CHILD_PROCESS_INIT;
-
-		fprintf_ln(stderr, _("Creating branch '%s'"), new_branch);
 		cp.git_cmd = 1;
 		argv_array_push(&cp.args, "branch");
 		if (new_branch_force)
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index f72cb0eb0b..ad38507d00 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -204,7 +204,7 @@ test_expect_success '"add" checks out existing branch of dwimd name' '
 	test_cmp_rev HEAD~1 dwim &&
 	(
 		cd dwim &&
-		test_cmp_rev dwim HEAD
+		test_cmp_rev HEAD dwim
 	)
 '
 
@@ -215,6 +215,7 @@ test_expect_success '"add <path>" dwim fails with checked out branch' '
 '
 
 test_expect_success '"add --force" with existing dwimd name doesnt die' '
+	git checkout test-branch &&
 	git worktree add --force test-branch
 '
 
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index a160f78aba..95653a08ca 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -568,9 +568,4 @@ test_expect_success 'reset --mixed sets up work tree' '
 	test_cmp expect actual
 '
 
-test_expect_success 'reset --no-show-new-head-line suppresses "HEAD is now at" output' '
-	git reset --hard --no-show-new-head-line HEAD >actual &&
-	! grep "HEAD is now at" <actual
-'
-
 test_done

Thomas Gummerer (4):
  worktree: remove extra members from struct add_opts
  worktree: improve message when creating a new worktree
  worktree: factor out dwim_branch function
  worktree: teach "add" to check out existing branches

 Documentation/git-worktree.txt |   9 +++-
 builtin/worktree.c             | 101 ++++++++++++++++++++++++++++++-----------
 t/t2025-worktree-add.sh        |  26 ++++++++---
 3 files changed, 100 insertions(+), 36 deletions(-)

-- 
2.16.1.74.g6cd9b6cbe3