{"thread":{"id":"35671","subject":"git quietly fails on https:// URL, https errors are never reported to user","startedAt":"2014-01-16T12:27:03Z","lastAt":"2014-01-17T21:10:35Z","messageCount":9,"participants":["Yuri","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"233202","messageId":"52D7D017.107@rawbw.com","threadId":"35671","inReplyTo":null,"subject":"git quietly fails on https:// URL, https errors are never reported to user","fromName":"Yuri","fromEmail":"yuri@rawbw.com","sentAt":"2014-01-16T12:27:03Z","receivedAt":"2014-01-16T12:27:03Z","isPatch":false,"sender":{"key":"yuri@rawbw.com","avatar":null},"body":"On one of my FreeBSD systems I can't clone github through https URL.\nIt only says \"Cloning into 'MyProject'...\", writes some files, but then \ndeletes everything, without printing anything else at all. Exit code is 128.\n\nReplacing https:// with git:// makes it work fine.\n\nWhile debugging, I find that remove_junk() deletes all directories from \nunder __cxa_finalize.\nBefore this, exit(128) is called from recvline_fh (\"Debug: Remote helper \nquit.) And this function in turn is called from under\nrefs = transport_get_remote_refs(transport);\n\nI think you need to make sure that any https errors (in this and other \nlocations) are reported to the user, and git never quits on error \nwithout saying what the error is.\n\ngit-1.8.5.2\n\nYuri\n"},{"id":"233209","messageId":"20140116180310.GA27180@sigill.intra.peff.net","threadId":"35671","inReplyTo":"52D7D017.107@rawbw.com","subject":"Re: git quietly fails on https:// URL, https errors are never reported to user","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-16T18:03:10Z","receivedAt":"2014-01-16T18:03:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 16, 2014 at 04:27:03AM -0800, Yuri wrote:\n\n> While debugging, I find that remove_junk() deletes all directories\n> from under __cxa_finalize.\n> Before this, exit(128) is called from recvline_fh (\"Debug: Remote\n> helper quit.) And this function in turn is called from under\n> refs = transport_get_remote_refs(transport);\n> \n> I think you need to make sure that any https errors (in this and\n> other locations) are reported to the user, and git never quits on\n> error without saying what the error is.\n\nWe used to print \"Reading from helper 'git-remote-https' failed\" in this\ninstance. But in the majority of cases, remote-https has printed a\nuseful message already to stderr, and the extra line just confused\npeople. The downside, as you noticed, is that when the helper dies\nwithout printing an error, the user is left with no message.\n\nUnfortunately, detecting whether the helper printed a useful message is\nnon-trivial. It's possible we could do more detection based on the\nhelper's death (e.g., if it died by signal, print a message) and at\nleast say _something_.\n\nBut even if we do so, the message isn't going to tell you what went\nwrong, only that something unexpected happened. It is up to the helper\nto print something useful, and if it didn't, it should be fixed. So the\nmost important bug to fix here, IMHO, is figuring out why\ngit-remote-https died without printing an error message.\n\nIs it possible to strace (or truss) the helper process? What it was\ndoing when it died may be helpful. Rather than picking through \"strace\n-f\" output, you can use this hack to trace just the helper process:\n\n cat >/in/your/$PATH/git-remote-strace <<\\EOF\n #!/bin/sh\n protocol=$(echo \"$2\" | cut -d: -f1)\n exec strace git-remote-$protocol \"$@\"\n EOF\n\n git clone strace::https://github.com/your/repo.git\n\n-Peff\n"},{"id":"233214","messageId":"52D832CB.1010403@rawbw.com","threadId":"35671","inReplyTo":"20140116180310.GA27180@sigill.intra.peff.net","subject":"Re: git quietly fails on https:// URL, https errors are never reported to user","fromName":"Yuri","fromEmail":"yuri@rawbw.com","sentAt":"2014-01-16T19:28:11Z","receivedAt":"2014-01-16T19:28:11Z","isPatch":false,"sender":{"key":"yuri@rawbw.com","avatar":null},"body":"On 01/16/2014 10:03, Jeff King wrote:\n> We used to print \"Reading from helper 'git-remote-https' failed\" in this\n> instance. But in the majority of cases, remote-https has printed a\n> useful message already to stderr, and the extra line just confused\n> people. The downside, as you noticed, is that when the helper dies\n> without printing an error, the user is left with no message.\n\nIn my case it was some random misconfiguration of the libraries handling \nhttps that caused them to fail in some way silently. Full update of the \nsupporting libraries fixed this. But previous forced git update didn't \nfix it.\n\nI think your should bring back this printout. Errors only happen in a \nvery low percentage of cases, and printing some more would be very \nhelpful for troubleshooting. I am not sure what happened in \ngit-remote-https, stream may have looked legit to it, or maybe it got \nsome error from the supporting libraries and didn't report it. I can't \ntell now because I updated and the problem is gone.\n\nYuri\n"},{"id":"233271","messageId":"52D8FAA6.1010601@rawbw.com","threadId":"35671","inReplyTo":"20140116180310.GA27180@sigill.intra.peff.net","subject":"Re: git quietly fails on https:// URL, https errors are never reported to user","fromName":"Yuri","fromEmail":"yuri@rawbw.com","sentAt":"2014-01-17T09:40:54Z","receivedAt":"2014-01-17T09:40:54Z","isPatch":false,"sender":{"key":"yuri@rawbw.com","avatar":null},"body":"On 01/16/2014 10:03, Jeff King wrote:\n> We used to print \"Reading from helper 'git-remote-https' failed\" in this\n> instance. But in the majority of cases, remote-https has printed a\n> useful message already to stderr, and the extra line just confused\n> people. The downside, as you noticed, is that when the helper dies\n> without printing an error, the user is left with no message.\n\nI would like to suggest to return this printout, see patch below. This \nwould be a revert of this commit:\ncommit 266f1fdfa99f5d29ca7ce455966e7960c00a82e4\nAuthor: Jeff King \nDate: Fri Jun 21 03:05:39 2013 -0400\n\nI think that in a rare case of error this extra-printout wouldn't hurt. \nI also made this message more user friendly, without mentioning the term \n\"helper\".\n\nYuri\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 2010674..5ea2831 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -58,7 +58,7 @@ static int recvline_fh(FILE *helper, struct strbuf \n*buffer, const char *name)\n if (strbuf_getline(buffer, helper, '\\n') == EOF) {\n if (debug)\n fprintf(stderr, \"Debug: Remote helper quit.\\n\");\n- exit(128);\n+ die(\"Failure in '%s' protocol reader\", name);\n }\n\n if (debug)\n"},{"id":"233307","messageId":"xmqqmwiuwg0o.fsf@gitster.dls.corp.google.com","threadId":"35671","inReplyTo":"52D8FAA6.1010601@rawbw.com","subject":"Re: git quietly fails on https:// URL, https errors are never reported to user","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-17T19:43:35Z","receivedAt":"2014-01-17T19:43:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yuri writes:\n\n> I think that in a rare case of error this extra-printout wouldn't\n> hurt.\n\nIf the \"error is rare, extra verbiage does not hurt\" were a valid\nattitude, \"error is rare, non-zero exit is enough\" would be equally\nvalid ;-)\n\nAlso that statement contradicts with the rationale given by 266f1fdf\n(transport-helper: be quiet on read errors from helpers,\n2013-06-21), no?\n\n However, this makes a much more common case worse: when a helper\n does die with a useful message, we print the extra \"Reading from\n 'git-remote-foo failed\" message. This can also end up confusing\n users, as they may not even know what remote helpers are (e.g.,\n the fact that http support comes through git-remote-https is\n purely an implementation detail that most users do not know or\n care about).\n \nYour change is not an exact revert and rewords the message to read\n\n\tFailure in 'http' protocol reader.\n\ninstead of\n\n\tReading from helper 'git-remote-http' failed.\n\nwhich avoids the \"helper\" word and replacing it with \"protocol\nreader\" [*1*] in an attempt to make it less likely to \"end up\nconfusing users\", but I am not sure if \"protocol reader\" is good\nenough for those who get confused with \"helper\" in the first place.\nThey will ask their resident guru or favourite search engine about\nthe message and will be told that your http connection did not go\nwell either way, but not many people have seen this new message.\n\nIf we were to reinstate the extra final line in the error message, I\nthink we would be off doing a straight revert without any rewording\nthat introduces yet another word \"protocol reader\" that is not found\nanywhere in our glossary.\n\nI think I am OK with adding one more line after \"Reading from\n... failed\" that explains a more detailed error message might be\nthere above that line, but I am not sure what the good phrasing\nwould be.\n\n\n[Footnote]\n\n*1* It may introduce a new confusion \"was it 'read' that failed, or\n'write'?\", though ;-)\n"},{"id":"233316","messageId":"20140117201325.GB775@sigill.intra.peff.net","threadId":"35671","inReplyTo":"xmqqmwiuwg0o.fsf@gitster.dls.corp.google.com","subject":"Re: git quietly fails on https:// URL, https errors are never reported to user","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-17T20:13:25Z","receivedAt":"2014-01-17T20:13:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 17, 2014 at 11:43:35AM -0800, Junio C Hamano wrote:\n\n> Yuri writes:\n> \n> > I think that in a rare case of error this extra-printout wouldn't\n> > hurt.\n> \n> If the \"error is rare, extra verbiage does not hurt\" were a valid\n> attitude, \"error is rare, non-zero exit is enough\" would be equally\n> valid ;-)\n\nI think the problem is that error is _not_ rare. For years, we did not\nprint the extra verbiage, and nobody complained. Then, within days of us\nmaking a release that included the extra line, somebody complained[1].\n\nThe real issue is that the remote-helper hanging up _should_ be an\nexceptional condition, but it's not. The remote-helper protocol sucks,\nand has no way to signal failure of an operation besides just hanging\nup. So you end up with junk like:\n\n $ git clone https://google.com/foo.git\n Cloning into 'foo'...\n fatal: repository 'https://google.com/foo.git/' not found\n fatal: Reading from helper 'git-remote-https' failed\n\nThat second line is not adding anything, and IMHO is making things\nuglier and more confusing. We _expected_ the helper to hang up; that's\nhow it signals an error to us. It is not an unexpected condition at all.\nThe exit(128) we do is simply propagating the error report of the\nhelper.\n\nThat's the common error case: the message is redundant and annoying. The\n_uncommon_ case is the one Yuri hit: some library misconfiguration that\ncauses the helper not to run at all. Adding back any message is hurting\nthe common case to help the uncommon one.\n\n> I think I am OK with adding one more line after \"Reading from\n> ... failed\" that explains a more detailed error message might be\n> there above that line, but I am not sure what the good phrasing\n> would be.\n\nI'd really rather not. Hopefully the explanation above makes it clear\nwhy not.\n\nThe \"most right\" solution is to fix the helper protocol to allow error\nreporting. That may be somewhat complicated to retain backward\ncompatibility (we have to introduce a capability, probe for it, handle\nold helpers, etc).\n\nWe _may_ be able to do better by annotating certain commands with\nwhether we expect them to hangup. The big one, I think, would be the\ninitial \"capabilities\" command. Something like the patch below would\ndetect any startup problems in the helper. From Yuri's description, that\nwould catch his case or any similar ones.\n\nAnything after startup should be the responsibility of the helper to\nreport. If it doesn't, that's a bug in the helper. The one exception may\nbe signals. We could waitpid() on the helper and report any signal\ndeath (e.g., it obviously cannot report its own SIGKILL death, and I'd\nguess that most do not report their own SIGPIPE death).\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 2010674..af03f1a 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -50,7 +50,8 @@ static void sendline(struct helper_data *helper, struct strbuf *buffer)\n \t\tdie_errno(\"Full write to remote helper failed\");\n }\n \n-static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)\n+static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name,\n+\t\t int die_on_failure)\n {\n \tstrbuf_reset(buffer);\n \tif (debug)\n@@ -58,7 +59,9 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)\n \tif (strbuf_getline(buffer, helper, '\\n') == EOF) {\n \t\tif (debug)\n \t\t\tfprintf(stderr, \"Debug: Remote helper quit.\\n\");\n-\t\texit(128);\n+\t\tif (die_on_failure)\n+\t\t\texit(128);\n+\t\treturn -1;\n \t}\n \n \tif (debug)\n@@ -68,7 +71,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name)\n \n static int recvline(struct helper_data *helper, struct strbuf *buffer)\n {\n-\treturn recvline_fh(helper->out, buffer, helper->name);\n+\treturn recvline_fh(helper->out, buffer, helper->name, 1);\n }\n \n static void xchgline(struct helper_data *helper, struct strbuf *buffer)\n@@ -163,7 +166,9 @@ static struct child_process *get_helper(struct transport *transport)\n \twhile (1) {\n \t\tconst char *capname;\n \t\tint mandatory = 0;\n-\t\trecvline(data, &buf);\n+\n+\t\tif (recvline_fh(data->out, &buf, data->name, 0) < 0)\n+\t\t\tdie(\"remote helper '%s' aborted session\", data->name);\n \n \t\tif (!*buf.buf)\n \t\t\tbreak;\n@@ -557,7 +562,7 @@ static int process_connect_service(struct transport *transport,\n \t\tgoto exit;\n \n \tsendline(data, &cmdbuf);\n-\trecvline_fh(input, &cmdbuf, name);\n+\trecvline_fh(input, &cmdbuf, name, 1);\n \tif (!strcmp(cmdbuf.buf, \"\")) {\n \t\tdata->no_disconnect_req = 1;\n \t\tif (debug)\n"},{"id":"233318","messageId":"20140117201858.GA3362@sigill.intra.peff.net","threadId":"35671","inReplyTo":"20140117201325.GB775@sigill.intra.peff.net","subject":"Re: git quietly fails on https:// URL, https errors are never reported to user","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-17T20:18:58Z","receivedAt":"2014-01-17T20:18:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 17, 2014 at 03:13:25PM -0500, Jeff King wrote:\n\n> On Fri, Jan 17, 2014 at 11:43:35AM -0800, Junio C Hamano wrote:\n> \n> > Yuri writes:\n> > \n> > > I think that in a rare case of error this extra-printout wouldn't\n> > > hurt.\n> > \n> > If the \"error is rare, extra verbiage does not hurt\" were a valid\n> > attitude, \"error is rare, non-zero exit is enough\" would be equally\n> > valid ;-)\n> \n> I think the problem is that error is _not_ rare. For years, we did not\n> print the extra verbiage, and nobody complained. Then, within days of us\n> making a release that included the extra line, somebody complained[1].\n\nForgot my footnote here, but it was:\n\n http://article.gmane.org/gmane.comp.version-control.git/228498\n\nwhich led to 266f1fd (transport-helper: be quiet on read errors from\nhelpers, 2013-06-21).\n\n-Peff\n"},{"id":"233320","messageId":"52D9950B.3030300@rawbw.com","threadId":"35671","inReplyTo":"20140117201325.GB775@sigill.intra.peff.net","subject":"Re: git quietly fails on https:// URL, https errors are never reported to user","fromName":"Yuri","fromEmail":"yuri@rawbw.com","sentAt":"2014-01-17T20:39:39Z","receivedAt":"2014-01-17T20:39:39Z","isPatch":false,"sender":{"key":"yuri@rawbw.com","avatar":null},"body":"On 01/17/2014 12:13, Jeff King wrote:\n> $ git clonehttps://google.com/foo.git\n> Cloning into 'foo'...\n> fatal: repository 'https://google.com/foo.git/' not found\n> fatal: Reading from helper 'git-remote-https' failed\n>\n> That second line is not adding anything, and IMHO is making things\n> uglier and more confusing. We_expected_ the helper to hang up; that's\n> how it signals an error to us. It is not an unexpected condition at all.\n> The exit(128) we do is simply propagating the error report of the\n> helper.\n>\n> That's the common error case: the message is redundant and annoying. The\n> _uncommon_ case is the one Yuri hit: some library misconfiguration that\n> causes the helper not to run at all. Adding back any message is hurting\n> the common case to help the uncommon one.\n\nBut you can use the error code value to convey the cause of the failure \nto git, and avoid an unnecessary message in git itself. Based on the \nerror code value git could tell if the error has already been reported \nto user.\n\nYuri\n"},{"id":"233322","messageId":"20140117211035.GA8447@sigill.intra.peff.net","threadId":"35671","inReplyTo":"52D9950B.3030300@rawbw.com","subject":"Re: git quietly fails on https:// URL, https errors are never reported to user","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-17T21:10:35Z","receivedAt":"2014-01-17T21:10:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 17, 2014 at 12:39:39PM -0800, Yuri wrote:\n\n> >That second line is not adding anything, and IMHO is making things\n> >uglier and more confusing. We_expected_ the helper to hang up; that's\n> >how it signals an error to us. It is not an unexpected condition at all.\n> >The exit(128) we do is simply propagating the error report of the\n> >helper.\n> >\n> >That's the common error case: the message is redundant and annoying. The\n> >_uncommon_ case is the one Yuri hit: some library misconfiguration that\n> >causes the helper not to run at all. Adding back any message is hurting\n> >the common case to help the uncommon one.\n> \n> But you can use the error code value to convey the cause of the\n> failure to git, and avoid an unnecessary message in git itself. Based\n> on the error code value git could tell if the error has already been\n> reported to user.\n\nYes, we can, but that is in the same boat as a protocol change: you have\nto teach every remote helper (some of which are written by third\nparties) to communicate over this sideband channel.\n\nIt's should be slightly easier than a change to the protocol text,\nbecause it's mostly backwards compatible (helpers should already be\nreturning a non-zero error code). I think there is some complication\nwith exit codes and remote-helpers, where you cannot expect just check\nthe exit code at any time. I _think_ from previous discussions that it\nis safe to waitpid() on the helper after we have gotten EOF on the\nreading pipe, though.\n\n-Peff\n"}]}