Re: [PATCH] remote-curl: Fix push status report when all branches fail

From: Shawn Pearce <spearce@spearce.org>
Date: Feb 22, 2012, 15:22 UTC
Message-ID: <CAJo=hJsFDrt4rsxVAnx86bxZDY3yfWc1=GDd8opUU+9z7esLnw@mail.gmail.com>
In-Reply-To: <20120222101302.GA11606@sigill.intra.peff.net>

On Wed, Feb 22, 2012 at 02:13, Jeff King <peff@peff.net> wrote:

> On Fri, Jan 20, 2012 at 09:03:31AM -0800, Shawn O. Pearce wrote:
> This hunk is causing intermittent failures of t5541 for me, especially
> when the system is under heavy load (e.g., make -j32 test).

...

Show 27 quoted lines

> @@ -220,15 +221,21 @@ static struct child_process *get_helper(struct transport *transport)
>  static int disconnect_helper(struct transport *transport)
>  {
>        struct helper_data *data = transport->data;
> -       struct strbuf buf = STRBUF_INIT;
>        int res = 0;
>
>        if (data->helper) {
>                if (debug)
>                        fprintf(stderr, "Debug: Disconnecting.\n");
>                if (!data->no_disconnect_req) {
> -                       strbuf_addf(&buf, "\n");
> -                       sendline(data, &buf);
> +                       /*
> +                        * Ignore write errors; there's nothing we can do,
> +                        * since we're about to close the pipe anyway. And the
> +                        * most likely error is EPIPE due to the helper dying
> +                        * to report an error itself.
> +                        */
> +                       sigchain_push(SIGPIPE, SIG_IGN);
> +                       xwrite(data->helper->in, "\n", 1);
> +                       sigchain_pop(SIGPIPE);
>                }
>                close(data->helper->in);
>                close(data->helper->out);
>
> which makes the t5541 failures go away for me. What do you think?

This sounds right to me. Its unfortunate that we missed the error status output when we built the remote helper protocol, but your patch above might be the best we can do now.

Eh, well, actually we could have the helper advertise a new capability that can be enabled to return exit status. That is a much bigger change, and even if we do it for remote-curl (since that is in tree and easy to update) we still need your patch for the same race condition for out of tree helpers (which Google actually has so I care about out of tree helpers too).