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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 12, 2018, 06:44 UTC
Message-ID: <xmqq36s63idd.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20181110120707.25846-1-pclouds@gmail.com>

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

Show 15 quoted lines

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

Show 10 quoted lines

>  		/*
>  		 * 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);
		...