print an error when remote helpers die during capabilities

The transport-helper code generally relies on the
remote-helper to provide an informative message to the user
when it encounters an error. In the rare cases where the
helper does not do so, the output can be quite confusing.
E.g.:

  $ git clone https://example.com/foo.git
  Cloning into 'foo'...
  $ echo $?
  128
  $ ls foo
  /bin/ls: cannot access foo: No such file or directory

We tried to address this with 81d340d (transport-helper:
report errors properly, 2013-04-10).

But that makes the common case much more confusing. The
remote helper protocol's method for signaling normal errors
is to simply hang up. So when the helper does encounter a
routine error and prints something to stderr, the extra
error message is redundant and misleading. So we dropped it
again in 266f1fd (transport-helper: be quiet on read errors
from helpers, 2013-06-21).

This puts the uncommon case right back where it started. We
may be able to do a little better, though. It is common for
the helper to die during a "real" command, like fetching the
list of remote refs. It is not common for it to die during
the initial "capabilities" negotiation, right after we
start. Reporting failure here is likely to catch fundamental
problems that prevent the helper from running (and reporting
errors) at all. Anything after that is the responsibility of
the helper itself to report.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index d386076dbd3f8db7611ab843efcbddca362fa06c..5185ed2441b41ad02bffe634db1383b39a0590a3 100755 (executable)
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -321,4 +321,15 @@ test_expect_success 'fetch tag' '
        compare_refs local v1.0 server v1.0
 '
 
+test_expect_success 'totally broken helper reports failure message' '
+       write_script git-remote-broken <<-\EOF &&
+       read cap_cmd
+       exit 1
+       EOF
+       test_must_fail \
+               env PATH="$PWD:$PATH" \
+               git clone broken://example.com/foo.git 2>stderr &&
+       grep aborted stderr
+'
+
 test_done

diff --git a/transport-helper.c b/transport-helper.c
index 3ea7c2bb5ad8c462ae0f54c9466af210bc27d0c2..bd9e02ff93d8f82a5501597770777c14e3127a4c 100644 (file)
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -83,11 +83,18 @@ static int recvline(struct helper_data *helper, struct strbuf *buffer)
        return recvline_fh(helper->out, buffer);
 }
 
-static void write_constant(int fd, const char *str)
+static int write_constant_gently(int fd, const char *str)
 {
        if (debug)
                fprintf(stderr, "Debug: Remote helper: -> %s", str);
        if (write_in_full(fd, str, strlen(str)) < 0)
+               return -1;
+       return 0;
+}
+
+static void write_constant(int fd, const char *str)
+{
+       if (write_constant_gently(fd, str) < 0)
                die_errno(_("full write to remote helper failed"));
 }
 
@@ -161,13 +168,16 @@ static struct child_process *get_helper(struct transport *transport)
                die_errno(_("can't dup helper output fd"));
        data->out = xfdopen(duped, "r");
 
-       write_constant(helper->in, "capabilities\n");
+       sigchain_push(SIGPIPE, SIG_IGN);
+       if (write_constant_gently(helper->in, "capabilities\n") < 0)
+               die("remote helper '%s' aborted session", data->name);
+       sigchain_pop(SIGPIPE);
 
        while (1) {
                const char *capname, *arg;
                int mandatory = 0;
                if (recvline(data, &buf))
-                       exit(128);
+                       die("remote helper '%s' aborted session", data->name);
 
                if (!*buf.buf)
                        break;