threads / discuss / 37084

"Unexpected end of command stream" message looks irrelevant when I try to pull a non-existing branch

Subject: "Unexpected end of command stream" message looks irrelevant when I try to pull a non-existing branch

## tl;dr

8 messages between Jul 9, 2014 and Jul 9, 2014.

replies: 7people: 3as markdown or json

Dmitry· Jul 9, 2014, 07:37 UTC · lore

Hi,

I'm using Git 1.8.1 and when I run the following command:

git pull origin matser

I get the following output:

fatal: couldn't find remote ref matser Unexpected end of command stream

The first line in the output is right on the money but the second one looks completely irrelevant - the command is well formed except I perhaps mistyped the branch name. It looks like there's some bug that prevents the program from just exiting after printing the first line and so the second line is being output.

Could you please fix this?

Thank you. Dmitry. --

Jeff King· Jul 9, 2014, 20:59 UTC · re: Dmitry · lore

Re: "Unexpected end of command stream" message looks irrelevant when I try to pull a non-existing branch

On Wed, Jul 09, 2014 at 11:37:51AM +0400, Dmitry wrote:

Show 14 quoted lines

> I'm using Git 1.8.1 and when I run the following command:
> 
> git pull origin matser
> 
> I get the following output:
> 
> fatal: couldn't find remote ref matser
> Unexpected end of command stream
> 
> The first line in the output is right on the money but the second one
> looks completely irrelevant - the command is well formed except I
> perhaps mistyped the branch name. It looks like there's some bug that
> prevents the program from just exiting after printing the first line
> and so the second line is being output.

I imagine your origin remote is over http. For some protocols, git delegates the hard work to a helper program and communicates over a pipe. In this case, the parent git process detects a problem and dies. The second message comes from the helper, who is surprised that the parent has gone away.

Probably the right solution is teaching the parent to properly hang up the connection with the helper before exiting (alternatively, we could just silence the helper; that means we would get less output when the parent really does unexpectedly go away, but that isn't supposed to ever happen).

-Peff

Jeff King· Jul 9, 2014, 21:20 UTC · re: Jeff King · lore

[PATCH] remote-curl: do not complain on EOF from parent git

The parent git process is supposed to send us an empty line to indicate that the conversation is over. However, the parent process may die() if there is a problem with the operaiton (e.g., we try to fetch a ref that does not exist). In this case, it produces a useful message, but then remote-curl _also_ produces an unhelpful message:

  $ git pull origin matser
  fatal: couldn't find remote ref matser
  Unexpected end of command stream

The "right" way to fix this is to teach the parent git to always cleanly close the connection to the helper, letting it know that we are done. Implementing that is rather clunky, though, as it would involve either replacing die() operations with returning errors up the stack (until we disconnect the transport), or adding an atexit handler to clean up any transport helpers left open.

It's much simpler to just suppress the EOF message in remote-curl. It was not added to address any real-world situation in the first place, but rather a "we should probably report unexpected things" suggestion[1].

It is the parent git which drives the operation, and whose exit value actually matters. If the parent dies, then the helper has no need to complain (except as a debugging aid). In the off chance that the pipe is closed without the parent dying, the parent can still notice the non-zero exit code.

[1] http://article.gmane.org/gmane.comp.version-control.git/176036

Reported-by: Dmitry <wipedout@yandex.ru>
Signed-off-by: Jeff King <peff@peff.net>
---
The original discussion that led to this code being implemented was due
to us checking the helper's exit code in the first place. However, we
seem to be inconsistent about doing so. I'm not inclined to pursue it
further, though, as these subtle details of the transport helper code
usually turn into a can of worms, and more importantly, I don't think it
hurts anything in the real world. Either the parent git gets the
expected protocol output from the helper or it doesn't, and we report
errors on that. An error from a helper after the operation completes is
not really important to the parent git either way.

 remote-curl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 4493b38..0454ffc 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -971,8 +971,6 @@ int main(int argc, const char **argv)
 		if (strbuf_getline(&buf, stdin, '\n') == EOF) {
 			if (ferror(stdin))
 				fprintf(stderr, "Error reading command stream\n");
-			else
-				fprintf(stderr, "Unexpected end of command stream\n");
 			return 1;
 		}
 		if (buf.len == 0)

-- 
2.0.0.566.gfe3e6b2

Keller, Jacob E· Jul 9, 2014, 21:25 UTC · re: Jeff King · lore

Re: [PATCH] remote-curl: do not complain on EOF from parent git

On Wed, 2014-07-09 at 17:20 -0400, Jeff King wrote:

> The parent git process is supposed to send us an empty line
> to indicate that the conversation is over. However, the
> parent process may die() if there is a problem with the
> operaiton (e.g., we try to fetch a ref that does not exist).

Nitpick, but you probably meant operation here.

Thanks, Jake

Show 57 quoted lines

> In this case, it produces a useful message, but then
> remote-curl _also_ produces an unhelpful message:
> 
>   $ git pull origin matser
>   fatal: couldn't find remote ref matser
>   Unexpected end of command stream
> 
> The "right" way to fix this is to teach the parent git to
> always cleanly close the connection to the helper, letting
> it know that we are done. Implementing that is rather
> clunky, though, as it would involve either replacing die()
> operations with returning errors up the stack (until we
> disconnect the transport), or adding an atexit handler to
> clean up any transport helpers left open.
> 
> It's much simpler to just suppress the EOF message in
> remote-curl. It was not added to address any real-world
> situation in the first place, but rather a "we should
> probably report unexpected things" suggestion[1].
> 
> It is the parent git which drives the operation, and whose
> exit value actually matters. If the parent dies, then the
> helper has no need to complain (except as a debugging aid).
> In the off chance that the pipe is closed without the parent
> dying, the parent can still notice the non-zero exit code.
> 
> [1] http://article.gmane.org/gmane.comp.version-control.git/176036
> 
> Reported-by: Dmitry <wipedout@yandex.ru>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> The original discussion that led to this code being implemented was due
> to us checking the helper's exit code in the first place. However, we
> seem to be inconsistent about doing so. I'm not inclined to pursue it
> further, though, as these subtle details of the transport helper code
> usually turn into a can of worms, and more importantly, I don't think it
> hurts anything in the real world. Either the parent git gets the
> expected protocol output from the helper or it doesn't, and we report
> errors on that. An error from a helper after the operation completes is
> not really important to the parent git either way.
> 
>  remote-curl.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/remote-curl.c b/remote-curl.c
> index 4493b38..0454ffc 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -971,8 +971,6 @@ int main(int argc, const char **argv)
>  		if (strbuf_getline(&buf, stdin, '\n') == EOF) {
>  			if (ferror(stdin))
>  				fprintf(stderr, "Error reading command stream\n");
> -			else
> -				fprintf(stderr, "Unexpected end of command stream\n");
>  			return 1;
>  		}
>  		if (buf.len == 0)

Jeff King· Jul 9, 2014, 21:42 UTC · re: Keller, Jacob E · lore

Re: [PATCH] remote-curl: do not complain on EOF from parent git

On Wed, Jul 09, 2014 at 09:25:18PM +0000, Keller, Jacob E wrote:

Show 7 quoted lines

> On Wed, 2014-07-09 at 17:20 -0400, Jeff King wrote:
> > The parent git process is supposed to send us an empty line
> > to indicate that the conversation is over. However, the
> > parent process may die() if there is a problem with the
> > operaiton (e.g., we try to fetch a ref that does not exist). 
> 
> Nitpick, but you probably meant operation here.

I did (and I probably meant to turn on spellcheck, too...).

I'll repost in a minute, as I have some follow-on patches. Thanks.

-Peff

Jeff King· Jul 9, 2014, 21:45 UTC · re: Jeff King · lore

[PATCH 1/3] remote-curl: do not complain on EOF from parent git

The parent git process is supposed to send us an empty line to indicate that the conversation is over. However, the parent process may die() if there is a problem with the operation (e.g., we try to fetch a ref that does not exist). In this case, it produces a useful message, but then remote-curl _also_ produces an unhelpful message:

  $ git pull origin matser
  fatal: couldn't find remote ref matser
  Unexpected end of command stream

[1] http://article.gmane.org/gmane.comp.version-control.git/176036

Signed-off-by: Jeff King <peff@peff.net>
---
Repost fixing commit message typo (and indicating it as the first of 3
patches).

 remote-curl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 4493b38..0454ffc 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -971,8 +971,6 @@ int main(int argc, const char **argv)
 		if (strbuf_getline(&buf, stdin, '\n') == EOF) {
 			if (ferror(stdin))
 				fprintf(stderr, "Error reading command stream\n");
-			else
-				fprintf(stderr, "Unexpected end of command stream\n");
 			return 1;
 		}
 		if (buf.len == 0)

-- 
2.0.0.566.gfe3e6b2

Jeff King· Jul 9, 2014, 21:47 UTC · re: Jeff King · lore

[PATCH 2/3] remote-curl: use error instead of fprintf(stderr)

We usually prefix our error messages with "error: ", but many error messages from remote-curl are simply printed with fprintf. This can make the output a little harder to read (especially because such message may be intermingled with errors from the parent git process).

There is no reason to avoid error(), as we are already calling it many places (in addition to libgit.a functions which use it).

While we're adjusting the messages, we can also drop the capitalization which makes them unlike other git error messages.

Signed-off-by: Jeff King <peff@peff.net>
---
I suspect there may be some more improvements we can make (e.g., the
"fetch failed" below seems like it might be redundant with what "git
fetch" will print). But hunting them is a lot of work for little gain;
I'd rather wait to see them in practice and fix them on a case by case
basis.

 remote-curl.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 0454ffc..a619b64 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -399,7 +399,7 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
 			rpc->pos = 0;
 			return CURLIOE_OK;
 		}
-		fprintf(stderr, "Unable to rewind rpc post data - try increasing http.postBuffer\n");
+		error("unable to rewind rpc post data - try increasing http.postBuffer");
 		return CURLIOE_FAILRESTART;
 
 	default:
@@ -709,7 +709,7 @@ static int fetch_dumb(int nr_heads, struct ref **to_fetch)
 		free(targets[i]);
 	free(targets);
 
-	return ret ? error("Fetch failed.") : 0;
+	return ret ? error("fetch failed.") : 0;
 }
 
 static int fetch_git(struct discovery *heads,
@@ -949,7 +949,7 @@ int main(int argc, const char **argv)
 	git_extract_argv0_path(argv[0]);
 	setup_git_directory_gently(&nongit);
 	if (argc < 2) {
-		fprintf(stderr, "Remote needed\n");
+		error("remote needed");
 		return 1;
 	}
 
@@ -970,7 +970,7 @@ int main(int argc, const char **argv)
 	do {
 		if (strbuf_getline(&buf, stdin, '\n') == EOF) {
 			if (ferror(stdin))
-				fprintf(stderr, "Error reading command stream\n");
+				error("error reading command stream");
 			return 1;
 		}
 		if (buf.len == 0)
@@ -1014,7 +1014,7 @@ int main(int argc, const char **argv)
 			printf("\n");
 			fflush(stdout);
 		} else {
-			fprintf(stderr, "Unknown command '%s'\n", buf.buf);
+			error("unknown command '%s'", buf.buf);
 			return 1;
 		}
 		strbuf_reset(&buf);

-- 
2.0.0.566.gfe3e6b2

Jeff King· Jul 9, 2014, 21:47 UTC · re: Jeff King · lore

[PATCH 3/3] remote-curl: mark helper-protocol errors more clearly

When we encounter an error in remote-curl, we generally just report it to stderr. There is no need for the user to care that the "could not connect to server" error was generated by git-remote-https rather than a function in the parent git-fetch process.

However, when the error is in the protocol between git and the helper, it makes sense to clearly identify which side is complaining. These cases shouldn't ever happen, but when they do, we can make them less confusing by being more verbose.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote-curl.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index a619b64..1f6905d 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -949,7 +949,7 @@ int main(int argc, const char **argv)
 	git_extract_argv0_path(argv[0]);
 	setup_git_directory_gently(&nongit);
 	if (argc < 2) {
-		error("remote needed");
+		error("remote-curl: usage: git remote-curl <remote> [<url>]");
 		return 1;
 	}
 
@@ -970,14 +970,14 @@ int main(int argc, const char **argv)
 	do {
 		if (strbuf_getline(&buf, stdin, '\n') == EOF) {
 			if (ferror(stdin))
-				error("error reading command stream");
+				error("remote-curl: error reading command stream from git");
 			return 1;
 		}
 		if (buf.len == 0)
 			break;
 		if (starts_with(buf.buf, "fetch ")) {
 			if (nongit)
-				die("Fetch attempted without a local repo");
+				die("remote-curl: fetch attempted without a local repo");
 			parse_fetch(&buf);
 
 		} else if (!strcmp(buf.buf, "list") || starts_with(buf.buf, "list ")) {
@@ -1014,7 +1014,7 @@ int main(int argc, const char **argv)
 			printf("\n");
 			fflush(stdout);
 		} else {
-			error("unknown command '%s'", buf.buf);
+			error("remote-curl: unknown command '%s' from git", buf.buf);
 			return 1;
 		}
 		strbuf_reset(&buf);

-- 
2.0.0.566.gfe3e6b2