[PATCH] worktree add: improve message for ambiguous remote branch name

[PATCH] worktree add: improve message for ambiguous remote branch name

[Yoichi NAKAYAMA via GitGitGadget]

From: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>

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.

Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
---
    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(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 654d27c3e1..46bc305116 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -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);
 			}
 		}
 
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 87b926728a..4d21c8eba4 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -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

Re: [PATCH] worktree add: improve message for ambiguous remote branch name

[Junio C Hamano]

"Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
>
> 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.
>
> Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
> ---
>     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(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 654d27c3e1..46bc305116 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -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"

This is shown in two places, but what did the user exactly type in
these two situations?  Can their intent be different, in which case
different suggestions might be more appropriate to each of them?

Let's see.

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

The worktree.guessremote configuration is set.  dwim_branch() is
called when "git worktree add A/B/X" is run with a single argument
"A/B/X", which comes here as "path", and that is munged into the
branchname "X".

We used to pass NULL as the second parameter to unique_tracking_name(),
so we were only interested in the case where we have exactly one
matching remote, and if there is 0 or multiple remotes with the
named branch, we returned NULL from here.

The patch does not change that, but using the branch name, we try to
see if there are multiple matches, in that case, we give the advice
message to say "hey, don't be so lazy, as X appears in more than one
remote, so tell me which one you mean".

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

But shouldn't we do a bit better than 

    git worktree add <path> origin/<name>

The above makes the user think that just like 'git', 'worktree' and
'add', 'origin/' is a fixed part, and they would need to substitute
<path> and <name>, but that is not really what we want to tell them.
The most crucial part to correct is 'origin/', as that is what we
could not guess from the given information.

We know that the user gave us "A/B/X" (path) and probably they want
to create local "X" from it.  Or not.  We also should know, in
caller's opt_track and used_new_branch_options, that the user gave
us "-t -b Y" from the command line.

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

Style: overly long line, with {braces} around a single statement block.

What does this case handle?  Can you make a similar analysis to come
up with the list of things we know the user gave us, to give a bit
better command line to suggest here?

>  			}
>  		}

Thanks.

Re: [PATCH] worktree add: improve message for ambiguous remote branch name

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +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"
>> ...
>> +		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;
>>  	}
>
> The worktree.guessremote configuration is set.  dwim_branch() is
> called when "git worktree add A/B/X" is run with a single argument
> "A/B/X", which comes here as "path", and that is munged into the
> branchname "X".
>
> We used to pass NULL as the second parameter to unique_tracking_name(),
> so we were only interested in the case where we have exactly one
> matching remote, and if there is 0 or multiple remotes with the
> named branch, we returned NULL from here.
>
> The patch does not change that, but using the branch name, we try to
> see if there are multiple matches, in that case, we give the advice
> message to say "hey, don't be so lazy, as X appears in more than one
> remote, so tell me which one you mean".

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.

Re: [PATCH] worktree add: improve message for ambiguous remote branch name

[Harald Nordgren]

This is an interesting idea!


Harald