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

"Yoichi NAKAYAMA via GitGitGadget" <[email protected]> writes:

> 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.
>
> Signed-off-by: Yoichi NAKAYAMA <[email protected]>
> ---
>  builtin/worktree.c      | 35 +++++++++++++++++++++++++++++++++--
>  t/t2400-worktree-add.sh |  4 ++--
>  2 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 22c8e5e131..8286c283e0 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -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:"));

The subject "Branches" calls for plural verb "appear" (not
"appears").  The same issue appears in [PATCH 2/3].

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

This appears inside "} else if (ac == 2) {" to catch an invocation
like

	git worktree add ../over-there topic-branch

where the origin of topic-branch is ambiguous (in other words,
appears in multiple remotes).  But don't we have the same issue for
1 argument case that appears just above this (ac == 2) case that
handles

	git worktree add ../topic-branch

invocation?  The code reads like:

	} else if (ac < 2) {
		/* DWIM: Guess branch name from path. */
		char *s = dwim_branch(path, &new_branch_to_free);
		if (s)
			branch = branch_to_free = s;
		new_branch = new_branch_to_free;

		/* DWIM: Infer --orphan when repo has no refs. */
		opts.orphan = (!s) && dwim_orphan(&opts, !!opt_track, 1);
	} else if (ac == 2) {

where the branch name "topic-branch" is guessed from the path by
calling dwim_branch(), and we would get NULL in s.  branch is left
as-is, so it becomes "HEAD" that was assigned much earlier in the
same function.

        branch = ac < 2 ? "HEAD" : av[1];

We would create a new directory in ../topic-branch next door, and
then which branch would we check out?  Would dwim_orphan() kick in?

Perhaps we want to update that code path to disambiguate the same way?

> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index 87b926728a..5c105cf252 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