Re: [PATCH] checkout: disambiguate dwim tracking branches and local files

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> @@ -1079,9 +1079,15 @@ static int parse_branchname_arg(int argc, const char **argv,
>  		 */
>  		int recover_with_dwim = dwim_new_local_branch_ok;
>  
> -		if (!has_dash_dash &&
> -		    (check_filename(opts->prefix, arg) || !no_wildcard(arg)))
> -			recover_with_dwim = 0;
> +		/*
> +		 * If both refs/remotes/origin/master and the file
> +		 * 'master'. Don't blindly go for 'master' file
> +		 * because it's ambiguous. Leave it for the user to
> +		 * decide.
> +		 */
> +		int disambi_local_file = !has_dash_dash &&
> +			(check_filename(opts->prefix, arg) || !no_wildcard(arg));

What you are computing on the right hand side is if the arg is
ambiguous.  And the code that looks at this variable does not
disambiguate, but it just punts and makes it responsibility to the
user (which is by the way the correct thing to do).

When a file with exact name is in the working tree, we do not
declare it is a pathspec and not a rev, as we may be allowed to dwim
and create a rev with that name and at that point we'd be in an
ambigous situation.  If the arg _has_ wildcard, however, it is a
strong sign that it *is* a pathspec, isn't it?  It is iffy that a
single variable that cannot be used to distinguish these two cases
is introduced---one of these cases will be mishandled.

Also how does the patch ensures that this new logic does not kick in
for those who refuse to let the command dwim to create a new branch?

>  		/*
>  		 * Accept "git checkout foo" and "git checkout foo --"
>  		 * as candidates for dwim.
> @@ -1094,6 +1100,9 @@ static int parse_branchname_arg(int argc, const char **argv,
>  			const char *remote = unique_tracking_name(arg, rev,
>  								  dwim_remotes_matched);
>  			if (remote) {
> +				if (disambi_local_file)
> +					die(_("'%s' could be both a local file and a tracking branch.\n"
> +					      "Please use -- to disambiguate"), arg);

Ah, the only user of this variable triggers when recover_with_dwim
is true, so that is how dwim-less case is handled, OK.

That still leaves the question if it is OK to handle these two cases
the same way in a repository without 'next' branch whose origin has
one:

    $ >next; git checkout --guess next
    $ >next; git checkout --guess 'n??t'

Perhaps the variable should be named "local_file_crashes_with_rev"
and its the scope narrowed to the same block as "remote" is
computed.  Or just remove the variable and check the condition right
there where you need to.  I.e.

	if (remote) {
		if (!has_dash_dash &&
		    check_filename(opts->prefix, arg))
			die("did you mean a branch or path?: '%s'", arg);
		...