On Mon, Nov 12, 2018 at 7:44 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> 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.
Is it that different between an exact path name and a pathspec?
Suppose it is a pathspec (with wildcards) that matches some paths, and
we happen to have the remote branch origin/<that-pathspec>, then it is
still ambiguous whether we should go create branch
"<that-pathspec>" or go check out the paths matched by the pathspec.
> 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?
I would hope that it would be "--" to settle all disputes. But it
looks like "git checkout foo --" is explicitly a candidate for dwim.
We have a hidden option --no-guess to disable dwim. Maybe it's a good
idea to make that one visible. It's at least clearer than using "--"
even if "--" worked on this case.
>
> > /*
> > * 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);
> ...
>
I still think handing both cases the same way is right. In the second
case we would not find 'origin/n??t' so we fall back to checking out
pathspec 'n??t' anyway (expected from you, I think). But just in case
the remote branch 'origin/n??t' exists, we kick and scream.
I think you see 'n??t' as a pathspec, but I'm thinking about a user
who sees 'n??t' as a branch name, not pathspec and he would have a
different expectation.
--
Duy
