worktree: teach "add" to check out existing branches

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

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 <[email protected]>

---

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(-)

Documentation/git-worktree.txt +0 -0

index 41585f535d..98731b71a7 100644
@@ -61,8 +61,13 @@ $ git worktree add --track -b <branch> <path> <remote>/<branch>
 ------------
 +

Documentation/git-worktree.txt +0 -0

 list::
 

builtin/worktree.c +0 -0

index 7cef5b120b..148a864bb9 100644
@@ -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;

builtin/worktree.c +10 -0

+                       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;
+                       }
                }
        }
 

t/t2025-worktree-add.sh +12 -4

index 2b95944973..721b0e4c26 100755
@@ -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

Quoted 10 lines · Thomas Gummerer

[...]
+
If <commit-ish> is omitted and neither -b nor -B nor --detach used,

Documentation/git-worktree.txt +5 -2

-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

Documentation/git-worktree.txt $ git worktree add --track -b <branch> <path> <remote>/<branch> · line 70

Quoted 2 lines

Documentation/git-worktree.txt +2 -0

+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

Quoted 14 lines

list::

builtin/worktree.c +0 -3

index 7cef5b120b..148a864bb9 100644
@@ -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;

builtin/worktree.c static int add(int ac, const char **av, const char *prefix) · line 415

Quoted 6 lines

builtin/worktree.c +2 -4

-                       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.

builtin/worktree.c static int add(int ac, const char **av, const char *prefix) · line 418

Quoted 3 lines

builtin/worktree.c +3 -0

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

Quoted 46 lines

builtin/worktree.c +10 -0

+                       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;
+                       }
                }
        }
 

t/t2025-worktree-add.sh +12 -4

index 2b95944973..721b0e4c26 100755
@@ -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

Signature · 1 line

Duy

builtin/worktree.c +1 -1

index 7cef5b120b..d1549e441d 100644
@@ -303,7 +303,7 @@ static int add_worktree(const char *path, const char *refname,
 	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
 	write_file(sb.buf, "../..");
 
-	fprintf_ln(stderr, _("Preparing %s (identifier %s)"), path, name);
+	fprintf(stderr, _("Preparing %s (identifier %s)"), path, name);
 
 	argv_array_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
 	argv_array_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);

builtin/worktree.c +3 -1

+	fputc('\n', stderr);
+
 	if (opts->checkout) {
 		cp.argv = NULL;
 		argv_array_clear(&cp.args);
-		argv_array_pushl(&cp.args, "reset", "--hard", NULL);
+		argv_array_pushl(&cp.args, "reset", "--hard", "--quiet", NULL);
 		cp.env = child_env.argv;
 		ret = run_command(&cp);
 		if (ret)

Signature · 1 line

2.16.1.101.gde0f0111ea

Quoted 17 lines · Thomas Gummerer

builtin/worktree.c +1 -1

index 7cef5b120b..d1549e441d 100644
@@ -303,7 +303,7 @@ static int add_worktree(const char *path, const char *refname,
 	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
 	write_file(sb.buf, "../..");
 
-	fprintf_ln(stderr, _("Preparing %s (identifier %s)"), path, name);
+	fprintf(stderr, _("Preparing %s (identifier %s)"), path, name);
 
 	argv_array_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
 	argv_array_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);

@@ -320,10 +320,19 @@ static int add_worktree(const char *path, const char *refname,
 	if (ret)
 		goto done;
 

builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 323

Quoted 1 line

builtin/worktree.c +1 -0

+	fprintf(stderr, _(", setting HEAD to %s"),

As a former translator, I'm not thrilled to see a sentence broken into two pieces like this. I'm not a Japanese translator, but I think this sentence is translated differently when the translator sees the whole line "Preparing ..., setting ...".

Since the code between the first fprintf and this one should not take long to execute, perhaps we can just delete the first printf and print a whole [*] sentence here?

I think the purpose of "Preparing..." in the first place is to show something when git is busy checkout out the worktree. As long as we print it before git-reset, we should be good.

builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 329

Quoted 6 lines

builtin/worktree.c +6 -0

+		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);

[*] Yes I know it's not "whole" because of this piece. But this one is more or less a separate sentence already so it's probably ok.

Is it a bit too long to print everything in one line though? CMIT_FMT_ONELINE could already fill 70 columns easily.

Quoted 13 lines

builtin/worktree.c +3 -1

+	fputc('\n', stderr);
+
 	if (opts->checkout) {
 		cp.argv = NULL;
 		argv_array_clear(&cp.args);
-		argv_array_pushl(&cp.args, "reset", "--hard", NULL);
+		argv_array_pushl(&cp.args, "reset", "--hard", "--quiet", NULL);
 		cp.env = child_env.argv;
 		ret = run_command(&cp);
 		if (ret)

Signature · 1 line

2.16.1.101.gde0f0111ea

Quoted 14 lines · Thomas Gummerer

builtin/worktree.c +1 -1

index 7cef5b120b..d1549e441d 100644
@@ -303,7 +303,7 @@ static int add_worktree(const char *path, const char *refname,
 	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
 	write_file(sb.buf, "../..");
 
-	fprintf_ln(stderr, _("Preparing %s (identifier %s)"), path, name);
+	fprintf(stderr, _("Preparing %s (identifier %s)"), path, name);
 
 	argv_array_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
 	argv_array_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);

builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 323

Quoted 4 lines

builtin/worktree.c +1 -0

@@ -320,10 +320,19 @@ static int add_worktree(const char *path, const char *refname,
 	if (ret)
 		goto done;
 
+	fprintf(stderr, _(", setting HEAD to %s"),

As a former translator, I'm not thrilled to see a sentence broken into two pieces like this. I'm not a Japanese translator, but I think this sentence is translated differently when the translator sees the whole line "Preparing ..., setting ...".

Since the code between the first fprintf and this one should not take long to execute, perhaps we can just delete the first printf and print a whole [*] sentence here?

I think the purpose of "Preparing..." in the first place is to show something when git is busy checkout out the worktree. As long as we print it before git-reset, we should be good.

builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 329

Quoted 6 lines

builtin/worktree.c +6 -0

+		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);

[*] Yes I know it's not "whole" because of this piece. But this one is more or less a separate sentence already so it's probably ok.

Is it a bit too long to print everything in one line though? CMIT_FMT_ONELINE could already fill 70 columns easily.

Quoted 13 lines

builtin/worktree.c +3 -1

+	fputc('\n', stderr);
+
 	if (opts->checkout) {
 		cp.argv = NULL;
 		argv_array_clear(&cp.args);
-		argv_array_pushl(&cp.args, "reset", "--hard", NULL);
+		argv_array_pushl(&cp.args, "reset", "--hard", "--quiet", NULL);
 		cp.env = child_env.argv;
 		ret = run_command(&cp);
 		if (ret)

Signature · 1 line

2.16.1.101.gde0f0111ea

As a former translator, I'm not thrilled to see a sentence broken into two pieces like this. I'm not a Japanese translator, but I think this sentence is translated differently when the translator sees the whole line "Preparing ..., setting ...".

I think the purpose of "Preparing..." in the first place is to show something when git is busy checkout out the worktree. As long as we print it before git-reset, we should be good.

Quoted 8 lines · Duy Nguyen

As a former translator, I'm not thrilled to see a sentence broken into two pieces like this. I'm not a Japanese translator, but I think this sentence is translated differently when the translator sees the whole line "Preparing ..., setting ...".

I think the purpose of "Preparing..." in the first place is to show something when git is busy checkout out the worktree. As long as we print it before git-reset, we should be good.

The original message was "Enter <worktree>" which had the potential to confuse someone into thinking the working directory had changed[1], so it was changed to "Preparing...". The reason for keeping that message (rather than dropping it outright) was to provide context to messages printed after it, especially messages such as "HEAD is now at..." which might otherwise confuse the reader into thinking that HEAD in the current worktree changed rather than HEAD in the new worktree[2,3].

[1]: https://public-inbox.org/git/[email protected]/
[2]: https://public-inbox.org/git/CAPig+cRSHwmmF9cCUbRrDCCW4kvg9PeOUxP5VQpSGfxzMxHWOQ@mail.gmail.com/
[3]: https://public-inbox.org/git/CAPig+cSLs4-uKicVMBSKnEro_FYD722Hs1_U6qzTriM8Ciok2Q@mail.gmail.com/

This would loose the information about the identifier of the worktree, but from a coarse look at the man page it doesn't seem like we advertise that widely

...

So given that maybe it would even be better to hide the part about the identifier, as it seems more like an implementation detail than relevant to the end user?

Quoted 6 lines · Thomas Gummerer

This would loose the information about the identifier of the worktree, but from a coarse look at the man page it doesn't seem like we advertise that widely

...

Quoted 3 lines

So given that maybe it would even be better to hide the part about the identifier, as it seems more like an implementation detail than relevant to the end user?

Exactly. I'd rather hide it. I haven't found any good reason that a user needs to know these IDs unless they poke into $GIT_DIR/worktrees, but they should not need to.

builtin/worktree.c +4 -7

@@ -423,14 +427,25 @@ 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;
+
+		if (!strbuf_check_branch_ref(&ref, branchname) &&

builtin/worktree.c +4 -0

+				if (remote)
+					branch = remote;
+			}
 		}
+		strbuf_release(&ref);
 	}
 
 	if (ac == 2 && !opts.new_branch && !opts.detach) {

builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 328

Quoted 6 lines · Thomas Gummerer

builtin/worktree.c +4 -1

-	if (opts->new_branch)
+	if (opts->checkout_existing_branch)
+		fprintf(stderr, _(", checking out existing branch '%s'"),
+			refname);
+	else if (opts->new_branch)
 		fprintf(stderr, _(", creating new branch '%s'"), opts->new_branch);

I wonder if "creating branch" and "checkout out branch" are enough.

builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 328

Quoted 6 lines · Thomas Gummerer

builtin/worktree.c +4 -1

-	if (opts->new_branch)
+	if (opts->checkout_existing_branch)
+		fprintf(stderr, _(", checking out existing branch '%s'"),
+			refname);
+	else if (opts->new_branch)
 		fprintf(stderr, _(", creating new branch '%s'"), opts->new_branch);

I wonder if "creating branch" and "checkout out branch" are enough.

Quoted 20 lines

builtin/worktree.c +9 -7

@@ -423,14 +427,25 @@ 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;
+
+		if (!strbuf_check_branch_ref(&ref, branchname) &&
+		    ref_exists(ref.buf)) {
+			branch = branchname;
+			opts.checkout_existing_branch = 1;
+			UNLEAK(branch);
+		} else {

builtin/worktree.c static int add(int ac, const char **av, const char *prefix) · line 443

Quoted 5 lines

builtin/worktree.c +5 -0

+			opts.new_branch = branchname;
+			if (guess_remote) {
+				struct object_id oid;
+				const char *remote =
+					unique_tracking_name(opts.new_branch, &oid);

Deep indentation may be a sign that it's time to move all this code to a separate function, maybe dwim_branch() or something.

Quoted 8 lines

builtin/worktree.c +4 -0

+				if (remote)
+					branch = remote;
+			}
 		}
+		strbuf_release(&ref);
 	}
 
 	if (ac == 2 && !opts.new_branch && !opts.detach) {

Quoted 8 lines · Duy Nguyen

Quoted 6 lines · Thomas Gummerer

builtin/worktree.c +4 -1

-   if (opts->new_branch)
+   if (opts->checkout_existing_branch)
+           fprintf(stderr, _(", checking out existing branch '%s'"),
+                   refname);
+   else if (opts->new_branch)
            fprintf(stderr, _(", creating new branch '%s'"), opts->new_branch);

Quoted 1 line

I wonder if "creating branch" and "checkout out branch" are enough.

I thought printing the branch name might be a good idea just to show more clearly what the dwim did.

Quoted 11 lines · Thomas Gummerer

Quoted 9 lines · Duy Nguyen

builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 328

Quoted 6 lines · Thomas Gummerer

builtin/worktree.c +4 -1

-   if (opts->new_branch)
+   if (opts->checkout_existing_branch)
+           fprintf(stderr, _(", checking out existing branch '%s'"),
+                   refname);
+   else if (opts->new_branch)
            fprintf(stderr, _(", creating new branch '%s'"), opts->new_branch);

I wonder if "creating branch" and "checkout out branch" are enough.

Quoted 2 lines

I thought printing the branch name might be a good idea just to show more clearly what the dwim did.

No no printing branch name is definitely a good idea, especially when I think of one thing and type another. I shortened my example phrases too much. It should have been "creating branch %s" and "checkout out branch %s"

Signature · 1 line

Duy

Currently 'git worktree add' produces output like the following, when '--no-checkout' is not given:

Preparing foo (identifier foo)
HEAD is now at 26da330922 <title>

where the first line is written to stderr, and the second line coming from 'git reset --hard' is written to stdout, even though both lines are supposed to tell the user what has happened. In addition to someone not familiar with 'git worktree', this might seem as if the current HEAD was modified, not the HEAD in the new working tree.

If the '--no-checkout' flag is given, the output of 'git worktree add' is just:

Preparing foo (identifier foo)

even though the HEAD is set to a commit, which is just not checked out.

The identifier is also not particularly relevant for the user at the moment, as it's only used internally to distinguish between different worktrees that have the same $(basename <path>).

Fix these inconsistencies, and no longer show the identifier by making the 'git reset --hard' call quiet, and printing the message directly from the builtin command instead.

Signed-off-by Thomas Gummerer <[email protected]>

---
 builtin/worktree.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

builtin/worktree.c +0 -2

index 7cef5b120b..e5d04f0b4b 100644
@@ -303,8 +303,6 @@ static int add_worktree(const char *path, const char *refname,
        strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
        write_file(sb.buf, "../..");
 
-       fprintf_ln(stderr, _("Preparing %s (identifier %s)"), path, name);
-
        argv_array_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
        argv_array_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
        cp.git_cmd = 1;

builtin/worktree.c +9 -2

+               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);
+
        if (opts->checkout) {
                cp.argv = NULL;
                argv_array_clear(&cp.args);
-               argv_array_pushl(&cp.args, "reset", "--hard", NULL);
+               argv_array_pushl(&cp.args, "reset", "--hard", "--quiet", NULL);
                cp.env = child_env.argv;
                ret = run_command(&cp);
                if (ret)
--

2.17.0.rc0.231.g781580f06

builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 321

Quoted 5 lines

builtin/worktree.c +1 -0

@@ -320,10 +318,19 @@ static int add_worktree(const char *path, const char *refname,
        if (ret)
                goto done;
 
+       fprintf(stderr, _("worktree HEAD is now at %s"),

We use the term "working tree" for UI and documents. "worktree" is only used in code comments and stuff.

builtin/worktree.c +5 -0

+       strbuf_reset(&sb);
+       pp_commit_easy(CMIT_FMT_ONELINE, commit, &sb);
+       if (sb.len > 0)
+               fprintf(stderr, " %s", sb.buf);
+       fputc('\n', stderr);

Quoted 12 lines · Thomas Gummerer

[...]
Fix these inconsistencies, and no longer show the identifier by making the 'git reset --hard' call quiet, and printing the message directly from the builtin command instead.

Signed-off-by Thomas Gummerer <[email protected]>

---

builtin/worktree.c +0 -0

@@ -303,8 +303,6 @@ static int add_worktree(const char *path, const char *refname,
        strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
        write_file(sb.buf, "../..");
 

builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 306

Quoted 1 line

builtin/worktree.c +0 -1

-       fprintf_ln(stderr, _("Preparing %s (identifier %s)"), path, name);

A minor regression with this change is that error messages from git-update-ref or git-symbolic-ref -- which could be emitted after this point but before the new "worktree HEAD is now at..." message -- are now somewhat orphaned. I'm not sure that it matters, though.

builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 319

Quoted 6 lines

builtin/worktree.c +2 -0

        argv_array_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
        argv_array_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
        cp.git_cmd = 1;

@@ -320,10 +318,19 @@ static int add_worktree(const char *path, const char *refname,
+       fprintf(stderr, _("worktree HEAD is now at %s"),
+               find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));

I wonder if this should say "new worktree HEAD is now at..." to clearly indicate that it is talking about HEAD in the _new_ worktree, not HEAD in the current worktree.

Quoted 5 lines

builtin/worktree.c +5 -0

+       strbuf_reset(&sb);
+       pp_commit_easy(CMIT_FMT_ONELINE, commit, &sb);
+       if (sb.len > 0)
+               fprintf(stderr, " %s", sb.buf);
+       fputc('\n', stderr);

Currently there is no indication in the "git worktree add" output that a new branch was created. This would be especially useful information in the case where the dwim of "git worktree add <path>" kicks in, as the user didn't explicitly ask for a new branch, but we create one from them.

Print some additional output showing that a branch was created and the branch name to help the user.

This will also be useful in the next commit, which introduces a new kind of dwim-ery of checking out the branch if it exists instead of refusing to create a new worktree in that case, and where it's nice to tell the user which kind of dwim-ery kicked in.

Signed-off-by Thomas Gummerer <[email protected]>

Currently there is no indication in the "git worktree add" output that a new branch was created. This would be especially useful information in the case where the dwim of "git worktree add <path>" kicks in, as the user didn't explicitly ask for a new branch, but we create one from them.

Print some additional output showing that a branch was created and the branch name to help the user.

[...]

Signed-off-by Thomas Gummerer <[email protected]>

---

Quoted 13 lines · Thomas Gummerer

Currently there is no indication in the "git worktree add" output that a new branch was created. This would be especially useful information in the case where the dwim of "git worktree add <path>" kicks in, as the user didn't explicitly ask for a new branch, but we create one from them.

Print some additional output showing that a branch was created and the branch name to help the user.

[...]

Signed-off-by Thomas Gummerer <[email protected]>

---

builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 322

Quoted 5 lines

builtin/worktree.c +3 -0

@@ -318,6 +318,9 @@ static int add_worktree(const char *path, const char *refname,
+       if (opts->new_branch)
+               fprintf(stderr, _("creating new branch '%s'"), opts->new_branch);
+
        fprintf(stderr, _("worktree HEAD is now at %s"),
                find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));

The "creating" message is missing a newline, which results in rather ugly output:

creating new branch 'foo'worktree HEAD is now at ...

Quoted 23 lines · Eric Sunshine

Quoted 13 lines · Thomas Gummerer

Currently there is no indication in the "git worktree add" output that a new branch was created. This would be especially useful information in the case where the dwim of "git worktree add <path>" kicks in, as the user didn't explicitly ask for a new branch, but we create one from them.

Print some additional output showing that a branch was created and the branch name to help the user.

[...]

Signed-off-by Thomas Gummerer <[email protected]>

---

builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 322

Quoted 5 lines

builtin/worktree.c +3 -0

@@ -318,6 +318,9 @@ static int add_worktree(const char *path, const char *refname,
+       if (opts->new_branch)
+               fprintf(stderr, _("creating new branch '%s'"), opts->new_branch);
+
        fprintf(stderr, _("worktree HEAD is now at %s"),
                find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));

The "creating" message is missing a newline, which results in rather ugly output:

Quoted 1 line

creating new branch 'foo'worktree HEAD is now at ...

Also, I believe that the agreement[1] was that this message should say merely "creating branch", not "creating _new_ branch". And, indeed, patch 4/4 stealthily drops "new" from the message, but it really ought to be introduced with correct text in this patch, not fixed by 4/4.

[1]: https://public-inbox.org/git/[email protected]/

[...]
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.
[...]
We will still 'die()' if the branch is checked out in another worktree, unless the --force flag is passed.

Signed-off-by Thomas Gummerer <[email protected]>

---

builtin/worktree.c +2 -0

+       else if (opts->new_branch)
+               fprintf(stderr, _("creating branch '%s'"), opts->new_branch);
 
        fprintf(stderr, _("worktree HEAD is now at %s"),
                find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));

t/t2025-worktree-add.sh +1 -1

@@ -198,13 +198,22 @@ test_expect_success '"add" with <branch> omitted' '
-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 &&

Quoted 13 lines · Thomas Gummerer

[...]
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.
[...]
We will still 'die()' if the branch is checked out in another worktree, unless the --force flag is passed.

Signed-off-by Thomas Gummerer <[email protected]>

---

Documentation/git-worktree.txt $ git worktree add --track -b <branch> <path> <remote>/<branch> · line 68

Quoted 10 lines

Documentation/git-worktree.txt +7 -2

@@ -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.

Should this mention --force?

... refuse to create the worktree (unless --force is used).

builtin/worktree.c struct add_opts { · line 32

Quoted 6 lines

builtin/worktree.c +1 -0

@@ -29,6 +29,7 @@ struct add_opts {
        int keep_locked;
        const char *new_branch;
        int force_new_branch;
+       int checkout_existing_branch;

As 'force_new_branch' and 'checkout_existing_branch' are mutually exclusive, I wonder if they should be combined into an enum to make it easier to reason about.

builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 321

Quoted 6 lines

builtin/worktree.c +3 -2

@@ -318,8 +319,11 @@ static int add_worktree(const char *path, const char *refname,
-       if (opts->new_branch)
-               fprintf(stderr, _("creating new branch '%s'"), opts->new_branch);
+       if (opts->checkout_existing_branch)
+               fprintf(stderr, _("checking out branch '%s'"),
+                       refname);

As with "creating branch" in 2/4, "checking out branch..." here is missing a newline.

Quoted 11 lines

builtin/worktree.c +2 -0

+       else if (opts->new_branch)
+               fprintf(stderr, _("creating branch '%s'"), opts->new_branch);
 
        fprintf(stderr, _("worktree HEAD is now at %s"),
                find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));

t/t2025-worktree-add.sh +1 -1

@@ -198,13 +198,22 @@ test_expect_success '"add" with <branch> omitted' '
-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 &&

t/t2025-worktree-add.sh test_expect_success '"add" with <branch> omitted' ' · line 208

Quoted 10 lines

t/t2025-worktree-add.sh +6 -2

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

This test is no longer checking auto-vivification ("bringing a new branch to life automatically"), but rather branch name inference, so the title is now misleading. Perhaps retitle it to '"add" checks out existing branch of dwim'd name' or something.

(The name "precious" is also now misleading since it's no longer checking that a precious branch does not get clobbered, however, changing the name would make the diff unnecessarily noisy, so it's probably okay as is.)

t/t2025-worktree-add.sh test_expect_success '"add" with <branch> omitted' ' · line 217

Quoted 5 lines

t/t2025-worktree-add.sh +4 -0

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

Should we also have a corresponding test that this "fail" can be overridden by --force? (I realize that --force is tested elsewhere, but only with an explicitly-provided branch name, whereas this dwims the branch name.)

Currently there is no indication in the "git worktree add" output that a new branch was created. This would be especially useful information in the case where the dwim of "git worktree add <path>" kicks in, as the user didn't explicitly ask for a new branch, but we create one from them.

Print some additional output showing that a branch was created and the branch name to help the user.

This will also be useful in the next commit, which introduces a new kind

builtin/worktree.c +0 -0

        fprintf(stderr, _("new worktree HEAD is now at %s"),
                find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));

Quoted 5 lines · Thomas Gummerer

Currently there is no indication in the "git worktree add" output that a new branch was created. This would be especially useful information in the case where the dwim of "git worktree add <path>" kicks in, as the user didn't explicitly ask for a new branch, but we create one from them.

Failed to notice this last time around: s/from/for/

Perhaps Junio can tweak it when queuing.

Quoted 4 lines

Print some additional output showing that a branch was created and the branch name to help the user.

This will also be useful in the next commit, which introduces a new kind

It's no longer the _next_ commit which does this. Perhaps say instead "a subsequent commit". (Again, perhaps Junio can tweak it since it's not worth a re-roll.)

builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 319

Quoted 10 lines

of dwim-ery of checking out the branch if it exists instead of refusing to create a new worktree in that case, and where it's nice to tell the user which kind of dwim-ery kicked in.

Signed-off-by Thomas Gummerer <[email protected]>

---

builtin/worktree.c +2 -0

@@ -318,6 +318,9 @@ static int add_worktree(const char *path, const char *refname,
+       if (opts->new_branch)
+               fprintf_ln(stderr, _("creating branch '%s'"), opts->new_branch);

I didn't think of this before, but what happens with the original dwim-ery you added which checks out a remote branch matching the worktree name if no such local branch exists? I was wondering if we'd want to see a distinct and more informative message in that case, such as "creating branch '%s' from remote '%s'" or something. However, I see that we're already getting a message:

Branch 'foo' set up to track remote branch 'foo' from 'origin'. creating branch 'foo'
new worktree HEAD is now at d3adb33f boople

which is a bit weird since tracking appears to be set up before the branch itself is created. I thought that this was another UI regression of the sort I mentioned in [1], however, the messages were out of order even before the current patch series:

Branch 'foo' set up to track remote branch 'foo' from 'origin'. Preparing foo (identifier foo)
Checking out files: 100% (9999/9999), done.
HEAD is now at d3adb33f boople

This highlights another regression introduced by this series. Patch 1/6 suppresses the "Checking out files:" message, which is a bit unfortunate for decent sized worktrees. I'm not sure I'm entirely happy about that loss. Thoughts?

[1]: https://public-inbox.org/git/CAPig+cT8i9L9kbhx=b0sG4_QYNdoEDPW-1xypM9xzBqPmqR__Q@mail.gmail.com/

Quoted 2 lines

builtin/worktree.c +0 -0

        fprintf(stderr, _("new worktree HEAD is now at %s"),
                find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));

The 'force_new_branch' flag in 'struct add_opts' is only used inside the add function, where we already have the same information stored in the 'new_branch_force' variable. Avoid that unnecessary duplication.

Signed-off-by Thomas Gummerer <[email protected]>

Quoted 3 lines · Thomas Gummerer

The 'force_new_branch' flag in 'struct add_opts' is only used inside the add function, where we already have the same information stored in the 'new_branch_force' variable. Avoid that unnecessary duplication.

When I was reviewing your original dwim-ery series (inferring 'foo' from 'origin/foo'), I noticed that 'struct add_opts' had accumulated a number of unnecessary members over time, including this one. My plan was to submit a patch removing all those pointless members after your dwim-ery series settled, however, I never got around to it.

This patch might be a good opportunity for doing that cleanup wholesale, removing all unneeded members rather than just the one. (If so, you'd probably want to move to this patch to the front of the series as cleanup before the meatier changes.) Anyhow, it's just a thought; feel free to respond with "it's outside the scope of this series" if you're not interested in doing it.

Quoted 1 line

Signed-off-by Thomas Gummerer <[email protected]>

Factor out a dwim_branch function, which takes care of the dwim'ery in 'git worktree add <path>'. It's not too much code currently, but we're adding a new kind of dwim in a subsequent patch, at which point it makes more sense to have it as a separate function.

[...]

Signed-off-by Thomas Gummerer <[email protected]>

---

builtin/worktree.c +0 -3

@@ -417,16 +431,9 @@ 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);

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 <[email protected]>

---

builtin/worktree.c +1 -0

+       else if (opts->new_branch)
                fprintf_ln(stderr, _("creating branch '%s'"), opts->new_branch);

t/t2025-worktree-add.sh +1 -1

@@ -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 &&

t/t2025-worktree-add.sh +8 -0

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

Quoted 14 lines · Thomas Gummerer

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 <[email protected]>

---

builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 320

Quoted 4 lines

builtin/worktree.c +3 -1

@@ -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.

Quoted 8 lines

builtin/worktree.c +1 -0

+       else if (opts->new_branch)
                fprintf_ln(stderr, _("creating branch '%s'"), opts->new_branch);

t/t2025-worktree-add.sh +1 -1

@@ -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 &&

t/t2025-worktree-add.sh test_expect_success '"add" with <branch> omitted' ' · line 208

Quoted 10 lines

t/t2025-worktree-add.sh +6 -2

        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.

Quoted 9 lines

t/t2025-worktree-add.sh +8 -0

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

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.

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:

Quoted 3 lines · Thomas Gummerer

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).

Quoted 4 lines

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.

Quoted 3 lines

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.

Introduce a new --show-new-head-line command line option, that determines whether the "HEAD is now at ..." message is printed or not. It is enabled by default to preserve the current behaviour.

It will be used in a subsequent commit to disable printing the "HEAD is now at ..." line in 'git worktree add', so it can be replaced with a custom one, that explains the behaviour better.

We cannot just pass the --quite flag from 'git worktree add', as that would also hide progress output when checking out large working trees, which is undesirable.

As this option is only for internal use, which we probably don't want to advertise to our users, at least until there's a need for it, make it a hidden flag.

Quoted 8 lines · Thomas Gummerer

Introduce a new --show-new-head-line command line option, that determines whether the "HEAD is now at ..." message is printed or not. It is enabled by default to preserve the current behaviour.

It will be used in a subsequent commit to disable printing the "HEAD is now at ..." line in 'git worktree add', so it can be replaced with a custom one, that explains the behaviour better.

Quoted 7 lines

We cannot just pass the --quite flag from 'git worktree add', as that would also hide progress output when checking out large working trees, which is undesirable.

As this option is only for internal use, which we probably don't want to advertise to our users, at least until there's a need for it, make it a hidden flag.

As a temporary hack, adding something like this may be OK, but in the longer run, I think we should (at least) consider refactoring "reset --hard" codepath to a freestanding and silent helper function so that both cmd_reset() and add_worktree() can call it and report the outcome in their own phrasing.

And I support the decision not to advertise this as a new feature or an option by implementing it as hidden-bool.

This is completely offtopic tangent, but I wonder how hidden-bool or hidden options[] element in general interacts with the recent addition of helping command line completion. Are we already doing the right thing?

This is completely offtopic tangent, but I wonder how hidden-bool or hidden options[] element in general interacts with the recent addition of helping command line completion. Are we already doing the right thing?

	for (; opts->type != OPTION_END; opts++) {
		const char *suffix = "";

		if (!opts->long_name)
			continue;
		if (opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE))
			continue;

t/t7102-reset.sh test_expect_success 'reset --mixed sets up work tree' ' · line 574

Quoted 4 lines · Thomas Gummerer

t/t7102-reset.sh +4 -0

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

As builtin/reset.c::print_new_head_line() does this:

	printf(_("HEAD is now at %s"), hex);

this needs to use "test_i18ngrep !" instead, no?

[...]
Fix these inconsistencies, and no longer show the identifier by making the 'git reset --hard' call not print the "HEAD is now at ..." line using the newly introduced flag from the previous commit, and printing the message directly from the builtin command instead.

Signed-off-by Thomas Gummerer <[email protected]>

---

builtin/worktree.c +3 -0

@@ -322,12 +320,22 @@ static int add_worktree(const char *path, const char *refname,
                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));