{"thread":{"id":"31885","subject":"[PATCH] transport-helper: check when helpers fail","startedAt":"2012-10-21T19:19:18Z","lastAt":"2012-10-22T19:35:00Z","messageCount":9,"participants":["Felipe Contreras","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"201645","messageId":"1350847158-14154-1-git-send-email-felipe.contreras@gmail.com","threadId":"31885","inReplyTo":null,"subject":"[PATCH] transport-helper: check when helpers fail","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-21T19:19:18Z","receivedAt":"2012-10-21T19:19:18Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Otherwise transport-helper will continue checking for refs and other\nthings what will confuse the user more.\n\nSigned-off-by: Felipe Contreras \n---\n git-remote-testgit.py | 3 +++\n run-command.c | 17 +++++++++++++++++\n run-command.h | 1 +\n t/t5800-remote-helpers.sh | 6 ++++++\n transport-helper.c | 8 ++++++++\n 5 files changed, 35 insertions(+)\n\ndiff --git a/git-remote-testgit.py b/git-remote-testgit.py\nindex 5f3ebd2..355e3f5 100644\n--- a/git-remote-testgit.py\n+++ b/git-remote-testgit.py\n@@ -159,6 +159,9 @@ def do_import(repo, args):\n ref = line[7:].strip()\n refs.append(ref)\n \n+ if os.environ.get(\"GIT_REMOTE_TESTGIT_FAILURE\"):\n+ die('Told to fail')\n+\n repo = update_local_repo(repo)\n repo.exporter.export_repo(repo.gitdir, refs)\n \ndiff --git a/run-command.c b/run-command.c\nindex 1101ef7..2852e9d 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -559,6 +559,23 @@ int run_command(struct child_process *cmd)\n \treturn finish_command(cmd);\n }\n \n+int check_command(struct child_process *cmd)\n+{\n+\tint status;\n+\tpid_t pid;\n+\n+\tpid = waitpid(cmd->pid, &status, WNOHANG);\n+\n+\tif (pid < 0)\n+\t\treturn -1;\n+\tif (WIFSIGNALED(status))\n+\t\treturn WTERMSIG(status);\n+\tif (WIFEXITED(status))\n+\t\treturn WEXITSTATUS(status);\n+\n+\treturn 0;\n+}\n+\n static void prepare_run_command_v_opt(struct child_process *cmd,\n \t\t\t\t const char **argv,\n \t\t\t\t int opt)\ndiff --git a/run-command.h b/run-command.h\nindex 44f7d2b..9019e38 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -45,6 +45,7 @@ struct child_process {\n int start_command(struct child_process *);\n int finish_command(struct child_process *);\n int run_command(struct child_process *);\n+int check_command(struct child_process *cmd);\n \n extern int run_hook(const char *index_file, const char *name, ...);\n \ndiff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh\nindex e7dc668..d4b17ae 100755\n--- a/t/t5800-remote-helpers.sh\n+++ b/t/t5800-remote-helpers.sh\n@@ -145,4 +145,10 @@ test_expect_failure 'push new branch with old:new refspec' '\n \tcompare_refs clone HEAD server refs/heads/new-refspec\n '\n \n+test_expect_success 'proper failure checks' '\n+\texport GIT_REMOTE_TESTGIT_FAILURE=1 &&\n+\t! git clone \"testgit::$PWD/server\" failure 2> errors &&\n+\tgrep -q \"Error while running helper\" errors\n+'\n+\n test_done\ndiff --git a/transport-helper.c b/transport-helper.c\nindex cfe0988..fbd923d 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -441,6 +441,10 @@ static int fetch_with_import(struct transport *transport,\n \n \tif (finish_command(&fastimport))\n \t\tdie(\"Error while running fast-import\");\n+\n+\tif (check_command(data->helper))\n+\t\tdie(\"Error while running helper\");\n+\n \tfree(fastimport.argv);\n \tfastimport.argv = NULL;\n \n@@ -784,6 +788,10 @@ static int push_refs_with_export(struct transport *transport,\n \n \tif (finish_command(&exporter))\n \t\tdie(\"Error while running fast-export\");\n+\n+\tif (check_command(data->helper))\n+\t\tdie(\"Error while running helper\");\n+\n \tpush_update_refs_status(data, remote_refs);\n \treturn 0;\n }\n-- \n1.8.0.rc2\n"},{"id":"201653","messageId":"7v7gqjftuj.fsf@alter.siamese.dyndns.org","threadId":"31885","inReplyTo":"1350847158-14154-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH] transport-helper: check when helpers fail","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-21T20:33:40Z","receivedAt":"2012-10-21T20:33:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> Otherwise transport-helper will continue checking for refs and other\n> things what will confuse the user more.\n>\n> Signed-off-by: Felipe Contreras \n> ---\n> git-remote-testgit.py | 3 +++\n> run-command.c | 17 +++++++++++++++++\n> run-command.h | 1 +\n> t/t5800-remote-helpers.sh | 6 ++++++\n> transport-helper.c | 8 ++++++++\n> 5 files changed, 35 insertions(+)\n>\n> diff --git a/git-remote-testgit.py b/git-remote-testgit.py\n> index 5f3ebd2..355e3f5 100644\n> --- a/git-remote-testgit.py\n> +++ b/git-remote-testgit.py\n> @@ -159,6 +159,9 @@ def do_import(repo, args):\n> ref = line[7:].strip()\n> refs.append(ref)\n> \n> + if os.environ.get(\"GIT_REMOTE_TESTGIT_FAILURE\"):\n> + die('Told to fail')\n> +\n\nFun.\n\n> diff --git a/run-command.c b/run-command.c\n> index 1101ef7..2852e9d 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -559,6 +559,23 @@ int run_command(struct child_process *cmd)\n> \treturn finish_command(cmd);\n> }\n> \n> +int check_command(struct child_process *cmd)\n> +{\n> +\tint status;\n> +\tpid_t pid;\n> +\n> +\tpid = waitpid(cmd->pid, &status, WNOHANG);\n> +\n> +\tif (pid < 0)\n> +\t\treturn -1;\n> +\tif (WIFSIGNALED(status))\n> +\t\treturn WTERMSIG(status);\n> +\tif (WIFEXITED(status))\n> +\t\treturn WEXITSTATUS(status);\n> +\n> +\treturn 0;\n> +}\n\nIt is nice to have a separate helper that would theoretically be\nuseful by anybody who runs a child process, but I have to wonder:\n\n - How other codepaths that run child process check their results?\n Do they ignore the result altogether? Do they check the results\n in an ad-hoc way without a good reason? Do they check the\n results differently because their error handling need to be\n different?\n\n With this, I am not requesting to port these other codepaths to\n use this helper in this patch series. But designing the helper\n with potential others' use in mind is within the scope of this\n patch.\n\n - How does the caller use the return value from this helper? I can\n see that \"zero is success, non-zero is failure\", unless there is\n a platform what defines a signum 0 for some signal that can kill\n the child process. But I am not sure if the caller can tell more\n than that from the return value (\"did it die with a signal, or\n did it exit with non-zero status\"?)\n\n It looks to me that this helper loses information; does it belong\n to the public part of API in run-command.c?\n\n> diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh\n> index e7dc668..d4b17ae 100755\n> --- a/t/t5800-remote-helpers.sh\n> +++ b/t/t5800-remote-helpers.sh\n> @@ -145,4 +145,10 @@ test_expect_failure 'push new branch with old:new refspec' '\n> \tcompare_refs clone HEAD server refs/heads/new-refspec\n> '\n> \n> +test_expect_success 'proper failure checks' '\n> +\texport GIT_REMOTE_TESTGIT_FAILURE=1 &&\n> +\t! git clone \"testgit::$PWD/server\" failure 2> errors &&\n> +\tgrep -q \"Error while running helper\" errors\n> +'\n> +\n> test_done\n\nPlease be nicer to people who come *after* you are done with this\nchange. Forcing failure will propagate to any new test added after\nthis piece with this patch.\n\nPerhaps like this:\n\n test_expect_success 'proper failure checks' '\n\t\t(\n\t GIT_REMOTE_TESTGIT_FAILURE=1 &&\n\t export GIT_REMOTE_TESTGIT_FAILURE &&\n\t test_must_fail git clone \"testgit::$PWD/server\" failure\n\t\t) 2>errors &&\n grep \"Error while running helper\" errors\n '\n\nPoints to note:\n\n - The environment does not propagate outside this test this way.\n\n - Avoid \"export VAR=VAL\" to help other people's shell; cf. 69ae92b\n (shell portability: no \"export VAR=VAL\", 2010-10-13).\n\n - Detect uncontrolled exit of \"git clone\" by using test_must_fail.\n\n - Avoid \"grep -q\"; cf. the latter half of aadbe44 (grep portability\n fix: don't use \"-e\" or \"-q\", 2008-03-12).\n\nThanks.\n"},{"id":"201667","messageId":"CAMP44s2Gs8Mucw5hkcDg2vA_MpCypWEzUVFLAT8zRG4opKMPkg@mail.gmail.com","threadId":"31885","inReplyTo":"7v7gqjftuj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] transport-helper: check when helpers fail","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-21T22:20:34Z","receivedAt":"2012-10-21T22:20:34Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Oct 21, 2012 at 10:33 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n\n>> +int check_command(struct child_process *cmd)\n>> +{\n>> + int status;\n>> + pid_t pid;\n>> +\n>> + pid = waitpid(cmd->pid, &status, WNOHANG);\n>> +\n>> + if (pid < 0)\n>> + return -1;\n>> + if (WIFSIGNALED(status))\n>> + return WTERMSIG(status);\n>> + if (WIFEXITED(status))\n>> + return WEXITSTATUS(status);\n>> +\n>> + return 0;\n>> +}\n>\n> It is nice to have a separate helper that would theoretically be\n> useful by anybody who runs a child process, but I have to wonder:\n>\n> - How other codepaths that run child process check their results?\n> Do they ignore the result altogether? Do they check the results\n> in an ad-hoc way without a good reason? Do they check the\n> results differently because their error handling need to be\n> different?\n\nThey probably check at the end, truly waiting for the process to\nfinish, that's not what we want, thus the WNOHANG that apparently\nnobody else in git is using.\n\nThe reason is that the transport-helpers are special; they are\nre-used, so we can't wait for the process to finish after running a\ncertain command.\n\n> With this, I am not requesting to port these other codepaths to\n> use this helper in this patch series. But designing the helper\n> with potential others' use in mind is within the scope of this\n> patch.\n\nThat's what I did. If somebody else needs to check the status of the\ncommand mid-stream, they can do that.\n\n> - How does the caller use the return value from this helper? I can\n> see that \"zero is success, non-zero is failure\", unless there is\n> a platform what defines a signum 0 for some signal that can kill\n> the child process. But I am not sure if the caller can tell more\n> than that from the return value (\"did it die with a signal, or\n> did it exit with non-zero status\"?)\n\nI don't really care, I'm fine returning -1 on all errors.\n\n>> diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh\n>> index e7dc668..d4b17ae 100755\n>> --- a/t/t5800-remote-helpers.sh\n>> +++ b/t/t5800-remote-helpers.sh\n>> @@ -145,4 +145,10 @@ test_expect_failure 'push new branch with old:new refspec' '\n>> compare_refs clone HEAD server refs/heads/new-refspec\n>> '\n>>\n>> +test_expect_success 'proper failure checks' '\n>> + export GIT_REMOTE_TESTGIT_FAILURE=1 &&\n>> + ! git clone \"testgit::$PWD/server\" failure 2> errors &&\n>> + grep -q \"Error while running helper\" errors\n>> +'\n>> +\n>> test_done\n>\n> Please be nicer to people who come *after* you are done with this\n> change. Forcing failure will propagate to any new test added after\n> this piece with this patch.\n>\n> Perhaps like this:\n>\n> test_expect_success 'proper failure checks' '\n> (\n> GIT_REMOTE_TESTGIT_FAILURE=1 &&\n> export GIT_REMOTE_TESTGIT_FAILURE &&\n> test_must_fail git clone \"testgit::$PWD/server\" failure\n> ) 2>errors &&\n> grep \"Error while running helper\" errors\n> '\n\nLGTM.\n\n-- \nFelipe Contreras\n"},{"id":"201682","messageId":"5084E931.3010809@viscovery.net","threadId":"31885","inReplyTo":"1350847158-14154-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH] transport-helper: check when helpers fail","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-22T06:35:29Z","receivedAt":"2012-10-22T06:35:29Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/21/2012 21:19, schrieb Felipe Contreras:\n> diff --git a/run-command.c b/run-command.c\n> index 1101ef7..2852e9d 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -559,6 +559,23 @@ int run_command(struct child_process *cmd)\n> \treturn finish_command(cmd);\n> }\n> \n> +int check_command(struct child_process *cmd)\n> +{\n> +\tint status;\n> +\tpid_t pid;\n> +\n> +\tpid = waitpid(cmd->pid, &status, WNOHANG);\n> +\n> +\tif (pid < 0)\n> +\t\treturn -1;\n> +\tif (WIFSIGNALED(status))\n> +\t\treturn WTERMSIG(status);\n> +\tif (WIFEXITED(status))\n> +\t\treturn WEXITSTATUS(status);\n> +\n> +\treturn 0;\n> +}\n> +\n\nIn this form, the function is not suitable as a public run-command API: If\nthe child did exit, it does not allow finish_command() to do its thing.\nThe only thing the caller of this function can do is to die() if it\nreturns non-zero. It doesn't report treat error cases in the same way as\nwait_or_whine().\n\nI would expect the function to be usable in this way:\n\n\tstart_command(&proc);\n\n\tloop {\n\t\tif (check_command(&proc))\n\t\t\tbreak;\n\t}\n\n\tfinish_command(&proc);\n\nbut it would require a bit more work because it would have to cache the\nexit status in struct child_process.\n\nBTW, you should check for return value 0 from waitpid() explicitly.\n\nAnother thought: In your use-case, isn't it so that it would be an error\nthat the process exited for whatever reason? I.e., even if it exited with\ncode 0 (\"success\"), it would be an error because it violated the protocol?\n\n-- Hannes\n"},{"id":"201684","messageId":"CAMP44s2XDkLhKkqvxnGH+U5X=42dXU1550xVQvyQk=WD2p0c6Q@mail.gmail.com","threadId":"31885","inReplyTo":"5084E931.3010809@viscovery.net","subject":"Re: [PATCH] transport-helper: check when helpers fail","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-22T11:50:00Z","receivedAt":"2012-10-22T11:50:00Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Oct 22, 2012 at 8:35 AM, Johannes Sixt wrote:\n> Am 10/21/2012 21:19, schrieb Felipe Contreras:\n\n> I would expect the function to be usable in this way:\n>\n> start_command(&proc);\n>\n> loop {\n> if (check_command(&proc))\n> break;\n> }\n>\n> finish_command(&proc);\n>\n> but it would require a bit more work because it would have to cache the\n> exit status in struct child_process.\n\nYes, I would expect that as well. I just noticed transport-helper also\nfails with that, but some reason that's not enough to actually fail\nthe tests, so something weird is going on.\n\n> BTW, you should check for return value 0 from waitpid() explicitly.\n\nRight.\n\n> Another thought: In your use-case, isn't it so that it would be an error\n> that the process exited for whatever reason? I.e., even if it exited with\n> code 0 (\"success\"), it would be an error because it violated the protocol?\n\nHow is that violating the protocol?\n\n-- \nFelipe Contreras\n"},{"id":"201693","messageId":"50854E20.1040303@viscovery.net","threadId":"31885","inReplyTo":"CAMP44s2XDkLhKkqvxnGH+U5X=42dXU1550xVQvyQk=WD2p0c6Q@mail.gmail.com","subject":"Re: [PATCH] transport-helper: check when helpers fail","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-22T13:46:08Z","receivedAt":"2012-10-22T13:46:08Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/22/2012 13:50, schrieb Felipe Contreras:\n> On Mon, Oct 22, 2012 at 8:35 AM, Johannes Sixt wrote:\n>> Another thought: In your use-case, isn't it so that it would be an error\n>> that the process exited for whatever reason? I.e., even if it exited with\n>> code 0 (\"success\"), it would be an error because it violated the protocol?\n> \n> How is that violating the protocol?\n\nBecause the helper stops talking too early. But as I said, I actually\ndon't know the protocol.\n\nI was just infering what I saw in transport-helper.c: get_helper() dup's\nthe output of the helper process and stores it in data->out (after\nfdopen()ing on it). (The original file descriptor is handed over to\nfast-import or fast-export.)\n\nActually, I didn't find a spot where data->out was used except to fclose()\nit. But I take it that there is a reason that it exists and infer that\nfurther output from the helper is expected by something after fast-import\nor fast-export have exited.\n\nBut I may be completely off...\n\n-- Hannes\n"},{"id":"201696","messageId":"CAMP44s1m9eVqqrgJFuWOBa3DCZAzAqpVwG8Nxn-6MbXWbF_2fw@mail.gmail.com","threadId":"31885","inReplyTo":"50854E20.1040303@viscovery.net","subject":"Re: [PATCH] transport-helper: check when helpers fail","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-22T14:31:22Z","receivedAt":"2012-10-22T14:31:22Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Oct 22, 2012 at 3:46 PM, Johannes Sixt wrote:\n> Am 10/22/2012 13:50, schrieb Felipe Contreras:\n>> On Mon, Oct 22, 2012 at 8:35 AM, Johannes Sixt wrote:\n>>> Another thought: In your use-case, isn't it so that it would be an error\n>>> that the process exited for whatever reason? I.e., even if it exited with\n>>> code 0 (\"success\"), it would be an error because it violated the protocol?\n>>\n>> How is that violating the protocol?\n>\n> Because the helper stops talking too early. But as I said, I actually\n> don't know the protocol.\n\nWe could use the 'feature done' of fast-import, but this causes\nproblems because of the way transport-helper uses it:\n\n-> import refs/heads/master\n<- exported stuff\n<- done\n-> import refs/heads/devel\n<- exported stuff\n<- done\n\n'done' will terminate the fast-import process, so the second exported\nstuff won't be processed; the fast-import process is reused.\n\nFor some reason remote-testgit doesn't exercise this multiple import\nstuff properly, but my remote-hg certainly does, so I can't just say\n'done'.\n\nIt would be much better if the transport-helper protocol was something\nlike this:\n\n-> import-begin\n<- feature X\n<- feature Y\n-> import refs/heads/master\n<- exported stuff\n-> import refs/heads/devel\n<- exported stuff\n-> import-end\n<- done\n\nThis would certainly makes things easier for transport-helpers that\nsupport multiple ref selections (like my remote-hg). Maybe I should\nadd code that does this if certain feature is specified (so it doesn't\nbreak other helpers)\n\nBut at least on my tests, even with 'feature done' the crash is not\ndetected properly, either by the transport-helper, or fast-import.\n\nAnd also, the msysgit branch does the same check for fast-export,\nwhich actually uses the 'done' feature always, so it should work fine,\nbut perhaps because of the strange issue with fast-import I just\nmentioned, it's not actually detected. I should add tests for this\ntoo.\n\n> I was just infering what I saw in transport-helper.c: get_helper() dup's\n> the output of the helper process and stores it in data->out (after\n> fdopen()ing on it). (The original file descriptor is handed over to\n> fast-import or fast-export.)\n>\n> Actually, I didn't find a spot where data->out was used except to fclose()\n> it. But I take it that there is a reason that it exists and infer that\n> further output from the helper is expected by something after fast-import\n> or fast-export have exited.\n>\n> But I may be completely off...\n\nYes, further output is expected, or at least in theory.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"201704","messageId":"CAMP44s27oPMu8Goded-Tcw9_fmgRux3yiNvu0FBPUxHOdp1Zgw@mail.gmail.com","threadId":"31885","inReplyTo":"CAMP44s1m9eVqqrgJFuWOBa3DCZAzAqpVwG8Nxn-6MbXWbF_2fw@mail.gmail.com","subject":"Re: [PATCH] transport-helper: check when helpers fail","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-22T17:12:06Z","receivedAt":"2012-10-22T17:12:06Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Oct 22, 2012 at 4:31 PM, Felipe Contreras\n wrote:\n\n> -> import-begin\n> <- feature X\n> <- feature Y\n> -> import refs/heads/master\n> <- exported stuff\n> -> import refs/heads/devel\n> <- exported stuff\n> -> import-end\n> <- done\n>\n> This would certainly makes things easier for transport-helpers that\n> support multiple ref selections (like my remote-hg). Maybe I should\n> add code that does this if certain feature is specified (so it doesn't\n> break other helpers)\n\nNever mind this, it's possible to do the same by assuming that all the\nimports will be together, and finished by a line feed, so the code can\ndo:\n\nif import\n do import-begin stuff\n while import\n import stuff\n do import-end stuff\n\nOf course, this could break if for some reason transport-helper\nchanges, but that seems unlikely.\n\n> But at least on my tests, even with 'feature done' the crash is not\n> detected properly, either by the transport-helper, or fast-import.\n\nNever mind this either; I was forcing the error before exporting that\nfeature. See the code at the end.\n\n> And also, the msysgit branch does the same check for fast-export,\n> which actually uses the 'done' feature always, so it should work fine,\n> but perhaps because of the strange issue with fast-import I just\n> mentioned, it's not actually detected. I should add tests for this\n> too.\n\nI have added tests for this, and the failure is detected reliably...\nbut only with remote-testgit, not with my remote-hg, and I've no idea\nwhat is different.\n\nI've tried everything, and yet a SIGPIPE is detected only with\nremote-testgit, not with my code, and they both exit the same way, and\nat the same time, and fast-export exits the main function (apparently\na process can finish with SIGPIPE after main?)\n\nI have no idea what's going on, so I don't know if we need any extra\ncode in transport-helper at all.\n\nAny ideas?\n\nHere is what I have so far:\n\ndiff --git a/git-remote-testgit.py b/git-remote-testgit.py\nindex 5f3ebd2..b8707e6 100644\n--- a/git-remote-testgit.py\n+++ b/git-remote-testgit.py\n@@ -159,6 +159,11 @@ def do_import(repo, args):\n ref = line[7:].strip()\n refs.append(ref)\n\n+ print \"feature done\"\n+\n+ if os.getenv(\"GIT_REMOTE_TESTGIT_FAILURE\"):\n+ die('Told to fail')\n+\n repo = update_local_repo(repo)\n repo.exporter.export_repo(repo.gitdir, refs)\n\n@@ -172,6 +177,9 @@ def do_export(repo, args):\n if not repo.gitdir:\n die(\"Need gitdir to export\")\n\n+ if os.getenv(\"GIT_REMOTE_TESTGIT_FAILURE\"):\n+ die('Told to fail')\n+\n update_local_repo(repo)\n changed = repo.importer.do_import(repo.gitdir)\n\ndiff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh\nindex e7dc668..00b0cd3 100755\n--- a/t/t5800-remote-helpers.sh\n+++ b/t/t5800-remote-helpers.sh\n@@ -145,4 +145,16 @@ test_expect_failure 'push new branch with old:new\nrefspec' '\n \tcompare_refs clone HEAD server refs/heads/new-refspec\n '\n\n+test_expect_success 'proper failure checks for fetching' '\n+\texport GIT_REMOTE_TESTGIT_FAILURE=1 &&\n+\t(cd localclone && ! git fetch) 2> errors &&\n+\tgrep -q \"Error while running fast-import\" errors\n+'\n+\n+test_expect_success 'proper failure checks for pushing' '\n+\texport GIT_REMOTE_TESTGIT_FAILURE=1 &&\n+\t(cd localclone && ! git push --all) 2> errors &&\n+\tgrep -q \"Error while running fast-export\" errors\n+'\n+\n test_done\n\n-- \nFelipe Contreras\n"},{"id":"201706","messageId":"CAMP44s2HRHjoohFOdLT1F6cbGhknmaT3Z5tFfp1gpokoGMxCdQ@mail.gmail.com","threadId":"31885","inReplyTo":"CAMP44s27oPMu8Goded-Tcw9_fmgRux3yiNvu0FBPUxHOdp1Zgw@mail.gmail.com","subject":"Re: [PATCH] transport-helper: check when helpers fail","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2012-10-22T19:35:00Z","receivedAt":"2012-10-22T19:35:00Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Oct 22, 2012 at 7:12 PM, Felipe Contreras\n wrote:\n> On Mon, Oct 22, 2012 at 4:31 PM, Felipe Contreras\n\n> I've tried everything, and yet a SIGPIPE is detected only with\n> remote-testgit, not with my code, and they both exit the same way, and\n> at the same time, and fast-export exits the main function (apparently\n> a process can finish with SIGPIPE after main?)\n>\n> I have no idea what's going on, so I don't know if we need any extra\n> code in transport-helper at all.\n>\n> Any ideas?\n\nMust be a timing issue:\n\nsh -c 'echo hello' | sh -c 'exit 1' -> no signal\nsh -c 'echo hello' | /usr/bin/false -> SIGPIPE\n\nI can trigger it by adding an extra delay:\n\nThis works:\n\ntest_expect_success 'proper failure checks for pushing 1' '\n\texport GIT_REMOTE_TESTGIT_FAILURE=1 &&\n\t(cd localclone && ! git push --all) 2> errors &&\n\tgrep -q \"Error while running fast-export\" errors\n'\n\nThis doesn't:\n\ntest_expect_success 'proper failure checks for pushing 2' '\n\texport GIT_REMOTE_TESTGIT_FAILURE=1 &&\n\texport GIT_REMOTE_TESTGIT_SLEEPY=1 &&\n\t(cd localclone && ! git push --all) 2> errors &&\n\tgrep -q \"Error while running fast-export\" errors\n'\n\nThis does:\n\ntest_expect_success 'proper failure checks for pushing 3' '\n\texport GIT_REMOTE_TESTGIT_FAILURE=1 &&\n\texport GIT_REMOTE_TESTGIT_SLEEPY=1 &&\n\t(cd localclone && ! git push --all) 2> errors &&\n\tgrep -q \"Told to fail\" errors\n'\n\nSo, depending on your luck, transport-helper might or might display an\nerror, it will exit at the right place nonetheless, because of:\n\n\tif (strbuf_getline(buffer, helper, '\\n') == EOF) {\n\t\tif (debug)\n\t\t\tfprintf(stderr, \"Debug: Remote helper quit.\\n\");\n\t\texit(128);\n\t}\n\nNot ideal, but I guess it's not a big deal.\n\nCheers.\n\n-- \nFelipe Contreras\n"}]}