{"thread":{"id":"29406","subject":"[PATCH] remote-curl: Fix push status report when all branches fail","startedAt":"2012-01-19T22:24:59Z","lastAt":"2012-02-23T19:11:54Z","messageCount":14,"participants":["Shawn O. Pearce","Junio C Hamano","Shawn Pearce","Thomas Rast","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"182819","messageId":"1327011899-18883-1-git-send-email-spearce@spearce.org","threadId":"29406","inReplyTo":null,"subject":"[PATCH] remote-curl: Fix push status report when all branches fail","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2012-01-19T22:24:59Z","receivedAt":"2012-01-19T22:24:59Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"From: \"Shawn O. Pearce\" \n\nThe protocol between transport-helper.c and remote-curl requires\nremote-curl to always print a blank line after the push command\nhas run. If the blank line is ommitted, transport-helper kills its\ncontainer process (the git push the user started) with exit(128)\nand no message indicating a problem, assuming the helper already\nprinted reasonable error text to the console.\n\nHowever if the remote rejects all branches with \"ng\" commands in the\nreport-status reply, send-pack terminates with non-zero status, and\nin turn remote-curl exited with non-zero status before outputting\nthe blank line after the helper status printed by send-pack. No\nerror messages reach the user.\n\nThis caused users to see the following from git push over HTTP\nwhen the remote side's update hook rejected the branch:\n\n $ git push http://... master\n Counting objects: 4, done.\n Delta compression using up to 6 threads.\n Compressing objects: 100% (2/2), done.\n Writing objects: 100% (3/3), 301 bytes, done.\n Total 3 (delta 0), reused 0 (delta 0)\n $\n\nAlways print a blank line after the send-pack process terminates,\nensuring the helper status report (if it was output) will be\ncorrectly parsed by the calling transport-helper.c. This ensures\nthe helper doesn't abort before the status report can be shown to\nthe user.\n\nSigned-off-by: Shawn O. Pearce \n---\n remote-curl.c | 9 ++++-----\n 1 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 48c20b8..d6054e2 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -805,7 +805,7 @@ static int push(int nr_spec, char **specs)\n static void parse_push(struct strbuf *buf)\n {\n \tchar **specs = NULL;\n-\tint alloc_spec = 0, nr_spec = 0, i;\n+\tint alloc_spec = 0, nr_spec = 0, i, ret;\n \n \tdo {\n \t\tif (!prefixcmp(buf->buf, \"push \")) {\n@@ -822,12 +822,11 @@ static void parse_push(struct strbuf *buf)\n \t\t\tbreak;\n \t} while (1);\n \n-\tif (push(nr_spec, specs))\n+\tret = push(nr_spec, specs);\n+\txwrite(1, \"\\n\", 1);\n+\tif (ret)\n \t\texit(128); /* error already reported */\n \n-\tprintf(\"\\n\");\n-\tfflush(stdout);\n-\n free_specs:\n \tfor (i = 0; i < nr_spec; i++)\n \t\tfree(specs[i]);\n-- \n1.7.8.4.dirty\n"},{"id":"182820","messageId":"7vzkdjgv1i.fsf@alter.siamese.dyndns.org","threadId":"29406","inReplyTo":"1327011899-18883-1-git-send-email-spearce@spearce.org","subject":"Re: [PATCH] remote-curl: Fix push status report when all branches fail","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-19T22:57:29Z","receivedAt":"2012-01-19T22:57:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Always print a blank line after the send-pack process terminates,\n> ensuring the helper status report (if it was output) will be\n> correctly parsed by the calling transport-helper.c. This ensures\n> the helper doesn't abort before the status report can be shown to\n> the user.\n>\n> Signed-off-by: Shawn O. Pearce \n> ---\n\nAnybody wants to add a simple test for this failure mode?\n\n> remote-curl.c | 9 ++++-----\n> 1 files changed, 4 insertions(+), 5 deletions(-)\n>\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 48c20b8..d6054e2 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -805,7 +805,7 @@ static int push(int nr_spec, char **specs)\n> static void parse_push(struct strbuf *buf)\n> {\n> \tchar **specs = NULL;\n> -\tint alloc_spec = 0, nr_spec = 0, i;\n> +\tint alloc_spec = 0, nr_spec = 0, i, ret;\n> \n> \tdo {\n> \t\tif (!prefixcmp(buf->buf, \"push \")) {\n> @@ -822,12 +822,11 @@ static void parse_push(struct strbuf *buf)\n> \t\t\tbreak;\n> \t} while (1);\n> \n> -\tif (push(nr_spec, specs))\n> +\tret = push(nr_spec, specs);\n> +\txwrite(1, \"\\n\", 1);\n> +\tif (ret)\n> \t\texit(128); /* error already reported */\n> \n> -\tprintf(\"\\n\");\n> -\tfflush(stdout);\n> -\n\nThis is not a fault of this patch, but could we fix this ugly mixture of\nxwrite() and printf() in the same program? I can see that the loop in the\nmain() function carefully tries to call fflush(stdout) to make sure that\nnothing is pending after processing a single command so using xwrite() may\nnot cause any harm here, but the thing is that you do not check the error\nreturn from this xwrite(), so use of it is not giving us any potential\nbenefit of being able to detect I/O errors in a finer grained manner,\ni.e. it is no better than the printf(\"\\n\"); fflush(stdout); sequence it\nreplaces.\n\nThanks.\n"},{"id":"182823","messageId":"1327029129-11424-1-git-send-email-spearce@spearce.org","threadId":"29406","inReplyTo":"7vzkdjgv1i.fsf@alter.siamese.dyndns.org","subject":"[PATCH] remote-curl: Fix push status report when all branches fail","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2012-01-20T03:12:09Z","receivedAt":"2012-01-20T03:12:09Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"From: \"Shawn O. Pearce\" \n\nThe protocol between transport-helper.c and remote-curl requires\nremote-curl to always print a blank line after the push command\nhas run. If the blank line is ommitted, transport-helper kills its\ncontainer process (the git push the user started) with exit(128)\nand no message indicating a problem, assuming the helper already\nprinted reasonable error text to the console.\n\nHowever if the remote rejects all branches with \"ng\" commands in the\nreport-status reply, send-pack terminates with non-zero status, and\nin turn remote-curl exited with non-zero status before outputting\nthe blank line after the helper status printed by send-pack. No\nerror messages reach the user.\n\nThis caused users to see the following from git push over HTTP\nwhen the remote side's update hook rejected the branch:\n\n $ git push http://... master\n Counting objects: 4, done.\n Delta compression using up to 6 threads.\n Compressing objects: 100% (2/2), done.\n Writing objects: 100% (3/3), 301 bytes, done.\n Total 3 (delta 0), reused 0 (delta 0)\n $\n\nAlways print a blank line after the send-pack process terminates,\nensuring the helper status report (if it was output) will be\ncorrectly parsed by the calling transport-helper.c. This ensures\nthe helper doesn't abort before the status report can be shown to\nthe user.\n\nSigned-off-by: Shawn O. Pearce \n---\n remote-curl.c | 9 +++++----\n t/t5541-http-push.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 4 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 48c20b8..25c1af7 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -805,7 +805,7 @@ static int push(int nr_spec, char **specs)\n static void parse_push(struct strbuf *buf)\n {\n \tchar **specs = NULL;\n-\tint alloc_spec = 0, nr_spec = 0, i;\n+\tint alloc_spec = 0, nr_spec = 0, i, ret;\n \n \tdo {\n \t\tif (!prefixcmp(buf->buf, \"push \")) {\n@@ -822,12 +822,13 @@ static void parse_push(struct strbuf *buf)\n \t\t\tbreak;\n \t} while (1);\n \n-\tif (push(nr_spec, specs))\n-\t\texit(128); /* error already reported */\n-\n+\tret = push(nr_spec, specs);\n \tprintf(\"\\n\");\n \tfflush(stdout);\n \n+\tif (ret)\n+\t\texit(128); /* error already reported */\n+\n free_specs:\n \tfor (i = 0; i < nr_spec; i++)\n \t\tfree(specs[i]);\ndiff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh\nindex 9b85d42..4723930 100755\n--- a/t/t5541-http-push.sh\n+++ b/t/t5541-http-push.sh\n@@ -95,6 +95,31 @@ test_expect_success 'create and delete remote branch' '\n \ttest_must_fail git show-ref --verify refs/remotes/origin/dev\n '\n \n+cat >\"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\" <exp < dev2 (hook declined)\n+error: failed to push some refs to 'http://127.0.0.1:5541/smart/test_repo.git'\n+EOF\n+\n+test_expect_success 'rejected update prints status' '\n+\tcd \"$ROOT_PATH\"/test_repo_clone &&\n+\tgit checkout -b dev2 &&\n+\t: >path4 &&\n+\tgit add path4 &&\n+\ttest_tick &&\n+\tgit commit -m dev2 &&\n+\tgit push origin dev2 2>act\n+\ttest_cmp exp act\n+'\n+rm -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n+\n cat >exp < writes:\n\n> diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh\n> index 9b85d42..4723930 100755\n> --- a/t/t5541-http-push.sh\n> +++ b/t/t5541-http-push.sh\n> @@ -95,6 +95,31 @@ test_expect_success 'create and delete remote branch' '\n> \ttest_must_fail git show-ref --verify refs/remotes/origin/dev\n> '\n> \n> +cat >\"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\" < +#!/bin/sh\n> +exit 1\n> +EOF\n> +chmod a+x \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n> +\n> +cat >exp < +remote: error: hook declined to update refs/heads/dev2 \n\nCurious. Where do we get these eight trailing whitespaces?\n\nThe call to rp_error(\"hook declined to update %s\", name) seems to be\ngiving the name properly.\n"},{"id":"182857","messageId":"CAJo=hJtCb=WFfuSKWvPk+S4sRQmSGemG_Ugqj+k1TZCOJj9vLQ@mail.gmail.com","threadId":"29406","inReplyTo":"7vobtyhq16.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] remote-curl: Fix push status report when all branches fail","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2012-01-20T15:15:02Z","receivedAt":"2012-01-20T15:15:02Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Thu, Jan 19, 2012 at 22:00, Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n>> +cat >exp <> +remote: error: hook declined to update refs/heads/dev2\n>\n> Curious. Where do we get these eight trailing whitespaces?\n\nI think this is padding being added to the end of the line by\nrecv_sideband(). I noticed the trailing whitespace in the diff, but\nthe test passed with it present, so I had to leave it in.\n\n> The call to rp_error(\"hook declined to update %s\", name) seems to be\n> giving the name properly.\n\nYea, I think the server is sending the correct data in the sideband\nchannel, its just the sideband client padding out the line. I think\nthis padding is a fudge against progress meters that are being written\nand over-written with \\r lines in subsequent sideband packets.\n"},{"id":"182859","messageId":"8739bacpql.fsf@thomas.inf.ethz.ch","threadId":"29406","inReplyTo":"CAJo=hJtCb=WFfuSKWvPk+S4sRQmSGemG_Ugqj+k1TZCOJj9vLQ@mail.gmail.com","subject":"Re: [PATCH] remote-curl: Fix push status report when all branches fail","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-01-20T16:17:54Z","receivedAt":"2012-01-20T16:17:54Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Shawn Pearce writes:\n\n> On Thu, Jan 19, 2012 at 22:00, Junio C Hamano wrote:\n>> \"Shawn O. Pearce\" writes:\n>>> +cat >exp <>> +remote: error: hook declined to update refs/heads/dev2\n>>\n>> Curious. Where do we get these eight trailing whitespaces?\n>\n> I think this is padding being added to the end of the line by\n> recv_sideband(). I noticed the trailing whitespace in the diff, but\n> the test passed with it present, so I had to leave it in.\n\nISTR we had a policy to guard such whitespace at EOL? Compare\ne.g. c1376c12b7.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"182861","messageId":"1327079011-24788-1-git-send-email-spearce@spearce.org","threadId":"29406","inReplyTo":"8739bacpql.fsf@thomas.inf.ethz.ch","subject":"[PATCH] remote-curl: Fix push status report when all branches fail","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2012-01-20T17:03:31Z","receivedAt":"2012-01-20T17:03:31Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"From: \"Shawn O. Pearce\" \n\nThe protocol between transport-helper.c and remote-curl requires\nremote-curl to always print a blank line after the push command\nhas run. If the blank line is ommitted, transport-helper kills its\ncontainer process (the git push the user started) with exit(128)\nand no message indicating a problem, assuming the helper already\nprinted reasonable error text to the console.\n\nHowever if the remote rejects all branches with \"ng\" commands in the\nreport-status reply, send-pack terminates with non-zero status, and\nin turn remote-curl exited with non-zero status before outputting\nthe blank line after the helper status printed by send-pack. No\nerror messages reach the user.\n\nThis caused users to see the following from git push over HTTP\nwhen the remote side's update hook rejected the branch:\n\n $ git push http://... master\n Counting objects: 4, done.\n Delta compression using up to 6 threads.\n Compressing objects: 100% (2/2), done.\n Writing objects: 100% (3/3), 301 bytes, done.\n Total 3 (delta 0), reused 0 (delta 0)\n $\n\nAlways print a blank line after the send-pack process terminates,\nensuring the helper status report (if it was output) will be\ncorrectly parsed by the calling transport-helper.c. This ensures\nthe helper doesn't abort before the status report can be shown to\nthe user.\n\nSigned-off-by: Shawn O. Pearce \n---\n remote-curl.c | 9 +++++----\n t/t5541-http-push.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 4 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 48c20b8..25c1af7 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -805,7 +805,7 @@ static int push(int nr_spec, char **specs)\n static void parse_push(struct strbuf *buf)\n {\n \tchar **specs = NULL;\n-\tint alloc_spec = 0, nr_spec = 0, i;\n+\tint alloc_spec = 0, nr_spec = 0, i, ret;\n \n \tdo {\n \t\tif (!prefixcmp(buf->buf, \"push \")) {\n@@ -822,12 +822,13 @@ static void parse_push(struct strbuf *buf)\n \t\t\tbreak;\n \t} while (1);\n \n-\tif (push(nr_spec, specs))\n-\t\texit(128); /* error already reported */\n-\n+\tret = push(nr_spec, specs);\n \tprintf(\"\\n\");\n \tfflush(stdout);\n \n+\tif (ret)\n+\t\texit(128); /* error already reported */\n+\n free_specs:\n \tfor (i = 0; i < nr_spec; i++)\n \t\tfree(specs[i]);\ndiff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh\nindex 9b85d42..c68cbf3 100755\n--- a/t/t5541-http-push.sh\n+++ b/t/t5541-http-push.sh\n@@ -95,6 +95,31 @@ test_expect_success 'create and delete remote branch' '\n \ttest_must_fail git show-ref --verify refs/remotes/origin/dev\n '\n \n+cat >\"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\" <exp\n+cat >>exp < dev2 (hook declined)\n+error: failed to push some refs to 'http://127.0.0.1:5541/smart/test_repo.git'\n+EOF\n+\n+test_expect_success 'rejected update prints status' '\n+\tcd \"$ROOT_PATH\"/test_repo_clone &&\n+\tgit checkout -b dev2 &&\n+\t: >path4 &&\n+\tgit add path4 &&\n+\ttest_tick &&\n+\tgit commit -m dev2 &&\n+\tgit push origin dev2 2>act\n+\ttest_cmp exp act\n+'\n+rm -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n+\n cat >exp < writes:\n\n> Always print a blank line after the send-pack process terminates,\n> ensuring the helper status report (if it was output) will be\n> correctly parsed by the calling transport-helper.c. This ensures\n> the helper doesn't abort before the status report can be shown to\n> the user.\n>\n> Signed-off-by: Shawn O. Pearce \n> ---\n\nThanks; let's do the following on top of this patch, so that:\n\n - We won't miss a \"git push\" that errorneously succeeds; and\n\n - We won't be affected by any future change in the sideband #2\n demultiplexor of the amount of \"padding\".\n\n t/t5541-http-push.sh | 9 +++++----\n 1 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git b/t/t5541-http-push.sh a/t/t5541-http-push.sh\nindex d3e340e..b8f4c2a 100755\n--- b/t/t5541-http-push.sh\n+++ a/t/t5541-http-push.sh\n@@ -101,8 +101,8 @@ exit 1\n EOF\n chmod a+x \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n \n-printf 'remote: error: hook declined to update refs/heads/dev2 \\n' >exp\n-cat >>exp <exp < dev2 (hook declined)\n error: failed to push some refs to 'http://127.0.0.1:5541/smart/test_repo.git'\n@@ -115,8 +115,9 @@ test_expect_success 'rejected update prints status' '\n \tgit add path4 &&\n \ttest_tick &&\n \tgit commit -m dev2 &&\n-\tgit push origin dev2 2>act\n-\ttest_cmp exp act\n+\ttest_must_fail git push origin dev2 2>act &&\n+\tsed -e \"/^remote: /s/ *$//\" cmp &&\n+\ttest_cmp exp cmp\n '\n rm -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n \n\n \n"},{"id":"185162","messageId":"20120222101302.GA11606@sigill.intra.peff.net","threadId":"29406","inReplyTo":"1327079011-24788-1-git-send-email-spearce@spearce.org","subject":"Re: [PATCH] remote-curl: Fix push status report when all branches fail","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-22T10:13:02Z","receivedAt":"2012-02-22T10:13:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 20, 2012 at 09:03:31AM -0800, Shawn O. Pearce wrote:\n\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 48c20b8..25c1af7 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -822,12 +822,13 @@ static void parse_push(struct strbuf *buf)\n> \t\t\tbreak;\n> \t} while (1);\n> \n> -\tif (push(nr_spec, specs))\n> -\t\texit(128); /* error already reported */\n> -\n> +\tret = push(nr_spec, specs);\n> \tprintf(\"\\n\");\n> \tfflush(stdout);\n> \n> +\tif (ret)\n> +\t\texit(128); /* error already reported */\n> +\n\nThis hunk is causing intermittent failures of t5541 for me, especially\nwhen the system is under heavy load (e.g., make -j32 test). Before your\npatch, this is what happened:\n\n 1. remote-curl relays the status lines from send-pack, then sees that\n send-pack reported error, and it exits\n\n 2. push reads the status lines, looking for a blank line to terminate\n them. It sees EOF instead of the blank line and exits(128) itself.\n\nAfter your patch, this happens:\n\n 1. remote-curl relays the status lines, alway appends the blank line\n terminator, and then exits\n\n 2. push reads the status lines, including the blank line terminator,\n and reports them to the user.\n\n 3. push then disconnects the remote-curl helper by writing a blank\n line to it (to signal end-of-input), followed by finish_command().\n The latter propagates the error code from the exit in step 1, and\n we use that to signal failure from \"git push\".\n\nThere's a race condition now in step 3. The push process may write to\nthe pipe going to remote-curl after it has exited, causing it to receive\nSIGPIPE and die. We can block SIGPIPE, but that's not sufficient; we'll\nstill notice that our write() returns EPIPE and die.\n\nObviously we can't not print the post-push \"\\n\" in remote-curl, for the\nreasons you outlined in the commit message of this patch. We also can't\nnot exit from remote-curl on error. Even though in the test in t5541 we\nhave signaled error via the ref statuses, we might have received an\nerror that does not come through a ref status (e.g., if we couldn't run\nsend-pack at all).\n\nWe can't not write the \"\\n\" to signal end-of-input to remote-curl,\nbecause we don't actually know yet that there's an error (we find out\nwhen we wait() on the process). Barring any asynchronous SIGCHLD\nhandling, of course, but I don't think we want to get into that.\n\nSo it's kind of a bug in the remote helper protocol. The helpers can\nsignal failure only by dying, but we can find out about that failure\nonly after disconnecting, which involves writing to them. It would be\nmuch more sane if the helpers returned an overall text status from each\ncommand (e.g., printed \"error push failed\" instead of dying).\n\nBut that would involve changing the protocol, of course. I think our\nbest option is to work around it by considering the final blank line we\nsend before disconnect as \"best effort\". That is, it is a courtesy to\nthe remote helper to tell it we are hanging up cleanly, and if it does\nnot arrive, then we can ignore the problem and proceed with closing the\npipe. I.e., something like:\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 6f227e2..f6b3b1f 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -9,6 +9,7 @@\n #include \"remote.h\"\n #include \"string-list.h\"\n #include \"thread-utils.h\"\n+#include \"sigchain.h\"\n \n static int debug;\n \n@@ -220,15 +221,21 @@ static struct child_process *get_helper(struct transport *transport)\n static int disconnect_helper(struct transport *transport)\n {\n \tstruct helper_data *data = transport->data;\n-\tstruct strbuf buf = STRBUF_INIT;\n \tint res = 0;\n \n \tif (data->helper) {\n \t\tif (debug)\n \t\t\tfprintf(stderr, \"Debug: Disconnecting.\\n\");\n \t\tif (!data->no_disconnect_req) {\n-\t\t\tstrbuf_addf(&buf, \"\\n\");\n-\t\t\tsendline(data, &buf);\n+\t\t\t/*\n+\t\t\t * Ignore write errors; there's nothing we can do,\n+\t\t\t * since we're about to close the pipe anyway. And the\n+\t\t\t * most likely error is EPIPE due to the helper dying\n+\t\t\t * to report an error itself.\n+\t\t\t */\n+\t\t\tsigchain_push(SIGPIPE, SIG_IGN);\n+\t\t\txwrite(data->helper->in, \"\\n\", 1);\n+\t\t\tsigchain_pop(SIGPIPE);\n \t\t}\n \t\tclose(data->helper->in);\n \t\tclose(data->helper->out);\n\nwhich makes the t5541 failures go away for me. What do you think?\n\n-Peff\n"},{"id":"185177","messageId":"CAJo=hJsFDrt4rsxVAnx86bxZDY3yfWc1=GDd8opUU+9z7esLnw@mail.gmail.com","threadId":"29406","inReplyTo":"20120222101302.GA11606@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: Fix push status report when all branches fail","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2012-02-22T15:22:10Z","receivedAt":"2012-02-22T15:22:10Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Wed, Feb 22, 2012 at 02:13, Jeff King wrote:\n> On Fri, Jan 20, 2012 at 09:03:31AM -0800, Shawn O. Pearce wrote:\n> This hunk is causing intermittent failures of t5541 for me, especially\n> when the system is under heavy load (e.g., make -j32 test).\n...\n> @@ -220,15 +221,21 @@ static struct child_process *get_helper(struct transport *transport)\n>  static int disconnect_helper(struct transport *transport)\n>  {\n>        struct helper_data *data = transport->data;\n> -       struct strbuf buf = STRBUF_INIT;\n>        int res = 0;\n>\n>        if (data->helper) {\n>                if (debug)\n>                        fprintf(stderr, \"Debug: Disconnecting.\\n\");\n>                if (!data->no_disconnect_req) {\n> -                       strbuf_addf(&buf, \"\\n\");\n> -                       sendline(data, &buf);\n> +                       /*\n> +                        * Ignore write errors; there's nothing we can do,\n> +                        * since we're about to close the pipe anyway. And the\n> +                        * most likely error is EPIPE due to the helper dying\n> +                        * to report an error itself.\n> +                        */\n> +                       sigchain_push(SIGPIPE, SIG_IGN);\n> +                       xwrite(data->helper->in, \"\\n\", 1);\n> +                       sigchain_pop(SIGPIPE);\n>                }\n>                close(data->helper->in);\n>                close(data->helper->out);\n>\n> which makes the t5541 failures go away for me. What do you think?\n\nThis sounds right to me. Its unfortunate that we missed the error\nstatus output when we built the remote helper protocol, but your patch\nabove might be the best we can do now.\n\nEh, well, actually we could have the helper advertise a new capability\nthat can be enabled to return exit status. That is a much bigger\nchange, and even if we do it for remote-curl (since that is in tree\nand easy to update) we still need your patch for the same race\ncondition for out of tree helpers (which Google actually has so I care\nabout out of tree helpers too).\n"},{"id":"185198","messageId":"20120222204050.GB6781@sigill.intra.peff.net","threadId":"29406","inReplyTo":"CAJo=hJsFDrt4rsxVAnx86bxZDY3yfWc1=GDd8opUU+9z7esLnw@mail.gmail.com","subject":"Re: [PATCH] remote-curl: Fix push status report when all branches fail","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-22T20:40:50Z","receivedAt":"2012-02-22T20:40:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 22, 2012 at 07:22:10AM -0800, Shawn O. Pearce wrote:\n\n> > +                       /*\n> > +                        * Ignore write errors; there's nothing we can do,\n> > +                        * since we're about to close the pipe anyway. And the\n> > +                        * most likely error is EPIPE due to the helper dying\n> > +                        * to report an error itself.\n> > +                        */\n> > +                       sigchain_push(SIGPIPE, SIG_IGN);\n> > +                       xwrite(data->helper->in, \"\\n\", 1);\n> > +                       sigchain_pop(SIGPIPE);\n> [...]\n> \n> This sounds right to me. Its unfortunate that we missed the error\n> status output when we built the remote helper protocol, but your patch\n> above might be the best we can do now.\n> \n> Eh, well, actually we could have the helper advertise a new capability\n> that can be enabled to return exit status. That is a much bigger\n> change, and even if we do it for remote-curl (since that is in tree\n> and easy to update) we still need your patch for the same race\n> condition for out of tree helpers (which Google actually has so I care\n> about out of tree helpers too).\n\nI don't think it's worth a new capability. This is one of those \"it\nwould be nice if it were designed that way from day one\" cases, but it\nwasn't. And while this is a minor hack, I don't think it has any\nfunctional downsides. So adding a new capability on top of the hack just\nmakes things more complex.\n\nI'll re-send the patch with a stand-alone commit message.\n\n-Peff\n"},{"id":"185256","messageId":"20120223100434.GA3083@sigill.intra.peff.net","threadId":"29406","inReplyTo":"20120222204050.GB6781@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: Fix push status report when all branches fail","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-23T10:04:34Z","receivedAt":"2012-02-23T10:04:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 22, 2012 at 03:40:50PM -0500, Jeff King wrote:\n\n> I'll re-send the patch with a stand-alone commit message.\n\nHere it is.\n\n-- >8 --\nSubject: [PATCH] disconnect from remote helpers more gently\n\nWhen git spawns a remote helper program (like git-remote-http),\nthe last thing we do before closing the pipe to the child\nprocess is to send a blank line, telling the helper that we\nare done issuing commands. However, the helper may already\nhave exited, in which case the parent git process will\nreceive SIGPIPE and die.\n\nIn particular, this can happen with the remote-curl helper\nwhen it encounters errors during a push. The helper reports\nindividual errors for each ref back to git-push, and then\nexits with a non-zero exit code. Depending on the exact\ntiming of the write, the parent process may or may not\nreceive SIGPIPE.\n\nThis causes intermittent test failure in t5541.8, and is a\nside effect of 5238cbf (remote-curl: Fix push status report\nwhen all branches fail). Before that commit, remote-curl\nwould not send the final blank line to indicate that the\nlist of status lines was complete; it would just exit,\nclosing the pipe. The parent git-push would notice the\nclosed pipe while reading the status report and exit\nimmediately itself, propagating the failing exit code. But\npost-5238cbf, remote-curl completes the status list before\nexiting, git-push actually runs to completion, and then it\ntries to cleanly disconnect the helper, leading to the\nSIGPIPE race above.\n\nThis patch drops all error-checking when sending the final\n\"we are about to hang up\" blank line to helpers. There is\nnothing useful for the parent process to do about errors at\nthat point anyway, and certainly failing to send our \"we are\ndone with commands\" line to a helper that has already exited\nis not a problem.\n\nSigned-off-by: Jeff King \n---\n transport-helper.c | 13 ++++++++++---\n 1 file changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 6f227e2..f6b3b1f 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -9,6 +9,7 @@\n #include \"remote.h\"\n #include \"string-list.h\"\n #include \"thread-utils.h\"\n+#include \"sigchain.h\"\n \n static int debug;\n \n@@ -220,15 +221,21 @@ static struct child_process *get_helper(struct transport *transport)\n static int disconnect_helper(struct transport *transport)\n {\n \tstruct helper_data *data = transport->data;\n-\tstruct strbuf buf = STRBUF_INIT;\n \tint res = 0;\n \n \tif (data->helper) {\n \t\tif (debug)\n \t\t\tfprintf(stderr, \"Debug: Disconnecting.\\n\");\n \t\tif (!data->no_disconnect_req) {\n-\t\t\tstrbuf_addf(&buf, \"\\n\");\n-\t\t\tsendline(data, &buf);\n+\t\t\t/*\n+\t\t\t * Ignore write errors; there's nothing we can do,\n+\t\t\t * since we're about to close the pipe anyway. And the\n+\t\t\t * most likely error is EPIPE due to the helper dying\n+\t\t\t * to report an error itself.\n+\t\t\t */\n+\t\t\tsigchain_push(SIGPIPE, SIG_IGN);\n+\t\t\txwrite(data->helper->in, \"\\n\", 1);\n+\t\t\tsigchain_pop(SIGPIPE);\n \t\t}\n \t\tclose(data->helper->in);\n \t\tclose(data->helper->out);\n-- \n1.7.8.4.8.g10fac\n"},{"id":"185289","messageId":"7vhayh4b5x.fsf@alter.siamese.dyndns.org","threadId":"29406","inReplyTo":"20120223100434.GA3083@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: Fix push status report when all branches fail","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-23T19:11:54Z","receivedAt":"2012-02-23T19:11:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"}]}