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

>
> 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.

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