{"thread":{"id":"37084","subject":"\"Unexpected end of command stream\" message looks irrelevant when I try to pull a non-existing branch","startedAt":"2014-07-09T07:37:51Z","lastAt":"2014-07-09T21:47:20Z","messageCount":8,"participants":["Dmitry","Jeff King","Keller, Jacob E"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"245613","messageId":"787461404891471@web12h.yandex.ru","threadId":"37084","inReplyTo":null,"subject":"\"Unexpected end of command stream\" message looks irrelevant when I try to pull a non-existing branch","fromName":"Dmitry","fromEmail":"wipedout@yandex.ru","sentAt":"2014-07-09T07:37:51Z","receivedAt":"2014-07-09T07:37:51Z","isPatch":false,"sender":{"key":"wipedout@yandex.ru","avatar":null},"body":"Hi,\n\nI'm using Git 1.8.1 and when I run the following command:\n\ngit pull origin matser\n\nI get the following output:\n\nfatal: couldn't find remote ref matser\nUnexpected end of command stream\n\nThe 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.\n\nCould you please fix this?\n\nThank you.\nDmitry.\n--\n"},{"id":"245655","messageId":"20140709205902.GB25854@sigill.intra.peff.net","threadId":"37084","inReplyTo":"787461404891471@web12h.yandex.ru","subject":"Re: \"Unexpected end of command stream\" message looks irrelevant when I try to pull a non-existing branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-09T20:59:02Z","receivedAt":"2014-07-09T20:59:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 09, 2014 at 11:37:51AM +0400, Dmitry wrote:\n\n> I'm using Git 1.8.1 and when I run the following command:\n> \n> git pull origin matser\n> \n> I get the following output:\n> \n> fatal: couldn't find remote ref matser\n> Unexpected end of command stream\n> \n> The first line in the output is right on the money but the second one\n> looks completely irrelevant - the command is well formed except I\n> perhaps mistyped the branch name. It looks like there's some bug that\n> prevents the program from just exiting after printing the first line\n> and so the second line is being output.\n\nI imagine your origin remote is over http. For some protocols, git\ndelegates the hard work to a helper program and communicates over a\npipe. In this case, the parent git process detects a problem and dies.\nThe second message comes from the helper, who is surprised that the\nparent has gone away.\n\nProbably the right solution is teaching the parent to properly hang up\nthe connection with the helper before exiting (alternatively, we could\njust silence the helper; that means we would get less output when the\nparent really does unexpectedly go away, but that isn't supposed to ever\nhappen).\n\n-Peff\n"},{"id":"245657","messageId":"20140709212043.GC25854@sigill.intra.peff.net","threadId":"37084","inReplyTo":"20140709205902.GB25854@sigill.intra.peff.net","subject":"[PATCH] remote-curl: do not complain on EOF from parent git","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-09T21:20:43Z","receivedAt":"2014-07-09T21:20:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The parent git process is supposed to send us an empty line\nto indicate that the conversation is over. However, the\nparent process may die() if there is a problem with the\noperaiton (e.g., we try to fetch a ref that does not exist).\nIn this case, it produces a useful message, but then\nremote-curl _also_ produces an unhelpful message:\n\n $ git pull origin matser\n fatal: couldn't find remote ref matser\n Unexpected end of command stream\n\nThe \"right\" way to fix this is to teach the parent git to\nalways cleanly close the connection to the helper, letting\nit know that we are done. Implementing that is rather\nclunky, though, as it would involve either replacing die()\noperations with returning errors up the stack (until we\ndisconnect the transport), or adding an atexit handler to\nclean up any transport helpers left open.\n\nIt's much simpler to just suppress the EOF message in\nremote-curl. It was not added to address any real-world\nsituation in the first place, but rather a \"we should\nprobably report unexpected things\" suggestion[1].\n\nIt is the parent git which drives the operation, and whose\nexit value actually matters. If the parent dies, then the\nhelper has no need to complain (except as a debugging aid).\nIn the off chance that the pipe is closed without the parent\ndying, the parent can still notice the non-zero exit code.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/176036\n\nReported-by: Dmitry \nSigned-off-by: Jeff King \n---\nThe original discussion that led to this code being implemented was due\nto us checking the helper's exit code in the first place. However, we\nseem to be inconsistent about doing so. I'm not inclined to pursue it\nfurther, though, as these subtle details of the transport helper code\nusually turn into a can of worms, and more importantly, I don't think it\nhurts anything in the real world. Either the parent git gets the\nexpected protocol output from the helper or it doesn't, and we report\nerrors on that. An error from a helper after the operation completes is\nnot really important to the parent git either way.\n\n remote-curl.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 4493b38..0454ffc 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -971,8 +971,6 @@ int main(int argc, const char **argv)\n \t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF) {\n \t\t\tif (ferror(stdin))\n \t\t\t\tfprintf(stderr, \"Error reading command stream\\n\");\n-\t\t\telse\n-\t\t\t\tfprintf(stderr, \"Unexpected end of command stream\\n\");\n \t\t\treturn 1;\n \t\t}\n \t\tif (buf.len == 0)\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245659","messageId":"1404941118.23510.34.camel@jekeller-desk1.amr.corp.intel.com","threadId":"37084","inReplyTo":"20140709212043.GC25854@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: do not complain on EOF from parent git","fromName":"Keller, Jacob E","fromEmail":"jacob.e.keller@intel.com","sentAt":"2014-07-09T21:25:18Z","receivedAt":"2014-07-09T21:25:18Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, 2014-07-09 at 17:20 -0400, Jeff King wrote:\n> The parent git process is supposed to send us an empty line\n> to indicate that the conversation is over. However, the\n> parent process may die() if there is a problem with the\n> operaiton (e.g., we try to fetch a ref that does not exist). \n\nNitpick, but you probably meant operation here.\n\nThanks,\nJake\n\n> In this case, it produces a useful message, but then\n> remote-curl _also_ produces an unhelpful message:\n> \n> $ git pull origin matser\n> fatal: couldn't find remote ref matser\n> Unexpected end of command stream\n> \n> The \"right\" way to fix this is to teach the parent git to\n> always cleanly close the connection to the helper, letting\n> it know that we are done. Implementing that is rather\n> clunky, though, as it would involve either replacing die()\n> operations with returning errors up the stack (until we\n> disconnect the transport), or adding an atexit handler to\n> clean up any transport helpers left open.\n> \n> It's much simpler to just suppress the EOF message in\n> remote-curl. It was not added to address any real-world\n> situation in the first place, but rather a \"we should\n> probably report unexpected things\" suggestion[1].\n> \n> It is the parent git which drives the operation, and whose\n> exit value actually matters. If the parent dies, then the\n> helper has no need to complain (except as a debugging aid).\n> In the off chance that the pipe is closed without the parent\n> dying, the parent can still notice the non-zero exit code.\n> \n> [1] http://article.gmane.org/gmane.comp.version-control.git/176036\n> \n> Reported-by: Dmitry \n> Signed-off-by: Jeff King \n> ---\n> The original discussion that led to this code being implemented was due\n> to us checking the helper's exit code in the first place. However, we\n> seem to be inconsistent about doing so. I'm not inclined to pursue it\n> further, though, as these subtle details of the transport helper code\n> usually turn into a can of worms, and more importantly, I don't think it\n> hurts anything in the real world. Either the parent git gets the\n> expected protocol output from the helper or it doesn't, and we report\n> errors on that. An error from a helper after the operation completes is\n> not really important to the parent git either way.\n> \n> remote-curl.c | 2 --\n> 1 file changed, 2 deletions(-)\n> \n> diff --git a/remote-curl.c b/remote-curl.c\n> index 4493b38..0454ffc 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -971,8 +971,6 @@ int main(int argc, const char **argv)\n> \t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF) {\n> \t\t\tif (ferror(stdin))\n> \t\t\t\tfprintf(stderr, \"Error reading command stream\\n\");\n> -\t\t\telse\n> -\t\t\t\tfprintf(stderr, \"Unexpected end of command stream\\n\");\n> \t\t\treturn 1;\n> \t\t}\n> \t\tif (buf.len == 0)\n\n\n"},{"id":"245660","messageId":"20140709214217.GD25854@sigill.intra.peff.net","threadId":"37084","inReplyTo":"1404941118.23510.34.camel@jekeller-desk1.amr.corp.intel.com","subject":"Re: [PATCH] remote-curl: do not complain on EOF from parent git","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-09T21:42:17Z","receivedAt":"2014-07-09T21:42:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 09, 2014 at 09:25:18PM +0000, Keller, Jacob E wrote:\n\n> On Wed, 2014-07-09 at 17:20 -0400, Jeff King wrote:\n> > The parent git process is supposed to send us an empty line\n> > to indicate that the conversation is over. However, the\n> > parent process may die() if there is a problem with the\n> > operaiton (e.g., we try to fetch a ref that does not exist). \n> \n> Nitpick, but you probably meant operation here.\n\nI did (and I probably meant to turn on spellcheck, too...).\n\nI'll repost in a minute, as I have some follow-on patches. Thanks.\n\n-Peff\n"},{"id":"245661","messageId":"20140709214542.GA26999@sigill.intra.peff.net","threadId":"37084","inReplyTo":"20140709212043.GC25854@sigill.intra.peff.net","subject":"[PATCH 1/3] remote-curl: do not complain on EOF from parent git","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-09T21:45:43Z","receivedAt":"2014-07-09T21:45:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The parent git process is supposed to send us an empty line\nto indicate that the conversation is over. However, the\nparent process may die() if there is a problem with the\noperation (e.g., we try to fetch a ref that does not exist).\nIn this case, it produces a useful message, but then\nremote-curl _also_ produces an unhelpful message:\n\n $ git pull origin matser\n fatal: couldn't find remote ref matser\n Unexpected end of command stream\n\nThe \"right\" way to fix this is to teach the parent git to\nalways cleanly close the connection to the helper, letting\nit know that we are done. Implementing that is rather\nclunky, though, as it would involve either replacing die()\noperations with returning errors up the stack (until we\ndisconnect the transport), or adding an atexit handler to\nclean up any transport helpers left open.\n\nIt's much simpler to just suppress the EOF message in\nremote-curl. It was not added to address any real-world\nsituation in the first place, but rather a \"we should\nprobably report unexpected things\" suggestion[1].\n\nIt is the parent git which drives the operation, and whose\nexit value actually matters. If the parent dies, then the\nhelper has no need to complain (except as a debugging aid).\nIn the off chance that the pipe is closed without the parent\ndying, it can still notice the non-zero exit code.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/176036\n\nSigned-off-by: Jeff King \n---\nRepost fixing commit message typo (and indicating it as the first of 3\npatches).\n\n remote-curl.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 4493b38..0454ffc 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -971,8 +971,6 @@ int main(int argc, const char **argv)\n \t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF) {\n \t\t\tif (ferror(stdin))\n \t\t\t\tfprintf(stderr, \"Error reading command stream\\n\");\n-\t\t\telse\n-\t\t\t\tfprintf(stderr, \"Unexpected end of command stream\\n\");\n \t\t\treturn 1;\n \t\t}\n \t\tif (buf.len == 0)\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245662","messageId":"20140709214705.GB26999@sigill.intra.peff.net","threadId":"37084","inReplyTo":"20140709212043.GC25854@sigill.intra.peff.net","subject":"[PATCH 2/3] remote-curl: use error instead of fprintf(stderr)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-09T21:47:05Z","receivedAt":"2014-07-09T21:47:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We usually prefix our error messages with \"error: \", but\nmany error messages from remote-curl are simply printed with\nfprintf. This can make the output a little harder to read\n(especially because such message may be intermingled with\nerrors from the parent git process).\n\nThere is no reason to avoid error(), as we are already\ncalling it many places (in addition to libgit.a functions\nwhich use it).\n\nWhile we're adjusting the messages, we can also drop the\ncapitalization which makes them unlike other git error\nmessages.\n\nSigned-off-by: Jeff King \n---\nI suspect there may be some more improvements we can make (e.g., the\n\"fetch failed\" below seems like it might be redundant with what \"git\nfetch\" will print). But hunting them is a lot of work for little gain;\nI'd rather wait to see them in practice and fix them on a case by case\nbasis.\n\n remote-curl.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 0454ffc..a619b64 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -399,7 +399,7 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n \t\t\trpc->pos = 0;\n \t\t\treturn CURLIOE_OK;\n \t\t}\n-\t\tfprintf(stderr, \"Unable to rewind rpc post data - try increasing http.postBuffer\\n\");\n+\t\terror(\"unable to rewind rpc post data - try increasing http.postBuffer\");\n \t\treturn CURLIOE_FAILRESTART;\n \n \tdefault:\n@@ -709,7 +709,7 @@ static int fetch_dumb(int nr_heads, struct ref **to_fetch)\n \t\tfree(targets[i]);\n \tfree(targets);\n \n-\treturn ret ? error(\"Fetch failed.\") : 0;\n+\treturn ret ? error(\"fetch failed.\") : 0;\n }\n \n static int fetch_git(struct discovery *heads,\n@@ -949,7 +949,7 @@ int main(int argc, const char **argv)\n \tgit_extract_argv0_path(argv[0]);\n \tsetup_git_directory_gently(&nongit);\n \tif (argc < 2) {\n-\t\tfprintf(stderr, \"Remote needed\\n\");\n+\t\terror(\"remote needed\");\n \t\treturn 1;\n \t}\n \n@@ -970,7 +970,7 @@ int main(int argc, const char **argv)\n \tdo {\n \t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF) {\n \t\t\tif (ferror(stdin))\n-\t\t\t\tfprintf(stderr, \"Error reading command stream\\n\");\n+\t\t\t\terror(\"error reading command stream\");\n \t\t\treturn 1;\n \t\t}\n \t\tif (buf.len == 0)\n@@ -1014,7 +1014,7 @@ int main(int argc, const char **argv)\n \t\t\tprintf(\"\\n\");\n \t\t\tfflush(stdout);\n \t\t} else {\n-\t\t\tfprintf(stderr, \"Unknown command '%s'\\n\", buf.buf);\n+\t\t\terror(\"unknown command '%s'\", buf.buf);\n \t\t\treturn 1;\n \t\t}\n \t\tstrbuf_reset(&buf);\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245663","messageId":"20140709214720.GC26999@sigill.intra.peff.net","threadId":"37084","inReplyTo":"20140709212043.GC25854@sigill.intra.peff.net","subject":"[PATCH 3/3] remote-curl: mark helper-protocol errors more clearly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-09T21:47:20Z","receivedAt":"2014-07-09T21:47:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we encounter an error in remote-curl, we generally just\nreport it to stderr. There is no need for the user to care\nthat the \"could not connect to server\" error was generated\nby git-remote-https rather than a function in the parent\ngit-fetch process.\n\nHowever, when the error is in the protocol between git and\nthe helper, it makes sense to clearly identify which side is\ncomplaining. These cases shouldn't ever happen, but when\nthey do, we can make them less confusing by being more\nverbose.\n\nSigned-off-by: Jeff King \n---\n remote-curl.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex a619b64..1f6905d 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -949,7 +949,7 @@ int main(int argc, const char **argv)\n \tgit_extract_argv0_path(argv[0]);\n \tsetup_git_directory_gently(&nongit);\n \tif (argc < 2) {\n-\t\terror(\"remote needed\");\n+\t\terror(\"remote-curl: usage: git remote-curl []\");\n \t\treturn 1;\n \t}\n \n@@ -970,14 +970,14 @@ int main(int argc, const char **argv)\n \tdo {\n \t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF) {\n \t\t\tif (ferror(stdin))\n-\t\t\t\terror(\"error reading command stream\");\n+\t\t\t\terror(\"remote-curl: error reading command stream from git\");\n \t\t\treturn 1;\n \t\t}\n \t\tif (buf.len == 0)\n \t\t\tbreak;\n \t\tif (starts_with(buf.buf, \"fetch \")) {\n \t\t\tif (nongit)\n-\t\t\t\tdie(\"Fetch attempted without a local repo\");\n+\t\t\t\tdie(\"remote-curl: fetch attempted without a local repo\");\n \t\t\tparse_fetch(&buf);\n \n \t\t} else if (!strcmp(buf.buf, \"list\") || starts_with(buf.buf, \"list \")) {\n@@ -1014,7 +1014,7 @@ int main(int argc, const char **argv)\n \t\t\tprintf(\"\\n\");\n \t\t\tfflush(stdout);\n \t\t} else {\n-\t\t\terror(\"unknown command '%s'\", buf.buf);\n+\t\t\terror(\"remote-curl: unknown command '%s' from git\", buf.buf);\n \t\t\treturn 1;\n \t\t}\n \t\tstrbuf_reset(&buf);\n-- \n2.0.0.566.gfe3e6b2\n"}]}