{"thread":{"id":"56734","subject":"[PATCH 0/2] handling REF_STATUS_EXPECTING_REPORT over http","startedAt":"2021-10-18T19:35:05Z","lastAt":"2021-10-19T23:59:18Z","messageCount":5,"participants":["Jeff King","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"438998","messageId":"YW3MY6lAI5l+BOnP@coredump.intra.peff.net","threadId":"56734","inReplyTo":null,"subject":"[PATCH 0/2] handling REF_STATUS_EXPECTING_REPORT over http","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-18T19:34:59Z","receivedAt":"2021-10-18T19:35:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I recently saw a case where GitHub's server implementation failed to\ncorrectly report ref statuses. In this case it was because it hit an\ninternal timeout, but the root cause doesn't really matter; the server\nis doing the wrong thing.\n\nBut what's interesting on the Git client side is that we produce quite a\nconfusing message for http. For ssh, we correctly say something like (in\nthis example, I'm pushing an absurd number of tags that causes the\ntimeout):\n\n To ssh://github.com/some/repo\n ! [remote failure] tag-1 -> tag-1 (remote failed to report status)\n [...etc...]\n ! [remote failure] tag-999 -> tag-999 (remote failed to report status)\n error: failed to push some refs to 'ssh://github.com/some/repo'\n\nbut for http, I get just:\n\n Everything up-to-date\n\nWhich is misleading to say the least. The issue is communication between\nsend-pack and remote-curl on the client side.\n\nThe first patch does the minimal fix. The second one has some cosmetic\nimprovements. They arguably could be squashed together, but I think\nkeeping them split shows exactly what the second patch is actually\naccomplishing (especially with the minor change to the expected output\nin the test).\n\nThis bug has been around since smart http was added in 2009. I just\nprepared it on master, but I expect it could also go to maint.\n\n [1/2]: send-pack: complain about \"expecting report\" with --helper-status\n [2/2]: transport-helper: recognize \"expecting report\" error from send-pack\n\n builtin/send-pack.c | 4 ++++\n t/lib-httpd.sh | 1 +\n t/lib-httpd/apache.conf | 4 ++++\n t/lib-httpd/error-no-report.sh | 6 ++++++\n t/t5541-http-push-smart.sh | 16 ++++++++++++++++\n transport-helper.c | 4 ++++\n 6 files changed, 35 insertions(+)\n create mode 100644 t/lib-httpd/error-no-report.sh\n\n-Peff\n"},{"id":"438999","messageId":"YW3OcxqcOsY9AFQ8@coredump.intra.peff.net","threadId":"56734","inReplyTo":"YW3MY6lAI5l+BOnP@coredump.intra.peff.net","subject":"[PATCH 1/2] send-pack: complain about \"expecting report\" with --helper-status","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-18T19:43:47Z","receivedAt":"2021-10-18T19:43:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When pushing to a server which erroneously omits the final ref-status\nreport, the client side should complain about the refs for which we\ndidn't receive the status (because we can't just assume they were\nupdated). This works over most transports like ssh, but for http we'll\nprint a very misleading \"Everything up-to-date\".\n\nIt works for ssh because send-pack internally sets the status of each\nref to REF_STATUS_EXPECTING_REPORT, and then if the server doesn't tell\nus about a particular ref, it will stay at that value. When we print the\nfinal status table, we'll see that we're still on EXPECTING_REPORT and\ncomplain then.\n\nBut for http, we go through remote-curl, which invokes send-pack with\n\"--stateless-rpc --helper-status\". The latter option causes send-pack to\nreturn a machine-readable list of ref statuses to the remote helper. But\never since its inception in de1a2fdd38 (Smart push over HTTP: client\nside, 2009-10-30), the send-pack code has simply omitted mention of any\nref which ended up in EXPECTING_REPORT.\n\nIn the remote helper, we then take the absence of any status report\nfrom send-pack to mean that the ref was not even something we tried to\nsend, and thus it prints \"Everything up-to-date\". Fortunately it does\ndetect the eventual non-zero exit from send-pack, and propagates that in\nits own non-zero exit code. So at least a careful script invoking \"git\npush\" would notice the failure. But sending the misleading message on\nstderr is certainly confusing for humans (not to mention the\nmachine-readable \"push --porcelain\" output, though again, any careful\nscript should be checking the exit code from push, too).\n\nNobody seems to have noticed because the server in this instance has to\nbe misbehaving: it has promised to support the ref-status capability\n(otherwise the client will not set EXPECTING_REPORT at all), but didn't\nsend us any. If the connection were simply cut, then send-pack would\ncomplain about getting EOF while trying to read the status. But if the\nserver actually sends a flush packet (i.e., saying \"now you have all of\nthe ref statuses\" without actually sending any), then the client ends up\nin this confused situation.\n\nThe fix is simple: we should return an error message from \"send-pack\n--helper-status\", just like we would for any other error per-ref error\ncondition (in the test I included, the server simply omits all ref\nstatus responses, but a more insidious version of this would skip only\nsome of them).\n\nSigned-off-by: Jeff King \n---\n builtin/send-pack.c | 4 ++++\n t/lib-httpd.sh | 1 +\n t/lib-httpd/apache.conf | 4 ++++\n t/lib-httpd/error-no-report.sh | 6 ++++++\n t/t5541-http-push-smart.sh | 16 ++++++++++++++++\n 5 files changed, 31 insertions(+)\n create mode 100644 t/lib-httpd/error-no-report.sh\n\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 8932142312..69c432ef1a 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -87,6 +87,10 @@ static void print_helper_status(struct ref *ref)\n \t\t\tbreak;\n \n \t\tcase REF_STATUS_EXPECTING_REPORT:\n+\t\t\tres = \"error\";\n+\t\t\tmsg = \"expecting report\";\n+\t\t\tbreak;\n+\n \t\tdefault:\n \t\t\tcontinue;\n \t\t}\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex d2edfa4c50..782891908d 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -131,6 +131,7 @@ prepare_httpd() {\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n \tinstall_script incomplete-length-upload-pack-v2-http.sh\n \tinstall_script incomplete-body-upload-pack-v2-http.sh\n+\tinstall_script error-no-report.sh\n \tinstall_script broken-smart-http.sh\n \tinstall_script error-smart-http.sh\n \tinstall_script error.sh\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 180a41fe96..497b9b9d92 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -122,6 +122,7 @@ Alias /auth/dumb/ www/auth/dumb/\n \n ScriptAlias /smart/incomplete_length/git-upload-pack incomplete-length-upload-pack-v2-http.sh/\n ScriptAlias /smart/incomplete_body/git-upload-pack incomplete-body-upload-pack-v2-http.sh/\n+ScriptAlias /smart/no_report/git-receive-pack error-no-report.sh/\n ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/\n ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1\n ScriptAlias /broken_smart/ broken-smart-http.sh/\n@@ -137,6 +138,9 @@ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1\n \n \tOptions ExecCGI\n \n+\n+\tOptions ExecCGI\n+\n \n \tOptions ExecCGI\n \ndiff --git a/t/lib-httpd/error-no-report.sh b/t/lib-httpd/error-no-report.sh\nnew file mode 100644\nindex 0000000000..39ff75bbc4\n--- /dev/null\n+++ b/t/lib-httpd/error-no-report.sh\n@@ -0,0 +1,6 @@\n+echo \"Content-Type: application/x-git-receive-pack-result\"\n+echo\n+printf '0013\\001000eunpack ok\\n'\n+printf '0015\\002skipping report\\n'\n+printf '0009\\0010000'\n+printf '0000'\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex c024fa2818..9c61dccc24 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -509,4 +509,20 @@ test_expect_success 'colorize errors/hints' '\n \ttest_i18ngrep ! \"^hint: \" decoded\n '\n \n+test_expect_success 'report error server does not provide ref status' '\n+\tgit init \"$HTTPD_DOCUMENT_ROOT_PATH/no_report\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/no_report\" config http.receivepack true &&\n+\ttest_must_fail git push --porcelain \\\n+\t\t$HTTPD_URL_USER_PASS/smart/no_report \\\n+\t\tHEAD:refs/tags/will-fail >actual &&\n+\ttest_must_fail git -C \"$HTTPD_DOCUMENT_ROOT_PATH/no_report\" \\\n+\t\trev-parse --verify refs/tags/will-fail &&\n+\tcat >expect <<-EOF &&\n+\tTo $HTTPD_URL/smart/no_report\n+\t!\tHEAD:refs/tags/will-fail\t[remote rejected] (expecting report)\n+\tDone\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.33.1.1223.g80c1dbe6e5\n\n"},{"id":"439000","messageId":"YW3O9EYdHq4BhQqD@coredump.intra.peff.net","threadId":"56734","inReplyTo":"YW3MY6lAI5l+BOnP@coredump.intra.peff.net","subject":"[PATCH 2/2] transport-helper: recognize \"expecting report\" error from send-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-18T19:45:56Z","receivedAt":"2021-10-18T19:45:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When a transport helper pushes via send-pack, it passes --helper-status\nto get a machine-readable status back for each ref. The previous commit\ntaught the send-pack code to hand back \"error expecting report\" if the\nserver did not send us the proper ref-status. And that's enough to cause\nus to recognize that an error occurred for the ref and print something\nsensible in our final status table.\n\nBut we do interpret these messages on the remote-helper side to turn\nthem back into REF_STATUS_* enum values. Recognizing this token to turn\nit back into REF_STATUS_EXPECTING_REPORT has two advantages:\n\n 1. We now print exactly the same message in the human-readable (and\n machine-readable --porcelain) output for this situation whether the\n transport went through a helper (e.g., http) or not (e.g., ssh).\n\n 2. If any code in the helper really cares about distinguishing\n EXPECT_REPORT from more generic error conditions, it could now do\n so. I didn't find any, so this is mostly future-proofing.\n\nSo this is mostly cosmetic for now, but it seems like the\nleast-surprising thing for the transport-helper code to be doing.\n\nSigned-off-by: Jeff King \n---\nYes, the \"else if\" is not cuddled below. It matches the rest of the\nexisting if/else chain.\n\n t/t5541-http-push-smart.sh | 2 +-\n transport-helper.c | 4 ++++\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex 9c61dccc24..8ca50f8b18 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -519,7 +519,7 @@ test_expect_success 'report error server does not provide ref status' '\n \t\trev-parse --verify refs/tags/will-fail &&\n \tcat >expect <<-EOF &&\n \tTo $HTTPD_URL/smart/no_report\n-\t!\tHEAD:refs/tags/will-fail\t[remote rejected] (expecting report)\n+\t!\tHEAD:refs/tags/will-fail\t[remote failure] (remote failed to report status)\n \tDone\n \tEOF\n \ttest_cmp expect actual\ndiff --git a/transport-helper.c b/transport-helper.c\nindex e8dbdd1153..a0297b0986 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -845,6 +845,10 @@ static int push_update_ref_status(struct strbuf *buf,\n \t\t\tforced = 1;\n \t\t\tFREE_AND_NULL(msg);\n \t\t}\n+\t\telse if (!strcmp(msg, \"expecting report\")) {\n+\t\t\tstatus = REF_STATUS_EXPECTING_REPORT;\n+\t\t\tFREE_AND_NULL(msg);\n+\t\t}\n \t}\n \n \tif (state->hint)\n-- \n2.33.1.1223.g80c1dbe6e5\n"},{"id":"439003","messageId":"xmqqsfwyuljl.fsf@gitster.g","threadId":"56734","inReplyTo":"YW3OcxqcOsY9AFQ8@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] send-pack: complain about \"expecting report\" with --helper-status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-18T20:26:22Z","receivedAt":"2021-10-18T20:26:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index 8932142312..69c432ef1a 100644\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -87,6 +87,10 @@ static void print_helper_status(struct ref *ref)\n> \t\t\tbreak;\n> \n> \t\tcase REF_STATUS_EXPECTING_REPORT:\n> +\t\t\tres = \"error\";\n> +\t\t\tmsg = \"expecting report\";\n> +\t\t\tbreak;\n> +\n> \t\tdefault:\n> \t\t\tcontinue;\n> \t\t}\n\nThat is obviously correct, minimum and surprisingly simple ;-)\n\nThanks.\n"},{"id":"439072","messageId":"YW9bsUdgpm3/C4+R@camp.crustytoothpaste.net","threadId":"56734","inReplyTo":"YW3MY6lAI5l+BOnP@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] handling REF_STATUS_EXPECTING_REPORT over http","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-10-19T23:58:41Z","receivedAt":"2021-10-19T23:59:18Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-10-18 at 19:34:59, Jeff King wrote:\n> I recently saw a case where GitHub's server implementation failed to\n> correctly report ref statuses. In this case it was because it hit an\n> internal timeout, but the root cause doesn't really matter; the server\n> is doing the wrong thing.\n> \n> But what's interesting on the Git client side is that we produce quite a\n> confusing message for http. For ssh, we correctly say something like (in\n> this example, I'm pushing an absurd number of tags that causes the\n> timeout):\n> \n> To ssh://github.com/some/repo\n> ! [remote failure] tag-1 -> tag-1 (remote failed to report status)\n> [...etc...]\n> ! [remote failure] tag-999 -> tag-999 (remote failed to report status)\n> error: failed to push some refs to 'ssh://github.com/some/repo'\n> \n> but for http, I get just:\n> \n> Everything up-to-date\n> \n> Which is misleading to say the least. The issue is communication between\n> send-pack and remote-curl on the client side.\n> \n> The first patch does the minimal fix. The second one has some cosmetic\n> improvements. They arguably could be squashed together, but I think\n> keeping them split shows exactly what the second patch is actually\n> accomplishing (especially with the minor change to the expected output\n> in the test).\n> \n> This bug has been around since smart http was added in 2009. I just\n> prepared it on master, but I expect it could also go to maint.\n\nThis seems sane to me, and I'm delighted you managed to add a test for\nthis case (which I was not expecting to be possible).\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"}]}