Fwd: Errors when diff'ing arbitrary upstream remotes are not intuitive if git pull --all hasn't been done

Fwd: Errors when diff'ing arbitrary upstream remotes are not intuitive if git pull --all hasn't been done

[yaneurabeya .]

Hi!
    I added an arbitrary upstream remote thinking that I could just
git diff the upstream remote's master. Turns out I needed to run git
pull --all in order to be able to diff the file (I forgot that step).
    Could this error message be improved for interactive commands by
first checking to see whether or not the path starts with a remote,
then recommend that the remote be pulled? If this seems sane, I could
whip up a patch and post it in a github pull request.
Thanks!
-Garrett

$ git remote add pkohut [...]
$ git diff upstream/master -- conf/ | wc -l
16
$ git diff pkohut/master -- conf/ | wc -l
fatal: bad revision 'pkohut/master'
0
$ git pull --all
Fetching origin
Fetching upstream
Fetching pkohut
...
$ git diff pkohut/master -- conf/ | wc -l
46
$ git --version; uname -a
git version 1.7.9
CYGWIN_NT-6.1 ZL00757 1.7.25(0.270/5/3) 2013-08-31 20:37 x86_64 Cygwin

Re: Fwd: Errors when diff'ing arbitrary upstream remotes are not intuitive if git pull --all hasn't been done

[Jeff King]

On Thu, Oct 24, 2013 at 11:07:05AM -0700, yaneurabeya . wrote:

>     I added an arbitrary upstream remote thinking that I could just
> git diff the upstream remote's master. Turns out I needed to run git
> pull --all in order to be able to diff the file (I forgot that step).

Actually, you can just run "git fetch"; you just need to fetch the
commits from the remote, but you do not need to merge them (and pull is
a fetch plus a merge).

>     Could this error message be improved for interactive commands by
> first checking to see whether or not the path starts with a remote,
> then recommend that the remote be pulled?

That might be worth doing. We cannot definitely say the branch exists
without hitting the network (which we would not want to do in the
general case), but I think it is reasonable for git to give suggestions
(we could also give a "did you mean X..." for near-typos, as we do for
typo-ed commands like "git dif".

If you do try it, please don't just check for the remote name, but
actually complete the right-hand side of the fetch refspec for each
remote. They are equivalent in the default config, but aren't
necessarily so (and there has been talk of adjusting the layout of
remote refspecs). I don't recall offhand what functions we have to help
you, but I believe Johan (cc'd) was working in this area recently and
might be able to say more.

> If this seems sane, I could whip up a patch and post it in a github
> pull request.

If you do write a patch, please send it to the list, as the github
repository is a mirror, and actual development happens here. Details are
in Documentation/SubmittingPatches.

-Peff

Re: Fwd: Errors when diff'ing arbitrary upstream remotes are not intuitive if git pull --all hasn't been done

[Jeff King]

On Fri, Oct 25, 2013 at 02:14:07AM -0400, Jeff King wrote:

> >     Could this error message be improved for interactive commands by
> > first checking to see whether or not the path starts with a remote,
> > then recommend that the remote be pulled?
> 
> That might be worth doing. We cannot definitely say the branch exists
> without hitting the network (which we would not want to do in the
> general case), but I think it is reasonable for git to give suggestions
> (we could also give a "did you mean X..." for near-typos, as we do for
> typo-ed commands like "git dif".
> 
> If you do try it, please don't just check for the remote name, but
> actually complete the right-hand side of the fetch refspec for each
> remote. They are equivalent in the default config, but aren't
> necessarily so (and there has been talk of adjusting the layout of
> remote refspecs). I don't recall offhand what functions we have to help
> you, but I believe Johan (cc'd) was working in this area recently and
> might be able to say more.

...and of course I forgot to cc Johan in the original.

-Peff

Re: Fwd: Errors when diff'ing arbitrary upstream remotes are not intuitive if git pull --all hasn't been done

[Johan Herland]

On Fri, Oct 25, 2013 at 8:14 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Oct 25, 2013 at 02:14:07AM -0400, Jeff King wrote:
>> >     Could this error message be improved for interactive commands by
>> > first checking to see whether or not the path starts with a remote,
>> > then recommend that the remote be pulled?
>>
>> That might be worth doing. We cannot definitely say the branch exists
>> without hitting the network (which we would not want to do in the
>> general case), but I think it is reasonable for git to give suggestions
>> (we could also give a "did you mean X..." for near-typos, as we do for
>> typo-ed commands like "git dif".
>>
>> If you do try it, please don't just check for the remote name, but
>> actually complete the right-hand side of the fetch refspec for each
>> remote. They are equivalent in the default config, but aren't
>> necessarily so (and there has been talk of adjusting the layout of
>> remote refspecs). I don't recall offhand what functions we have to help
>> you, but I believe Johan (cc'd) was working in this area recently and
>> might be able to say more.

Actually, I don't think there's much refspec stuff to be done here.
When running "git diff $remote/$branch", there are 3 possible
outcomes:

 - $remote is not a valid remote name, the user probably meant
something different (like "nested/branch"). The current error message
is fine.

 - $remote is a valid remote name, but $branch has not (yet) been
fetched from there. Suggest the user run "git fetch $remote"

 - $remote/$branch is a valid remote-tracking branch. The diff works! No errors.

So, AFAICS, the patch should simply:

 1. Split the input on the first '/' into $remote/$branch, and use the
preceding part ($remote) as a potential remote name, and the following
part ($branch) as a potential branch name. (Although it is
theoretically possible to have remote names containing slashes, I
don't think anybody uses them, and we have considered disallowing
them, mainly because of this very issue: it makes "$remote/$branch"
parsing (even more) ambiguous)

 2. See if a remote called $remote exists. If it does, suggest to the
user to run "git fetch $remote". If $remote does not exist, leave the
current error message in place.

Hope this helps,

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net

Re: Fwd: Errors when diff'ing arbitrary upstream remotes are not intuitive if git pull --all hasn't been done

[Jeff King]

On Fri, Oct 25, 2013 at 09:03:41AM +0200, Johan Herland wrote:

> Actually, I don't think there's much refspec stuff to be done here.
> When running "git diff $remote/$branch", there are 3 possible
> outcomes:
> 
>  - $remote is not a valid remote name, the user probably meant
> something different (like "nested/branch"). The current error message
> is fine.
> 
>  - $remote is a valid remote name, but $branch has not (yet) been
> fetched from there. Suggest the user run "git fetch $remote"
> 
>  - $remote/$branch is a valid remote-tracking branch. The diff works! No errors.

Right, I think it is the second case we are talking about.

> So, AFAICS, the patch should simply:
> 
>  1. Split the input on the first '/' into $remote/$branch, and use the
> preceding part ($remote) as a potential remote name, and the following
> part ($branch) as a potential branch name. (Although it is
> theoretically possible to have remote names containing slashes, I
> don't think anybody uses them, and we have considered disallowing
> them, mainly because of this very issue: it makes "$remote/$branch"
> parsing (even more) ambiguous)

What I specifically meant is that this breaks with a remote like:

  [remote "foo"]
    fetch = +refs/heads/*:refs/remotes/bar/*

The correct advice for "bar/branch" is to recommend "git fetch foo", and
the correct advice for "foo/branch" is nothing at all.

I know such config is unusual, but I thought there was a recent push for
us to be accurate about finding the local side of remote tracking
branches, rather than just assuming they start with "$remote". Maybe I
am misremembering, though; I thought it was related to potentially
shifting the default refspecs.

The procedure along those lines would be:

  for each remote
    for each fetch-refspec in remote
      if refspec.rhs contains "refs/remotes/$failed_branch"
        recommend "git fetch $remote"

I was just wondering if we had something to make that "does this refspec
contain this ref" part easier.

-Peff

Re: Fwd: Errors when diff'ing arbitrary upstream remotes are not intuitive if git pull --all hasn't been done

[Johan Herland]

On Fri, Oct 25, 2013 at 9:10 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Oct 25, 2013 at 09:03:41AM +0200, Johan Herland wrote:
>>  1. Split the input on the first '/' into $remote/$branch, and use the
>> preceding part ($remote) as a potential remote name, and the following
>> part ($branch) as a potential branch name. (Although it is
>> theoretically possible to have remote names containing slashes, I
>> don't think anybody uses them, and we have considered disallowing
>> them, mainly because of this very issue: it makes "$remote/$branch"
>> parsing (even more) ambiguous)
>
> What I specifically meant is that this breaks with a remote like:
>
>   [remote "foo"]
>     fetch = +refs/heads/*:refs/remotes/bar/*
>
> The correct advice for "bar/branch" is to recommend "git fetch foo", and
> the correct advice for "foo/branch" is nothing at all.
>
> I know such config is unusual, but I thought there was a recent push for
> us to be accurate about finding the local side of remote tracking
> branches, rather than just assuming they start with "$remote". Maybe I
> am misremembering, though; I thought it was related to potentially
> shifting the default refspecs.

Obviously, you're right. Sorry about that, haven't had my morning coffee yet. :(

> The procedure along those lines would be:
>
>   for each remote
>     for each fetch-refspec in remote
>       if refspec.rhs contains "refs/remotes/$failed_branch"
>         recommend "git fetch $remote"
>
> I was just wondering if we had something to make that "does this refspec
> contain this ref" part easier.

Yes, I found the following code in branch.c (added in 41c21f2), which
does a similar thing. Might want to refactor that into something more
general:

+static int check_tracking_branch(struct remote *remote, void *cb_data)
+{
+       char *tracking_branch = cb_data;
+       struct refspec query;
+       memset(&query, 0, sizeof(struct refspec));
+       query.dst = tracking_branch;
+       return !(remote_find_tracking(remote, &query) ||
+                prefixcmp(query.src, "refs/heads/"));
+}
+
+static int validate_remote_tracking_branch(char *ref)
+{
+       return !for_each_remote(check_tracking_branch, ref);
+}


Hope this helps,

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net