Re: [PATCH 2/2] checkout: die() on ambiguous tracking branches

[Derrick Stolee <stolee@gmail.com>]

On 11/27/2019 4:48 AM, Alexandr Miloslavskiy via GitGitGadget wrote:
> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
> 
> Before this patch, when there were multiple DWIM candidates for remote
> branch, git decided to try the argument as pathspec instead. I believe
> that such behavior is a surprise: adding another remote suddenly causes
> git to discard file contents, because it was unsure which branch to
> pick. There was an incomplete attempt to prevent that in [3].
> 
> I understand that this was never intended:
> 
>   [1] introduces the unexpected behavior. Before, there was fallback
>   from not-a-ref to pathspec. This is reasonable DWIM. After, there is
>   another fallback from ambiguous-remote to pathspec. I understand that
>   it was kind of copy&paste oversight.
> 
>   [2] noticed the unexpected behavior but chose to semi-document it
>   instead of forbidding, because the goal of the patch series was
>   focused on something else.
> 
>   [3] adds `die()` when there is ambiguity between branch and file. The
>   case of multiple tracking branches is seemingly overlooked.
> 
> Change to complain about ambiguity instead of doing unexpected things.
> 
> [1] Commit 70c9ac2f ("DWIM "git checkout frotz" to "git checkout -b frotz origin/frotz"" 2009-10-18)
>     https://public-inbox.org/git/7vaazpxha4.fsf_-_@alter.siamese.dyndns.org/
> [2] Commit ad8d5104 ("checkout: add advice for ambiguous "checkout <branch>"", 2018-06-05)
>     https://public-inbox.org/git/20180502105452.17583-1-avarab@gmail.com/
> [3] Commit be4908f1 ("checkout: disambiguate dwim tracking branches and local files", 2018-11-13)
>     https://public-inbox.org/git/20181110120707.25846-1-pclouds@gmail.com/
> 
> Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
> ---
>  builtin/checkout.c       | 56 ++++++++++++++++++----------------------
>  t/t2024-checkout-dwim.sh | 23 +++++++++++++++--
>  2 files changed, 46 insertions(+), 33 deletions(-)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index e1b9df1543..6fb427990f 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1115,10 +1115,10 @@ static void setup_new_branch_info_and_source_tree(
>  
>  static const char *parse_remote_branch(const char *arg,
>  				       struct object_id *rev,
> -				       int could_be_checkout_paths,
> -				       int *dwim_remotes_matched)
> +				       int could_be_checkout_paths)
>  {
> -	const char *remote = unique_tracking_name(arg, rev, dwim_remotes_matched);
> +	int num_matches = 0;
> +	const char *remote = unique_tracking_name(arg, rev, &num_matches);
>  	
>  	if (remote && could_be_checkout_paths) {
>  		die(_("'%s' could be both a local file and a tracking branch.\n"
> @@ -1126,6 +1126,22 @@ static const char *parse_remote_branch(const char *arg,
>  		    arg);
>  	}
>  
> +	if (!remote && (num_matches > 1)) {

I believe the parentheses around "num_matched > 1" are not needed here.

> +	    if (advice_checkout_ambiguous_remote_branch_name) {
> +		    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 checkout --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."));
> +	    }
> +
> +	    die(_("'%s' matched multiple (%d) remote tracking branches"),
> +		arg, num_matches);
> +	}
> +
>  	return remote;
>  }
>  
> @@ -1133,8 +1149,7 @@ static int parse_branchname_arg(int argc, const char **argv,
>  				int dwim_new_local_branch_ok,
>  				struct branch_info *new_branch_info,
>  				struct checkout_opts *opts,
> -				struct object_id *rev,
> -				int *dwim_remotes_matched)
> +				struct object_id *rev)
>  {
>  	const char **new_branch = &opts->new_branch;
>  	int argcount = 0;
> @@ -1240,8 +1255,7 @@ static int parse_branchname_arg(int argc, const char **argv,
>  
>  		if (recover_with_dwim) {
>  			const char *remote = parse_remote_branch(arg, rev,
> -								 could_be_checkout_paths,
> -								 dwim_remotes_matched);
> +								 could_be_checkout_paths);
>  			if (remote) {
>  				*new_branch = arg;
>  				arg = remote;
> @@ -1505,7 +1519,6 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>  			 const char * const usagestr[])
>  {
>  	struct branch_info new_branch_info;
> -	int dwim_remotes_matched = 0;
>  	int parseopt_flags = 0;
>  
>  	memset(&new_branch_info, 0, sizeof(new_branch_info));
> @@ -1613,8 +1626,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>  			opts->track == BRANCH_TRACK_UNSPECIFIED &&
>  			!opts->new_branch;
>  		int n = parse_branchname_arg(argc, argv, dwim_ok,
> -					     &new_branch_info, opts, &rev,
> -					     &dwim_remotes_matched);
> +					     &new_branch_info, opts, &rev);
>  		argv += n;
>  		argc -= n;
>  	} else if (!opts->accept_ref && opts->from_treeish) {
> @@ -1672,28 +1684,10 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>  	}
>  
>  	UNLEAK(opts);
> -	if (opts->patch_mode || opts->pathspec.nr) {
> -		int ret = checkout_paths(opts, new_branch_info.name);
> -		if (ret && dwim_remotes_matched > 1 &&
> -		    advice_checkout_ambiguous_remote_branch_name)
> -			advise(_("'%s' matched more than one remote tracking branch.\n"
> -				 "We found %d remotes with a reference that matched. So we fell back\n"
> -				 "on trying to resolve the argument as a path, but failed there too!\n"
> -				 "\n"
> -				 "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 checkout --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."),
> -			       argv[0],
> -			       dwim_remotes_matched);
> -		return ret;
> -	} else {
> +	if (opts->patch_mode || opts->pathspec.nr)
> +		return checkout_paths(opts, new_branch_info.name);
> +	else
>  		return checkout_branch(opts, &new_branch_info);
> -	}
>  }
>  
>  int cmd_checkout(int argc, const char **argv, const char *prefix)
> diff --git a/t/t2024-checkout-dwim.sh b/t/t2024-checkout-dwim.sh
> index fa0718c730..707c88ceba 100755
> --- a/t/t2024-checkout-dwim.sh
> +++ b/t/t2024-checkout-dwim.sh
> @@ -37,7 +37,9 @@ test_expect_success 'setup' '
>  		git checkout -b foo &&
>  		test_commit a_foo &&
>  		git checkout -b bar &&
> -		test_commit a_bar
> +		test_commit a_bar &&
> +		git checkout -b ambiguous_branch_and_file &&
> +		test_commit a_ambiguous_branch_and_file
>  	) &&
>  	git init repo_b &&
>  	(
> @@ -46,7 +48,9 @@ test_expect_success 'setup' '
>  		git checkout -b foo &&
>  		test_commit b_foo &&
>  		git checkout -b baz &&
> -		test_commit b_baz
> +		test_commit b_baz &&
> +		git checkout -b ambiguous_branch_and_file &&
> +		test_commit b_ambiguous_branch_and_file
>  	) &&
>  	git remote add repo_a repo_a &&
>  	git remote add repo_b repo_b &&
> @@ -75,6 +79,21 @@ test_expect_success 'checkout of branch from multiple remotes fails #1' '
>  	test_branch master
>  '
>  
> +test_expect_success 'when arg matches multiple remotes, do not fallback to interpreting as pathspec' '
> +	git checkout -b t_ambiguous_branch_and_file &&
> +	>ambiguous_branch_and_file &&
> +	git add ambiguous_branch_and_file &&
> +	git commit -m "ambiguous_branch_and_file" &&
> +
> +	test_when_finished "git checkout -- ambiguous_branch_and_file" &&
> +	echo "file contents" >ambiguous_branch_and_file &&
> +	cp ambiguous_branch_and_file expect &&
> +
> +	test_must_fail git checkout ambiguous_branch_and_file &&
> +
> +	test_cmp expect ambiguous_branch_and_file
> +'

I like that you added a test for this non-obvious situation. One thing that
would help ensure that your test is checking the right thing is to redirect
stderr to a file and grep for the die() message you included. Something like:

	test_must_fail git checkout ambiguous_branch_and_file 2>err &&
	test_i18ngrep "matched multiple (2) remote tracking branches" err &&

Thanks,
-Stolee