On Fri, Jan 17, 2014 at 11:43:35AM -0800, Junio C Hamano wrote: > Yuri <yuri@xxxxxxxxx> writes: > > > I think that in a rare case of error this extra-printout wouldn't > > hurt. > > If the "error is rare, extra verbiage does not hurt" were a valid > attitude, "error is rare, non-zero exit is enough" would be equally > valid ;-) I think the problem is that error is _not_ rare. For years, we did not print the extra verbiage, and nobody complained. Then, within days of us making a release that included the extra line, somebody complained[1]. The real issue is that the remote-helper hanging up _should_ be an exceptional condition, but it's not. The remote-helper protocol sucks, and has no way to signal failure of an operation besides just hanging up. So you end up with junk like: $ git clone https://google.com/foo.git Cloning into 'foo'... fatal: repository 'https://google.com/foo.git/' not found fatal: Reading from helper 'git-remote-https' failed That second line is not adding anything, and IMHO is making things uglier and more confusing. We _expected_ the helper to hang up; that's how it signals an error to us. It is not an unexpected condition at all. The exit(128) we do is simply propagating the error report of the helper. That's the common error case: the message is redundant and annoying. The _uncommon_ case is the one Yuri hit: some library misconfiguration that causes the helper not to run at all. Adding back any message is hurting the common case to help the uncommon one. > I think I am OK with adding one more line after "Reading from > ... failed" that explains a more detailed error message might be > there above that line, but I am not sure what the good phrasing > would be. I'd really rather not. Hopefully the explanation above makes it clear why not. The "most right" solution is to fix the helper protocol to allow error reporting. That may be somewhat complicated to retain backward compatibility (we have to introduce a capability, probe for it, handle old helpers, etc). We _may_ be able to do better by annotating certain commands with whether we expect them to hangup. The big one, I think, would be the initial "capabilities" command. Something like the patch below would detect any startup problems in the helper. From Yuri's description, that would catch his case or any similar ones. Anything after startup should be the responsibility of the helper to report. If it doesn't, that's a bug in the helper. The one exception may be signals. We could waitpid() on the helper and report any signal death (e.g., it obviously cannot report its own SIGKILL death, and I'd guess that most do not report their own SIGPIPE death). diff --git a/transport-helper.c b/transport-helper.c index 2010674..af03f1a 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -50,7 +50,8 @@ static void sendline(struct helper_data *helper, struct strbuf *buffer) die_errno("Full write to remote helper failed"); } -static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name) +static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name, + int die_on_failure) { strbuf_reset(buffer); if (debug) @@ -58,7 +59,9 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name) if (strbuf_getline(buffer, helper, '\n') == EOF) { if (debug) fprintf(stderr, "Debug: Remote helper quit.\n"); - exit(128); + if (die_on_failure) + exit(128); + return -1; } if (debug) @@ -68,7 +71,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name) static int recvline(struct helper_data *helper, struct strbuf *buffer) { - return recvline_fh(helper->out, buffer, helper->name); + return recvline_fh(helper->out, buffer, helper->name, 1); } static void xchgline(struct helper_data *helper, struct strbuf *buffer) @@ -163,7 +166,9 @@ static struct child_process *get_helper(struct transport *transport) while (1) { const char *capname; int mandatory = 0; - recvline(data, &buf); + + if (recvline_fh(data->out, &buf, data->name, 0) < 0) + die("remote helper '%s' aborted session", data->name); if (!*buf.buf) break; @@ -557,7 +562,7 @@ static int process_connect_service(struct transport *transport, goto exit; sendline(data, &cmdbuf); - recvline_fh(input, &cmdbuf, name); + recvline_fh(input, &cmdbuf, name, 1); if (!strcmp(cmdbuf.buf, "")) { data->no_disconnect_req = 1; if (debug) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html