worktree add: improve message for ambiguous remote branch name

From: Yoichi NAKAYAMA <[email protected]>

Display a descriptive message when DWIM fails.

Add advice on how to work around this by specifying the fully qualified name or by setting checkout.defaultRemote.

---
worktree add: improve message for ambiguous remote branch name

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2197%2Fyoichi%2Fimprove-worktree-add-error-message-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2197/yoichi/improve-worktree-add-error-message-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2197

 builtin/worktree.c      | 30 ++++++++++++++++++++++++++----
 t/t2400-worktree-add.sh | 21 +++++++++++++++++++--
 2 files changed, 45 insertions(+), 6 deletions(-)

index 654d27c3e1..46bc305116 100644
@@ -116,6 +116,16 @@ static const char * const git_worktree_unlock_usage[] = {
 	NULL
 };
 
+static const char message_advice_ambiguous_remote_tracking_branch[] =
+	N_("If you meant to create a worktree from a remote tracking branch on,\n"
+	   "e.g. 'origin', you can do so by fully qualifying the name:\n"
+	   "\n"
+	   "    git worktree add <path> origin/<name>\n"
+	   "\n"
+	   "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
+	   "one remote, e.g. the 'origin' remote, consider setting\n"
+	   "checkout.defaultRemote=origin in your config.");
+
 struct add_opts {
 	int force;
 	int detach;

@@ -764,7 +774,7 @@ static int dwim_orphan(const struct add_opts *opts, int opt_track, int remote)
 	return 1;
 }
 
-static char *dwim_branch(const char *path, char **new_branch)
+static char *dwim_branch(const struct add_opts *opts, const char *path, char **new_branch)
 {
 	int n;
 	int branch_exists;

@@ -781,8 +791,14 @@ static char *dwim_branch(const char *path, char **new_branch)
 
 	*new_branch = branchname;
 	if (guess_remote) {
+		int num_matches = 0;
 		struct object_id oid;
-		char *remote = unique_tracking_name(*new_branch, &oid, NULL);
+		char *remote = unique_tracking_name(*new_branch, &oid, &num_matches);
+		if (!opts->quiet && !remote && num_matches > 1) {
+			if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
+				advise(_(message_advice_ambiguous_remote_tracking_branch));
+			warning(_("'%s' matched multiple (%d) remote tracking branches\n"), branchname, num_matches);
+		}
 		return remote;
 	}
 	return NULL;

@@ -890,7 +906,7 @@ static int add(int ac, const char **av, const char *prefix,
 		opts.orphan = dwim_orphan(&opts, !!opt_track, 0);
 	} else if (ac < 2) {
 		/* DWIM: Guess branch name from path. */
-		char *s = dwim_branch(path, &new_branch_to_free);
+		char *s = dwim_branch(&opts, path, &new_branch_to_free);
 		if (s)
 			branch = branch_to_free = s;
 		new_branch = new_branch_to_free;

@@ -904,10 +920,16 @@ static int add(int ac, const char **av, const char *prefix,
 
 		commit = lookup_commit_reference_by_name(branch);
 		if (!commit) {
-			remote = unique_tracking_name(branch, &oid, NULL);
+			int num_matches = 0;
+			remote = unique_tracking_name(branch, &oid, &num_matches);
 			if (remote) {
 				new_branch = branch;
 				branch = new_branch_to_free = remote;
+			} else if (num_matches > 1) {
+				if (!opts.quiet && advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME)) {
+					advise(_(message_advice_ambiguous_remote_tracking_branch));
+				}
+				die(_("'%s' matched multiple (%d) remote tracking branches"), branch, num_matches);
 			}
 		}
 

index 87b926728a..4d21c8eba4 100755
@@ -624,12 +624,12 @@ test_expect_success '"add" <path> <branch> dwims' '
 test_expect_success '"add" <path> <branch> dwims with checkout.defaultRemote' '
 	test_when_finished rm -rf repo_upstream repo_dwim foo &&
 	setup_remote_repo repo_upstream repo_dwim &&
-	git init repo_dwim &&
 	(
 		cd repo_dwim &&
 		git remote add repo_upstream2 ../repo_upstream &&
 		git fetch repo_upstream2 &&
-		test_must_fail git worktree add ../foo foo &&
+		test_must_fail git worktree add ../foo foo 2>error.actual &&
+		test_grep "matched multiple (2) remote tracking branches" error.actual &&
 		git -c checkout.defaultRemote=repo_upstream worktree add ../foo foo &&
 		git status -uno --porcelain >status.actual &&
 		test_must_be_empty status.actual

@@ -669,6 +669,23 @@ test_expect_success 'git worktree add --guess-remote sets up tracking' '
 		test_cmp_rev refs/remotes/repo_a/foo refs/heads/foo
 	)
 '
+test_expect_success 'git worktree add --guess-remote with ambiguous name' '
+	test_when_finished rm -rf repo_a repo_b foo &&
+	setup_remote_repo repo_a repo_b &&
+	(
+		cd repo_b &&
+		git remote add upstream2 ../repo_a &&
+		git fetch upstream2 &&
+		git worktree add --guess-remote ../foo 2>actual &&
+		test_grep "matched multiple (2) remote tracking branches" actual
+	) &&
+	(
+		cd foo &&
+		test_must_fail git config "branch.foo.remote" &&
+		test_must_fail git config "branch.foo.merge" &&
+		test_cmp_rev refs/heads/main refs/heads/foo
+	)
+'
 test_expect_success 'git worktree add --guess-remote sets up tracking (quiet)' '
 	test_when_finished rm -rf repo_a repo_b foo &&
 	setup_remote_repo repo_a repo_b &&

base-commit: 010afd3166ddc64c9863b1506f12cbcdda0d4ea1

gitgitgadget

Stepping back a bit, I think what I find lacking in the proposed warning message is not that we lose what the user gave us, such as '-b <branch>' or '-t'. While this loss makes it impossible to simply copy and paste to reproduce what the user may have intended, it is not the end of the world.

What disturbs me more is that the code holds back information only it possesses, which would immediately help the user if we shared it.

The reason we got this error may not be that the user did not know exactly how to spell out the necessary information (such as which branch to use from which remote) on the command line. It may be that the user did not remember some of the necessary details (such as which remotes have the branch they have in mind). Displaying the command line and advising them to use the fully qualified name might not be the best approach in that case. Telling them that they may have meant 'origin', 'upstream', or 'home' (all of which are remotes with the named branch, though we could not guess which one of the three to choose) may be much more helpful.

This is an interesting idea!

Harald

Sorry for piecemeal reviews, but I just noticed that you have a terminating LF at the end of a single-liner warning message. As die/error/warning ffamily of helpers give the terminating newline themselves, you must not. Unless you want to leave a blank line after your message, that is.

Thanks.

When expressing your opinion on what another said, quote a bit from the message you are responding to so that people know what you are referring to. I cannot easily tell which part of what I said you found interesting.

Thanks.

Sorry, yes this was about:

Harald

I thought the problem here was that it was impossible to distinguish whether the guess was successful, but it was not true. We can distinguish by the message:
branch 'name' set up to track 'remote/name'.
I will not make changes to this part.

Thanks,

Yoichi NAKAYAMA

I realized that instead of placing a burden on the user, we should present a solution.

When a multiple match occurs, the only decision the user needs to make is which remote to select.
For everything else, the hint should give a specific command with arguments that achieve the same
behavior as when exactly one remote matches.

Rather than presenting a list of candidates, I think it is preferable to explain how to generate that list.
This allows users to process the list e.g. by piping it into a command.

I'll submit an updated patch.

Thanks,

Yoichi NAKAYAMA

Hi Yoichi,

[snip]

D. Ben Knoble

Before the change, it calles lookup_commit_reference_by_name() again in the if condition and die() at:

    if (!opts.orphan && !lookup_commit_reference_by_name(branch)) {
        /* snip */
        die(_("invalid reference: %s"), branch);
    }

The motivation for the fix was that this error message did not accurately reflect the situation.

Thanks,

Yoichi NAKAYAMA

The location of this die() is a tad away from the places that the patch touched. The proposed log message could be made a bit more helpful by mentioning it. What was posted reads:

Display a descriptive message when DWIM fails.

Add advice on how to work around this by specifying the fully qualified name or by setting checkout.defaultRemote.

but telling the readers what they will see instead of a descriptive message and how that happens would be very helpful to understand why it is a good idea to die early. Perhaps

When the user runs 'git worktree add x y z' command that does not exactly say which remote they want to work with, we try to guess which remote by passing y. If there are multiple remotes that have branch named y, we silently gave up, leaving remote still NULL. This later causes A and B not happen, and we end up with passing an non-existing branch to lookup_commit_reference_by_name(), triggering "invalid reference" error and die.

or something like that that describes the issue to a similar degree as above mock-up message.

Thanks.

From: Yoichi NAKAYAMA <[email protected]>

Display a hint and a descriptive error message when DWIM fails.

---
worktree add: improve message for ambiguous remote branch name

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2197%2Fyoichi%2Fimprove-worktree-add-error-message-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2197/yoichi/improve-worktree-add-error-message-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2197

Range-diff vs v1:

1: 00b814fe09 ! 1: 1bc57ce497 worktree add: improve message for ambiguous remote branch name

     @@ Metadata
       ## Commit message ##
          worktree add: improve message for ambiguous remote branch name
      
     -    Display a descriptive message when DWIM fails.
     -
     -    Add advice on how to work around this by specifying the fully
     -    qualified name or by setting checkout.defaultRemote.
     +    Display a hint and a descriptive error message when DWIM fails.
      
          Signed-off-by: Yoichi NAKAYAMA <[email protected]>
      
       ## builtin/worktree.c ##
     -@@ builtin/worktree.c: static const char * const git_worktree_unlock_usage[] = {
     - 	NULL
     - };
     +@@
     + 	"\n" \
     + 	"    git worktree add --orphan %s\n")
       
     -+static const char message_advice_ambiguous_remote_tracking_branch[] =
     -+	N_("If you meant to create a worktree from a remote tracking branch on,\n"
     -+	   "e.g. 'origin', you can do so by fully qualifying the name:\n"
     -+	   "\n"
     -+	   "    git worktree add <path> origin/<name>\n"
     -+	   "\n"
     -+	   "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
     -+	   "one remote, e.g. the 'origin' remote, consider setting\n"
     -+	   "checkout.defaultRemote=origin in your config.");
     ++#define WORKTREE_ADD_AMBIGUOUS_REMOTE_BRANCH_NAME_HINT_TEXT \
     ++	_("Matched multiple remote tracking branches, you can list them by:\n" \
     ++	"\n" \
     ++	"    git branch -r --list \"*/%s\"\n" \
     ++	"\n" \
     ++	"If you meant to create a worktree from a remote tracking branch on,\n" \
     ++	"e.g. 'origin', you can do so by:\n" \
     ++	"\n" \
     ++	"    git worktree add -b %s %s origin/%s\n" \
     ++	"\n" \
     ++	"If you'd like to always prefer some remote, e.g. 'origin',\n" \
     ++	"consider setting checkout.defaultRemote=origin in your config.")
      +
     - struct add_opts {
     - 	int force;
     - 	int detach;
     -@@ builtin/worktree.c: static int dwim_orphan(const struct add_opts *opts, int opt_track, int remote)
     - 	return 1;
     - }
     - 
     --static char *dwim_branch(const char *path, char **new_branch)
     -+static char *dwim_branch(const struct add_opts *opts, const char *path, char **new_branch)
     - {
     - 	int n;
     - 	int branch_exists;
     -@@ builtin/worktree.c: static char *dwim_branch(const char *path, char **new_branch)
     - 
     - 	*new_branch = branchname;
     - 	if (guess_remote) {
     -+		int num_matches = 0;
     - 		struct object_id oid;
     --		char *remote = unique_tracking_name(*new_branch, &oid, NULL);
     -+		char *remote = unique_tracking_name(*new_branch, &oid, &num_matches);
     -+		if (!opts->quiet && !remote && num_matches > 1) {
     -+			if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
     -+				advise(_(message_advice_ambiguous_remote_tracking_branch));
     -+			warning(_("'%s' matched multiple (%d) remote tracking branches\n"), branchname, num_matches);
     -+		}
     - 		return remote;
     - 	}
     - 	return NULL;
     -@@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix,
     - 		opts.orphan = dwim_orphan(&opts, !!opt_track, 0);
     - 	} else if (ac < 2) {
     - 		/* DWIM: Guess branch name from path. */
     --		char *s = dwim_branch(path, &new_branch_to_free);
     -+		char *s = dwim_branch(&opts, path, &new_branch_to_free);
     - 		if (s)
     - 			branch = branch_to_free = s;
     - 		new_branch = new_branch_to_free;
     + static const char * const git_worktree_usage[] = {
     + 	BUILTIN_WORKTREE_ADD_USAGE,
     + 	BUILTIN_WORKTREE_LIST_USAGE,
      @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix,
       
       		commit = lookup_commit_reference_by_name(branch);
     @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix,
       				new_branch = branch;
       				branch = new_branch_to_free = remote;
      +			} else if (num_matches > 1) {
     -+				if (!opts.quiet && advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME)) {
     -+					advise(_(message_advice_ambiguous_remote_tracking_branch));
     -+				}
     -+				die(_("'%s' matched multiple (%d) remote tracking branches"), branch, num_matches);
     ++				if (!opts.quiet)
     ++					advise_if_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,
     ++							  WORKTREE_ADD_AMBIGUOUS_REMOTE_BRANCH_NAME_HINT_TEXT,
     ++							  branch, branch, path, branch);
     ++				die(_("'%s' matched multiple (%d) remote tracking branches"),
     ++				    branch, num_matches);
       			}
       		}
       
     @@ t/t2400-worktree-add.sh: test_expect_success '"add" <path> <branch> dwims' '
       		git -c checkout.defaultRemote=repo_upstream worktree add ../foo foo &&
       		git status -uno --porcelain >status.actual &&
       		test_must_be_empty status.actual
     -@@ t/t2400-worktree-add.sh: test_expect_success 'git worktree add --guess-remote sets up tracking' '
     - 		test_cmp_rev refs/remotes/repo_a/foo refs/heads/foo
     - 	)
     - '
     -+test_expect_success 'git worktree add --guess-remote with ambiguous name' '
     -+	test_when_finished rm -rf repo_a repo_b foo &&
     -+	setup_remote_repo repo_a repo_b &&
     -+	(
     -+		cd repo_b &&
     -+		git remote add upstream2 ../repo_a &&
     -+		git fetch upstream2 &&
     -+		git worktree add --guess-remote ../foo 2>actual &&
     -+		test_grep "matched multiple (2) remote tracking branches" actual
     -+	) &&
     -+	(
     -+		cd foo &&
     -+		test_must_fail git config "branch.foo.remote" &&
     -+		test_must_fail git config "branch.foo.merge" &&
     -+		test_cmp_rev refs/heads/main refs/heads/foo
     -+	)
     -+'
     - test_expect_success 'git worktree add --guess-remote sets up tracking (quiet)' '
     - 	test_when_finished rm -rf repo_a repo_b foo &&
     - 	setup_remote_repo repo_a repo_b &&

 builtin/worktree.c      | 23 ++++++++++++++++++++++-
 t/t2400-worktree-add.sh |  4 ++--
 2 files changed, 24 insertions(+), 3 deletions(-)

index 654d27c3e1..b29c3a3755 100644
@@ -64,6 +64,19 @@
 	"\n" \
 	"    git worktree add --orphan %s\n")
 
+#define WORKTREE_ADD_AMBIGUOUS_REMOTE_BRANCH_NAME_HINT_TEXT \
+	_("Matched multiple remote tracking branches, you can list them by:\n" \
+	"\n" \
+	"    git branch -r --list \"*/%s\"\n" \
+	"\n" \
+	"If you meant to create a worktree from a remote tracking branch on,\n" \
+	"e.g. 'origin', you can do so by:\n" \
+	"\n" \
+	"    git worktree add -b %s %s origin/%s\n" \
+	"\n" \
+	"If you'd like to always prefer some remote, e.g. 'origin',\n" \
+	"consider setting checkout.defaultRemote=origin in your config.")
+
 static const char * const git_worktree_usage[] = {
 	BUILTIN_WORKTREE_ADD_USAGE,
 	BUILTIN_WORKTREE_LIST_USAGE,

@@ -904,10 +917,18 @@ static int add(int ac, const char **av, const char *prefix,
 
 		commit = lookup_commit_reference_by_name(branch);
 		if (!commit) {
-			remote = unique_tracking_name(branch, &oid, NULL);
+			int num_matches = 0;
+			remote = unique_tracking_name(branch, &oid, &num_matches);
 			if (remote) {
 				new_branch = branch;
 				branch = new_branch_to_free = remote;
+			} else if (num_matches > 1) {
+				if (!opts.quiet)
+					advise_if_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,
+							  WORKTREE_ADD_AMBIGUOUS_REMOTE_BRANCH_NAME_HINT_TEXT,
+							  branch, branch, path, branch);
+				die(_("'%s' matched multiple (%d) remote tracking branches"),
+				    branch, num_matches);
 			}
 		}
 

index 87b926728a..5c105cf252 100755
@@ -624,12 +624,12 @@ test_expect_success '"add" <path> <branch> dwims' '
 test_expect_success '"add" <path> <branch> dwims with checkout.defaultRemote' '
 	test_when_finished rm -rf repo_upstream repo_dwim foo &&
 	setup_remote_repo repo_upstream repo_dwim &&
-	git init repo_dwim &&
 	(
 		cd repo_dwim &&
 		git remote add repo_upstream2 ../repo_upstream &&
 		git fetch repo_upstream2 &&
-		test_must_fail git worktree add ../foo foo &&
+		test_must_fail git worktree add ../foo foo 2>error.actual &&
+		test_grep "matched multiple (2) remote tracking branches" error.actual &&
 		git -c checkout.defaultRemote=repo_upstream worktree add ../foo foo &&
 		git status -uno --porcelain >status.actual &&
 		test_must_be_empty status.actual

base-commit: 010afd3166ddc64c9863b1506f12cbcdda0d4ea1

gitgitgadget

From: Yoichi NAKAYAMA <[email protected]>

When the user runs 'git worktree add x y' command that does not exactly say which remote they want to work with, and there is no local branch named y, we try to guess which remote by passing y then create a new branch named y which tracks the remote branch.

If there are multiple remotes that have branch named y, we silently gave up, leaving the variable branch intact. This later causes creating local branch and worktree not happen, and we end up with passing an non-existing branch to lookup_commit_reference_by_name(), triggering "invalid reference" error and die.

To resolve this issue, display a hint and a descriptive error message and die immediately when multiple mathing branches are found.

---
worktree add: improve message for ambiguous remote branch name

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2197%2Fyoichi%2Fimprove-worktree-add-error-message-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2197/yoichi/improve-worktree-add-error-message-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2197

Range-diff vs v2:

1: 1bc57ce497 ! 1: 1b9364da7e worktree add: improve message for ambiguous remote branch name
@@ Metadata
## Commit message ##
worktree add: improve message for ambiguous remote branch name

- Display a hint and a descriptive error message when DWIM fails.
+ When the user runs 'git worktree add x y' command that does not
+ exactly say which remote they want to work with, and there is no local
+ branch named y, we try to guess which remote by passing y then create
+ a new branch named y which tracks the remote branch.
+
+ If there are multiple remotes that have branch named y, we silently
+ gave up, leaving the variable branch intact. This later causes
+ creating local branch and worktree not happen, and we end up with
+ passing an non-existing branch to lookup_commit_reference_by_name(),
+ triggering "invalid reference" error and die.
+
+ To resolve this issue, display a hint and a descriptive error message
+ and die immediately when multiple mathing branches are found.

 builtin/worktree.c      | 23 ++++++++++++++++++++++-
 t/t2400-worktree-add.sh |  4 ++--
 2 files changed, 24 insertions(+), 3 deletions(-)

index 654d27c3e1..b29c3a3755 100644
@@ -64,6 +64,19 @@
 	"\n" \
 	"    git worktree add --orphan %s\n")
 
+#define WORKTREE_ADD_AMBIGUOUS_REMOTE_BRANCH_NAME_HINT_TEXT \
+	_("Matched multiple remote tracking branches, you can list them by:\n" \
+	"\n" \
+	"    git branch -r --list \"*/%s\"\n" \
+	"\n" \
+	"If you meant to create a worktree from a remote tracking branch on,\n" \
+	"e.g. 'origin', you can do so by:\n" \
+	"\n" \
+	"    git worktree add -b %s %s origin/%s\n" \
+	"\n" \
+	"If you'd like to always prefer some remote, e.g. 'origin',\n" \
+	"consider setting checkout.defaultRemote=origin in your config.")
+
 static const char * const git_worktree_usage[] = {
 	BUILTIN_WORKTREE_ADD_USAGE,
 	BUILTIN_WORKTREE_LIST_USAGE,

@@ -904,10 +917,18 @@ static int add(int ac, const char **av, const char *prefix,
 
 		commit = lookup_commit_reference_by_name(branch);
 		if (!commit) {
-			remote = unique_tracking_name(branch, &oid, NULL);
+			int num_matches = 0;
+			remote = unique_tracking_name(branch, &oid, &num_matches);
 			if (remote) {
 				new_branch = branch;
 				branch = new_branch_to_free = remote;
+			} else if (num_matches > 1) {
+				if (!opts.quiet)
+					advise_if_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,
+							  WORKTREE_ADD_AMBIGUOUS_REMOTE_BRANCH_NAME_HINT_TEXT,
+							  branch, branch, path, branch);
+				die(_("'%s' matched multiple (%d) remote tracking branches"),
+				    branch, num_matches);
 			}
 		}
 

index 87b926728a..5c105cf252 100755
@@ -624,12 +624,12 @@ test_expect_success '"add" <path> <branch> dwims' '
 test_expect_success '"add" <path> <branch> dwims with checkout.defaultRemote' '
 	test_when_finished rm -rf repo_upstream repo_dwim foo &&
 	setup_remote_repo repo_upstream repo_dwim &&
-	git init repo_dwim &&
 	(
 		cd repo_dwim &&
 		git remote add repo_upstream2 ../repo_upstream &&
 		git fetch repo_upstream2 &&
-		test_must_fail git worktree add ../foo foo &&
+		test_must_fail git worktree add ../foo foo 2>error.actual &&
+		test_grep "matched multiple (2) remote tracking branches" error.actual &&
 		git -c checkout.defaultRemote=repo_upstream worktree add ../foo foo &&
 		git status -uno --porcelain >status.actual &&
 		test_must_be_empty status.actual

base-commit: 010afd3166ddc64c9863b1506f12cbcdda0d4ea1

gitgitgadget

I removed the change (advise and warn) here in the latest patch. But I am still wondering what I should do. I think a warning would be excessive if there is no match,
but the user might want to know if there are multiple matches.

Thanks,

Yoichi NAKAYAMA

I used x and y as placeholders. The readers would be helped if you used a more plausible sounding names, e.g., naming directory as something like foo-dir (the point being 'dir' somewhere in its name) and naming a branch as something like bar-topic. If this were 'git worktree add', it is probably more than likely that the destination directory would begin with ../ to have the new worktree next to the primary repository we are running in, no?

"This later causes" part still seems a bit too sketchy to help a totally new reader, even though I've stared at this code long enough so it would be sufficient for me personally. But these logs are not about helping me, but helping other developers, so...

Extending unique_tracking_name() would also affect the implementation in checkout.c, and since the goal here is to improve the messages (making them as helpful as those in checkout), I will hold off on doing that for now.

I will rewrite the log messages a bit.

If the user is aiming to create a new worktree with the HEAD detached, one would specify a fully qualified branch name like origin/bar-topic, starting with a remote name. If a branch name starts with a remote name, multiple matches (condition to show this hint) rarely occur.

Thanks,

Yoichi NAKAYAMA

From: Yoichi NAKAYAMA <[email protected]>

When the user runs 'git worktree add ../foo-dir bar-topic' command that does not exactly say which remote they want to work with, and there is no local branch named bar-topic, we try to guess which remote by passing bar-topic then create a new branch named bar-topic which tracks the remote branch.

If there are multiple remotes that have branch named bar-topic, we silently gave up, leaving the variable 'branch' intact. Then we entered the conditional clause 'if (!opts.orphan && !lookup_commit_reference_by_name(branch))' and triggered "invalid reference" error. This error message did not contain enough information to resolve the issue where the remote could not be guessed.

To improve the situation, we display a hint and a descriptive error message and die immediately when multiple matching branches are found.

---
worktree add: improve message for ambiguous remote branch name

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2197%2Fyoichi%2Fimprove-worktree-add-error-message-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2197/yoichi/improve-worktree-add-error-message-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/2197

Range-diff vs v3:

1: 1b9364da7e ! 1: f7c413b588 worktree add: improve message for ambiguous remote branch name
@@ Metadata
## Commit message ##
worktree add: improve message for ambiguous remote branch name

- When the user runs 'git worktree add x y' command that does not
- exactly say which remote they want to work with, and there is no local
- branch named y, we try to guess which remote by passing y then create
- a new branch named y which tracks the remote branch.
+ When the user runs 'git worktree add ../foo-dir bar-topic' command
+ that does not exactly say which remote they want to work with, and
+ there is no local branch named bar-topic, we try to guess which remote
+ by passing bar-topic then create a new branch named bar-topic which
+ tracks the remote branch.

- If there are multiple remotes that have branch named y, we silently
- gave up, leaving the variable branch intact. This later causes
- creating local branch and worktree not happen, and we end up with
- passing an non-existing branch to lookup_commit_reference_by_name(),
- triggering "invalid reference" error and die.
+ If there are multiple remotes that have branch named bar-topic, we
+ silently gave up, leaving the variable 'branch' intact. Then we
+ entered the conditional clause 'if (!opts.orphan &&
+ !lookup_commit_reference_by_name(branch))' and triggered "invalid
+ reference" error. This error message did not contain enough
+ information to resolve the issue where the remote could not be
+ guessed.

- To resolve this issue, display a hint and a descriptive error message
- and die immediately when multiple mathing branches are found.
+ To improve the situation, we display a hint and a descriptive error
+ message and die immediately when multiple matching branches are found.

 builtin/worktree.c      | 23 ++++++++++++++++++++++-
 t/t2400-worktree-add.sh |  4 ++--
 2 files changed, 24 insertions(+), 3 deletions(-)

index 654d27c3e1..b29c3a3755 100644
@@ -64,6 +64,19 @@
 	"\n" \
 	"    git worktree add --orphan %s\n")
 
+#define WORKTREE_ADD_AMBIGUOUS_REMOTE_BRANCH_NAME_HINT_TEXT \
+	_("Matched multiple remote tracking branches, you can list them by:\n" \
+	"\n" \
+	"    git branch -r --list \"*/%s\"\n" \
+	"\n" \
+	"If you meant to create a worktree from a remote tracking branch on,\n" \
+	"e.g. 'origin', you can do so by:\n" \
+	"\n" \
+	"    git worktree add -b %s %s origin/%s\n" \
+	"\n" \
+	"If you'd like to always prefer some remote, e.g. 'origin',\n" \
+	"consider setting checkout.defaultRemote=origin in your config.")
+
 static const char * const git_worktree_usage[] = {
 	BUILTIN_WORKTREE_ADD_USAGE,
 	BUILTIN_WORKTREE_LIST_USAGE,

@@ -904,10 +917,18 @@ static int add(int ac, const char **av, const char *prefix,
 
 		commit = lookup_commit_reference_by_name(branch);
 		if (!commit) {
-			remote = unique_tracking_name(branch, &oid, NULL);
+			int num_matches = 0;
+			remote = unique_tracking_name(branch, &oid, &num_matches);
 			if (remote) {
 				new_branch = branch;
 				branch = new_branch_to_free = remote;
+			} else if (num_matches > 1) {
+				if (!opts.quiet)
+					advise_if_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,
+							  WORKTREE_ADD_AMBIGUOUS_REMOTE_BRANCH_NAME_HINT_TEXT,
+							  branch, branch, path, branch);
+				die(_("'%s' matched multiple (%d) remote tracking branches"),
+				    branch, num_matches);
 			}
 		}
 

index 87b926728a..5c105cf252 100755
@@ -624,12 +624,12 @@ test_expect_success '"add" <path> <branch> dwims' '
 test_expect_success '"add" <path> <branch> dwims with checkout.defaultRemote' '
 	test_when_finished rm -rf repo_upstream repo_dwim foo &&
 	setup_remote_repo repo_upstream repo_dwim &&
-	git init repo_dwim &&
 	(
 		cd repo_dwim &&
 		git remote add repo_upstream2 ../repo_upstream &&
 		git fetch repo_upstream2 &&
-		test_must_fail git worktree add ../foo foo &&
+		test_must_fail git worktree add ../foo foo 2>error.actual &&
+		test_grep "matched multiple (2) remote tracking branches" error.actual &&
 		git -c checkout.defaultRemote=repo_upstream worktree add ../foo foo &&
 		git status -uno --porcelain >status.actual &&
 		test_must_be_empty status.actual

base-commit: 010afd3166ddc64c9863b1506f12cbcdda0d4ea1

gitgitgadget

Sorry if I was unclear: I don’t know what hits this particular code path offhand, and I don’t think adding the warning is wrong! I meant « as opposed to dying, a warning won’t break anyone but will let them know of potentially unintended consequences », maybe.

I’m not particularly invested in how we signal ambiguity, but if we don’t have a documented policy for resolving it in a particular way then I agree we should signal it.

Thank you for the additional information. I think the intention behind your comment was clear. The reason I was wondering was simply that I hadn't yet found a clear direction for what I should do.

The git worktree add --guess-remote ../foo command (without <commit-ish>) is designed to successfully create the worktree even if the guess fails. If we are to
issue a warning and advice, we need to indicate what to do next, given that the worktree and the branch has already been created. It would be something like
git -C ../foo reset –hard origin/foo
git -C ../foo branch –set-upstream-to=origin/foo
but they are too complicated. So I now think it is a valid choice to interpret the
behavior as normal and refrain from issuing a warning or advice.

Thanks,

Yoichi NAKAYAMA

Sorry but I do not quite understand this logic.

Giving unique_tracking_name() the optional ability to report which remotes have a branch with the given name does not have to affect other callers of the function at all; that is the definition of a new feature being "optional."

Furthermore, the goal of improving these messages falls short if we withhold the list of remotes the user could have meant, which we are already computing internally to decide that the original request is ambiguous.

I am not sure about this part, either. After all, we are trying to help a user who made a mistake composing their command-line arguments. If they specify a fully qualified branch name like 'origin/bar-topic' (regardless of whether they want to create a detached 'HEAD' or not), they would not hit the 'saying bar-topic alone is ambiguous' error path, would they?

Here is a patch to show what I mean. To 'improve the message for an ambiguous remote branch name' for the 'checkout' command, we can add this optional feature to the unique_tracking_name() function and we can do so without affecting the 'worktree add' command.

Regardless of whether we modify unique_tracking_name(), I think refactoring be_explicit() is worth doing, as the original code in parse_remote_branch() was badly misindented.

 builtin/checkout.c | 74 ++++++++++++++++++++++++++++++++----------------------
 builtin/worktree.c |  4 +--
 checkout.c         | 14 +++++++++--
 checkout.h         |  5 +++-
 4 files changed, 62 insertions(+), 35 deletions(-)

index 55e3a89a85..8d61973a11 100644
@@ -1343,13 +1343,50 @@ enum checkout_command {
 	CHECKOUT_RESTORE = 3,
 };
 
+static void be_explicit(enum checkout_command which_command,
+			const struct string_list *matched_remote_names)
+{
+	const char *cmdname;
+	struct string_list_item *item;
+
+	switch (which_command) {
+	case CHECKOUT_CHECKOUT:
+		cmdname = "checkout";
+		break;
+	case CHECKOUT_SWITCH:
+		cmdname = "switch";
+		break;
+	default:
+		BUG("command <%d> should not reach parse_remote_branch",
+		     which_command);
+		break;
+	}
+
+	advise(_("Branches with the same name appears in multiple remotes:"));
+	for_each_string_list_item(item, matched_remote_names) {
+		advise(_("  %s"), item->string);
+	}
+	advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
+		 "you can do so by fully qualifying the name with the --track option:\n"
+		 "\n"
+		 "    git %s --track origin/<name>\n"
+		 "\n"
+		 "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
+		 "one remote, e.g. the 'origin' remote, consider setting\n"
+		 "checkout.defaultRemote=origin in your config."),
+	       cmdname);
+}
+
 static char *parse_remote_branch(const char *arg,
 				 struct object_id *rev,
 				 int could_be_checkout_paths,
 				 enum checkout_command which_command)
 {
 	int num_matches = 0;
-	char *remote = unique_tracking_name(arg, rev, &num_matches);
+	struct string_list matched_remote_names = STRING_LIST_INIT_DUP;
+
+	char *remote = unique_tracking_name(arg, rev, &num_matches,
+					    &matched_remote_names);
 
 	if (remote && could_be_checkout_paths) {
 		die(_("'%s' could be both a local file and a tracking branch.\n"

@@ -1358,37 +1395,14 @@ static char *parse_remote_branch(const char *arg,
 	}
 
 	if (!remote && num_matches > 1) {
-	    if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME)) {
-		    const char *cmdname;
-
-		    switch (which_command) {
-		    case CHECKOUT_CHECKOUT:
-			    cmdname = "checkout";
-			    break;
-		    case CHECKOUT_SWITCH:
-			    cmdname = "switch";
-			    break;
-		    default:
-			    BUG("command <%d> should not reach parse_remote_branch",
-				which_command);
-			    break;
-		    }
-
-		    advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
-			     "you can do so by fully qualifying the name with the --track option:\n"
-			     "\n"
-			     "    git %s --track origin/<name>\n"
-			     "\n"
-			     "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
-			     "one remote, e.g. the 'origin' remote, consider setting\n"
-			     "checkout.defaultRemote=origin in your config."),
-			   cmdname);
-	    }
-
-	    die(_("'%s' matched multiple (%d) remote tracking branches"),
-		arg, num_matches);
+		if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
+			be_explicit(which_command, &matched_remote_names);
+		die(_("'%s' matched multiple (%d) remote tracking branches"),
+		    arg, num_matches);
 	}
 
+	string_list_clear(&matched_remote_names, 0);
+
 	return remote;
 }
 

index 654d27c3e1..22c8e5e131 100644
@@ -782,7 +782,7 @@ static char *dwim_branch(const char *path, char **new_branch)
 	*new_branch = branchname;
 	if (guess_remote) {
 		struct object_id oid;
-		char *remote = unique_tracking_name(*new_branch, &oid, NULL);
+		char *remote = unique_tracking_name(*new_branch, &oid, NULL, NULL);
 		return remote;
 	}
 	return NULL;

@@ -904,7 +904,7 @@ static int add(int ac, const char **av, const char *prefix,
 
 		commit = lookup_commit_reference_by_name(branch);
 		if (!commit) {
-			remote = unique_tracking_name(branch, &oid, NULL);
+			remote = unique_tracking_name(branch, &oid, NULL, NULL);
 			if (remote) {
 				new_branch = branch;
 				branch = new_branch_to_free = remote;

index 1588b116ee..2806b783ec 100644
@@ -8,6 +8,7 @@
 #include "checkout.h"
 #include "config.h"
 #include "strbuf.h"
+#include "string-list.h"
 
 struct tracking_name_data {
 	/* const */ char *src_ref;

@@ -17,6 +18,7 @@ struct tracking_name_data {
 	const char *default_remote;
 	char *default_dst_ref;
 	struct object_id *default_dst_oid;
+	struct string_list **remote_names;
 };
 
 #define TRACKING_NAME_DATA_INIT { 0 }

@@ -39,6 +41,8 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
 		oidcpy(dst, cb->dst_oid);
 		cb->default_dst_oid = dst;
 	}
+	if (cb->remote_names)
+		string_list_append(*cb->remote_names, remote->name);
 	if (cb->dst_ref) {
 		free(query.dst);
 		return 0;

@@ -48,14 +52,20 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
 }
 
 char *unique_tracking_name(const char *name, struct object_id *oid,
-			   int *dwim_remotes_matched)
+			   int *dwim_remotes_matched,
+			   struct string_list *dwim_remote_names)
 {
 	struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
 	const char *default_remote = NULL;
-	if (!repo_config_get_string_tmp(the_repository, "checkout.defaultremote", &default_remote))
+
+	if (!repo_config_get_string_tmp(the_repository,
+					"checkout.defaultremote",
+					&default_remote))
 		cb_data.default_remote = default_remote;
 	cb_data.src_ref = xstrfmt("refs/heads/%s", name);
 	cb_data.dst_oid = oid;
+	if (dwim_remote_names)
+		cb_data.remote_names = &dwim_remote_names;
 	for_each_remote(check_tracking_name, &cb_data);
 	if (dwim_remotes_matched)
 		*dwim_remotes_matched = cb_data.num_matches;

index 55920e7aeb..0b185a0fc9 100644
@@ -3,6 +3,8 @@
 
 #include "hash.h"
 
+struct string_list;
+
 /*
  * Check if the branch name uniquely matches a branch name on a remote
  * tracking branch.  Return the name of the remote if such a branch

@@ -10,6 +12,7 @@
  */
 char *unique_tracking_name(const char *name,
 			   struct object_id *oid,
-			   int *dwim_remotes_matched);
+			   int *dwim_remotes_matched,
+			   struct string_list *dwim_remote_names);
 
 #endif /* CHECKOUT_H */

That is certainly true.
I think the modification to unique_tracking_name() that you shared is good.

If the user specifies a fully qualified branch name like
git worktree add ../foo-dir origin/bar-topic
the 2nd argument 'origin/bar-topic' will be resolved by

    commit = lookup_commit_reference_by_name(branch);

before calling unique_tracking_name(), so the ambiguous warning won't occur.

In a worst-case scenario, a user might make a typo in the 2nd argument (e.g. origin/bar-topik) that fails to match a local ref (e.g. refs/remotes/origin/bar-topic)
but accidentally matches multiple remote branch names (e.g. branches related to refs/remotes/remote1/origin/bar-topik and refs/remotes/remote2/origin/bar-topik),
then it could cause an issue. However, I think this is an edge case that doesn't need to be considered.

Thanks,

Yoichi NAKAYAMA

'git worktree add ../foo-dir bar-topic' fails to dwim when there are multiple remote branches with name `bar-topic'. But it doesn't display meaningful message as 'git checkout bar-topic' does under the same situation.

We improve this by adding advice and modify the error message for worktree add.

By Junio's suggestion, we include matched remote names in the advice. It is applied to checkout, too.

The changes to 'checkout' are almost identical to what Junio proposed; I have made minor adjustments to use the specified branch name. I'm not sure how to handle the "Author" field in this case, so I've set it to myself for now, but I'll correct it if that's not appropriate.

Yoichi NAKAYAMA (2):
checkout: improve message for ambiguous remote branch name worktree add: improve message for ambiguous remote branch name

 builtin/checkout.c      | 75 ++++++++++++++++++++++++-----------------
 builtin/worktree.c      | 36 ++++++++++++++++++--
 checkout.c              | 14 ++++++--
 checkout.h              |  5 ++-
 t/t2400-worktree-add.sh |  4 +--
 5 files changed, 96 insertions(+), 38 deletions(-)

base-commit: dea0ea3582e6980ddbc1173cc8e3e9f9db91cde0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2197%2Fyoichi%2Fimprove-worktree-add-error-message-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2197/yoichi/improve-worktree-add-error-message-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/2197

Range-diff vs v4:

 -:  ---------- > 1:  b838fdabb7 checkout: improve message for ambiguous remote branch name
 1:  f7c413b588 ! 2:  777862235e worktree add: improve message for ambiguous remote branch name
     @@ Commit message
          Signed-off-by: Yoichi NAKAYAMA <[email protected]>
      
       ## builtin/worktree.c ##
     -@@
     - 	"\n" \
     - 	"    git worktree add --orphan %s\n")
     +@@ builtin/worktree.c: static char *dwim_branch(const char *path, char **new_branch)
     + 	return NULL;
     + }
       
     -+#define WORKTREE_ADD_AMBIGUOUS_REMOTE_BRANCH_NAME_HINT_TEXT \
     -+	_("Matched multiple remote tracking branches, you can list them by:\n" \
     -+	"\n" \
     -+	"    git branch -r --list \"*/%s\"\n" \
     -+	"\n" \
     -+	"If you meant to create a worktree from a remote tracking branch on,\n" \
     -+	"e.g. 'origin', you can do so by:\n" \
     -+	"\n" \
     -+	"    git worktree add -b %s %s origin/%s\n" \
     -+	"\n" \
     -+	"If you'd like to always prefer some remote, e.g. 'origin',\n" \
     -+	"consider setting checkout.defaultRemote=origin in your config.")
     ++static void advise_ambiguous_remote(const char *path, const char *branch,
     ++				    const struct string_list *matched_remote_names)
     ++{
     ++	struct string_list_item *item;
      +
     - static const char * const git_worktree_usage[] = {
     - 	BUILTIN_WORKTREE_ADD_USAGE,
     - 	BUILTIN_WORKTREE_LIST_USAGE,
     ++	advise(_("Branches with the same name appears in multiple remotes:"));
     ++	for_each_string_list_item(item, matched_remote_names) {
     ++		advise(_("  %s"), item->string);
     ++	}
     ++	advise(_("If you meant to create a worktree from a remote tracking branch on\n"
     ++		 "<remote>, you can do so by:\n"
     ++		 "\n"
     ++		 "    git worktree add -b %s %s <remote>/%s\n"
     ++		 "\n"
     ++		 "If you'd like to always prefer some remote, e.g. 'origin',\n"
     ++		 "consider setting checkout.defaultRemote=origin in your config."),
     ++	       branch, path, branch);
     ++}
     ++
     + static int add(int ac, const char **av, const char *prefix,
     + 	       struct repository *repo UNUSED)
     + {
      @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix,
     + 	} else if (ac == 2) {
     + 		struct object_id oid;
     + 		struct commit *commit;
     +-		char *remote;
       
       		commit = lookup_commit_reference_by_name(branch);
       		if (!commit) {
     --			remote = unique_tracking_name(branch, &oid, NULL);
     +-			remote = unique_tracking_name(branch, &oid, NULL, NULL);
     ++			char *remote;
      +			int num_matches = 0;
     -+			remote = unique_tracking_name(branch, &oid, &num_matches);
     ++			struct string_list matched_remote_names = STRING_LIST_INIT_DUP;
     ++
     ++			remote = unique_tracking_name(branch, &oid, &num_matches,
     ++						      &matched_remote_names);
       			if (remote) {
       				new_branch = branch;
       				branch = new_branch_to_free = remote;
      +			} else if (num_matches > 1) {
     -+				if (!opts.quiet)
     -+					advise_if_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,
     -+							  WORKTREE_ADD_AMBIGUOUS_REMOTE_BRANCH_NAME_HINT_TEXT,
     -+							  branch, branch, path, branch);
     ++				if (!opts.quiet &&
     ++				    advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
     ++					advise_ambiguous_remote(path, branch, &matched_remote_names);
      +				die(_("'%s' matched multiple (%d) remote tracking branches"),
      +				    branch, num_matches);
       			}
     ++			string_list_clear(&matched_remote_names, 0);
       		}
       
     + 		if (!strcmp(branch, "HEAD"))
      
       ## t/t2400-worktree-add.sh ##
      @@ t/t2400-worktree-add.sh: test_expect_success '"add" <path> <branch> dwims' '

gitgitgadget

From: Yoichi NAKAYAMA <[email protected]>

When the user runs 'git checkout bar-topic' command that does not exactly say which remote they want to work with, and there is no local branch named bar-topic, we try to guess which remote by passing bar-topic then create a new branch named bar-topic which tracks the remote branch.

If multiple remotes have a branch named bar-topic, we cannot determine a single specific remote. Therefore, we provide information that the user can utilize to resolve the issue.

To make the advice more feasible, we will provide matched remote names for the specified branch name.

To achive that, we add an optional feature to the unique_tracking_name() function that allows the matched remote name to be exposed to the caller.

---
 builtin/checkout.c | 75 +++++++++++++++++++++++++++-------------------
 builtin/worktree.c |  4 +--
 checkout.c         | 14 +++++++--
 checkout.h         |  5 +++-
 4 files changed, 63 insertions(+), 35 deletions(-)

index 55e3a89a85..a2749352e6 100644
@@ -1343,13 +1343,51 @@ enum checkout_command {
 	CHECKOUT_RESTORE = 3,
 };
 
+static void be_explicit(const char *branch,
+			enum checkout_command which_command,
+			const struct string_list *matched_remote_names)
+{
+	const char *cmdname;
+	struct string_list_item *item;
+
+	switch (which_command) {
+	case CHECKOUT_CHECKOUT:
+		cmdname = "checkout";
+		break;
+	case CHECKOUT_SWITCH:
+		cmdname = "switch";
+		break;
+	default:
+		BUG("command <%d> should not reach parse_remote_branch",
+		     which_command);
+		break;
+	}
+
+	advise(_("Branches with the same name appears in multiple remotes:"));
+	for_each_string_list_item(item, matched_remote_names) {
+		advise(_("  %s"), item->string);
+	}
+	advise(_("If you meant to check out a remote tracking branch on <remote>,\n"
+		 "you can do so by fully qualifying the name with the --track option:\n"
+		 "\n"
+		 "    git %s --track <remote>/%s\n"
+		 "\n"
+		 "If you'd like to always have checkouts of an ambiguous name prefer\n"
+		 "one remote, e.g. the 'origin' remote, consider setting\n"
+		 "checkout.defaultRemote=origin in your config."),
+	       cmdname, branch);
+}
+
 static char *parse_remote_branch(const char *arg,
 				 struct object_id *rev,
 				 int could_be_checkout_paths,
 				 enum checkout_command which_command)
 {
 	int num_matches = 0;
-	char *remote = unique_tracking_name(arg, rev, &num_matches);
+	struct string_list matched_remote_names = STRING_LIST_INIT_DUP;
+
+	char *remote = unique_tracking_name(arg, rev, &num_matches,
+					    &matched_remote_names);
 
 	if (remote && could_be_checkout_paths) {
 		die(_("'%s' could be both a local file and a tracking branch.\n"

@@ -1358,37 +1396,14 @@ static char *parse_remote_branch(const char *arg,
 	}
 
 	if (!remote && num_matches > 1) {
-	    if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME)) {
-		    const char *cmdname;
-
-		    switch (which_command) {
-		    case CHECKOUT_CHECKOUT:
-			    cmdname = "checkout";
-			    break;
-		    case CHECKOUT_SWITCH:
-			    cmdname = "switch";
-			    break;
-		    default:
-			    BUG("command <%d> should not reach parse_remote_branch",
-				which_command);
-			    break;
-		    }
-
-		    advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
-			     "you can do so by fully qualifying the name with the --track option:\n"
-			     "\n"
-			     "    git %s --track origin/<name>\n"
-			     "\n"
-			     "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
-			     "one remote, e.g. the 'origin' remote, consider setting\n"
-			     "checkout.defaultRemote=origin in your config."),
-			   cmdname);
-	    }
-
-	    die(_("'%s' matched multiple (%d) remote tracking branches"),
-		arg, num_matches);
+		if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
+			be_explicit(arg, which_command, &matched_remote_names);
+		die(_("'%s' matched multiple (%d) remote tracking branches"),
+		    arg, num_matches);
 	}
 
+	string_list_clear(&matched_remote_names, 0);
+
 	return remote;
 }
 

index 654d27c3e1..22c8e5e131 100644
@@ -782,7 +782,7 @@ static char *dwim_branch(const char *path, char **new_branch)
 	*new_branch = branchname;
 	if (guess_remote) {
 		struct object_id oid;
-		char *remote = unique_tracking_name(*new_branch, &oid, NULL);
+		char *remote = unique_tracking_name(*new_branch, &oid, NULL, NULL);
 		return remote;
 	}
 	return NULL;

@@ -904,7 +904,7 @@ static int add(int ac, const char **av, const char *prefix,
 
 		commit = lookup_commit_reference_by_name(branch);
 		if (!commit) {
-			remote = unique_tracking_name(branch, &oid, NULL);
+			remote = unique_tracking_name(branch, &oid, NULL, NULL);
 			if (remote) {
 				new_branch = branch;
 				branch = new_branch_to_free = remote;

index 1588b116ee..2806b783ec 100644
@@ -8,6 +8,7 @@
 #include "checkout.h"
 #include "config.h"
 #include "strbuf.h"
+#include "string-list.h"
 
 struct tracking_name_data {
 	/* const */ char *src_ref;

@@ -17,6 +18,7 @@ struct tracking_name_data {
 	const char *default_remote;
 	char *default_dst_ref;
 	struct object_id *default_dst_oid;
+	struct string_list **remote_names;
 };
 
 #define TRACKING_NAME_DATA_INIT { 0 }

@@ -39,6 +41,8 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
 		oidcpy(dst, cb->dst_oid);
 		cb->default_dst_oid = dst;
 	}
+	if (cb->remote_names)
+		string_list_append(*cb->remote_names, remote->name);
 	if (cb->dst_ref) {
 		free(query.dst);
 		return 0;

@@ -48,14 +52,20 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
 }
 
 char *unique_tracking_name(const char *name, struct object_id *oid,
-			   int *dwim_remotes_matched)
+			   int *dwim_remotes_matched,
+			   struct string_list *dwim_remote_names)
 {
 	struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
 	const char *default_remote = NULL;
-	if (!repo_config_get_string_tmp(the_repository, "checkout.defaultremote", &default_remote))
+
+	if (!repo_config_get_string_tmp(the_repository,
+					"checkout.defaultremote",
+					&default_remote))
 		cb_data.default_remote = default_remote;
 	cb_data.src_ref = xstrfmt("refs/heads/%s", name);
 	cb_data.dst_oid = oid;
+	if (dwim_remote_names)
+		cb_data.remote_names = &dwim_remote_names;
 	for_each_remote(check_tracking_name, &cb_data);
 	if (dwim_remotes_matched)
 		*dwim_remotes_matched = cb_data.num_matches;

index 55920e7aeb..0b185a0fc9 100644
@@ -3,6 +3,8 @@
 
 #include "hash.h"
 
+struct string_list;
+
 /*
  * Check if the branch name uniquely matches a branch name on a remote
  * tracking branch.  Return the name of the remote if such a branch

@@ -10,6 +12,7 @@
  */
 char *unique_tracking_name(const char *name,
 			   struct object_id *oid,
-			   int *dwim_remotes_matched);
+			   int *dwim_remotes_matched,
+			   struct string_list *dwim_remote_names);
 
 #endif /* CHECKOUT_H */

gitgitgadget

From: Yoichi NAKAYAMA <[email protected]>

When the user runs 'git worktree add ../foo-dir bar-topic' command that does not exactly say which remote they want to work with, and there is no local branch named bar-topic, we try to guess which remote by passing bar-topic then create a new branch named bar-topic which tracks the remote branch.

If there are multiple remotes that have branch named bar-topic, we silently gave up, leaving the variable 'branch' intact. Then we entered the conditional clause 'if (!opts.orphan && !lookup_commit_reference_by_name(branch))' and triggered "invalid reference" error. This error message did not contain enough information to resolve the issue where the remote could not be guessed.

To improve the situation, we display a hint and a descriptive error message and die immediately when multiple matching branches are found.

---
 builtin/worktree.c      | 34 ++++++++++++++++++++++++++++++++--
 t/t2400-worktree-add.sh |  4 ++--
 2 files changed, 34 insertions(+), 4 deletions(-)

index 22c8e5e131..30d7a8c340 100644
@@ -788,6 +788,25 @@ static char *dwim_branch(const char *path, char **new_branch)
 	return NULL;
 }
 
+static void advise_ambiguous_remote(const char *path, const char *branch,
+				    const struct string_list *matched_remote_names)
+{
+	struct string_list_item *item;
+
+	advise(_("Branches with the same name appears in multiple remotes:"));
+	for_each_string_list_item(item, matched_remote_names) {
+		advise(_("  %s"), item->string);
+	}
+	advise(_("If you meant to create a worktree from a remote tracking branch on\n"
+		 "<remote>, you can do so by:\n"
+		 "\n"
+		 "    git worktree add -b %s %s <remote>/%s\n"
+		 "\n"
+		 "If you'd like to always prefer some remote, e.g. 'origin',\n"
+		 "consider setting checkout.defaultRemote=origin in your config."),
+	       branch, path, branch);
+}
+
 static int add(int ac, const char **av, const char *prefix,
 	       struct repository *repo UNUSED)
 {

@@ -900,15 +919,26 @@ static int add(int ac, const char **av, const char *prefix,
 	} else if (ac == 2) {
 		struct object_id oid;
 		struct commit *commit;
-		char *remote;
 
 		commit = lookup_commit_reference_by_name(branch);
 		if (!commit) {
-			remote = unique_tracking_name(branch, &oid, NULL, NULL);
+			char *remote;
+			int num_matches = 0;
+			struct string_list matched_remote_names = STRING_LIST_INIT_DUP;
+
+			remote = unique_tracking_name(branch, &oid, &num_matches,
+						      &matched_remote_names);
 			if (remote) {
 				new_branch = branch;
 				branch = new_branch_to_free = remote;
+			} else if (num_matches > 1) {
+				if (!opts.quiet &&
+				    advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
+					advise_ambiguous_remote(path, branch, &matched_remote_names);
+				die(_("'%s' matched multiple (%d) remote tracking branches"),
+				    branch, num_matches);
 			}
+			string_list_clear(&matched_remote_names, 0);
 		}
 
 		if (!strcmp(branch, "HEAD"))

index 87b926728a..5c105cf252 100755
@@ -624,12 +624,12 @@ test_expect_success '"add" <path> <branch> dwims' '
 test_expect_success '"add" <path> <branch> dwims with checkout.defaultRemote' '
 	test_when_finished rm -rf repo_upstream repo_dwim foo &&
 	setup_remote_repo repo_upstream repo_dwim &&
-	git init repo_dwim &&
 	(
 		cd repo_dwim &&
 		git remote add repo_upstream2 ../repo_upstream &&
 		git fetch repo_upstream2 &&
-		test_must_fail git worktree add ../foo foo &&
+		test_must_fail git worktree add ../foo foo 2>error.actual &&
+		test_grep "matched multiple (2) remote tracking branches" error.actual &&
 		git -c checkout.defaultRemote=repo_upstream worktree add ../foo foo &&
 		git status -uno --porcelain >status.actual &&
 		test_must_be_empty status.actual

gitgitgadget

It stands for "tell the user to be more explicit". I agree with you that the refactoring should be done as a separate step, on top of which we should add the new feature, i.e., "give list of possible candidates", as a separate step.

I think it's a good idea to separate the steps, so I'll split the commits. I am considering "advise_disambiguating_remotes" as a better function name.

Thanks,

Yoichi NAKAYAMA

'git worktree add ../foo-dir bar-topic' fails to dwim when there are multiple remote branches with name `bar-topic'. But it doesn't display meaningful message as 'git checkout bar-topic' does under the same situation.

We improve this by adding advice and modify the error message for worktree add.

By Junio's suggestion, we include matched remote names in the advice. It is applied to checkout, too.

The changes to 'checkout' are almost identical to what Junio proposed; I have made minor adjustments to use the specified branch name. I'm not sure how to handle the "Author" field in this case, so I've set it to myself for now, but I'll correct it if that's not appropriate.

Yoichi NAKAYAMA (3):
checkout: extract function to display advice for ambiguous remotes
checkout: improve message for ambiguous remote branch name worktree add: improve message for ambiguous remote branch name

 builtin/checkout.c      | 76 +++++++++++++++++++++++++----------------
 builtin/worktree.c      | 37 ++++++++++++++++++--
 checkout.c              | 14 ++++++--
 checkout.h              |  5 ++-
 t/t2400-worktree-add.sh |  4 +--
 5 files changed, 98 insertions(+), 38 deletions(-)

base-commit: dea0ea3582e6980ddbc1173cc8e3e9f9db91cde0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2197%2Fyoichi%2Fimprove-worktree-add-error-message-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2197/yoichi/improve-worktree-add-error-message-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/2197

Range-diff vs v5:

 -:  ---------- > 1:  e3f7d88520 checkout: extract function to display advice for ambiguous remotes
 1:  b838fdabb7 ! 2:  97e99ae31e checkout: improve message for ambiguous remote branch name
     @@ Commit message
          To make the advice more feasible, we will provide matched remote names
          for the specified branch name.
      
     -    To achive that, we add an optional feature to the
     +    To achieve that, we add an optional feature to the
          `unique_tracking_name()` function that allows the matched remote name
          to be exposed to the caller.
      
     @@ builtin/checkout.c: enum checkout_command {
       	CHECKOUT_RESTORE = 3,
       };
       
     -+static void be_explicit(const char *branch,
     -+			enum checkout_command which_command,
     -+			const struct string_list *matched_remote_names)
     -+{
     -+	const char *cmdname;
     +-static void advice_disambiguating_remotes(enum checkout_command which_command)
     ++static void advise_disambiguating_remotes(enum checkout_command which_command,
     ++					  const char *branch,
     ++					  const struct string_list *matched_remote_names)
     + {
     + 	const char *cmdname;
      +	struct string_list_item *item;
     -+
     -+	switch (which_command) {
     -+	case CHECKOUT_CHECKOUT:
     -+		cmdname = "checkout";
     -+		break;
     -+	case CHECKOUT_SWITCH:
     -+		cmdname = "switch";
     -+		break;
     -+	default:
     -+		BUG("command <%d> should not reach parse_remote_branch",
     -+		     which_command);
     -+		break;
     -+	}
     -+
     + 
     + 	switch (which_command) {
     + 	case CHECKOUT_CHECKOUT:
     +@@ builtin/checkout.c: static void advice_disambiguating_remotes(enum checkout_command which_command)
     + 		break;
     + 	}
     + 
     +-	advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
      +	advise(_("Branches with the same name appears in multiple remotes:"));
      +	for_each_string_list_item(item, matched_remote_names) {
      +		advise(_("  %s"), item->string);
      +	}
      +	advise(_("If you meant to check out a remote tracking branch on <remote>,\n"
     -+		 "you can do so by fully qualifying the name with the --track option:\n"
     -+		 "\n"
     + 		 "you can do so by fully qualifying the name with the --track option:\n"
     + 		 "\n"
     +-		 "    git %s --track origin/<name>\n"
      +		 "    git %s --track <remote>/%s\n"
     -+		 "\n"
     + 		 "\n"
     +-		 "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
      +		 "If you'd like to always have checkouts of an ambiguous name prefer\n"
     -+		 "one remote, e.g. the 'origin' remote, consider setting\n"
     -+		 "checkout.defaultRemote=origin in your config."),
     + 		 "one remote, e.g. the 'origin' remote, consider setting\n"
     + 		 "checkout.defaultRemote=origin in your config."),
     +-	       cmdname);
      +	       cmdname, branch);
     -+}
     -+
     + }
     + 
       static char *parse_remote_branch(const char *arg,
     - 				 struct object_id *rev,
     - 				 int could_be_checkout_paths,
     +@@ builtin/checkout.c: static char *parse_remote_branch(const char *arg,
       				 enum checkout_command which_command)
       {
       	int num_matches = 0;
     @@ builtin/checkout.c: enum checkout_command {
       	if (remote && could_be_checkout_paths) {
       		die(_("'%s' could be both a local file and a tracking branch.\n"
      @@ builtin/checkout.c: static char *parse_remote_branch(const char *arg,
     - 	}
       
       	if (!remote && num_matches > 1) {
     --	    if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME)) {
     --		    const char *cmdname;
     --
     --		    switch (which_command) {
     --		    case CHECKOUT_CHECKOUT:
     --			    cmdname = "checkout";
     --			    break;
     --		    case CHECKOUT_SWITCH:
     --			    cmdname = "switch";
     --			    break;
     --		    default:
     --			    BUG("command <%d> should not reach parse_remote_branch",
     --				which_command);
     --			    break;
     --		    }
     --
     --		    advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
     --			     "you can do so by fully qualifying the name with the --track option:\n"
     --			     "\n"
     --			     "    git %s --track origin/<name>\n"
     --			     "\n"
     --			     "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
     --			     "one remote, e.g. the 'origin' remote, consider setting\n"
     --			     "checkout.defaultRemote=origin in your config."),
     --			   cmdname);
     --	    }
     + 		if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
     +-			advice_disambiguating_remotes(which_command);
      -
     --	    die(_("'%s' matched multiple (%d) remote tracking branches"),
     --		arg, num_matches);
     -+		if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
     -+			be_explicit(arg, which_command, &matched_remote_names);
     -+		die(_("'%s' matched multiple (%d) remote tracking branches"),
     -+		    arg, num_matches);
     ++			advise_disambiguating_remotes(which_command, arg,
     ++						      &matched_remote_names);
     + 		die(_("'%s' matched multiple (%d) remote tracking branches"),
     + 		    arg, num_matches);
       	}
       
      +	string_list_clear(&matched_remote_names, 0);
 2:  777862235e ! 3:  dcb84a69a6 worktree add: improve message for ambiguous remote branch name
     @@ builtin/worktree.c: static char *dwim_branch(const char *path, char **new_branch
       	return NULL;
       }
       
     -+static void advise_ambiguous_remote(const char *path, const char *branch,
     -+				    const struct string_list *matched_remote_names)
     ++static void advise_disambiguating_remotes(const char *path, const char *branch,
     ++					  const struct string_list *matched_remote_names)
      +{
      +	struct string_list_item *item;
      +
     @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix,
      +			} else if (num_matches > 1) {
      +				if (!opts.quiet &&
      +				    advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
     -+					advise_ambiguous_remote(path, branch, &matched_remote_names);
     ++					advise_disambiguating_remotes(path, branch,
     ++								      &matched_remote_names);
      +				die(_("'%s' matched multiple (%d) remote tracking branches"),
      +				    branch, num_matches);
       			}

gitgitgadget

From: Yoichi NAKAYAMA <[email protected]>

---
 builtin/checkout.c | 62 ++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

index 55e3a89a85..650eda735f 100644
@@ -1343,6 +1343,34 @@ enum checkout_command {
 	CHECKOUT_RESTORE = 3,
 };
 
+static void advice_disambiguating_remotes(enum checkout_command which_command)
+{
+	const char *cmdname;
+
+	switch (which_command) {
+	case CHECKOUT_CHECKOUT:
+		cmdname = "checkout";
+		break;
+	case CHECKOUT_SWITCH:
+		cmdname = "switch";
+		break;
+	default:
+		BUG("command <%d> should not reach parse_remote_branch",
+		    which_command);
+		break;
+	}
+
+	advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
+		 "you can do so by fully qualifying the name with the --track option:\n"
+		 "\n"
+		 "    git %s --track origin/<name>\n"
+		 "\n"
+		 "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
+		 "one remote, e.g. the 'origin' remote, consider setting\n"
+		 "checkout.defaultRemote=origin in your config."),
+	       cmdname);
+}
+
 static char *parse_remote_branch(const char *arg,
 				 struct object_id *rev,
 				 int could_be_checkout_paths,

@@ -1358,35 +1386,11 @@ static char *parse_remote_branch(const char *arg,
 	}
 
 	if (!remote && num_matches > 1) {
-	    if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME)) {
-		    const char *cmdname;
-
-		    switch (which_command) {
-		    case CHECKOUT_CHECKOUT:
-			    cmdname = "checkout";
-			    break;
-		    case CHECKOUT_SWITCH:
-			    cmdname = "switch";
-			    break;
-		    default:
-			    BUG("command <%d> should not reach parse_remote_branch",
-				which_command);
-			    break;
-		    }
-
-		    advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
-			     "you can do so by fully qualifying the name with the --track option:\n"
-			     "\n"
-			     "    git %s --track origin/<name>\n"
-			     "\n"
-			     "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
-			     "one remote, e.g. the 'origin' remote, consider setting\n"
-			     "checkout.defaultRemote=origin in your config."),
-			   cmdname);
-	    }
-
-	    die(_("'%s' matched multiple (%d) remote tracking branches"),
-		arg, num_matches);
+		if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
+			advice_disambiguating_remotes(which_command);
+
+		die(_("'%s' matched multiple (%d) remote tracking branches"),
+		    arg, num_matches);
 	}
 
 	return remote;

gitgitgadget

From: Yoichi NAKAYAMA <[email protected]>

When the user runs 'git checkout bar-topic' command that does not exactly say which remote they want to work with, and there is no local branch named bar-topic, we try to guess which remote by passing bar-topic then create a new branch named bar-topic which tracks the remote branch.

If multiple remotes have a branch named bar-topic, we cannot determine a single specific remote. Therefore, we provide information that the user can utilize to resolve the issue.

To make the advice more feasible, we will provide matched remote names for the specified branch name.

To achieve that, we add an optional feature to the unique_tracking_name() function that allows the matched remote name to be exposed to the caller.

---
 builtin/checkout.c | 28 ++++++++++++++++++++--------
 builtin/worktree.c |  4 ++--
 checkout.c         | 14 ++++++++++++--
 checkout.h         |  5 ++++-
 4 files changed, 38 insertions(+), 13 deletions(-)

index 650eda735f..35227f7cbd 100644
@@ -1343,9 +1343,12 @@ enum checkout_command {
 	CHECKOUT_RESTORE = 3,
 };
 
-static void advice_disambiguating_remotes(enum checkout_command which_command)
+static void advise_disambiguating_remotes(enum checkout_command which_command,
+					  const char *branch,
+					  const struct string_list *matched_remote_names)
 {
 	const char *cmdname;
+	struct string_list_item *item;
 
 	switch (which_command) {
 	case CHECKOUT_CHECKOUT:

@@ -1360,15 +1363,19 @@ static void advice_disambiguating_remotes(enum checkout_command which_command)
 		break;
 	}
 
-	advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
+	advise(_("Branches with the same name appears in multiple remotes:"));
+	for_each_string_list_item(item, matched_remote_names) {
+		advise(_("  %s"), item->string);
+	}
+	advise(_("If you meant to check out a remote tracking branch on <remote>,\n"
 		 "you can do so by fully qualifying the name with the --track option:\n"
 		 "\n"
-		 "    git %s --track origin/<name>\n"
+		 "    git %s --track <remote>/%s\n"
 		 "\n"
-		 "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
+		 "If you'd like to always have checkouts of an ambiguous name prefer\n"
 		 "one remote, e.g. the 'origin' remote, consider setting\n"
 		 "checkout.defaultRemote=origin in your config."),
-	       cmdname);
+	       cmdname, branch);
 }
 
 static char *parse_remote_branch(const char *arg,

@@ -1377,7 +1384,10 @@ static char *parse_remote_branch(const char *arg,
 				 enum checkout_command which_command)
 {
 	int num_matches = 0;
-	char *remote = unique_tracking_name(arg, rev, &num_matches);
+	struct string_list matched_remote_names = STRING_LIST_INIT_DUP;
+
+	char *remote = unique_tracking_name(arg, rev, &num_matches,
+					    &matched_remote_names);
 
 	if (remote && could_be_checkout_paths) {
 		die(_("'%s' could be both a local file and a tracking branch.\n"

@@ -1387,12 +1397,14 @@ static char *parse_remote_branch(const char *arg,
 
 	if (!remote && num_matches > 1) {
 		if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
-			advice_disambiguating_remotes(which_command);
-
+			advise_disambiguating_remotes(which_command, arg,
+						      &matched_remote_names);
 		die(_("'%s' matched multiple (%d) remote tracking branches"),
 		    arg, num_matches);
 	}
 
+	string_list_clear(&matched_remote_names, 0);
+
 	return remote;
 }
 

index 654d27c3e1..22c8e5e131 100644
@@ -782,7 +782,7 @@ static char *dwim_branch(const char *path, char **new_branch)
 	*new_branch = branchname;
 	if (guess_remote) {
 		struct object_id oid;
-		char *remote = unique_tracking_name(*new_branch, &oid, NULL);
+		char *remote = unique_tracking_name(*new_branch, &oid, NULL, NULL);
 		return remote;
 	}
 	return NULL;

@@ -904,7 +904,7 @@ static int add(int ac, const char **av, const char *prefix,
 
 		commit = lookup_commit_reference_by_name(branch);
 		if (!commit) {
-			remote = unique_tracking_name(branch, &oid, NULL);
+			remote = unique_tracking_name(branch, &oid, NULL, NULL);
 			if (remote) {
 				new_branch = branch;
 				branch = new_branch_to_free = remote;

index 1588b116ee..2806b783ec 100644
@@ -8,6 +8,7 @@
 #include "checkout.h"
 #include "config.h"
 #include "strbuf.h"
+#include "string-list.h"
 
 struct tracking_name_data {
 	/* const */ char *src_ref;

@@ -17,6 +18,7 @@ struct tracking_name_data {
 	const char *default_remote;
 	char *default_dst_ref;
 	struct object_id *default_dst_oid;
+	struct string_list **remote_names;
 };
 
 #define TRACKING_NAME_DATA_INIT { 0 }

@@ -39,6 +41,8 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
 		oidcpy(dst, cb->dst_oid);
 		cb->default_dst_oid = dst;
 	}
+	if (cb->remote_names)
+		string_list_append(*cb->remote_names, remote->name);
 	if (cb->dst_ref) {
 		free(query.dst);
 		return 0;

@@ -48,14 +52,20 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
 }
 
 char *unique_tracking_name(const char *name, struct object_id *oid,
-			   int *dwim_remotes_matched)
+			   int *dwim_remotes_matched,
+			   struct string_list *dwim_remote_names)
 {
 	struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
 	const char *default_remote = NULL;
-	if (!repo_config_get_string_tmp(the_repository, "checkout.defaultremote", &default_remote))
+
+	if (!repo_config_get_string_tmp(the_repository,
+					"checkout.defaultremote",
+					&default_remote))
 		cb_data.default_remote = default_remote;
 	cb_data.src_ref = xstrfmt("refs/heads/%s", name);
 	cb_data.dst_oid = oid;
+	if (dwim_remote_names)
+		cb_data.remote_names = &dwim_remote_names;
 	for_each_remote(check_tracking_name, &cb_data);
 	if (dwim_remotes_matched)
 		*dwim_remotes_matched = cb_data.num_matches;

index 55920e7aeb..0b185a0fc9 100644
@@ -3,6 +3,8 @@
 
 #include "hash.h"
 
+struct string_list;
+
 /*
  * Check if the branch name uniquely matches a branch name on a remote
  * tracking branch.  Return the name of the remote if such a branch

@@ -10,6 +12,7 @@
  */
 char *unique_tracking_name(const char *name,
 			   struct object_id *oid,
-			   int *dwim_remotes_matched);
+			   int *dwim_remotes_matched,
+			   struct string_list *dwim_remote_names);
 
 #endif /* CHECKOUT_H */

gitgitgadget

From: Yoichi NAKAYAMA <[email protected]>

When the user runs 'git worktree add ../foo-dir bar-topic' command that does not exactly say which remote they want to work with, and there is no local branch named bar-topic, we try to guess which remote by passing bar-topic then create a new branch named bar-topic which tracks the remote branch.

If there are multiple remotes that have branch named bar-topic, we silently gave up, leaving the variable 'branch' intact. Then we entered the conditional clause 'if (!opts.orphan && !lookup_commit_reference_by_name(branch))' and triggered "invalid reference" error. This error message did not contain enough information to resolve the issue where the remote could not be guessed.

To improve the situation, we display a hint and a descriptive error message and die immediately when multiple matching branches are found.

---
 builtin/worktree.c      | 35 +++++++++++++++++++++++++++++++++--
 t/t2400-worktree-add.sh |  4 ++--
 2 files changed, 35 insertions(+), 4 deletions(-)

index 22c8e5e131..8286c283e0 100644
@@ -788,6 +788,25 @@ static char *dwim_branch(const char *path, char **new_branch)
 	return NULL;
 }
 
+static void advise_disambiguating_remotes(const char *path, const char *branch,
+					  const struct string_list *matched_remote_names)
+{
+	struct string_list_item *item;
+
+	advise(_("Branches with the same name appears in multiple remotes:"));
+	for_each_string_list_item(item, matched_remote_names) {
+		advise(_("  %s"), item->string);
+	}
+	advise(_("If you meant to create a worktree from a remote tracking branch on\n"
+		 "<remote>, you can do so by:\n"
+		 "\n"
+		 "    git worktree add -b %s %s <remote>/%s\n"
+		 "\n"
+		 "If you'd like to always prefer some remote, e.g. 'origin',\n"
+		 "consider setting checkout.defaultRemote=origin in your config."),
+	       branch, path, branch);
+}
+
 static int add(int ac, const char **av, const char *prefix,
 	       struct repository *repo UNUSED)
 {

@@ -900,15 +919,27 @@ static int add(int ac, const char **av, const char *prefix,
 	} else if (ac == 2) {
 		struct object_id oid;
 		struct commit *commit;
-		char *remote;
 
 		commit = lookup_commit_reference_by_name(branch);
 		if (!commit) {
-			remote = unique_tracking_name(branch, &oid, NULL, NULL);
+			char *remote;
+			int num_matches = 0;
+			struct string_list matched_remote_names = STRING_LIST_INIT_DUP;
+
+			remote = unique_tracking_name(branch, &oid, &num_matches,
+						      &matched_remote_names);
 			if (remote) {
 				new_branch = branch;
 				branch = new_branch_to_free = remote;
+			} else if (num_matches > 1) {
+				if (!opts.quiet &&
+				    advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
+					advise_disambiguating_remotes(path, branch,
+								      &matched_remote_names);
+				die(_("'%s' matched multiple (%d) remote tracking branches"),
+				    branch, num_matches);
 			}
+			string_list_clear(&matched_remote_names, 0);
 		}
 
 		if (!strcmp(branch, "HEAD"))

index 87b926728a..5c105cf252 100755
@@ -624,12 +624,12 @@ test_expect_success '"add" <path> <branch> dwims' '
 test_expect_success '"add" <path> <branch> dwims with checkout.defaultRemote' '
 	test_when_finished rm -rf repo_upstream repo_dwim foo &&
 	setup_remote_repo repo_upstream repo_dwim &&
-	git init repo_dwim &&
 	(
 		cd repo_dwim &&
 		git remote add repo_upstream2 ../repo_upstream &&
 		git fetch repo_upstream2 &&
-		test_must_fail git worktree add ../foo foo &&
+		test_must_fail git worktree add ../foo foo 2>error.actual &&
+		test_grep "matched multiple (2) remote tracking branches" error.actual &&
 		git -c checkout.defaultRemote=repo_upstream worktree add ../foo foo &&
 		git status -uno --porcelain >status.actual &&
 		test_must_be_empty status.actual

gitgitgadget

I overlooked that. Thank you.
Rather than simply matching the verb to the subject, I want to clarify what (as specified by the user) exists on multiple remotes:

    advise(_("Branch name '%s' appears in multiple remotes:"), branch);

In the case of
git worktree add ../topic-branch
invocation, multiple match can occur in dwim_branch() if there is a 'worktree.guessremote=true' config or one specifies '--guess-remote' option.Then it creates a branch named 'topic-branch' from HEAD, and the command exits with success.

My initial patch included a warning and advice here, but now I don't think they are necessary.

Even if multiple remotes match here, the command completes successfully. This could well be the intended behavior (just as when there is no match). In that case, a warning or advice might be superfluous.

From the perspective of offering advice that actually helps the user, since the branch and worktree have already been created, the appropriate guidance would be to suggest deleting them and starting over. That, however, would likely make the message even longer.

If there were an option (which currently doesn't exist) to make the command fail when remote inference fails, then I think it would be appropriate to issue the same advice and error message as in "ac == 2" case.

Thanks,
--
Yoichi NAKAYAMA

In other words, there is no point in calling dwim_branch() from that code path, as the end result is exactly the same whether no remotes match, exactly one remote matches, or two or more remotes match? Would it then make sense to leave a note there to consider later if the dwim_branch() call can be removed?

It is a bit hard to believe that is the intended behavior, but OK. It does not regress the current behavior in any way.

Thanks.

No. The exit codes of the command 'git worktree add ../topic-branch' are the same (== 0). but the results are different.

If there is a unique match found in dwim_branch(), it creates a local branch named topic-branch which tracks <remote>/topic-branch. In case of no match or multiple matches, it creates a local branch named topic-branch from HEAD.

Since Git treats both cases as successful, either can be considered the intended behavior.
(Although, if there are multiple matches, there is a fair chance the result might not be what was intended.)

I am confident that it is appropriate to provide a hint when a command fails, but it is difficult to decide what to do when a command succeeds.

Thanks,

Yoichi NAKAYAMA

'git worktree add ../foo-dir bar-topic' fails to dwim when there are multiple remote branches with name `bar-topic'. But it doesn't display meaningful message as 'git checkout bar-topic' does under the same situation.

We improve this by adding advice and modify the error message for worktree add.

By Junio's suggestion, we include matched remote names in the advice. It is applied to checkout, too.

Changes from the previous patch:

* fix grammatical errors in hints
* narrow the scope of local variable oid

Yoichi NAKAYAMA (3):
checkout: extract function to display advice for ambiguous remotes
checkout: improve message for ambiguous remote branch name worktree add: improve message for ambiguous remote branch name

 builtin/checkout.c      | 76 +++++++++++++++++++++++++----------------
 builtin/worktree.c      | 39 ++++++++++++++++++---
 checkout.c              | 14 ++++++--
 checkout.h              |  5 ++-
 t/t2400-worktree-add.sh |  4 +--
 5 files changed, 99 insertions(+), 39 deletions(-)

base-commit: dea0ea3582e6980ddbc1173cc8e3e9f9db91cde0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2197%2Fyoichi%2Fimprove-worktree-add-error-message-v7
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2197/yoichi/improve-worktree-add-error-message-v7
Pull-Request: https://github.com/gitgitgadget/git/pull/2197

Range-diff vs v6:

 1:  e3f7d88520 = 1:  e3f7d88520 checkout: extract function to display advice for ambiguous remotes
 2:  97e99ae31e ! 2:  89c0f4d303 checkout: improve message for ambiguous remote branch name

@@ Metadata
## Commit message ##
checkout: improve message for ambiguous remote branch name

- When the user runs 'git checkout bar-topic' command that does not
- exactly say which remote they want to work with, and there is no local
- branch named bar-topic, we try to guess which remote by passing
- bar-topic then create a new branch named bar-topic which tracks the
- remote branch.
+ When the user runs 'git checkout bar-topic' without specifying a
+ remote, and there is no local branch named bar-topic, we try to guess
+ which remote branch bar-topic refers to, then create a new branch
+ named bar-topic that tracks the remote branch.

If multiple remotes have a branch named bar-topic, we cannot determine
- a single specific remote. Therefore, we provide information that the
- user can utilize to resolve the issue.
+ a single remote.

- To make the advice more feasible, we will provide matched remote names
- for the specified branch name.
+ To make it easier to resolve the ambiguity, provide the names of the
+ matching remotes for the specified branch name.

- To achieve that, we add an optional feature to the
- unique_tracking_name() function that allows the matched remote name
- to be exposed to the caller.
+ To achieve that, add an optional feature to the
+ unique_tracking_name() function that allows the matching remote
+ names to be exposed to the caller.

@@ builtin/checkout.c: static void advice_disambiguating_remotes(enum checkout_comm
}

- advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
-+ advise(_("Branches with the same name appears in multiple remotes:"));
++ advise(_("Branch name '%s' appears in multiple remotes:"), branch);
+ for_each_string_list_item(item, matched_remote_names) {
+ advise(_(" %s"), item->string);
+ }
3: dcb84a69a6 ! 3: 095a5796d4 worktree add: improve message for ambiguous remote branch name
@@ Metadata
## Commit message ##
worktree add: improve message for ambiguous remote branch name

- When the user runs 'git worktree add ../foo-dir bar-topic' command
- that does not exactly say which remote they want to work with, and
- there is no local branch named bar-topic, we try to guess which remote
- by passing bar-topic then create a new branch named bar-topic which
- tracks the remote branch.
+ When the user runs 'git worktree add ../foo-dir bar-topic' without
+ specifying a remote, and there is no local branch named bar-topic, we
+ try to guess which remote branch bar-topic refers to, then create a
+ new branch named bar-topic that tracks the remote branch.

- If there are multiple remotes that have branch named bar-topic, we
- silently gave up, leaving the variable 'branch' intact. Then we
- entered the conditional clause 'if (!opts.orphan &&
- !lookup_commit_reference_by_name(branch))' and triggered "invalid
- reference" error. This error message did not contain enough
- information to resolve the issue where the remote could not be
- guessed.
+ If multiple remotes have a branch named bar-topic, we silently gave
+ up, leaving the variable 'branch' intact. We then entered the
+ conditional clause 'if (!opts.orphan &&
+ !lookup_commit_reference_by_name(branch))' and triggered an "invalid
+ reference" error. This error message did not provide enough
+ information to resolve the ambiguity.

- To improve the situation, we display a hint and a descriptive error
- message and die immediately when multiple matching branches are found.
+ When multiple matching branches are found, display a hint and a
+ descriptive error message and die immediately.

@@ builtin/worktree.c: static char *dwim_branch(const char *path, char **new_branch
+{
+ struct string_list_item *item;
+
-+ advise(_("Branches with the same name appears in multiple remotes:"));
++ advise(_("Branch name '%s' appears in multiple remotes:"), branch);
+ for_each_string_list_item(item, matched_remote_names) {
+ advise(_(" %s"), item->string);
+ }
@@ builtin/worktree.c: static char *dwim_branch(const char *path, char **new_branch
struct repository *repo UNUSED)
{
@@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix,
+ /* DWIM: Infer --orphan when repo has no refs. */
+ opts.orphan = (!s) && dwim_orphan(&opts, !!opt_track, 1);
} else if (ac == 2) {
- struct object_id oid;
+- struct object_id oid;
struct commit *commit;
- char *remote;

commit = lookup_commit_reference_by_name(branch); if (!commit) {
- remote = unique_tracking_name(branch, &oid, NULL, NULL);
++ struct object_id oid;
+ char *remote;
+ int num_matches = 0;
+ struct string_list matched_remote_names = STRING_LIST_INIT_DUP;

gitgitgadget

From: Yoichi NAKAYAMA <[email protected]>

---
 builtin/checkout.c | 62 ++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

index 55e3a89a85..650eda735f 100644
@@ -1343,6 +1343,34 @@ enum checkout_command {
 	CHECKOUT_RESTORE = 3,
 };
 
+static void advice_disambiguating_remotes(enum checkout_command which_command)
+{
+	const char *cmdname;
+
+	switch (which_command) {
+	case CHECKOUT_CHECKOUT:
+		cmdname = "checkout";
+		break;
+	case CHECKOUT_SWITCH:
+		cmdname = "switch";
+		break;
+	default:
+		BUG("command <%d> should not reach parse_remote_branch",
+		    which_command);
+		break;
+	}
+
+	advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
+		 "you can do so by fully qualifying the name with the --track option:\n"
+		 "\n"
+		 "    git %s --track origin/<name>\n"
+		 "\n"
+		 "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
+		 "one remote, e.g. the 'origin' remote, consider setting\n"
+		 "checkout.defaultRemote=origin in your config."),
+	       cmdname);
+}
+
 static char *parse_remote_branch(const char *arg,
 				 struct object_id *rev,
 				 int could_be_checkout_paths,

@@ -1358,35 +1386,11 @@ static char *parse_remote_branch(const char *arg,
 	}
 
 	if (!remote && num_matches > 1) {
-	    if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME)) {
-		    const char *cmdname;
-
-		    switch (which_command) {
-		    case CHECKOUT_CHECKOUT:
-			    cmdname = "checkout";
-			    break;
-		    case CHECKOUT_SWITCH:
-			    cmdname = "switch";
-			    break;
-		    default:
-			    BUG("command <%d> should not reach parse_remote_branch",
-				which_command);
-			    break;
-		    }
-
-		    advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
-			     "you can do so by fully qualifying the name with the --track option:\n"
-			     "\n"
-			     "    git %s --track origin/<name>\n"
-			     "\n"
-			     "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
-			     "one remote, e.g. the 'origin' remote, consider setting\n"
-			     "checkout.defaultRemote=origin in your config."),
-			   cmdname);
-	    }
-
-	    die(_("'%s' matched multiple (%d) remote tracking branches"),
-		arg, num_matches);
+		if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
+			advice_disambiguating_remotes(which_command);
+
+		die(_("'%s' matched multiple (%d) remote tracking branches"),
+		    arg, num_matches);
 	}
 
 	return remote;

gitgitgadget

From: Yoichi NAKAYAMA <[email protected]>

When the user runs 'git checkout bar-topic' without specifying a remote, and there is no local branch named bar-topic, we try to guess which remote branch bar-topic refers to, then create a new branch named bar-topic that tracks the remote branch.

If multiple remotes have a branch named bar-topic, we cannot determine a single remote.

To make it easier to resolve the ambiguity, provide the names of the matching remotes for the specified branch name.

To achieve that, add an optional feature to the unique_tracking_name() function that allows the matching remote names to be exposed to the caller.

---
 builtin/checkout.c | 28 ++++++++++++++++++++--------
 builtin/worktree.c |  4 ++--
 checkout.c         | 14 ++++++++++++--
 checkout.h         |  5 ++++-
 4 files changed, 38 insertions(+), 13 deletions(-)

index 650eda735f..07373615d8 100644
@@ -1343,9 +1343,12 @@ enum checkout_command {
 	CHECKOUT_RESTORE = 3,
 };
 
-static void advice_disambiguating_remotes(enum checkout_command which_command)
+static void advise_disambiguating_remotes(enum checkout_command which_command,
+					  const char *branch,
+					  const struct string_list *matched_remote_names)
 {
 	const char *cmdname;
+	struct string_list_item *item;
 
 	switch (which_command) {
 	case CHECKOUT_CHECKOUT:

@@ -1360,15 +1363,19 @@ static void advice_disambiguating_remotes(enum checkout_command which_command)
 		break;
 	}
 
-	advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
+	advise(_("Branch name '%s' appears in multiple remotes:"), branch);
+	for_each_string_list_item(item, matched_remote_names) {
+		advise(_("  %s"), item->string);
+	}
+	advise(_("If you meant to check out a remote tracking branch on <remote>,\n"
 		 "you can do so by fully qualifying the name with the --track option:\n"
 		 "\n"
-		 "    git %s --track origin/<name>\n"
+		 "    git %s --track <remote>/%s\n"
 		 "\n"
-		 "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
+		 "If you'd like to always have checkouts of an ambiguous name prefer\n"
 		 "one remote, e.g. the 'origin' remote, consider setting\n"
 		 "checkout.defaultRemote=origin in your config."),
-	       cmdname);
+	       cmdname, branch);
 }
 
 static char *parse_remote_branch(const char *arg,

@@ -1377,7 +1384,10 @@ static char *parse_remote_branch(const char *arg,
 				 enum checkout_command which_command)
 {
 	int num_matches = 0;
-	char *remote = unique_tracking_name(arg, rev, &num_matches);
+	struct string_list matched_remote_names = STRING_LIST_INIT_DUP;
+
+	char *remote = unique_tracking_name(arg, rev, &num_matches,
+					    &matched_remote_names);
 
 	if (remote && could_be_checkout_paths) {
 		die(_("'%s' could be both a local file and a tracking branch.\n"

@@ -1387,12 +1397,14 @@ static char *parse_remote_branch(const char *arg,
 
 	if (!remote && num_matches > 1) {
 		if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
-			advice_disambiguating_remotes(which_command);
-
+			advise_disambiguating_remotes(which_command, arg,
+						      &matched_remote_names);
 		die(_("'%s' matched multiple (%d) remote tracking branches"),
 		    arg, num_matches);
 	}
 
+	string_list_clear(&matched_remote_names, 0);
+
 	return remote;
 }
 

index 654d27c3e1..22c8e5e131 100644
@@ -782,7 +782,7 @@ static char *dwim_branch(const char *path, char **new_branch)
 	*new_branch = branchname;
 	if (guess_remote) {
 		struct object_id oid;
-		char *remote = unique_tracking_name(*new_branch, &oid, NULL);
+		char *remote = unique_tracking_name(*new_branch, &oid, NULL, NULL);
 		return remote;
 	}
 	return NULL;

@@ -904,7 +904,7 @@ static int add(int ac, const char **av, const char *prefix,
 
 		commit = lookup_commit_reference_by_name(branch);
 		if (!commit) {
-			remote = unique_tracking_name(branch, &oid, NULL);
+			remote = unique_tracking_name(branch, &oid, NULL, NULL);
 			if (remote) {
 				new_branch = branch;
 				branch = new_branch_to_free = remote;

index 1588b116ee..2806b783ec 100644
@@ -8,6 +8,7 @@
 #include "checkout.h"
 #include "config.h"
 #include "strbuf.h"
+#include "string-list.h"
 
 struct tracking_name_data {
 	/* const */ char *src_ref;

@@ -17,6 +18,7 @@ struct tracking_name_data {
 	const char *default_remote;
 	char *default_dst_ref;
 	struct object_id *default_dst_oid;
+	struct string_list **remote_names;
 };
 
 #define TRACKING_NAME_DATA_INIT { 0 }

@@ -39,6 +41,8 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
 		oidcpy(dst, cb->dst_oid);
 		cb->default_dst_oid = dst;
 	}
+	if (cb->remote_names)
+		string_list_append(*cb->remote_names, remote->name);
 	if (cb->dst_ref) {
 		free(query.dst);
 		return 0;

@@ -48,14 +52,20 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
 }
 
 char *unique_tracking_name(const char *name, struct object_id *oid,
-			   int *dwim_remotes_matched)
+			   int *dwim_remotes_matched,
+			   struct string_list *dwim_remote_names)
 {
 	struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
 	const char *default_remote = NULL;
-	if (!repo_config_get_string_tmp(the_repository, "checkout.defaultremote", &default_remote))
+
+	if (!repo_config_get_string_tmp(the_repository,
+					"checkout.defaultremote",
+					&default_remote))
 		cb_data.default_remote = default_remote;
 	cb_data.src_ref = xstrfmt("refs/heads/%s", name);
 	cb_data.dst_oid = oid;
+	if (dwim_remote_names)
+		cb_data.remote_names = &dwim_remote_names;
 	for_each_remote(check_tracking_name, &cb_data);
 	if (dwim_remotes_matched)
 		*dwim_remotes_matched = cb_data.num_matches;

index 55920e7aeb..0b185a0fc9 100644
@@ -3,6 +3,8 @@
 
 #include "hash.h"
 
+struct string_list;
+
 /*
  * Check if the branch name uniquely matches a branch name on a remote
  * tracking branch.  Return the name of the remote if such a branch

@@ -10,6 +12,7 @@
  */
 char *unique_tracking_name(const char *name,
 			   struct object_id *oid,
-			   int *dwim_remotes_matched);
+			   int *dwim_remotes_matched,
+			   struct string_list *dwim_remote_names);
 
 #endif /* CHECKOUT_H */

gitgitgadget

From: Yoichi NAKAYAMA <[email protected]>

When the user runs 'git worktree add ../foo-dir bar-topic' without specifying a remote, and there is no local branch named bar-topic, we try to guess which remote branch bar-topic refers to, then create a new branch named bar-topic that tracks the remote branch.

If multiple remotes have a branch named bar-topic, we silently gave up, leaving the variable 'branch' intact. We then entered the conditional clause 'if (!opts.orphan && !lookup_commit_reference_by_name(branch))' and triggered an "invalid reference" error. This error message did not provide enough information to resolve the ambiguity.

When multiple matching branches are found, display a hint and a descriptive error message and die immediately.

---
 builtin/worktree.c      | 37 ++++++++++++++++++++++++++++++++++---
 t/t2400-worktree-add.sh |  4 ++--
 2 files changed, 36 insertions(+), 5 deletions(-)

index 22c8e5e131..320b18873d 100644
@@ -788,6 +788,25 @@ static char *dwim_branch(const char *path, char **new_branch)
 	return NULL;
 }
 
+static void advise_disambiguating_remotes(const char *path, const char *branch,
+					  const struct string_list *matched_remote_names)
+{
+	struct string_list_item *item;
+
+	advise(_("Branch name '%s' appears in multiple remotes:"), branch);
+	for_each_string_list_item(item, matched_remote_names) {
+		advise(_("  %s"), item->string);
+	}
+	advise(_("If you meant to create a worktree from a remote tracking branch on\n"
+		 "<remote>, you can do so by:\n"
+		 "\n"
+		 "    git worktree add -b %s %s <remote>/%s\n"
+		 "\n"
+		 "If you'd like to always prefer some remote, e.g. 'origin',\n"
+		 "consider setting checkout.defaultRemote=origin in your config."),
+	       branch, path, branch);
+}
+
 static int add(int ac, const char **av, const char *prefix,
 	       struct repository *repo UNUSED)
 {

@@ -898,17 +917,29 @@ static int add(int ac, const char **av, const char *prefix,
 		/* DWIM: Infer --orphan when repo has no refs. */
 		opts.orphan = (!s) && dwim_orphan(&opts, !!opt_track, 1);
 	} else if (ac == 2) {
-		struct object_id oid;
 		struct commit *commit;
-		char *remote;
 
 		commit = lookup_commit_reference_by_name(branch);
 		if (!commit) {
-			remote = unique_tracking_name(branch, &oid, NULL, NULL);
+			struct object_id oid;
+			char *remote;
+			int num_matches = 0;
+			struct string_list matched_remote_names = STRING_LIST_INIT_DUP;
+
+			remote = unique_tracking_name(branch, &oid, &num_matches,
+						      &matched_remote_names);
 			if (remote) {
 				new_branch = branch;
 				branch = new_branch_to_free = remote;
+			} else if (num_matches > 1) {
+				if (!opts.quiet &&
+				    advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
+					advise_disambiguating_remotes(path, branch,
+								      &matched_remote_names);
+				die(_("'%s' matched multiple (%d) remote tracking branches"),
+				    branch, num_matches);
 			}
+			string_list_clear(&matched_remote_names, 0);
 		}
 
 		if (!strcmp(branch, "HEAD"))

index 87b926728a..5c105cf252 100755
@@ -624,12 +624,12 @@ test_expect_success '"add" <path> <branch> dwims' '
 test_expect_success '"add" <path> <branch> dwims with checkout.defaultRemote' '
 	test_when_finished rm -rf repo_upstream repo_dwim foo &&
 	setup_remote_repo repo_upstream repo_dwim &&
-	git init repo_dwim &&
 	(
 		cd repo_dwim &&
 		git remote add repo_upstream2 ../repo_upstream &&
 		git fetch repo_upstream2 &&
-		test_must_fail git worktree add ../foo foo &&
+		test_must_fail git worktree add ../foo foo 2>error.actual &&
+		test_grep "matched multiple (2) remote tracking branches" error.actual &&
 		git -c checkout.defaultRemote=repo_upstream worktree add ../foo foo &&
 		git status -uno --porcelain >status.actual &&
 		test_must_be_empty status.actual

gitgitgadget

I actually think it falls into the same class of bug you are fixing in this topic, which was caused by not considering the possibility that there can be any case other than 0-match and 1-match, and not thinking through the ramifications of treating 2-match and 0-match the same way.

It is of course OK to fix one bug and leave the other one unaddressed, to be fixed in a later follow-up effort.

The rest of this message is only for those who will tackle the "later follow-up effort" part after the dust settles once the current topic lands (aka #leftoverbits).

In the beginning, before Thomas Gummerer started his topic in November 2017 [*1*], 'git worktree add <path> [<branch>]' created a new branch from the checked-out HEAD, without looking at any remote.

- 'git worktree add <path> <branch>' before Thomas's effort errored out if <branch> did not exist. It was safe to add DWIM from remote-tracking branches without requiring any option.

- 'git worktree add <path>' used to create a new branch whose name is derived from basename(path) that points at the current HEAD, without erroring out. Enabling DWIM from remote-tracking branches unconditionally would have meant a silent behavior change. So DWIM was added to this case to require the '--guess-remote' option to enable [*2*].

Back then, unique_tracking_name() did not let the callers distinguish between 0-match and multiple-match cases, so when you had multiple matches, 'git worktree add <path> [<branch>]' triggered the same code path as 0-matches. When the DWIM feature was designed, handling the multiple-match case correctly was on nobody's radar.

Even when Ævar Arnfjörð Bjarmason updated unique_tracking_name() in 3c87aa946a (checkout: pass the "num_matches" up to callers, 2018-06-05), in a topic that ends at 8d7b558bae (checkout &
worktree: introduce checkout.defaultRemote, 2018-06-05), to allow callers to distinguish between 0-match and ambiguous multi-match cases, this work unfortunately concentrated on improving "git checkout", and callers of unique_tracking_name() in "git worktree" were updated to pass NULL, i.e., teaching them to count how many matches they got was postponed.

We know that the update to unique_tracking_name() in this work back then was not complete on the "git worktree" side. After all, that is how this topic arose to fix one of the two code paths that call the function so that we react differently between 0-match and multiple-match cases.

Now that we are aware of the issue, I think the code should error out, instead of creating the new branch out of HEAD, when there are multiple remotes with the name of the branch. In other words, the existing code that behaves the same way in 0-match and 2-match cases is buggy, and we should eventually fix it.

[Footnotes]

*1* https://lore.kernel.org/git/[email protected]/
*2* https://lore.kernel.org/git/[email protected]/

Both changes look sensible. I very much like the new advise() message that is much more concise.

Will queue.

Thank you for the analysis. I believe the behavior of treating multiple matches as an error is appropriate.

I feel that now is the time to implement the fix that had been postponed. I'll make another commit for it.

Thanks,

Yoichi NAKAYAMA

'git worktree add ../foo-dir bar-topic' fails to dwim when there are multiple remote branches with name `bar-topic'. But it doesn't display meaningful message as 'git checkout bar-topic' does under the same situation.

We improve this by adding advice and modify the error message for worktree add.

By Junio's suggestion, we include matched remote names in the advice. It is applied to checkout, too.

We also fix the behavior of --guess-remote when there are multiple matches.

Changes from the previous patch:

* add another commit to fix the behavior of --guess-remote

Yoichi NAKAYAMA (4):
checkout: extract function to display advice for ambiguous remotes
checkout: improve message for ambiguous remote branch name worktree add: improve message for ambiguous remote branch name worktree add: treat multiple matches with --guess-remote as an error

 Documentation/config/worktree.adoc |  5 +-
 Documentation/git-worktree.adoc    |  4 +-
 builtin/checkout.c                 | 76 ++++++++++++++++++------------
 builtin/worktree.c                 | 57 +++++++++++++++++++---
 checkout.c                         | 14 +++++-
 checkout.h                         |  5 +-
 t/t2400-worktree-add.sh            | 17 ++++++-
 7 files changed, 134 insertions(+), 44 deletions(-)

base-commit: dea0ea3582e6980ddbc1173cc8e3e9f9db91cde0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2197%2Fyoichi%2Fimprove-worktree-add-error-message-v8
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2197/yoichi/improve-worktree-add-error-message-v8
Pull-Request: https://github.com/gitgitgadget/git/pull/2197

Range-diff vs v7:

 1:  e3f7d88520 = 1:  e3f7d88520 checkout: extract function to display advice for ambiguous remotes
 2:  89c0f4d303 = 2:  89c0f4d303 checkout: improve message for ambiguous remote branch name
 3:  095a5796d4 = 3:  095a5796d4 worktree add: improve message for ambiguous remote branch name
 -:  ---------- > 4:  927856e0a0 worktree add: treat multiple matches with --guess-remote as an error

gitgitgadget

From: Yoichi NAKAYAMA <[email protected]>

---
 builtin/checkout.c | 62 ++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

index 55e3a89a85..650eda735f 100644
@@ -1343,6 +1343,34 @@ enum checkout_command {
 	CHECKOUT_RESTORE = 3,
 };
 
+static void advice_disambiguating_remotes(enum checkout_command which_command)
+{
+	const char *cmdname;
+
+	switch (which_command) {
+	case CHECKOUT_CHECKOUT:
+		cmdname = "checkout";
+		break;
+	case CHECKOUT_SWITCH:
+		cmdname = "switch";
+		break;
+	default:
+		BUG("command <%d> should not reach parse_remote_branch",
+		    which_command);
+		break;
+	}
+
+	advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
+		 "you can do so by fully qualifying the name with the --track option:\n"
+		 "\n"
+		 "    git %s --track origin/<name>\n"
+		 "\n"
+		 "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
+		 "one remote, e.g. the 'origin' remote, consider setting\n"
+		 "checkout.defaultRemote=origin in your config."),
+	       cmdname);
+}
+
 static char *parse_remote_branch(const char *arg,
 				 struct object_id *rev,
 				 int could_be_checkout_paths,

@@ -1358,35 +1386,11 @@ static char *parse_remote_branch(const char *arg,
 	}
 
 	if (!remote && num_matches > 1) {
-	    if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME)) {
-		    const char *cmdname;
-
-		    switch (which_command) {
-		    case CHECKOUT_CHECKOUT:
-			    cmdname = "checkout";
-			    break;
-		    case CHECKOUT_SWITCH:
-			    cmdname = "switch";
-			    break;
-		    default:
-			    BUG("command <%d> should not reach parse_remote_branch",
-				which_command);
-			    break;
-		    }
-
-		    advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
-			     "you can do so by fully qualifying the name with the --track option:\n"
-			     "\n"
-			     "    git %s --track origin/<name>\n"
-			     "\n"
-			     "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
-			     "one remote, e.g. the 'origin' remote, consider setting\n"
-			     "checkout.defaultRemote=origin in your config."),
-			   cmdname);
-	    }
-
-	    die(_("'%s' matched multiple (%d) remote tracking branches"),
-		arg, num_matches);
+		if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
+			advice_disambiguating_remotes(which_command);
+
+		die(_("'%s' matched multiple (%d) remote tracking branches"),
+		    arg, num_matches);
 	}
 
 	return remote;

gitgitgadget

From: Yoichi NAKAYAMA <[email protected]>

When the user runs 'git checkout bar-topic' without specifying a remote, and there is no local branch named bar-topic, we try to guess which remote branch bar-topic refers to, then create a new branch named bar-topic that tracks the remote branch.

If multiple remotes have a branch named bar-topic, we cannot determine a single remote.

To make it easier to resolve the ambiguity, provide the names of the matching remotes for the specified branch name.

To achieve that, add an optional feature to the unique_tracking_name() function that allows the matching remote names to be exposed to the caller.

---
 builtin/checkout.c | 28 ++++++++++++++++++++--------
 builtin/worktree.c |  4 ++--
 checkout.c         | 14 ++++++++++++--
 checkout.h         |  5 ++++-
 4 files changed, 38 insertions(+), 13 deletions(-)

index 650eda735f..07373615d8 100644
@@ -1343,9 +1343,12 @@ enum checkout_command {
 	CHECKOUT_RESTORE = 3,
 };
 
-static void advice_disambiguating_remotes(enum checkout_command which_command)
+static void advise_disambiguating_remotes(enum checkout_command which_command,
+					  const char *branch,
+					  const struct string_list *matched_remote_names)
 {
 	const char *cmdname;
+	struct string_list_item *item;
 
 	switch (which_command) {
 	case CHECKOUT_CHECKOUT:

@@ -1360,15 +1363,19 @@ static void advice_disambiguating_remotes(enum checkout_command which_command)
 		break;
 	}
 
-	advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
+	advise(_("Branch name '%s' appears in multiple remotes:"), branch);
+	for_each_string_list_item(item, matched_remote_names) {
+		advise(_("  %s"), item->string);
+	}
+	advise(_("If you meant to check out a remote tracking branch on <remote>,\n"
 		 "you can do so by fully qualifying the name with the --track option:\n"
 		 "\n"
-		 "    git %s --track origin/<name>\n"
+		 "    git %s --track <remote>/%s\n"
 		 "\n"
-		 "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
+		 "If you'd like to always have checkouts of an ambiguous name prefer\n"
 		 "one remote, e.g. the 'origin' remote, consider setting\n"
 		 "checkout.defaultRemote=origin in your config."),
-	       cmdname);
+	       cmdname, branch);
 }
 
 static char *parse_remote_branch(const char *arg,

@@ -1377,7 +1384,10 @@ static char *parse_remote_branch(const char *arg,
 				 enum checkout_command which_command)
 {
 	int num_matches = 0;
-	char *remote = unique_tracking_name(arg, rev, &num_matches);
+	struct string_list matched_remote_names = STRING_LIST_INIT_DUP;
+
+	char *remote = unique_tracking_name(arg, rev, &num_matches,
+					    &matched_remote_names);
 
 	if (remote && could_be_checkout_paths) {
 		die(_("'%s' could be both a local file and a tracking branch.\n"

@@ -1387,12 +1397,14 @@ static char *parse_remote_branch(const char *arg,
 
 	if (!remote && num_matches > 1) {
 		if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
-			advice_disambiguating_remotes(which_command);
-
+			advise_disambiguating_remotes(which_command, arg,
+						      &matched_remote_names);
 		die(_("'%s' matched multiple (%d) remote tracking branches"),
 		    arg, num_matches);
 	}
 
+	string_list_clear(&matched_remote_names, 0);
+
 	return remote;
 }
 

index 654d27c3e1..22c8e5e131 100644
@@ -782,7 +782,7 @@ static char *dwim_branch(const char *path, char **new_branch)
 	*new_branch = branchname;
 	if (guess_remote) {
 		struct object_id oid;
-		char *remote = unique_tracking_name(*new_branch, &oid, NULL);
+		char *remote = unique_tracking_name(*new_branch, &oid, NULL, NULL);
 		return remote;
 	}
 	return NULL;

@@ -904,7 +904,7 @@ static int add(int ac, const char **av, const char *prefix,
 
 		commit = lookup_commit_reference_by_name(branch);
 		if (!commit) {
-			remote = unique_tracking_name(branch, &oid, NULL);
+			remote = unique_tracking_name(branch, &oid, NULL, NULL);
 			if (remote) {
 				new_branch = branch;
 				branch = new_branch_to_free = remote;

index 1588b116ee..2806b783ec 100644
@@ -8,6 +8,7 @@
 #include "checkout.h"
 #include "config.h"
 #include "strbuf.h"
+#include "string-list.h"
 
 struct tracking_name_data {
 	/* const */ char *src_ref;

@@ -17,6 +18,7 @@ struct tracking_name_data {
 	const char *default_remote;
 	char *default_dst_ref;
 	struct object_id *default_dst_oid;
+	struct string_list **remote_names;
 };
 
 #define TRACKING_NAME_DATA_INIT { 0 }

@@ -39,6 +41,8 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
 		oidcpy(dst, cb->dst_oid);
 		cb->default_dst_oid = dst;
 	}
+	if (cb->remote_names)
+		string_list_append(*cb->remote_names, remote->name);
 	if (cb->dst_ref) {
 		free(query.dst);
 		return 0;

@@ -48,14 +52,20 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
 }
 
 char *unique_tracking_name(const char *name, struct object_id *oid,
-			   int *dwim_remotes_matched)
+			   int *dwim_remotes_matched,
+			   struct string_list *dwim_remote_names)
 {
 	struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
 	const char *default_remote = NULL;
-	if (!repo_config_get_string_tmp(the_repository, "checkout.defaultremote", &default_remote))
+
+	if (!repo_config_get_string_tmp(the_repository,
+					"checkout.defaultremote",
+					&default_remote))
 		cb_data.default_remote = default_remote;
 	cb_data.src_ref = xstrfmt("refs/heads/%s", name);
 	cb_data.dst_oid = oid;
+	if (dwim_remote_names)
+		cb_data.remote_names = &dwim_remote_names;
 	for_each_remote(check_tracking_name, &cb_data);
 	if (dwim_remotes_matched)
 		*dwim_remotes_matched = cb_data.num_matches;

index 55920e7aeb..0b185a0fc9 100644
@@ -3,6 +3,8 @@
 
 #include "hash.h"
 
+struct string_list;
+
 /*
  * Check if the branch name uniquely matches a branch name on a remote
  * tracking branch.  Return the name of the remote if such a branch

@@ -10,6 +12,7 @@
  */
 char *unique_tracking_name(const char *name,
 			   struct object_id *oid,
-			   int *dwim_remotes_matched);
+			   int *dwim_remotes_matched,
+			   struct string_list *dwim_remote_names);
 
 #endif /* CHECKOUT_H */

gitgitgadget

From: Yoichi NAKAYAMA <[email protected]>

When the user runs 'git worktree add ../foo-dir bar-topic' without specifying a remote, and there is no local branch named bar-topic, we try to guess which remote branch bar-topic refers to, then create a new branch named bar-topic that tracks the remote branch.

If multiple remotes have a branch named bar-topic, we silently gave up, leaving the variable 'branch' intact. We then entered the conditional clause 'if (!opts.orphan && !lookup_commit_reference_by_name(branch))' and triggered an "invalid reference" error. This error message did not provide enough information to resolve the ambiguity.

When multiple matching branches are found, display a hint and a descriptive error message and die immediately.

---
 builtin/worktree.c      | 37 ++++++++++++++++++++++++++++++++++---
 t/t2400-worktree-add.sh |  4 ++--
 2 files changed, 36 insertions(+), 5 deletions(-)

index 22c8e5e131..320b18873d 100644
@@ -788,6 +788,25 @@ static char *dwim_branch(const char *path, char **new_branch)
 	return NULL;
 }
 
+static void advise_disambiguating_remotes(const char *path, const char *branch,
+					  const struct string_list *matched_remote_names)
+{
+	struct string_list_item *item;
+
+	advise(_("Branch name '%s' appears in multiple remotes:"), branch);
+	for_each_string_list_item(item, matched_remote_names) {
+		advise(_("  %s"), item->string);
+	}
+	advise(_("If you meant to create a worktree from a remote tracking branch on\n"
+		 "<remote>, you can do so by:\n"
+		 "\n"
+		 "    git worktree add -b %s %s <remote>/%s\n"
+		 "\n"
+		 "If you'd like to always prefer some remote, e.g. 'origin',\n"
+		 "consider setting checkout.defaultRemote=origin in your config."),
+	       branch, path, branch);
+}
+
 static int add(int ac, const char **av, const char *prefix,
 	       struct repository *repo UNUSED)
 {

@@ -898,17 +917,29 @@ static int add(int ac, const char **av, const char *prefix,
 		/* DWIM: Infer --orphan when repo has no refs. */
 		opts.orphan = (!s) && dwim_orphan(&opts, !!opt_track, 1);
 	} else if (ac == 2) {
-		struct object_id oid;
 		struct commit *commit;
-		char *remote;
 
 		commit = lookup_commit_reference_by_name(branch);
 		if (!commit) {
-			remote = unique_tracking_name(branch, &oid, NULL, NULL);
+			struct object_id oid;
+			char *remote;
+			int num_matches = 0;
+			struct string_list matched_remote_names = STRING_LIST_INIT_DUP;
+
+			remote = unique_tracking_name(branch, &oid, &num_matches,
+						      &matched_remote_names);
 			if (remote) {
 				new_branch = branch;
 				branch = new_branch_to_free = remote;
+			} else if (num_matches > 1) {
+				if (!opts.quiet &&
+				    advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
+					advise_disambiguating_remotes(path, branch,
+								      &matched_remote_names);
+				die(_("'%s' matched multiple (%d) remote tracking branches"),
+				    branch, num_matches);
 			}
+			string_list_clear(&matched_remote_names, 0);
 		}
 
 		if (!strcmp(branch, "HEAD"))

index 87b926728a..5c105cf252 100755
@@ -624,12 +624,12 @@ test_expect_success '"add" <path> <branch> dwims' '
 test_expect_success '"add" <path> <branch> dwims with checkout.defaultRemote' '
 	test_when_finished rm -rf repo_upstream repo_dwim foo &&
 	setup_remote_repo repo_upstream repo_dwim &&
-	git init repo_dwim &&
 	(
 		cd repo_dwim &&
 		git remote add repo_upstream2 ../repo_upstream &&
 		git fetch repo_upstream2 &&
-		test_must_fail git worktree add ../foo foo &&
+		test_must_fail git worktree add ../foo foo 2>error.actual &&
+		test_grep "matched multiple (2) remote tracking branches" error.actual &&
 		git -c checkout.defaultRemote=repo_upstream worktree add ../foo foo &&
 		git status -uno --porcelain >status.actual &&
 		test_must_be_empty status.actual

gitgitgadget

From: Yoichi NAKAYAMA <[email protected]>

When 'git worktree add <path>' is invoked without <commit-ish> and with the --guess-remote option (or when worktree.guessRemote is set to true), it tries to find a remote-tracking branch matching the basename of <path>.

Currently, the behavior when multiple matches are found is the same as when no match is found: it falls back to creating a branch from HEAD. This has been the behavior since 71d6682d8c (worktree: add --guess-remote option to add subcommand, 2017-11-29), when the option was first introduced.

However, if the specified <path> matches any remote-tracking branch, we infer that the user intended to use one of the remote-tracking branches as the start-point rather than HEAD. So we abort the creation of the branch and worktree when there are multiple matches, and instruct the user to choose the start-point.

---
 Documentation/config/worktree.adoc |  5 +--
 Documentation/git-worktree.adoc    |  4 ++-
 builtin/worktree.c                 | 58 ++++++++++++++++++------------
 t/t2400-worktree-add.sh            | 13 +++++++
 4 files changed, 55 insertions(+), 25 deletions(-)

index a248076ea5..0930183b91 100644
@@ -5,8 +5,9 @@
 	set to true, `worktree add` tries to find a remote-tracking
 	branch whose name uniquely matches the new branch name.  If
 	such a branch exists, it is checked out and set as "upstream"
-	for the new branch.  If no such match can be found, it falls
-	back to creating a new branch from the current `HEAD`.
+	for the new branch.  If multiple matches are found, the command
+	fails.  If no such match can be found, it falls back to
+	creating a new branch from the current `HEAD`.
 
 `worktree.useRelativePaths`::
 	Link worktrees using relative paths (when "`true`") or absolute

index fbf8426cd9..32787eacc3 100644
@@ -219,7 +219,9 @@ To remove a locked worktree, specify `--force` twice.
 	of creating a new branch from `HEAD`, if there exists a tracking
 	branch in exactly one remote matching the basename of _<path>_,
 	base the new branch on the remote-tracking branch, and mark
-	the remote-tracking branch as "upstream" from the new branch.
+	the remote-tracking branch as "upstream" from the new branch. If
+	there are multiple matches, the command fails. If there is no
+	match, the command falls back to creating a new branch from `HEAD`.
 +
 This can also be set up as the default behaviour by using the
 `worktree.guessRemote` config option.

index 5c105cf252..a37137042d 100755
@@ -669,6 +669,19 @@ test_expect_success 'git worktree add --guess-remote sets up tracking' '
 		test_cmp_rev refs/remotes/repo_a/foo refs/heads/foo
 	)
 '
+
+test_expect_success 'git worktree add --guess-remote fails if there are multiple matches' '
+	test_when_finished rm -rf repo_a repo_b foo &&
+	setup_remote_repo repo_a repo_b &&
+	(
+		cd repo_b &&
+		git remote add repo_a2 ../repo_a &&
+		git fetch repo_a2 &&
+		test_must_fail git worktree add --guess-remote ../foo 2>actual &&
+		test_grep "matched multiple (2) remote tracking branches" actual
+	)
+'
+
 test_expect_success 'git worktree add --guess-remote sets up tracking (quiet)' '
 	test_when_finished rm -rf repo_a repo_b foo &&
 	setup_remote_repo repo_a repo_b &&

gitgitgadget

'git worktree add ../foo-dir bar-topic' fails to dwim when there are multiple remote branches with name `bar-topic'. But it doesn't display meaningful message as 'git checkout bar-topic' does under the same situation.

We improve this by adding advice and modify the error message for worktree add.

By Junio's suggestion, we include matched remote names in the advice. It is applied to checkout, too.

We also fix the behavior of --guess-remote when there are multiple matches.

Changes from the previous patch:

* move advise_disambiguating_remotes() definition upper in the 3rd commit

Yoichi NAKAYAMA (4):
checkout: extract function to display advice for ambiguous remotes
checkout: improve message for ambiguous remote branch name worktree add: improve message for ambiguous remote branch name worktree add: treat multiple matches with --guess-remote as an error

 Documentation/config/worktree.adoc |  5 +-
 Documentation/git-worktree.adoc    |  4 +-
 builtin/checkout.c                 | 76 ++++++++++++++++++------------
 builtin/worktree.c                 | 57 +++++++++++++++++++---
 checkout.c                         | 14 +++++-
 checkout.h                         |  5 +-
 t/t2400-worktree-add.sh            | 17 ++++++-
 7 files changed, 134 insertions(+), 44 deletions(-)

base-commit: dea0ea3582e6980ddbc1173cc8e3e9f9db91cde0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2197%2Fyoichi%2Fimprove-worktree-add-error-message-v9
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2197/yoichi/improve-worktree-add-error-message-v9
Pull-Request: https://github.com/gitgitgadget/git/pull/2197

Range-diff vs v8:

 1:  e3f7d88520 = 1:  e3f7d88520 checkout: extract function to display advice for ambiguous remotes
 2:  89c0f4d303 = 2:  89c0f4d303 checkout: improve message for ambiguous remote branch name
 3:  095a5796d4 ! 3:  1010ac3295 worktree add: improve message for ambiguous remote branch name

@@ Commit message

## builtin/worktree.c ##
-@@ builtin/worktree.c: static char *dwim_branch(const char *path, char **new_branch)
- return NULL;
+@@ builtin/worktree.c: static int dwim_orphan(const struct add_opts *opts, int opt_track, int remote)
+ return 1;
}

+static void advise_disambiguating_remotes(const char *path, const char *branch,
@@ builtin/worktree.c: static char *dwim_branch(const char *path, char **new_branch
+ branch, path, branch);
+}
+
- static int add(int ac, const char **av, const char *prefix,
- struct repository *repo UNUSED)
+ static char *dwim_branch(const char *path, char **new_branch)
{
+ int n;
@@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix,
/* DWIM: Infer --orphan when repo has no refs. */
opts.orphan = (!s) && dwim_orphan(&opts, !!opt_track, 1);
4: 927856e0a0 ! 4: edb88b658a worktree add: treat multiple matches with --guess-remote as an error

     @@ Documentation/git-worktree.adoc: To remove a locked worktree, specify `--force`
       `worktree.guessRemote` config option.
      
       ## builtin/worktree.c ##
     -@@ builtin/worktree.c: static int dwim_orphan(const struct add_opts *opts, int opt_track, int remote)
     - 	return 1;
     +@@ builtin/worktree.c: static void advise_disambiguating_remotes(const char *path, const char *branch,
     + 	       branch, path, branch);
       }
       
      -static char *dwim_branch(const char *path, char **new_branch)
     -+static void advise_disambiguating_remotes(const char *path, const char *branch,
     -+					  const struct string_list *matched_remote_names)
     -+{
     -+	struct string_list_item *item;
     -+
     -+	advise(_("Branch name '%s' appears in multiple remotes:"), branch);
     -+	for_each_string_list_item(item, matched_remote_names) {
     -+		advise(_("  %s"), item->string);
     -+	}
     -+	advise(_("If you meant to create a worktree from a remote tracking branch on\n"
     -+		 "<remote>, you can do so by:\n"
     -+		 "\n"
     -+		 "    git worktree add -b %s %s <remote>/%s\n"
     -+		 "\n"
     -+		 "If you'd like to always prefer some remote, e.g. 'origin',\n"
     -+		 "consider setting checkout.defaultRemote=origin in your config."),
     -+	       branch, path, branch);
     -+}
     -+
      +static char *dwim_branch(const struct add_opts *opts, const char *path, char **new_branch)
       {
       	int n;
     @@ builtin/worktree.c: static char *dwim_branch(const char *path, char **new_branch
       		return remote;
       	}
       	return NULL;
     - }
     - 
     --static void advise_disambiguating_remotes(const char *path, const char *branch,
     --					  const struct string_list *matched_remote_names)
     --{
     --	struct string_list_item *item;
     --
     --	advise(_("Branch name '%s' appears in multiple remotes:"), branch);
     --	for_each_string_list_item(item, matched_remote_names) {
     --		advise(_("  %s"), item->string);
     --	}
     --	advise(_("If you meant to create a worktree from a remote tracking branch on\n"
     --		 "<remote>, you can do so by:\n"
     --		 "\n"
     --		 "    git worktree add -b %s %s <remote>/%s\n"
     --		 "\n"
     --		 "If you'd like to always prefer some remote, e.g. 'origin',\n"
     --		 "consider setting checkout.defaultRemote=origin in your config."),
     --	       branch, path, branch);
     --}
     --
     - static int add(int ac, const char **av, const char *prefix,
     - 	       struct repository *repo UNUSED)
     - {
      @@ builtin/worktree.c: static int add(int ac, const char **av, const char *prefix,
       		opts.orphan = dwim_orphan(&opts, !!opt_track, 0);
       	} else if (ac < 2) {

gitgitgadget

From: Yoichi NAKAYAMA <[email protected]>

---
 builtin/checkout.c | 62 ++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

index 55e3a89a85..650eda735f 100644
@@ -1343,6 +1343,34 @@ enum checkout_command {
 	CHECKOUT_RESTORE = 3,
 };
 
+static void advice_disambiguating_remotes(enum checkout_command which_command)
+{
+	const char *cmdname;
+
+	switch (which_command) {
+	case CHECKOUT_CHECKOUT:
+		cmdname = "checkout";
+		break;
+	case CHECKOUT_SWITCH:
+		cmdname = "switch";
+		break;
+	default:
+		BUG("command <%d> should not reach parse_remote_branch",
+		    which_command);
+		break;
+	}
+
+	advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
+		 "you can do so by fully qualifying the name with the --track option:\n"
+		 "\n"
+		 "    git %s --track origin/<name>\n"
+		 "\n"
+		 "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
+		 "one remote, e.g. the 'origin' remote, consider setting\n"
+		 "checkout.defaultRemote=origin in your config."),
+	       cmdname);
+}
+
 static char *parse_remote_branch(const char *arg,
 				 struct object_id *rev,
 				 int could_be_checkout_paths,

@@ -1358,35 +1386,11 @@ static char *parse_remote_branch(const char *arg,
 	}
 
 	if (!remote && num_matches > 1) {
-	    if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME)) {
-		    const char *cmdname;
-
-		    switch (which_command) {
-		    case CHECKOUT_CHECKOUT:
-			    cmdname = "checkout";
-			    break;
-		    case CHECKOUT_SWITCH:
-			    cmdname = "switch";
-			    break;
-		    default:
-			    BUG("command <%d> should not reach parse_remote_branch",
-				which_command);
-			    break;
-		    }
-
-		    advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
-			     "you can do so by fully qualifying the name with the --track option:\n"
-			     "\n"
-			     "    git %s --track origin/<name>\n"
-			     "\n"
-			     "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
-			     "one remote, e.g. the 'origin' remote, consider setting\n"
-			     "checkout.defaultRemote=origin in your config."),
-			   cmdname);
-	    }
-
-	    die(_("'%s' matched multiple (%d) remote tracking branches"),
-		arg, num_matches);
+		if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
+			advice_disambiguating_remotes(which_command);
+
+		die(_("'%s' matched multiple (%d) remote tracking branches"),
+		    arg, num_matches);
 	}
 
 	return remote;

gitgitgadget

From: Yoichi NAKAYAMA <[email protected]>

When the user runs 'git checkout bar-topic' without specifying a remote, and there is no local branch named bar-topic, we try to guess which remote branch bar-topic refers to, then create a new branch named bar-topic that tracks the remote branch.

If multiple remotes have a branch named bar-topic, we cannot determine a single remote.

To make it easier to resolve the ambiguity, provide the names of the matching remotes for the specified branch name.

To achieve that, add an optional feature to the unique_tracking_name() function that allows the matching remote names to be exposed to the caller.

---
 builtin/checkout.c | 28 ++++++++++++++++++++--------
 builtin/worktree.c |  4 ++--
 checkout.c         | 14 ++++++++++++--
 checkout.h         |  5 ++++-
 4 files changed, 38 insertions(+), 13 deletions(-)

index 650eda735f..07373615d8 100644
@@ -1343,9 +1343,12 @@ enum checkout_command {
 	CHECKOUT_RESTORE = 3,
 };
 
-static void advice_disambiguating_remotes(enum checkout_command which_command)
+static void advise_disambiguating_remotes(enum checkout_command which_command,
+					  const char *branch,
+					  const struct string_list *matched_remote_names)
 {
 	const char *cmdname;
+	struct string_list_item *item;
 
 	switch (which_command) {
 	case CHECKOUT_CHECKOUT:

@@ -1360,15 +1363,19 @@ static void advice_disambiguating_remotes(enum checkout_command which_command)
 		break;
 	}
 
-	advise(_("If you meant to check out a remote tracking branch on, e.g. 'origin',\n"
+	advise(_("Branch name '%s' appears in multiple remotes:"), branch);
+	for_each_string_list_item(item, matched_remote_names) {
+		advise(_("  %s"), item->string);
+	}
+	advise(_("If you meant to check out a remote tracking branch on <remote>,\n"
 		 "you can do so by fully qualifying the name with the --track option:\n"
 		 "\n"
-		 "    git %s --track origin/<name>\n"
+		 "    git %s --track <remote>/%s\n"
 		 "\n"
-		 "If you'd like to always have checkouts of an ambiguous <name> prefer\n"
+		 "If you'd like to always have checkouts of an ambiguous name prefer\n"
 		 "one remote, e.g. the 'origin' remote, consider setting\n"
 		 "checkout.defaultRemote=origin in your config."),
-	       cmdname);
+	       cmdname, branch);
 }
 
 static char *parse_remote_branch(const char *arg,

@@ -1377,7 +1384,10 @@ static char *parse_remote_branch(const char *arg,
 				 enum checkout_command which_command)
 {
 	int num_matches = 0;
-	char *remote = unique_tracking_name(arg, rev, &num_matches);
+	struct string_list matched_remote_names = STRING_LIST_INIT_DUP;
+
+	char *remote = unique_tracking_name(arg, rev, &num_matches,
+					    &matched_remote_names);
 
 	if (remote && could_be_checkout_paths) {
 		die(_("'%s' could be both a local file and a tracking branch.\n"

@@ -1387,12 +1397,14 @@ static char *parse_remote_branch(const char *arg,
 
 	if (!remote && num_matches > 1) {
 		if (advice_enabled(ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME))
-			advice_disambiguating_remotes(which_command);
-
+			advise_disambiguating_remotes(which_command, arg,
+						      &matched_remote_names);
 		die(_("'%s' matched multiple (%d) remote tracking branches"),
 		    arg, num_matches);
 	}
 
+	string_list_clear(&matched_remote_names, 0);
+
 	return remote;
 }
 

index 654d27c3e1..22c8e5e131 100644
@@ -782,7 +782,7 @@ static char *dwim_branch(const char *path, char **new_branch)
 	*new_branch = branchname;
 	if (guess_remote) {
 		struct object_id oid;
-		char *remote = unique_tracking_name(*new_branch, &oid, NULL);
+		char *remote = unique_tracking_name(*new_branch, &oid, NULL, NULL);
 		return remote;
 	}
 	return NULL;

@@ -904,7 +904,7 @@ static int add(int ac, const char **av, const char *prefix,
 
 		commit = lookup_commit_reference_by_name(branch);
 		if (!commit) {
-			remote = unique_tracking_name(branch, &oid, NULL);
+			remote = unique_tracking_name(branch, &oid, NULL, NULL);
 			if (remote) {
 				new_branch = branch;
 				branch = new_branch_to_free = remote;

index 1588b116ee..2806b783ec 100644
@@ -8,6 +8,7 @@
 #include "checkout.h"
 #include "config.h"
 #include "strbuf.h"
+#include "string-list.h"
 
 struct tracking_name_data {
 	/* const */ char *src_ref;

@@ -17,6 +18,7 @@ struct tracking_name_data {
 	const char *default_remote;
 	char *default_dst_ref;
 	struct object_id *default_dst_oid;
+	struct string_list **remote_names;
 };
 
 #define TRACKING_NAME_DATA_INIT { 0 }

@@ -39,6 +41,8 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
 		oidcpy(dst, cb->dst_oid);
 		cb->default_dst_oid = dst;
 	}
+	if (cb->remote_names)
+		string_list_append(*cb->remote_names, remote->name);
 	if (cb->dst_ref) {
 		free(query.dst);
 		return 0;

@@ -48,14 +52,20 @@ static int check_tracking_name(struct remote *remote, void *cb_data)
 }
 
 char *unique_tracking_name(const char *name, struct object_id *oid,
-			   int *dwim_remotes_matched)
+			   int *dwim_remotes_matched,
+			   struct string_list *dwim_remote_names)
 {
 	struct tracking_name_data cb_data = TRACKING_NAME_DATA_INIT;
 	const char *default_remote = NULL;
-	if (!repo_config_get_string_tmp(the_repository, "checkout.defaultremote", &default_remote))
+
+	if (!repo_config_get_string_tmp(the_repository,
+					"checkout.defaultremote",
+					&default_remote))
 		cb_data.default_remote = default_remote;
 	cb_data.src_ref = xstrfmt("refs/heads/%s", name);
 	cb_data.dst_oid = oid;
+	if (dwim_remote_names)
+		cb_data.remote_names = &dwim_remote_names;
 	for_each_remote(check_tracking_name, &cb_data);
 	if (dwim_remotes_matched)
 		*dwim_remotes_matched = cb_data.num_matches;

index 55920e7aeb..0b185a0fc9 100644
@@ -3,6 +3,8 @@
 
 #include "hash.h"
 
+struct string_list;
+
 /*
  * Check if the branch name uniquely matches a branch name on a remote
  * tracking branch.  Return the name of the remote if such a branch