patch, 2 partshandling REF_STATUS_EXPECTING_REPORT over http

I recently saw a case where GitHub's server implementation failed to correctly report ref statuses. In this case it was because it hit an internal timeout, but the root cause doesn't really matter; the server is doing the wrong thing.

But what's interesting on the Git client side is that we produce quite a confusing message for http. For ssh, we correctly say something like (in this example, I'm pushing an absurd number of tags that causes the timeout):

   To ssh://github.com/some/repo
    ! [remote failure]  tag-1 -> tag-1 (remote failed to report status)
   [...etc...]
    ! [remote failure]  tag-999 -> tag-999 (remote failed to report status)
   error: failed to push some refs to 'ssh://github.com/some/repo'

but for http, I get just:

  Everything up-to-date

Which is misleading to say the least. The issue is communication between send-pack and remote-curl on the client side.

The first patch does the minimal fix. The second one has some cosmetic improvements. They arguably could be squashed together, but I think keeping them split shows exactly what the second patch is actually accomplishing (especially with the minor change to the expected output in the test).

This bug has been around since smart http was added in 2009. I just prepared it on master, but I expect it could also go to maint.

  [1/2]: send-pack: complain about "expecting report" with --helper-status
  [2/2]: transport-helper: recognize "expecting report" error from send-pack

 builtin/send-pack.c            |  4 ++++
 t/lib-httpd.sh                 |  1 +
 t/lib-httpd/apache.conf        |  4 ++++
 t/lib-httpd/error-no-report.sh |  6 ++++++
 t/t5541-http-push-smart.sh     | 16 ++++++++++++++++
 transport-helper.c             |  4 ++++
 6 files changed, 35 insertions(+)
 create mode 100644 t/lib-httpd/error-no-report.sh

-Peff

When pushing to a server which erroneously omits the final ref-status report, the client side should complain about the refs for which we didn't receive the status (because we can't just assume they were updated). This works over most transports like ssh, but for http we'll print a very misleading "Everything up-to-date".

It works for ssh because send-pack internally sets the status of each ref to REF_STATUS_EXPECTING_REPORT, and then if the server doesn't tell us about a particular ref, it will stay at that value. When we print the final status table, we'll see that we're still on EXPECTING_REPORT and complain then.

But for http, we go through remote-curl, which invokes send-pack with "--stateless-rpc --helper-status". The latter option causes send-pack to return a machine-readable list of ref statuses to the remote helper. But ever since its inception in de1a2fdd38 (Smart push over HTTP: client side, 2009-10-30), the send-pack code has simply omitted mention of any ref which ended up in EXPECTING_REPORT.

In the remote helper, we then take the absence of any status report from send-pack to mean that the ref was not even something we tried to send, and thus it prints "Everything up-to-date". Fortunately it does detect the eventual non-zero exit from send-pack, and propagates that in its own non-zero exit code. So at least a careful script invoking "git push" would notice the failure. But sending the misleading message on stderr is certainly confusing for humans (not to mention the machine-readable "push --porcelain" output, though again, any careful script should be checking the exit code from push, too).

Nobody seems to have noticed because the server in this instance has to be misbehaving: it has promised to support the ref-status capability (otherwise the client will not set EXPECTING_REPORT at all), but didn't send us any. If the connection were simply cut, then send-pack would complain about getting EOF while trying to read the status. But if the server actually sends a flush packet (i.e., saying "now you have all of the ref statuses" without actually sending any), then the client ends up in this confused situation.

The fix is simple: we should return an error message from "send-pack --helper-status", just like we would for any other error per-ref error condition (in the test I included, the server simply omits all ref status responses, but a more insidious version of this would skip only some of them).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/send-pack.c            |  4 ++++
 t/lib-httpd.sh                 |  1 +
 t/lib-httpd/apache.conf        |  4 ++++
 t/lib-httpd/error-no-report.sh |  6 ++++++
 t/t5541-http-push-smart.sh     | 16 ++++++++++++++++
 5 files changed, 31 insertions(+)
 create mode 100644 t/lib-httpd/error-no-report.sh

Jeff King <peff@peff.net> writes:

That is obviously correct, minimum and surprisingly simple ;-)

Thanks.

When a transport helper pushes via send-pack, it passes --helper-status to get a machine-readable status back for each ref. The previous commit taught the send-pack code to hand back "error expecting report" if the server did not send us the proper ref-status. And that's enough to cause us to recognize that an error occurred for the ref and print something sensible in our final status table.

But we do interpret these messages on the remote-helper side to turn them back into REF_STATUS_* enum values. Recognizing this token to turn it back into REF_STATUS_EXPECTING_REPORT has two advantages:

  1. We now print exactly the same message in the human-readable (and
     machine-readable --porcelain) output for this situation whether the
     transport went through a helper (e.g., http) or not (e.g., ssh).

  2. If any code in the helper really cares about distinguishing
     EXPECT_REPORT from more generic error conditions, it could now do
     so. I didn't find any, so this is mostly future-proofing.

So this is mostly cosmetic for now, but it seems like the least-surprising thing for the transport-helper code to be doing.

Signed-off-by: Jeff King <peff@peff.net>
---
Yes, the "else if" is not cuddled below. It matches the rest of the
existing if/else chain.

 t/t5541-http-push-smart.sh | 2 +-
 transport-helper.c         | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

On 2021-10-18 at 19:34:59, Jeff King wrote:

This seems sane to me, and I'm delighted you managed to add a test for this case (which I was not expecting to be possible).