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

[Dmitry]

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]

Post by Dmitry
git pull origin matser
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]

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 <***@yandex.ru>
Signed-off-by: Jeff King <***@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)

[Keller, Jacob E]

Post by Jeff King
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

Post by Jeff King
In this case, it produces a useful message, but then
$ 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
---
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;

[Jeff King]

Post by Keller, Jacob E

Post by Jeff King
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]

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

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, it can still notice the non-zero exit code.

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

Signed-off-by: Jeff King <***@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)

[Jeff King]

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

[Jeff King]

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