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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux