Confusing git pull error message

Confusing git pull error message

[John Tapsell]

Hi,

  Because of another bug in git https, sometimes "git ls-remote
origin" only sees a small subset of the actually available branches.
This lasts until someone using git:// makes a commit.

  So anyway, I tried to do a "git pull" and it gives me the misleading error:

You asked me to pull without telling me which branch you
want to merge with, and 'branch.master.merge' in
your configuration file does not tell me either.  Please
name which branch you want to merge on the command line and
try again (e.g. 'git pull <repository> <refspec>').
See git-pull(1) for details on the refspec.

If you often merge with the same branch, you may want to
configure the following variables in your configuration
file:

    branch.master.remote = <nickname>
    branch.master.merge = <remote-ref>
    remote.<nickname>.url = <url>
    remote.<nickname>.fetch = <refspec>

See git-config(1) for details.



But the actual problem is that "master" doesn't exist on origin.  So
basically I _have_ told it what branch.master.remote and
branch.master.merge etc is, it's just that they don't appear to exist.

Thanks,

John

Re: Confusing git pull error message

[Jeff King]

On Sat, Sep 12, 2009 at 11:01:45PM +0300, John Tapsell wrote:

>   So anyway, I tried to do a "git pull" and it gives me the misleading error:
> 
> You asked me to pull without telling me which branch you
> want to merge with, and 'branch.master.merge' in
> your configuration file does not tell me either.  Please
> name which branch you want to merge on the command line and
> try again (e.g. 'git pull <repository> <refspec>').
> See git-pull(1) for details on the refspec.
>
> [...]
>
> But the actual problem is that "master" doesn't exist on origin.  So
> basically I _have_ told it what branch.master.remote and
> branch.master.merge etc is, it's just that they don't appear to exist.

Ugh. That is horrible. It's an artifact of the (IMHO) brain-dead way
that pull and fetch interact.

Pull doesn't actually look at the configuration at all. It just calls
fetch, and then fetch writes out a file containing the refs it pulled,
some of which may be marked with a magic "not-for-merge" flag. So we see
that nothing is available for merging and guess that it must not have
been configured. Which is of course wrong in your case; it just didn't
exist.

I think it is enough for git-pull to just check whether the config
exists, and if so, guess that the ref was simply not fetched. IOW,
this:

-- >8 --
Subject: [PATCH] pull: make "nothing to merge" error message more accurate

When pull sees that no branches are available for merge and
that we have a current branch, it assumes the problem is
that we had no branch.*.merge configuration. But it may also
be the case that we simply didn't pull the configured ref
(probably because it doesn't exist). Let's distinguish those
two cases.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-pull.sh |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index 0bbd5bf..4ddd537 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -89,6 +89,9 @@ error_on_no_merge_candidates () {
 	done
 
 	curr_branch=${curr_branch#refs/heads/}
+	upstream=`git config "branch.$curr_branch.merge" ||
+		  git config "branch.$curr_branch.rebase"`
+	upstream_short=`echo "$upstream" | sed "s|refs/heads/||"`
 
 	if [ -z "$curr_branch" ]; then
 		echo "You are not currently on a branch, so I cannot use any"
@@ -96,7 +99,7 @@ error_on_no_merge_candidates () {
 		echo "Please specify which branch you want to merge on the command"
 		echo "line and try again (e.g. 'git pull <repository> <refspec>')."
 		echo "See git-pull(1) for details."
-	else
+	elif [ -z "$upstream" ]; then
 		echo "You asked me to pull without telling me which branch you"
 		echo "want to merge with, and 'branch.${curr_branch}.merge' in"
 		echo "your configuration file does not tell me either.	Please"
@@ -114,6 +117,10 @@ error_on_no_merge_candidates () {
 		echo "    remote.<nickname>.fetch = <refspec>"
 		echo
 		echo "See git-config(1) for details."
+	else
+		echo "Your configuration specified for us to pull the ref"
+		echo "'$upstream_short', but we were unable to fetch it from"
+		echo "the remote."
 	fi
 	exit 1
 }
-- 
1.6.5.rc0.201.g76b4d.dirty

Re: Confusing git pull error message

[John Tapsell]

2009/9/13 Jeff King <peff@peff.net>:

> +       else
> +               echo "Your configuration specified for us to pull the ref"
> +               echo "'$upstream_short', but we were unable to fetch it from"
> +               echo "the remote."

Thanks!

But why was it 'unable' ?  Are there cases other than it not existing?
 Maybe something less cryptic:

"Attempted to pull from ref $upstream_short but this branch does not
exist on remote."

Or something?

John

Re: Confusing git pull error message

[Jeff King]

On Sun, Sep 13, 2009 at 12:31:46AM +0300, John Tapsell wrote:

> 2009/9/13 Jeff King <peff@peff.net>:
> 
> > +       else
> > +               echo "Your configuration specified for us to pull the ref"
> > +               echo "'$upstream_short', but we were unable to fetch it from"
> > +               echo "the remote."
> 
> Thanks!
> 
> But why was it 'unable' ?  Are there cases other than it not existing?
>  Maybe something less cryptic:
> 
> "Attempted to pull from ref $upstream_short but this branch does not
> exist on remote."
> 
> Or something?

I'm not sure if there are other cases, so I left it unspecified. I think
that "does not exist" is probably the only one that should make it this
far, though, as any actual error would cause fetch to die, aborting the
pull.

And you can see that when you ask for a specific bogus ref, as in:

  $ git pull origin bogus
  fatal: Couldn't find remote ref bogus

I'll re-roll the patch with more specific wording.

-Peff

Re: Confusing git pull error message

[Sverre Rabbelier]

Heya,

On Sat, Sep 12, 2009 at 23:11, Jeff King <peff@peff.net> wrote:
>        if [ -z "$curr_branch" ]; then
>                echo "You are not currently on a branch, so I cannot use any"
> @@ -96,7 +99,7 @@ error_on_no_merge_candidates () {
>                echo "Please specify which branch you want to merge on the command"
>                echo "line and try again (e.g. 'git pull <repository> <refspec>')."
>                echo "See git-pull(1) for details."
> -       else
> +       elif [ -z "$upstream" ]; then
>                echo "You asked me to pull without telling me which branch you"
>                echo "want to merge with, and 'branch.${curr_branch}.merge' in"
>                echo "your configuration file does not tell me either.  Please"
> @@ -114,6 +117,10 @@ error_on_no_merge_candidates () {
>                echo "    remote.<nickname>.fetch = <refspec>"
>                echo
>                echo "See git-config(1) for details."
> +       else
> +               echo "Your configuration specified for us to pull the ref"
> +               echo "'$upstream_short', but we were unable to fetch it from"
> +               echo "the remote."
>        fi
>        exit 1
>  }

Pretty bad case of multiple personality disorder here? At first
there's a "me", then again a "me", and then all of a sudden a "we"?

-- 
Cheers,

Sverre Rabbelier

Re: Confusing git pull error message

[Jeff King]

On Sat, Sep 12, 2009 at 11:37:47PM +0200, Sverre Rabbelier wrote:

> > +               echo "Your configuration specified for us to pull the ref"
> > +               echo "'$upstream_short', but we were unable to fetch it from"
> > +               echo "the remote."
> >        fi
> >        exit 1
> >  }
> 
> Pretty bad case of multiple personality disorder here? At first
> there's a "me", then again a "me", and then all of a sudden a "we"?

Fair enough. I'll change it to "I was unable to fetch" in the re-roll.

-Peff

Re: Confusing git pull error message

[Sverre Rabbelier]

Heya,

On Sun, Sep 13, 2009 at 00:31, Jeff King <peff@peff.net> wrote:
> Fair enough. I'll change it to "I was unable to fetch" in the re-roll.

Actually, from grepping through the source there's a lot of we-ing
going on already (although mostly in the comments), and fairly little
I-ing (not based on actual numbers, just what I could judge at a
glance), so maybe changing the ones above to "we" instead is more
appropriate?

-- 
Cheers,

Sverre Rabbelier

Re: Confusing git pull error message

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think it is enough for git-pull to just check whether the config
> exists, and if so, guess that the ref was simply not fetched. IOW,
> this:

Thanks.

I saw some discussion on improving the wording.  Here is what I plan to
commit.

diff --git a/git-pull.sh b/git-pull.sh
index 0bbd5bf..2c2fa79 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -89,6 +89,8 @@ error_on_no_merge_candidates () {
 	done
 
 	curr_branch=${curr_branch#refs/heads/}
+	upstream=$(git config "branch.$curr_branch.merge" ||
+			git config "branch.$curr_branch.rebase")
 
 	if [ -z "$curr_branch" ]; then
 		echo "You are not currently on a branch, so I cannot use any"
@@ -96,7 +98,7 @@ error_on_no_merge_candidates () {
 		echo "Please specify which branch you want to merge on the command"
 		echo "line and try again (e.g. 'git pull <repository> <refspec>')."
 		echo "See git-pull(1) for details."
-	else
+	elif [ -z "$upstream" ]; then
 		echo "You asked me to pull without telling me which branch you"
 		echo "want to merge with, and 'branch.${curr_branch}.merge' in"
 		echo "your configuration file does not tell me either.	Please"
@@ -114,6 +116,10 @@ error_on_no_merge_candidates () {
 		echo "    remote.<nickname>.fetch = <refspec>"
 		echo
 		echo "See git-config(1) for details."
+	else
+		echo "Your configuration specifies to merge the ref"
+		echo "'${upstream#refs/heads/}' from the remote, but no such ref"
+		echo "was fetched."
 	fi
 	exit 1
 }

Re: Confusing git pull error message

[Jeff King]

On Sun, Sep 13, 2009 at 01:38:48PM -0700, Junio C Hamano wrote:

> I saw some discussion on improving the wording.  Here is what I plan to
> commit.

Thanks for picking this up, I meant to re-post with improvements.

> +	else
> +		echo "Your configuration specifies to merge the ref"
> +		echo "'${upstream#refs/heads/}' from the remote, but no such ref"
> +		echo "was fetched."

What you have here is precisely what we observed. But I think one of the
complaints was to say more explicitly "that ref doesn't exist on the
remote", which I think should be the case if we have got to this point
(anything else would have triggered an error in fetch).

I don't have a strong feeling either way, though.

-Peff

Re: Confusing git pull error message

[John Tapsell]

2009/9/13 Jeff King <peff@peff.net>:
> On Sun, Sep 13, 2009 at 01:38:48PM -0700, Junio C Hamano wrote:
>
>> I saw some discussion on improving the wording.  Here is what I plan to
>> commit.
>
> Thanks for picking this up, I meant to re-post with improvements.
>
>> +     else
>> +             echo "Your configuration specifies to merge the ref"
>> +             echo "'${upstream#refs/heads/}' from the remote, but no such ref"
>> +             echo "was fetched."
>
> What you have here is precisely what we observed. But I think one of the
> complaints was to say more explicitly "that ref doesn't exist on the
> remote", which I think should be the case if we have got to this point
> (anything else would have triggered an error in fetch).

Yeah, it kinda sounds like git is just being lazy, and can't be
bothered to fetch it :-)

Re: Confusing git pull error message

[Junio C Hamano]

John Tapsell <johnflux@gmail.com> writes:

> Yeah, it kinda sounds like git is just being lazy, and can't be
> bothered to fetch it :-)

Do you want it to say "you didn't tell us to fetch it"?

After all, we only do what the user instructs us to do, so I thought that
goes without saying it.

Re: Confusing git pull error message

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> What you have here is precisely what we observed. But I think one of the
> complaints was to say more explicitly "that ref doesn't exist on the
> remote", which I think should be the case if we have got to this point
> (anything else would have triggered an error in fetch).

Wouldn't you get into the situation with this?

	[remote "origin"]
        	fetch = refs/heads/master:refs/heads/master
	[branch "master"]
        	remote = origin
                merge = refs/heads/next

I think saying "does not exist" will repeat the same mistake of
overguessing you are trying to rectify.

Re: Confusing git pull error message

[Jeff King]

On Sun, Sep 13, 2009 at 01:57:37PM -0700, Junio C Hamano wrote:

> > What you have here is precisely what we observed. But I think one of the
> > complaints was to say more explicitly "that ref doesn't exist on the
> > remote", which I think should be the case if we have got to this point
> > (anything else would have triggered an error in fetch).
> 
> Wouldn't you get into the situation with this?
> 
> 	[remote "origin"]
>         	fetch = refs/heads/master:refs/heads/master
> 	[branch "master"]
>         	remote = origin
>                 merge = refs/heads/next
> 
> I think saying "does not exist" will repeat the same mistake of
> overguessing you are trying to rectify.

You are right, of course. I think your wording makes sense, then
(otherwise we get stuck with "well, we didn't fetch it. Maybe because it
didn't exist. Or maybe because your configuration didn't include it.").

-Peff

Re: Confusing git pull error message

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> I think saying "does not exist" will repeat the same mistake of
>> overguessing you are trying to rectify.
>
> You are right, of course. I think your wording makes sense, then
> (otherwise we get stuck with "well, we didn't fetch it. Maybe because it
> didn't exist. Or maybe because your configuration didn't include it.").

Yeah, the saddest part of this whole story is that the message given by
the very original was succinct and accurate.

   echo >&2 "Warning: No merge candidate found because value of config option
    \"branch.${curr_branch}.merge\" does not match any remote branch fetched."
   echo >&2 "No changes."

Then we piled up different messages for various cases, and it went
downhill from there.

 - First, 8fc293c (Make git-pull complain and give advice when there is
   nothing to merge with, 2007-10-02) made the message say essentially the
   same thing, but with a lot more verbosity, covering only one _common_
   case.

 - Then 441ed41 ("git pull --tags": error out with a better message.,
   2007-12-28) added a special case for --tags.  The description before
   8fc293c would have still been correct without this change.  This was
   necessary because 8fc293c talked only about the "I am on branch and
   there is no configuration" case.

 - Then 61e6108 (git-pull.sh: better warning message for "git pull" on
   detached head., 2009-04-08) added another special case, again because
   the "helpful" instruction given by 8fc293c did not make any sense in
   the detached HEAD case.

It definitely is worth to further cover cases the message by 8fc293c did
not consider, as this thread suggested, but at the same time, I think the
advice mechanism should be used to bring back a succinct and to-the-point
message, close to the original one, e.g.

	Warning: Not merging anything, as no default merge candidate ref
	was fetched from the remote.

so that people who know what they are doing can squelch potentially
misleading "helpful" advices and diagnose/think for their own.

Re: Confusing git pull error message

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Sep 13, 2009 at 01:38:48PM -0700, Junio C Hamano wrote:
>
>> I saw some discussion on improving the wording.  Here is what I plan to
>> commit.
>
> Thanks for picking this up, I meant to re-post with improvements.

I am _not_ going to commit the following patch, because it will interfere
with this clarification effort, but I think we want to do something along
this line after the "clarification" settles.

An observation I'd like to make is that this is way too ugly:

	[advice]
        	pullNoMergeFound = false
                pushNonFastForward = false
                statusHints = false

than

	[IKnowWhatIAmDoingThankYouVeryMuch]
        	pullNoMergeFound
                pushNonFastForward
                statusHints

but this feature is for people who know what they are doing, so I guess
the current set-up would be fine.

 git-pull.sh |   78 ++++++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 54 insertions(+), 24 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index 0bbd5bf..101545e 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -76,14 +76,64 @@ do
 	shift
 done
 
+advice_tags_only () {
+	if test -z "$1"
+	then
+		echo "Fetching tags only."
+		return
+	fi
+	echo "Fetching tags only, you probably meant:"
+	echo "  git fetch --tags"
+}
+
+advice_detached_head () {
+	if test -z "$1"
+	then
+		echo "No default merge candidate on a detached HEAD."
+		return
+	fi
+	echo "You are not currently on a branch, so I cannot use any"
+	echo "'branch.<branchname>.merge' in your configuration file."
+	echo "Please specify which branch you want to merge on the command"
+	echo "line and try again (e.g. 'git pull <repository> <refspec>')."
+	echo "See git-pull(1) for details."
+}
+
+advice_no_merge_candidates () {
+	if test -z "$1"
+	then
+		echo "No merge candidate for the current branch fetched."
+		return
+	fi
+	cat <<EOF
+You asked me to pull without telling me which branch you
+want to merge with, and 'branch.$2.merge' in
+your configuration file does not tell me either.  Please
+specify which branch you want to merge on the command line and
+try again (e.g. 'git pull <repository> <refspec>').
+See git-pull(1) for details.
+
+If you often merge with the same branch, you may want to
+configure the following variables in your configuration
+file:
+
+    branch.$2.remote = <nickname>
+    branch.$2.merge = <remote-ref>
+    remote.<nickname>.url = <url>"
+    remote.<nickname>.fetch = <refspec>
+
+See git-config(1) for details.
+EOF
+}
+
 error_on_no_merge_candidates () {
 	exec >&2
+	advice=$(git config --bool advice.pullNoMergeFound)
 	for opt
 	do
 		case "$opt" in
 		-t|--t|--ta|--tag|--tags)
-			echo "Fetching tags only, you probably meant:"
-			echo "  git fetch --tags"
+			advice_tags_only "$advice"
 			exit 1
 		esac
 	done
@@ -91,29 +141,9 @@ error_on_no_merge_candidates () {
 	curr_branch=${curr_branch#refs/heads/}
 
 	if [ -z "$curr_branch" ]; then
-		echo "You are not currently on a branch, so I cannot use any"
-		echo "'branch.<branchname>.merge' in your configuration file."
-		echo "Please specify which branch you want to merge on the command"
-		echo "line and try again (e.g. 'git pull <repository> <refspec>')."
-		echo "See git-pull(1) for details."
+		advice_detached_head "$advice"
 	else
-		echo "You asked me to pull without telling me which branch you"
-		echo "want to merge with, and 'branch.${curr_branch}.merge' in"
-		echo "your configuration file does not tell me either.	Please"
-		echo "specify which branch you want to merge on the command line and"
-		echo "try again (e.g. 'git pull <repository> <refspec>')."
-		echo "See git-pull(1) for details."
-		echo
-		echo "If you often merge with the same branch, you may want to"
-		echo "configure the following variables in your configuration"
-		echo "file:"
-		echo
-		echo "    branch.${curr_branch}.remote = <nickname>"
-		echo "    branch.${curr_branch}.merge = <remote-ref>"
-		echo "    remote.<nickname>.url = <url>"
-		echo "    remote.<nickname>.fetch = <refspec>"
-		echo
-		echo "See git-config(1) for details."
+		advice_no_merge_candidate "$advice" "$curr_branch"
 	fi
 	exit 1
 }

Re: Confusing git pull error message

[Jeff King]

On Sun, Sep 13, 2009 at 02:16:39PM -0700, Junio C Hamano wrote:

> I am _not_ going to commit the following patch, because it will interfere
> with this clarification effort, but I think we want to do something along
> this line after the "clarification" settles.

Please, yes. When I first suggested the advice.* mechanism, I had a
feeling in the back of my mind that there were several such messages
which had bothered me, but I didn't read through the code base looking
for them. But now that you mention it, this "no merge candidate" message
is definitely one of them (not even because it's wrong or anything, but
because of sheer _size_ of it).

> An observation I'd like to make is that this is way too ugly:
> 
> 	[advice]
>         	pullNoMergeFound = false
>                 pushNonFastForward = false
>                 statusHints = false
> 
> than
> 
> 	[IKnowWhatIAmDoingThankYouVeryMuch]
>         	pullNoMergeFound
>                 pushNonFastForward
>                 statusHints
> 
> but this feature is for people who know what they are doing, so I guess
> the current set-up would be fine.

Maybe a nice shorthand would be '!' for negative booleans. I.e.,

  [advice]
  !pullNoMergeFound
  !pushNonFastForward
  !statusHints

-Peff

Re: Confusing git pull error message

[Jeff King]

On Sun, Sep 13, 2009 at 01:38:48PM -0700, Junio C Hamano wrote:

> I saw some discussion on improving the wording.  Here is what I plan to
> commit.
> 
> diff --git a/git-pull.sh b/git-pull.sh
> index 0bbd5bf..2c2fa79 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -89,6 +89,8 @@ error_on_no_merge_candidates () {
>  	done
>  
>  	curr_branch=${curr_branch#refs/heads/}
> +	upstream=$(git config "branch.$curr_branch.merge" ||
> +			git config "branch.$curr_branch.rebase")

Argh, I made a mistake in my original patch which was retained in your
version. For some reason I was thinking that "branch.*.rebase" was an
_alternative_ to branch.*.merge, but it is in fact a bool that modifies
how we interpret branch.*.merge (I think when the feature was originally
discussed, that was one such proposal, and as I am not a user of the
feature, I proceeded with a totally bogus mental model since then).

So this really should read simply:

  upstream=$(git config "branch.$curr_branch.merge")

-Peff

Re: Confusing git pull error message

[Johannes Sixt]

Junio C Hamano schrieb:
> Jeff King <peff@peff.net> writes:
>> I think it is enough for git-pull to just check whether the config
>> exists, and if so, guess that the ref was simply not fetched. IOW,
>> this:
> 
> Thanks.
> 
> I saw some discussion on improving the wording.  Here is what I plan to
> commit.
> 
> diff --git a/git-pull.sh b/git-pull.sh
> index 0bbd5bf..2c2fa79 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -89,6 +89,8 @@ error_on_no_merge_candidates () {
>  	done
>  
>  	curr_branch=${curr_branch#refs/heads/}
> +	upstream=$(git config "branch.$curr_branch.merge" ||
> +			git config "branch.$curr_branch.rebase")
>  
>  	if [ -z "$curr_branch" ]; then
>  		echo "You are not currently on a branch, so I cannot use any"
> @@ -96,7 +98,7 @@ error_on_no_merge_candidates () {
>  		echo "Please specify which branch you want to merge on the command"
>  		echo "line and try again (e.g. 'git pull <repository> <refspec>')."
>  		echo "See git-pull(1) for details."
> -	else
> +	elif [ -z "$upstream" ]; then
>  		echo "You asked me to pull without telling me which branch you"
>  		echo "want to merge with, and 'branch.${curr_branch}.merge' in"
>  		echo "your configuration file does not tell me either.	Please"
> @@ -114,6 +116,10 @@ error_on_no_merge_candidates () {
>  		echo "    remote.<nickname>.fetch = <refspec>"
>  		echo
>  		echo "See git-config(1) for details."
> +	else
> +		echo "Your configuration specifies to merge the ref"
> +		echo "'${upstream#refs/heads/}' from the remote, but no such ref"
> +		echo "was fetched."
>  	fi
>  	exit 1
>  }

Unfortunately, this is not water-tight. See what I just observed:

  $ git pull hk
  From /exports/repos/hk/viscovery
     9455552..6429037  master     -> hk/master
  Your configuration specifies to merge the ref
  'master' from the remote, but no such ref
  was fetched.

The message is confusing when it says "'master' was not fetched" when
clearly master _was_ fetched.

More importantly, the message is wrong to say that "Your configuration
specifies to merge the ref 'master' from the remote", because I have this
configuration:

  $ git config -l | egrep '^(remote|branch)'
  remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
  remote.origin.url=/exports/repos/js/viscovery
  branch.master.remote=origin
  branch.master.merge=refs/heads/master
  remote.hk.url=/exports/repos/hk/viscovery
  remote.hk.fetch=+refs/heads/*:refs/remotes/hk/*

i.e. while on master, I merge master from "origin", not from "hk".

-- Hannes

Re: Confusing git pull error message

[Jeff King]

On Mon, Oct 05, 2009 at 01:32:34PM +0200, Johannes Sixt wrote:

> Unfortunately, this is not water-tight. See what I just observed:
> 
>   $ git pull hk
>   From /exports/repos/hk/viscovery
>      9455552..6429037  master     -> hk/master
>   Your configuration specifies to merge the ref
>   'master' from the remote, but no such ref
>   was fetched.
> 
> The message is confusing when it says "'master' was not fetched" when
> clearly master _was_ fetched.
> 
> More importantly, the message is wrong to say that "Your configuration
> specifies to merge the ref 'master' from the remote", because I have this
> configuration:

Ah, yeah. Looking at %(upstream) from for-each-ref (which is how we
determine to show that message) always uses the configured remote. But
if we have asked for another remote, then that doesn't make much sense.

So I think we need something like this. I wasn't able to figure out a
test case to trigger the first code path below, though. It may not be
possible; if we give a refspec on the command line, either it will be a
candidate for merging or, if it does not exist, fetch will barf. So it
may be that we can just collapse it down to a single case.

diff --git a/git-pull.sh b/git-pull.sh
index edf3ce3..a831db5 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -97,6 +97,18 @@ error_on_no_merge_candidates () {
 		echo "Please specify which branch you want to merge on the command"
 		echo "line and try again (e.g. 'git pull <repository> <refspec>')."
 		echo "See git-pull(1) for details."
+	elif [ -n "$1" ]; then
+		if [ $# -gt 1 ]; then
+			echo "You asked to pull from the remote '$1', but none"
+			echo "of the things you asked to fetch were candidates"
+			echo "for merging."
+		else
+			echo "You asked to pull from the remote '$1', but did"
+			echo "not specify a branch to merge. Because this is"
+			echo "not the default configured remote for your current"
+			echo "branch, you must specify a branch on the command"
+			echo "line."
+		fi
 	elif [ -z "$upstream" ]; then
 		echo "You asked me to pull without telling me which branch you"
 		echo "want to merge with, and 'branch.${curr_branch}.merge' in"

Re: Confusing git pull error message

[Johannes Sixt]

Jeff King schrieb:
> On Mon, Oct 05, 2009 at 01:32:34PM +0200, Johannes Sixt wrote:
> Ah, yeah. Looking at %(upstream) from for-each-ref (which is how we
> determine to show that message) always uses the configured remote. But
> if we have asked for another remote, then that doesn't make much sense.
> 
> So I think we need something like this. I wasn't able to figure out a
> test case to trigger the first code path below, though. It may not be
> possible; if we give a refspec on the command line, either it will be a
> candidate for merging or, if it does not exist, fetch will barf. So it
> may be that we can just collapse it down to a single case.
> 
> diff --git a/git-pull.sh b/git-pull.sh
> index edf3ce3..a831db5 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -97,6 +97,18 @@ error_on_no_merge_candidates () {
>  		echo "Please specify which branch you want to merge on the command"
>  		echo "line and try again (e.g. 'git pull <repository> <refspec>')."
>  		echo "See git-pull(1) for details."
> +	elif [ -n "$1" ]; then
> +		if [ $# -gt 1 ]; then
> +			echo "You asked to pull from the remote '$1', but none"
> +			echo "of the things you asked to fetch were candidates"
> +			echo "for merging."
> +		else
> +			echo "You asked to pull from the remote '$1', but did"
> +			echo "not specify a branch to merge. Because this is"
> +			echo "not the default configured remote for your current"
> +			echo "branch, you must specify a branch on the command"
> +			echo "line."
> +		fi
>  	elif [ -z "$upstream" ]; then
>  		echo "You asked me to pull without telling me which branch you"
>  		echo "want to merge with, and 'branch.${curr_branch}.merge' in"

Thanks, this gives a much better error message.

But, can we *please* have a more pleasantly wrapped message, even if this
grows lines in the source code beyond the 80 char limit? Like:

	echo "You asked to pull from the remote '$1',"
	echo "but did not specify a branch to merge. Because this is"
	echo "not the default configured remote for your current branch,"
	echo "you must specify a branch on the command line."

I.e.:

- Reserve more room for a long $1 in the first line.

- Don't wrap lines in "current branch" and "command line" when they are at
the end of a major logical unit of the sentence.

-- Hannes

Re: Confusing git pull error message

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So I think we need something like this. I wasn't able to figure out a
> test case to trigger the first code path below, though. It may not be
> possible; if we give a refspec on the command line, either it will be a
> candidate for merging or, if it does not exist, fetch will barf. So it
> may be that we can just collapse it down to a single case.

I think you are right.

By the way, I think the other case arms in the case statement that has the
sole caller of this function are never reached, no?

Back when you added the check in a74b170 (git-pull: disallow implicit
merging to detached HEAD, 2007-01-15), $? referred to the error status of
reading HEAD as a symbolic-ref so the check did make sense, but cd67e4d
(Teach 'git pull' about --rebase, 2007-11-28) made a stupid mistake that
nobody noticed.

> diff --git a/git-pull.sh b/git-pull.sh
> index edf3ce3..a831db5 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -97,6 +97,18 @@ error_on_no_merge_candidates () {
>  		echo "Please specify which branch you want to merge on the command"
>  		echo "line and try again (e.g. 'git pull <repository> <refspec>')."
>  		echo "See git-pull(1) for details."
> +	elif [ -n "$1" ]; then
> +		if [ $# -gt 1 ]; then
> +			echo "You asked to pull from the remote '$1', but none"
> +			echo "of the things you asked to fetch were candidates"
> +			echo "for merging."
> +		else
> +			echo "You asked to pull from the remote '$1', but did"
> +			echo "not specify a branch to merge. Because this is"
> +			echo "not the default configured remote for your current"
> +			echo "branch, you must specify a branch on the command"
> +			echo "line."
> +		fi
>  	elif [ -z "$upstream" ]; then
>  		echo "You asked me to pull without telling me which branch you"
>  		echo "want to merge with, and 'branch.${curr_branch}.merge' in"
>

Re: Confusing git pull error message

[Jeff King]

On Mon, Oct 05, 2009 at 12:08:38PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > So I think we need something like this. I wasn't able to figure out a
> > test case to trigger the first code path below, though. It may not be
> > possible; if we give a refspec on the command line, either it will be a
> > candidate for merging or, if it does not exist, fetch will barf. So it
> > may be that we can just collapse it down to a single case.
> 
> I think you are right.

Nope, I'm not. I figured out one more case that it needs to handle.
Revised patch coming up in a few minutes.

> By the way, I think the other case arms in the case statement that has the
> sole caller of this function are never reached, no?
> 
> Back when you added the check in a74b170 (git-pull: disallow implicit
> merging to detached HEAD, 2007-01-15), $? referred to the error status of
> reading HEAD as a symbolic-ref so the check did make sense, but cd67e4d
> (Teach 'git pull' about --rebase, 2007-11-28) made a stupid mistake that
> nobody noticed.

Hmm. I'm not sure. I don't see how $? could not be zero, though, because
the last thing we run is a subshell with sed and tr. But beyond that, we
actually handle the detached case in error_on_no_merge_candidates
already. So I think that case statement can simply be collapsed to the
first case.

-Peff

Re: Confusing git pull error message

[Jeff King]

On Mon, Oct 05, 2009 at 03:12:57PM -0400, Jeff King wrote:

> > I think you are right.
> 
> Nope, I'm not. I figured out one more case that it needs to handle.
> Revised patch coming up in a few minutes.

OK, here it is, which I think covers all of the cases. I also re-wrapped
the text, as I agree with JSixt that it was pretty ugly. I also
re-wrapped some of the existing text, as it gave the very choppy:

  Your configuration specifies to merge the ref
  'foo' from the remote, but no such ref
  was fetched.

It would be really nice to just pipe it through 'fmt', but I suspect
that will create portability problems.

-- >8 --
Subject: [PATCH] pull: improve advice for unconfigured error case

There are several reasons a git-pull invocation might not
have anything marked for merge:

  1. We're not on a branch, so there is no branch
     configuration.

  2. We're on a branch, but there is no configuration for
     this branch.

  3. We fetched from the configured remote, but the
     configured branch to merge didn't get fetched (either
     it doesn't exist, or wasn't part of the fetch refspec).

  4. We fetched from the non-default remote, but didn't
     specify a branch to merge. We can't use the configured
     one because it applies to the default remote.

  5. We fetched from a specified remote, and a refspec was
     given, but it ended up not fetching anything (this is
     actually hard to do; if the refspec points to a remote
     branch and it doesn't exist, then fetch will fail and
     we never make it to this code path. But if you provide
     a wildcard refspec like

       refs/bogus/*:refs/remotes/origin/*

     then you can see this failure).

We have handled (1) and (2) for some time. Recently, commit
a6dbf88 added code to handle case (3).

This patch handles cases (4) and (5), which previously just
fell under other cases, producing a confusing message.

While we're at it, let's rewrap the text for case (3), which
looks terribly ugly as it is.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-pull.sh |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index edf3ce3..8b0b6f6 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -90,8 +90,17 @@ error_on_no_merge_candidates () {
 
 	curr_branch=${curr_branch#refs/heads/}
 	upstream=$(git config "branch.$curr_branch.merge")
+	remote=$(git config "branch.$curr_branch.remote")
 
-	if [ -z "$curr_branch" ]; then
+	if [ $# -gt 1 ]; then
+		echo "There are no candidates for merging in the refs that you just fetched."
+		echo "Generally this means that you provided a wildcard refspec which had no"
+		echo "matches on the remote end."
+	elif [ $# -gt 0 ] && [ "$1" != "$remote" ]; then
+		echo "You asked to pull from the remote '$1', but did not specify"
+		echo "a branch to merge. Because this is not the default configured remote"
+		echo "for your current branch, you must specify a branch on the command line."
+	elif [ -z "$curr_branch" ]; then
 		echo "You are not currently on a branch, so I cannot use any"
 		echo "'branch.<branchname>.merge' in your configuration file."
 		echo "Please specify which branch you want to merge on the command"
@@ -116,9 +125,8 @@ error_on_no_merge_candidates () {
 		echo
 		echo "See git-config(1) for details."
 	else
-		echo "Your configuration specifies to merge the ref"
-		echo "'${upstream#refs/heads/}' from the remote, but no such ref"
-		echo "was fetched."
+		echo "Your configuration specifies to merge the ref '${upstream#refs/heads/}' from the"
+		echo "remote, but no such ref was fetched."
 	fi
 	exit 1
 }
-- 
1.6.5.rc2.219.g00ca

Re: Confusing git pull error message

[Nanako Shiraishi]

Quoting Jeff King <peff@peff.net>

> On Mon, Oct 05, 2009 at 03:12:57PM -0400, Jeff King wrote:
>
>> > I think you are right.
>> 
>> Nope, I'm not. I figured out one more case that it needs to handle.
>> Revised patch coming up in a few minutes.
>
> OK, here it is, which I think covers all of the cases. I also re-wrapped
> the text, as I agree with JSixt that it was pretty ugly. I also
> re-wrapped some of the existing text, as it gave the very choppy:
>
>   Your configuration specifies to merge the ref
>   'foo' from the remote, but no such ref
>   was fetched.
>
> It would be really nice to just pipe it through 'fmt', but I suspect
> that will create portability problems.
>
> -- >8 --
> Subject: [PATCH] pull: improve advice for unconfigured error case
>
> There are several reasons a git-pull invocation might not
> have anything marked for merge:
>
>   1. We're not on a branch, so there is no branch
>      configuration.
>
>   2. We're on a branch, but there is no configuration for
>      this branch.
>
>   3. We fetched from the configured remote, but the
>      configured branch to merge didn't get fetched (either
>      it doesn't exist, or wasn't part of the fetch refspec).
>
>   4. We fetched from the non-default remote, but didn't
>      specify a branch to merge. We can't use the configured
>      one because it applies to the default remote.
>
>   5. We fetched from a specified remote, and a refspec was
>      given, but it ended up not fetching anything (this is
>      actually hard to do; if the refspec points to a remote
>      branch and it doesn't exist, then fetch will fail and
>      we never make it to this code path. But if you provide
>      a wildcard refspec like
>
>        refs/bogus/*:refs/remotes/origin/*
>
>      then you can see this failure).
>
> We have handled (1) and (2) for some time. Recently, commit
> a6dbf88 added code to handle case (3).
>
> This patch handles cases (4) and (5), which previously just
> fell under other cases, producing a confusing message.
>
> While we're at it, let's rewrap the text for case (3), which
> looks terribly ugly as it is.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  git-pull.sh |   16 ++++++++++++----
>  1 files changed, 12 insertions(+), 4 deletions(-)

Junio, may I ask what happened to this patch?

Re: Confusing git pull error message

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> Quoting Jeff King <peff@peff.net>
>
>> Subject: [PATCH] pull: improve advice for unconfigured error case
>> ...
> Junio, may I ask what happened to this patch?

Thanks for prodding.  Unfortunately I lost track.  Will look at it again
but probably not tonight.

Re: Confusing git pull error message

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Nanako Shiraishi <nanako3@lavabit.com> writes:
>
>> Quoting Jeff King <peff@peff.net>
>>
>>> Subject: [PATCH] pull: improve advice for unconfigured error case
>>> ...
>> Junio, may I ask what happened to this patch?
>
> Thanks for prodding.  Unfortunately I lost track.  Will look at it again
> but probably not tonight.

Applied the patch as-is, as it looked quite good.

Thanks.

Re: Confusing git pull error message

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Oct 05, 2009 at 12:08:38PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > So I think we need something like this. I wasn't able to figure out a
>> > test case to trigger the first code path below, though. It may not be
>> > possible; if we give a refspec on the command line, either it will be a
>> > candidate for merging or, if it does not exist, fetch will barf. So it
>> > may be that we can just collapse it down to a single case.
>> 
>> I think you are right.
>
> Nope, I'm not. I figured out one more case that it needs to handle.
> Revised patch coming up in a few minutes.
>
>> By the way, I think the other case arms in the case statement that has the
>> sole caller of this function are never reached, no?
>> 
>> Back when you added the check in a74b170 (git-pull: disallow implicit
>> merging to detached HEAD, 2007-01-15), $? referred to the error status of
>> reading HEAD as a symbolic-ref so the check did make sense, but cd67e4d
>> (Teach 'git pull' about --rebase, 2007-11-28) made a stupid mistake that
>> nobody noticed.
>
> Hmm. I'm not sure. I don't see how $? could not be zero, though, because
> the last thing we run is a subshell with sed and tr. But beyond that, we
> actually handle the detached case in error_on_no_merge_candidates
> already. So I think that case statement can simply be collapsed to the
> first case.
>
> -Peff

-- >8 --
Subject: [PATCH] git-pull: dead code removal

Back when a74b170 (git-pull: disallow implicit merging to detached HEAD,
2007-01-15) added this check, $? referred to the error status of reading
HEAD as a symbolic-ref; but cd67e4d (Teach 'git pull' about --rebase,
2007-11-28) moved the command away from where the check is, and nobody
noticed the breakage.  Ever since, $? has always been 0 (tr at the end of
the pipe to find merge_head never fails) and other case arms were never
reached.

These days, error_on_no_merge_candidates function is prepared to handle a
detached HEAD case, which was what the code this patch removes used to
handle.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-pull.sh |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index edf3ce3..66d73eb 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -182,14 +182,7 @@ merge_head=$(sed -e '/	not-for-merge	/d' \
 
 case "$merge_head" in
 '')
-	case $? in
-	0) error_on_no_merge_candidates "$@";;
-	1) echo >&2 "You are not currently on a branch; you must explicitly"
-	   echo >&2 "specify which branch you wish to merge:"
-	   echo >&2 "  git pull <remote> <branch>"
-	   exit 1;;
-	*) exit $?;;
-	esac
+	error_on_no_merge_candidates "$@"
 	;;
 ?*' '?*)
 	if test -z "$orig_head"
-- 
1.6.5.rc2.77.g2ea4a

Re: Confusing git pull error message

[Jeff King]

On Mon, Oct 05, 2009 at 12:36:05PM -0700, Junio C Hamano wrote:

> -- >8 --
> Subject: [PATCH] git-pull: dead code removal
> 
> Back when a74b170 (git-pull: disallow implicit merging to detached HEAD,
> 2007-01-15) added this check, $? referred to the error status of reading
> HEAD as a symbolic-ref; but cd67e4d (Teach 'git pull' about --rebase,
> 2007-11-28) moved the command away from where the check is, and nobody
> noticed the breakage.  Ever since, $? has always been 0 (tr at the end of
> the pipe to find merge_head never fails) and other case arms were never
> reached.
> 
> These days, error_on_no_merge_candidates function is prepared to handle a
> detached HEAD case, which was what the code this patch removes used to
> handle.

Looks good to me.

Acked-by: Jeff King <peff@peff.net>

-Peff