patchremote-curl: Fix push status report when all branches fail

From: "Shawn O. Pearce" <spearce@spearce.org>

The protocol between transport-helper.c and remote-curl requires remote-curl to always print a blank line after the push command has run. If the blank line is ommitted, transport-helper kills its container process (the git push the user started) with exit(128) and no message indicating a problem, assuming the helper already printed reasonable error text to the console.

However if the remote rejects all branches with "ng" commands in the report-status reply, send-pack terminates with non-zero status, and in turn remote-curl exited with non-zero status before outputting the blank line after the helper status printed by send-pack. No error messages reach the user.

This caused users to see the following from git push over HTTP when the remote side's update hook rejected the branch:

  $ git push http://... master
  Counting objects: 4, done.
  Delta compression using up to 6 threads.
  Compressing objects: 100% (2/2), done.
  Writing objects: 100% (3/3), 301 bytes, done.
  Total 3 (delta 0), reused 0 (delta 0)
  $

Always print a blank line after the send-pack process terminates, ensuring the helper status report (if it was output) will be correctly parsed by the calling transport-helper.c. This ensures the helper doesn't abort before the status report can be shown to the user.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 remote-curl.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

"Shawn O. Pearce" <spearce@spearce.org> writes:

Anybody wants to add a simple test for this failure mode?

This is not a fault of this patch, but could we fix this ugly mixture of xwrite() and printf() in the same program? I can see that the loop in the main() function carefully tries to call fflush(stdout) to make sure that nothing is pending after processing a single command so using xwrite() may not cause any harm here, but the thing is that you do not check the error return from this xwrite(), so use of it is not giving us any potential benefit of being able to detect I/O errors in a finer grained manner, i.e. it is no better than the printf("\n"); fflush(stdout); sequence it replaces.

Thanks.

From: "Shawn O. Pearce" <spearce@spearce.org>

The protocol between transport-helper.c and remote-curl requires remote-curl to always print a blank line after the push command has run. If the blank line is ommitted, transport-helper kills its container process (the git push the user started) with exit(128) and no message indicating a problem, assuming the helper already printed reasonable error text to the console.

However if the remote rejects all branches with "ng" commands in the report-status reply, send-pack terminates with non-zero status, and in turn remote-curl exited with non-zero status before outputting the blank line after the helper status printed by send-pack. No error messages reach the user.

This caused users to see the following from git push over HTTP when the remote side's update hook rejected the branch:

  $ git push http://... master
  Counting objects: 4, done.
  Delta compression using up to 6 threads.
  Compressing objects: 100% (2/2), done.
  Writing objects: 100% (3/3), 301 bytes, done.
  Total 3 (delta 0), reused 0 (delta 0)
  $

Always print a blank line after the send-pack process terminates, ensuring the helper status report (if it was output) will be correctly parsed by the calling transport-helper.c. This ensures the helper doesn't abort before the status report can be shown to the user.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 remote-curl.c        |    9 +++++----
 t/t5541-http-push.sh |   27 +++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 4 deletions(-)

Thanks; will queue.

"Shawn O. Pearce" <spearce@spearce.org> writes:

Curious. Where do we get these eight trailing whitespaces?

The call to rp_error("hook declined to update %s", name) seems to be giving the name properly.

On Thu, Jan 19, 2012 at 22:00, Junio C Hamano <gitster@pobox.com> wrote:

I think this is padding being added to the end of the line by recv_sideband(). I noticed the trailing whitespace in the diff, but the test passed with it present, so I had to leave it in.

Yea, I think the server is sending the correct data in the sideband channel, its just the sideband client padding out the line. I think this padding is a fudge against progress meters that are being written and over-written with \r lines in subsequent sideband packets.

Shawn Pearce <spearce@spearce.org> writes:

ISTR we had a policy to guard such whitespace at EOL? Compare e.g. c1376c12b7.

From: "Shawn O. Pearce" <spearce@spearce.org>

The protocol between transport-helper.c and remote-curl requires remote-curl to always print a blank line after the push command has run. If the blank line is ommitted, transport-helper kills its container process (the git push the user started) with exit(128) and no message indicating a problem, assuming the helper already printed reasonable error text to the console.

However if the remote rejects all branches with "ng" commands in the report-status reply, send-pack terminates with non-zero status, and in turn remote-curl exited with non-zero status before outputting the blank line after the helper status printed by send-pack. No error messages reach the user.

This caused users to see the following from git push over HTTP when the remote side's update hook rejected the branch:

  $ git push http://... master
  Counting objects: 4, done.
  Delta compression using up to 6 threads.
  Compressing objects: 100% (2/2), done.
  Writing objects: 100% (3/3), 301 bytes, done.
  Total 3 (delta 0), reused 0 (delta 0)
  $

Always print a blank line after the send-pack process terminates, ensuring the helper status report (if it was output) will be correctly parsed by the calling transport-helper.c. This ensures the helper doesn't abort before the status report can be shown to the user.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 remote-curl.c        |    9 +++++----
 t/t5541-http-push.sh |   27 +++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 4 deletions(-)

"Shawn O. Pearce" <spearce@spearce.org> writes:

Thanks; let's do the following on top of this patch, so that:

 - We won't miss a "git push" that errorneously succeeds; and

 - We won't be affected by any future change in the sideband #2
   demultiplexor of the amount of "padding".

 t/t5541-http-push.sh |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

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). Before your patch, this is what happened:

  1. remote-curl relays the status lines from send-pack, then sees that
     send-pack reported error, and it exits

  2. push reads the status lines, looking for a blank line to terminate
     them. It sees EOF instead of the blank line and exits(128) itself.

After your patch, this happens:

  1. remote-curl relays the status lines, alway appends the blank line
     terminator, and then exits

  2. push reads the status lines, including the blank line terminator,
     and reports them to the user.

  3. push then disconnects the remote-curl helper by writing a blank
     line to it (to signal end-of-input), followed by finish_command().
     The latter propagates the error code from the exit in step 1, and
     we use that to signal failure from "git push".

There's a race condition now in step 3. The push process may write to the pipe going to remote-curl after it has exited, causing it to receive SIGPIPE and die. We can block SIGPIPE, but that's not sufficient; we'll still notice that our write() returns EPIPE and die.

Obviously we can't not print the post-push "\n" in remote-curl, for the reasons you outlined in the commit message of this patch. We also can't not exit from remote-curl on error. Even though in the test in t5541 we have signaled error via the ref statuses, we might have received an error that does not come through a ref status (e.g., if we couldn't run send-pack at all).

We can't not write the "\n" to signal end-of-input to remote-curl, because we don't actually know yet that there's an error (we find out when we wait() on the process). Barring any asynchronous SIGCHLD handling, of course, but I don't think we want to get into that.

So it's kind of a bug in the remote helper protocol. The helpers can signal failure only by dying, but we can find out about that failure only after disconnecting, which involves writing to them. It would be much more sane if the helpers returned an overall text status from each command (e.g., printed "error push failed" instead of dying).

But that would involve changing the protocol, of course. I think our best option is to work around it by considering the final blank line we send before disconnect as "best effort". That is, it is a courtesy to the remote helper to tell it we are hanging up cleanly, and if it does not arrive, then we can ignore the problem and proceed with closing the pipe. I.e., something like:

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

...

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).

On Wed, Feb 22, 2012 at 07:22:10AM -0800, Shawn O. Pearce wrote:

I don't think it's worth a new capability. This is one of those "it would be nice if it were designed that way from day one" cases, but it wasn't. And while this is a minor hack, I don't think it has any functional downsides. So adding a new capability on top of the hack just makes things more complex.

I'll re-send the patch with a stand-alone commit message.

-Peff

On Wed, Feb 22, 2012 at 03:40:50PM -0500, Jeff King wrote:

Here it is.

-- >8 --
Subject: [PATCH] disconnect from remote helpers more gently

When git spawns a remote helper program (like git-remote-http), the last thing we do before closing the pipe to the child process is to send a blank line, telling the helper that we are done issuing commands. However, the helper may already have exited, in which case the parent git process will receive SIGPIPE and die.

In particular, this can happen with the remote-curl helper when it encounters errors during a push. The helper reports individual errors for each ref back to git-push, and then exits with a non-zero exit code. Depending on the exact timing of the write, the parent process may or may not receive SIGPIPE.

This causes intermittent test failure in t5541.8, and is a side effect of 5238cbf (remote-curl: Fix push status report when all branches fail). Before that commit, remote-curl would not send the final blank line to indicate that the list of status lines was complete; it would just exit, closing the pipe. The parent git-push would notice the closed pipe while reading the status report and exit immediately itself, propagating the failing exit code. But post-5238cbf, remote-curl completes the status list before exiting, git-push actually runs to completion, and then it tries to cleanly disconnect the helper, leading to the SIGPIPE race above.

This patch drops all error-checking when sending the final "we are about to hang up" blank line to helpers. There is nothing useful for the parent process to do about errors at that point anyway, and certainly failing to send our "we are done with commands" line to a helper that has already exited is not a problem.

Signed-off-by: Jeff King <peff@peff.net>
---
 transport-helper.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Thanks.