{"thread":{"id":"33349","subject":"[PATCH v2 0/4] run-command: new check_command helper","startedAt":"2013-04-02T10:31:45Z","lastAt":"2013-04-03T00:35:51Z","messageCount":7,"participants":["Felipe Contreras","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"212869","messageId":"1364898709-21583-1-git-send-email-felipe.contreras@gmail.com","threadId":"33349","inReplyTo":null,"subject":"[PATCH v2 0/4] run-command: new check_command helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-02T10:31:45Z","receivedAt":"2013-04-02T10:31:45Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nHere's the second version of the patches incorporating input from Junio and Peff.\n\nThe first patch does all the work, the second patch uses it; basically, this is\nneeded so the transport-helper code is able to check if the remote-helper child\nis stilll running. Without this support, the status of the remote-helper files\nand configuration can end up very badly when errors occur, to the point where\nthe user is unable to use it any more.\n\nThe rest of the patches are for testing purposes only. I ran all the tests with\nthese, and I didn't see any problems.\n\nFelipe Contreras (4):\n  run-command: add new check_command helper\n  transport-helper: check if remote helper is alive\n  tmp: remote-helper: add timers to catch errors\n  tmp: run-command: code to exercise check_command\n\n git-remote-testgit        | 12 ++++++++++\n run-command.c             | 58 ++++++++++++++++++++++++++++++++++++++++++-----\n run-command.h             |  5 ++++\n t/t5801-remote-helpers.sh | 19 ++++++++++++++++\n transport-helper.c        | 11 +++++++++\n 5 files changed, 99 insertions(+), 6 deletions(-)\n\n-- \n1.8.2\n"},{"id":"212870","messageId":"1364898709-21583-2-git-send-email-felipe.contreras@gmail.com","threadId":"33349","inReplyTo":"1364898709-21583-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 1/4] run-command: add new check_command helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-02T10:31:46Z","receivedAt":"2013-04-02T10:31:46Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"And persistent_waitpid() to recover the information from the last run.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n run-command.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++------\n run-command.h |  5 +++++\n 2 files changed, 52 insertions(+), 6 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 07e27ff..b900c6e 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -226,14 +226,27 @@ static inline void set_cloexec(int fd)\n \t\tfcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n }\n \n-static int wait_or_whine(pid_t pid, const char *argv0)\n+static pid_t persistent_waitpid(struct child_process *cmd, pid_t pid, int *status)\n+{\n+\tpid_t waiting;\n+\n+\tif (cmd->last_status.valid) {\n+\t\t*status = cmd->last_status.status;\n+\t\treturn pid;\n+\t}\n+\n+\twhile ((waiting = waitpid(pid, status, 0)) < 0 && errno == EINTR)\n+\t\t;\t/* nothing */\n+\treturn waiting;\n+}\n+\n+static int wait_or_whine(struct child_process *cmd, pid_t pid, const char *argv0)\n {\n \tint status, code = -1;\n \tpid_t waiting;\n \tint failed_errno = 0;\n \n-\twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n-\t\t;\t/* nothing */\n+\twaiting = persistent_waitpid(cmd, pid, &status);\n \n \tif (waiting < 0) {\n \t\tfailed_errno = errno;\n@@ -276,6 +289,8 @@ int start_command(struct child_process *cmd)\n \tint failed_errno = failed_errno;\n \tchar *str;\n \n+\tcmd->last_status.valid = 0;\n+\n \t/*\n \t * In case of errors we must keep the promise to close FDs\n \t * that have been passed in via ->in and ->out.\n@@ -437,7 +452,7 @@ fail_pipe:\n \t\t * At this point we know that fork() succeeded, but execvp()\n \t\t * failed. Errors have been reported to our stderr.\n \t\t */\n-\t\twait_or_whine(cmd->pid, cmd->argv[0]);\n+\t\twait_or_whine(cmd, cmd->pid, cmd->argv[0]);\n \t\tfailed_errno = errno;\n \t\tcmd->pid = -1;\n \t}\n@@ -542,7 +557,7 @@ fail_pipe:\n \n int finish_command(struct child_process *cmd)\n {\n-\treturn wait_or_whine(cmd->pid, cmd->argv[0]);\n+\treturn wait_or_whine(cmd, cmd->pid, cmd->argv[0]);\n }\n \n int run_command(struct child_process *cmd)\n@@ -553,6 +568,32 @@ 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 waiting;\n+\n+\tif (cmd->last_status.valid)\n+\t\treturn 0;\n+\n+\twhile ((waiting = waitpid(cmd->pid, &status, WNOHANG)) < 0 && errno == EINTR)\n+\t\t; /* nothing */\n+\n+\tif (!waiting)\n+\t\treturn 1;\n+\n+\tif (waiting == cmd->pid) {\n+\t\tcmd->last_status.valid = 1;\n+\t\tcmd->last_status.status = status;\n+\t\treturn 0;\n+\t}\n+\n+\tif (waiting > 0)\n+\t\tdie(\"BUG: waitpid reported a random pid?\");\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)\n@@ -729,7 +770,7 @@ error:\n int finish_async(struct async *async)\n {\n #ifdef NO_PTHREADS\n-\treturn wait_or_whine(async->pid, \"child process\");\n+\treturn wait_or_whine(cmd, async->pid, \"child process\");\n #else\n \tvoid *ret = (void *)(intptr_t)(-1);\n \ndiff --git a/run-command.h b/run-command.h\nindex 221ce33..74a733d 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -39,11 +39,16 @@ struct child_process {\n \tunsigned stdout_to_stderr:1;\n \tunsigned use_shell:1;\n \tunsigned clean_on_exit:1;\n+\tstruct last_status {\n+\t\tunsigned valid:1;\n+\t\tint status;\n+\t} last_status;\n };\n \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 *);\n \n extern char *find_hook(const char *name);\n extern int run_hook(const char *index_file, const char *name, ...);\n-- \n1.8.2\n"},{"id":"212871","messageId":"1364898709-21583-3-git-send-email-felipe.contreras@gmail.com","threadId":"33349","inReplyTo":"1364898709-21583-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 2/4] transport-helper: check if remote helper is alive","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-02T10:31:47Z","receivedAt":"2013-04-02T10:31:47Z","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---\n git-remote-testgit        | 11 +++++++++++\n t/t5801-remote-helpers.sh | 19 +++++++++++++++++++\n transport-helper.c        |  8 ++++++++\n 3 files changed, 38 insertions(+)\n\ndiff --git a/git-remote-testgit b/git-remote-testgit\nindex b395c8d..ca0cf09 100755\n--- a/git-remote-testgit\n+++ b/git-remote-testgit\n@@ -61,12 +61,23 @@ do\n \t\t\techo \"feature import-marks=$gitmarks\"\n \t\t\techo \"feature export-marks=$gitmarks\"\n \t\tfi\n+\n+\t\tif test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n+\t\tthen\n+\t\t\texit -1\n+\t\tfi\n+\n \t\techo \"feature done\"\n \t\tgit fast-export \"${testgitmarks_args[@]}\" $refs |\n \t\tsed -e \"s#refs/heads/#${prefix}/heads/#g\"\n \t\techo \"done\"\n \t\t;;\n \texport)\n+\t\tif test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n+\t\tthen\n+\t\t\texit -1\n+\t\tfi\n+\n \t\tbefore=$(git for-each-ref --format='%(refname) %(objectname)')\n \n \t\tgit fast-import \"${testgitmarks_args[@]}\" --quiet\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex f387027..efe67e2 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -166,4 +166,23 @@ test_expect_success 'push ref with existing object' '\n \tcompare_refs local dup server dup\n '\n \n+test_expect_success 'proper failure checks for fetching' '\n+\t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n+\texport GIT_REMOTE_TESTGIT_FAILURE &&\n+\tcd local &&\n+\ttest_must_fail git fetch 2> error &&\n+\tgrep \"Error while running helper\" error\n+\t)\n+'\n+\n+# We sleep to give fast-export a chance to catch the SIGPIPE\n+test_expect_failure 'proper failure checks for pushing' '\n+\t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n+\texport GIT_REMOTE_TESTGIT_FAILURE &&\n+\tcd local &&\n+\ttest_must_fail git push --all 2> error &&\n+\tgrep \"Error while running helper\" error\n+\t)\n+'\n+\n test_done\ndiff --git a/transport-helper.c b/transport-helper.c\nindex cb3ef7d..dfdfa7a 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -460,6 +460,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 \targv_array_free_detached(fastimport.argv);\n \n \t/*\n@@ -818,6 +822,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.2\n"},{"id":"212872","messageId":"1364898709-21583-4-git-send-email-felipe.contreras@gmail.com","threadId":"33349","inReplyTo":"1364898709-21583-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 3/4] tmp: remote-helper: add timers to catch errors","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-02T10:31:48Z","receivedAt":"2013-04-02T10:31:48Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This way the test reliably succeeds (in catching the failure).\n\nNot sure what's the proper way to do this, but here it is for the\nrecord.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n git-remote-testgit        | 1 +\n t/t5801-remote-helpers.sh | 2 +-\n transport-helper.c        | 3 +++\n 3 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/git-remote-testgit b/git-remote-testgit\nindex ca0cf09..6ae1b7f 100755\n--- a/git-remote-testgit\n+++ b/git-remote-testgit\n@@ -75,6 +75,7 @@ do\n \texport)\n \t\tif test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n \t\tthen\n+\t\t\tsleep 1 # don't let fast-export get SIGPIPE\n \t\t\texit -1\n \t\tfi\n \ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex efe67e2..d6702b4 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -176,7 +176,7 @@ test_expect_success 'proper failure checks for fetching' '\n '\n \n # We sleep to give fast-export a chance to catch the SIGPIPE\n-test_expect_failure 'proper failure checks for pushing' '\n+test_expect_success 'proper failure checks for pushing' '\n \t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n \texport GIT_REMOTE_TESTGIT_FAILURE &&\n \tcd local &&\ndiff --git a/transport-helper.c b/transport-helper.c\nindex dfdfa7a..f0d28aa 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -823,6 +823,9 @@ static int push_refs_with_export(struct transport *transport,\n \tif (finish_command(&exporter))\n \t\tdie(\"Error while running fast-export\");\n \n+\tif (getenv(\"GIT_REMOTE_TESTGIT_FAILURE\"))\n+\t\tsleep(2);\n+\n \tif (!check_command(data->helper))\n \t\tdie(\"Error while running helper\");\n \n-- \n1.8.2\n"},{"id":"212873","messageId":"1364898709-21583-5-git-send-email-felipe.contreras@gmail.com","threadId":"33349","inReplyTo":"1364898709-21583-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 4/4] tmp: run-command: code to exercise check_command","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-02T10:31:49Z","receivedAt":"2013-04-02T10:31:49Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Do not merge!\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n run-command.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex b900c6e..2bb3dcf 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -246,6 +246,11 @@ static int wait_or_whine(struct child_process *cmd, pid_t pid, const char *argv0\n \tpid_t waiting;\n \tint failed_errno = 0;\n \n+\tdo {\n+\t\tif (!check_command(cmd))\n+\t\t\tbreak;\n+\t} while(1);\n+\n \twaiting = persistent_waitpid(cmd, pid, &status);\n \n \tif (waiting < 0) {\n-- \n1.8.2\n"},{"id":"212961","messageId":"515B2EAB.10402@kdbg.org","threadId":"33349","inReplyTo":"1364898709-21583-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 1/4] run-command: add new check_command helper","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-04-02T19:16:59Z","receivedAt":"2013-04-02T19:16:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 02.04.2013 12:31, schrieb Felipe Contreras:\n> And persistent_waitpid() to recover the information from the last run.\n\nI'm not a fan of this new API, because it looks like a workaround\nfor a problem that should have been solved in a cleaner way. But if\nwe can't avoid it, please also add a paragraph to\nDocumentation/technical/api-run-command.txt\n\n> +int check_command(struct child_process *cmd)\n> +{\n> +\tint status;\n> +\tpid_t waiting;\n> +\n> +\tif (cmd->last_status.valid)\n> +\t\treturn 0;\n> +\n> +\twhile ((waiting = waitpid(cmd->pid, &status, WNOHANG)) < 0 && errno == EINTR)\n> +\t\t; /* nothing */\n> +\n> +\tif (!waiting)\n> +\t\treturn 1;\n> +\n> +\tif (waiting == cmd->pid) {\n> +\t\tcmd->last_status.valid = 1;\n> +\t\tcmd->last_status.status = status;\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tif (waiting > 0)\n> +\t\tdie(\"BUG: waitpid reported a random pid?\");\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)\n> @@ -729,7 +770,7 @@ error:\n>  int finish_async(struct async *async)\n>  {\n>  #ifdef NO_PTHREADS\n> -\treturn wait_or_whine(async->pid, \"child process\");\n> +\treturn wait_or_whine(cmd, async->pid, \"child process\");\n\nThis breaks the NO_PTHREADS build because cmd is undeclared. Perhaps\nthis on top:\n\ndiff --git a/run-command.c b/run-command.c\nindex a9fa779..a02ef62 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -230,7 +230,7 @@ static pid_t persistent_waitpid(struct child_process *cmd, pid_t pid, int *statu\n {\n \tpid_t waiting;\n \n-\tif (cmd->last_status.valid) {\n+\tif (cmd && cmd->last_status.valid) {\n \t\t*status = cmd->last_status.status;\n \t\treturn pid;\n \t}\n@@ -771,7 +771,7 @@ int start_async(struct async *async)\n int finish_async(struct async *async)\n {\n #ifdef NO_PTHREADS\n-\treturn wait_or_whine(cmd, async->pid, \"child process\");\n+\treturn wait_or_whine(NULL, async->pid, \"child process\");\n #else\n \tvoid *ret = (void *)(intptr_t)(-1);\n \n"},{"id":"213012","messageId":"7v1uas8n88.fsf@alter.siamese.dyndns.org","threadId":"33349","inReplyTo":"1364898709-21583-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 2/4] transport-helper: check if remote helper is alive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-03T00:35:51Z","receivedAt":"2013-04-03T00:35:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Otherwise transport-helper will continue checking for refs and other\n> things what will confuse the user more.\n> ---\n\nSign-off?\n\n>  git-remote-testgit        | 11 +++++++++++\n>  t/t5801-remote-helpers.sh | 19 +++++++++++++++++++\n>  transport-helper.c        |  8 ++++++++\n>  3 files changed, 38 insertions(+)\n>\n> diff --git a/git-remote-testgit b/git-remote-testgit\n> index b395c8d..ca0cf09 100755\n> --- a/git-remote-testgit\n> +++ b/git-remote-testgit\n> @@ -61,12 +61,23 @@ do\n>  \t\t\techo \"feature import-marks=$gitmarks\"\n>  \t\t\techo \"feature export-marks=$gitmarks\"\n>  \t\tfi\n> +\n> +\t\tif test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n> +\t\tthen\n> +\t\t\texit -1\n\nThat looks somewhat funny.  Why not exit 1 or exit 127?\n\n> +\t\tfi\n> +\n\n\n>  \t\techo \"feature done\"\n>  \t\tgit fast-export \"${testgitmarks_args[@]}\" $refs |\n>  \t\tsed -e \"s#refs/heads/#${prefix}/heads/#g\"\n>  \t\techo \"done\"\n>  \t\t;;\n>  \texport)\n> +\t\tif test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n> +\t\tthen\n> +\t\t\texit -1\n\nDitto.\n\n> +\t\tfi\n> +\n>  \t\tbefore=$(git for-each-ref --format='%(refname) %(objectname)')\n>  \n>  \t\tgit fast-import \"${testgitmarks_args[@]}\" --quiet\n> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\n> index f387027..efe67e2 100755\n> --- a/t/t5801-remote-helpers.sh\n> +++ b/t/t5801-remote-helpers.sh\n> @@ -166,4 +166,23 @@ test_expect_success 'push ref with existing object' '\n>  \tcompare_refs local dup server dup\n>  '\n>  \n> +test_expect_success 'proper failure checks for fetching' '\n> +\t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n> +\texport GIT_REMOTE_TESTGIT_FAILURE &&\n> +\tcd local &&\n> +\ttest_must_fail git fetch 2> error &&\n> +\tgrep \"Error while running helper\" error\n> +\t)\n> +'\n> +\n> +# We sleep to give fast-export a chance to catch the SIGPIPE\n> +test_expect_failure 'proper failure checks for pushing' '\n> +\t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n> +\texport GIT_REMOTE_TESTGIT_FAILURE &&\n> +\tcd local &&\n> +\ttest_must_fail git push --all 2> error &&\n> +\tgrep \"Error while running helper\" error\n> +\t)\n> +'\n> +\n>  test_done\n> diff --git a/transport-helper.c b/transport-helper.c\n> index cb3ef7d..dfdfa7a 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -460,6 +460,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>  \targv_array_free_detached(fastimport.argv);\n>  \n>  \t/*\n> @@ -818,6 +822,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"}]}