{"thread":{"id":"33413","subject":"[PATCH v3 0/2] run-command: new check_command helper","startedAt":"2013-04-07T07:45:04Z","lastAt":"2013-04-08T21:21:23Z","messageCount":19,"participants":["Felipe Contreras","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"213386","messageId":"1365320706-13539-1-git-send-email-felipe.contreras@gmail.com","threadId":"33413","inReplyTo":null,"subject":"[PATCH v3 0/2] run-command: new check_command helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-07T07:45:04Z","receivedAt":"2013-04-07T07:45:04Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nThis is a reroll to fix the build with NO_PTHREADS, and also a bit of cleanup.\n\nThe interdiff:\n\n--- b/run-command.c\n+++ a/run-command.c\n@@ -226,27 +226,20 @@ static inline void set_cloexec(int fd)\n \t\tfcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n }\n \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-\twaiting = persistent_waitpid(cmd, pid, &status);\n+\t/* First try the last status from check_command() */\n+\tif (cmd && cmd->last_status.valid) {\n+\t\tstatus = cmd->last_status.status;\n+\t\twaiting = pid;\n+\t} else {\n+\t\twhile ((waiting = waitpid(pid, status, 0)) < 0 && errno == EINTR)\n+\t\t\t;\t/* nothing */\n+\t}\n \n \tif (waiting < 0) {\n \t\tfailed_errno = errno;\n@@ -770,7 +763,7 @@ error:\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 \nFelipe Contreras (2):\n run-command: add new check_command helper\n transport-helper: check if remote helper is alive\n\n git-remote-testgit | 11 +++++++++++\n run-command.c | 46 ++++++++++++++++++++++++++++++++++++++++------\n run-command.h | 5 +++++\n t/t5801-remote-helpers.sh | 19 +++++++++++++++++++\n transport-helper.c | 8 ++++++++\n 5 files changed, 83 insertions(+), 6 deletions(-)\n\n-- \n1.8.2\n"},{"id":"213387","messageId":"1365320706-13539-2-git-send-email-felipe.contreras@gmail.com","threadId":"33413","inReplyTo":"1365320706-13539-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v3 1/2] run-command: add new check_command helper","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-07T07:45:05Z","receivedAt":"2013-04-07T07:45:05Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"And to recover the information from the last run when running\nwait_or_whine().\n\nSigned-off-by: Felipe Contreras \n---\n run-command.c | 46 ++++++++++++++++++++++++++++++++++++++++------\n run-command.h | 5 +++++\n 2 files changed, 45 insertions(+), 6 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 07e27ff..5cb5114 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -226,14 +226,20 @@ 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 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+\t/* First try the last status from check_command() */\n+\tif (cmd && cmd->last_status.valid) {\n+\t\tstatus = cmd->last_status.status;\n+\t\twaiting = pid;\n+\t} else {\n+\t\twhile ((waiting = waitpid(pid, status, 0)) < 0 && errno == EINTR)\n+\t\t\t;\t/* nothing */\n+\t}\n \n \tif (waiting < 0) {\n \t\tfailed_errno = errno;\n@@ -276,6 +282,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 +445,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 +550,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 +561,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 +763,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(NULL, 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":"213388","messageId":"1365320706-13539-3-git-send-email-felipe.contreras@gmail.com","threadId":"33413","inReplyTo":"1365320706-13539-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v3 2/2] transport-helper: check if remote helper is alive","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-07T07:45:06Z","receivedAt":"2013-04-07T07:45:06Z","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":"213435","messageId":"7v1uam44hf.fsf@alter.siamese.dyndns.org","threadId":"33413","inReplyTo":"1365320706-13539-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v3 1/2] run-command: add new check_command helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-07T17:47:56Z","receivedAt":"2013-04-07T17:47:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> And to recover the information from the last run when running\n> wait_or_whine().\n>\n> Signed-off-by: Felipe Contreras \n> ---\n\nThe above says what the updated wait_or_whine() does (it returns the\nstate an earlier call to check_command() has already polled to\ndetermine), but lacks what the added check_command() does and why it\nis needed. I am guessing:\n\n\tIn a codeflow like _this_ calls _that_ calls _that-other_,\n\tand finally the original caller culls the subprocesses by\n\twait-or-whine, there is no good way for an intermediate\n\tlevel to detect dead child and abort the process early.\n\tThey can now poll the status with check_command() without\n\tblocking in order to do so.\n\n\twait_or_while() is adjusted to correctly report the status\n\tof the child that was already culled by an earlier call to\n\tcheck_command(). check_command() itself uses the same\n\tmechanism to indicate that the child was already culled, so\n\tthat it is safe to call it more than once on the same child\n\tprocess.\n\nmight be a more understandable description for this change, but\nthese _this_, _that_ and _that-other_ blanks need to be filled.\n\n> run-command.c | 46 ++++++++++++++++++++++++++++++++++++++++------\n> run-command.h | 5 +++++\n> 2 files changed, 45 insertions(+), 6 deletions(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 07e27ff..5cb5114 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -226,14 +226,20 @@ 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 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> +\t/* First try the last status from check_command() */\n> +\tif (cmd && cmd->last_status.valid) {\n> +\t\tstatus = cmd->last_status.status;\n> +\t\twaiting = pid;\n> +\t} else {\n> +\t\twhile ((waiting = waitpid(pid, status, 0)) < 0 && errno == EINTR)\n> +\t\t\t;\t/* nothing */\n> +\t}\n> \n> \tif (waiting < 0) {\n> \t\tfailed_errno = errno;\n> @@ -276,6 +282,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 +445,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 +550,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 +561,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 +763,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(NULL, async->pid, \"child process\");\n> #else\n> \tvoid *ret = (void *)(intptr_t)(-1);\n> \n> diff --git a/run-command.h b/run-command.h\n> index 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"},{"id":"213440","messageId":"7vwqse2pu0.fsf@alter.siamese.dyndns.org","threadId":"33413","inReplyTo":"1365320706-13539-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-07T17:49:43Z","receivedAt":"2013-04-07T17:49:43Z","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\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\nIt is rather unusual to see a negative return value returned from a\nshell script. Shouldn't this be something like 127 (or 1)?\n\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"},{"id":"213441","messageId":"7vsj322ppu.fsf@alter.siamese.dyndns.org","threadId":"33413","inReplyTo":"1365320706-13539-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v3 1/2] run-command: add new check_command helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-07T17:52:13Z","receivedAt":"2013-04-07T17:52:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> -\twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n> -\t\t;\t/* nothing */\n> +\t/* First try the last status from check_command() */\n> +\tif (cmd && cmd->last_status.valid) {\n> +\t\tstatus = cmd->last_status.status;\n> +\t\twaiting = pid;\n> +\t} else {\n> +\t\twhile ((waiting = waitpid(pid, status, 0)) < 0 && errno == EINTR)\n> +\t\t\t;\t/* nothing */\n\nwaitpid(pid, &status, 0), I presume.\n"},{"id":"213484","messageId":"20130408005155.GA24030@sigill.intra.peff.net","threadId":"33413","inReplyTo":"1365320706-13539-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-08T00:51:55Z","receivedAt":"2013-04-08T00:51:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 07, 2013 at 01:45:06AM -0600, Felipe Contreras wrote:\n\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\nHmm. If you drop the final \"grep\" (which is looking for the error\nmessage you add elsewhere in this patch), this test passes even without\nthe addition of the check_command calls added by your patch. Which made\nme wonder if we should be checking something else (repo state, error\nmessages, etc), since we seem to otherwise be detecting the error. More\nanalysis below on exactly what is going on there.\n\n> +# We sleep to give fast-export a chance to catch the SIGPIPE\n\nI'm not sure what this means; I don't see a sleep anywhere.\n\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\nI wondered why this one is marked for failure.\n\nEven without check_command, the push produces:\n\n error: fast-export died of signal 13\n fatal: Error while running fast-export\n\nwhich explains why the test fails: it does not even make it to the\ncheck_command call at all. Why do we need check_command here, then? Is\nit racy/non-deterministic for fast-export to die due to the pipe?\n\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\nI'm still not very excited about this approach, as it is racy to detect\nerrors. E.g., there is nothing to say that the helper does not close the\npipe to fast-import prematurely and then die afterwards, leaving the\nrefs unwritten, fast-import happy, but a failed exit code from the\nhelper.\n\nThe import test still passes even without the check_command part of your\npatch because some of the refs in refs/testgit do not exist prior to the\nfailed fetch, and therefore also do not exist afterwards. When fetch\ntries to feed their sha1s to check_everything_connected, you get the\nfunny:\n\n fatal: bad object 0000000000000000000000000000000000000000\n error: testgit::/home/peff/compile/git/t/trash\n directory.t5801-remote-helpers/server did not send all necessary\n objects\n\nBut if we change the test like this:\n\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex efe67e2..e968ea9 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -167,11 +167,11 @@ test_expect_success 'proper failure checks for fetching' '\n '\n \n test_expect_success 'proper failure checks for fetching' '\n-\t(GIT_REMOTE_TESTGIT_FAILURE=1 &&\n+\t(cd local &&\n+\tgit fetch &&\n+\tGIT_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+\ttest_must_fail git fetch 2> error\n \t)\n '\n \nwe can see that without your check_command, the failure in the second\nfetch is not noticed. Adding in your patch does detect this. _But_ it is\nonly checking a specific failure mode of the remote-helper: process\ndeath that results in closing the fast-import pipe, which is how your\nGIT_REMOTE_TESTGIT_FAILURE is implemented (closing the pipe first and\nthen dying is racy).\n\nIf we then add this on top of your series (fixing the \"status\" bug in\npatch 1 that Junio mentioned), the test will start failing (both the\noriginal, and the more robust one I showed above):\n\ndiff --git a/git-remote-testgit b/git-remote-testgit\nindex ca0cf09..41b0780 100755\n--- a/git-remote-testgit\n+++ b/git-remote-testgit\n@@ -64,6 +64,11 @@ do\n \n \t\tif test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n \t\tthen\n+\t\t\t# close stdout\n+\t\t\texec >/dev/null\n+\t\t\t# now sleep to make sure fast-import\n+\t\t\t# has time to die before we exit\n+\t\t\tsleep 1\n \t\t\texit -1\n \t\tfi\n \n\nI agree that the failure mode from your patch is probably the most\ncommon order for helpers to fail in (i.e., they just die and that's what\nkills the pipe). But I wonder if we can just cover all cases.\nSomething like:\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex dfdfa7a..8562df0 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -461,7 +461,31 @@ static int fetch_with_import(struct transport *transport,\n \tif (finish_command(&fastimport))\n \t\tdie(\"Error while running fast-import\");\n \n-\tif (!check_command(data->helper))\n+\t/*\n+\t * We must disconnect from the helper at this point, because even\n+\t * though fast-import may have succeeded, it may only be because the\n+\t * helper was not able to feed fast-import all of the data, and what\n+\t * fast-import got looked OK (e.g., it may have got nothing if the\n+\t * helper died early). We still need to check the return code of the\n+\t * helper to make sure it is happy with what it sent.\n+\t *\n+\t * Since the import command does not require the helper to ever report\n+\t * success/failure of the import, we have no mechanism to check for\n+\t * problems except to check its exit status.\n+\t *\n+\t * Callers of the transport code are allowed to make more requests\n+\t * of our helper, so we may be disconnecting before they expect in that\n+\t * case. However:\n+\t *\n+\t * 1. Current callers don't do that; after fetching refs, there\n+\t * is nothing left for the helper to do.\n+\t *\n+\t * 2. We transparently start the helper as necessary, so if we were\n+\t * to get another request (e.g., to import more refs), we would\n+\t * simply start a new instantiation of the helper.\n+\t *\n+\t */\n+\tif (disconnect_helper(transport) != 0)\n \t\tdie(\"Error while running helper\");\n \n \targv_array_free_detached(fastimport.argv);\n\nwhich passes both your original test and the more strict one above. Of\ncourse adding a done-import capability would be nice to fix the protocol\ndeficiency, but it is more code.\n\n> @@ -818,6 +822,10 @@ static int push_refs_with_export(struct transport *transport,\n> \n> if (finish_command(&exporter))\n> die(\"Error while running fast-export\");\n> +\n> + if (!check_command(data->helper))\n> + die(\"Error while running helper\");\n> +\n> push_update_refs_status(data, remote_refs);\n> return 0;\n> }\n\nAnd this one is even more likely to race than the import case, I think,\nas the exporter may send all of its data, exit, and then the helper\nchugs on it for a bit (converting, sending across the network, etc). If\nit is still chugging when we run check_command, we will not notice\nanything here.\n\nE.g., if we instrument the failure like this:\n\ndiff --git a/git-remote-testgit b/git-remote-testgit\nindex ca0cf09..a912bc1 100755\n--- a/git-remote-testgit\n+++ b/git-remote-testgit\n@@ -75,6 +75,12 @@ do\n \texport)\n \t\tif test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n \t\tthen\n+\t\t\t# imagine we read all of fast-export's data\n+\t\t\t# first, and then die while trying to convert\n+\t\t\t# it\n+\t\t\twhile read line; do\n+\t\t\t\ttest \"$line\" = \"done\" && break\n+\t\t\tdone\n \t\t\texit -1\n \t\tfi\n \n\nwe do not get the sigpipe from fast-export, and depending on the timing,\nyour check_command may or may not do anything (in my tests, it did not).\n\nBut unlike the import side, the export command _does_ give us a status\nreport back from the helper: it prints an ok/error line for each ref\nthat was exported, followed by a blank line. So we should be able to\nconfirm that we get the blank line at all, and then that each ref was\npresent, which would determine whether the export failed or not without\nbeing subject to race conditions.\n\nAnd we seem to do those checks already; the only problem I see is that\nrecvline does not print a message before dying. Would the patch below be\nsufficient?\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex dfdfa7a..cce2062 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer)\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\tdie(\"remote helper died unexpectedly\");\n \t}\n \n \tif (debug)\n\n-Peff\n"},{"id":"213483","messageId":"CAMP44s0PYARrCQxyBOiGzCAKTXqGp5yrKyfVwLjE1CyuxVSU2A@mail.gmail.com","threadId":"33413","inReplyTo":"20130408005155.GA24030@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-08T02:03:25Z","receivedAt":"2013-04-08T02:03:25Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Apr 7, 2013 at 7:51 PM, Jeff King wrote:\n> On Sun, Apr 07, 2013 at 01:45:06AM -0600, Felipe Contreras wrote:\n>\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>> compare_refs local dup server dup\n>> '\n>>\n>> +test_expect_success 'proper failure checks for fetching' '\n>> + (GIT_REMOTE_TESTGIT_FAILURE=1 &&\n>> + export GIT_REMOTE_TESTGIT_FAILURE &&\n>> + cd local &&\n>> + test_must_fail git fetch 2> error &&\n>> + grep \"Error while running helper\" error\n>> + )\n>> +'\n>\n> Hmm. If you drop the final \"grep\" (which is looking for the error\n> message you add elsewhere in this patch), this test passes even without\n> the addition of the check_command calls added by your patch. Which made\n> me wonder if we should be checking something else (repo state, error\n> messages, etc), since we seem to otherwise be detecting the error. More\n> analysis below on exactly what is going on there.\n\nIt fails all right, because the fetch process assumes the\nremote-helper succeeded and only later eventually fails with an error\nthat is anything but user-friendly:\n\nfatal: bad object 0000000000000000000000000000000000000000\nerror: testgit::/home/felipec/dev/git/t/trash\ndirectory.t5801-remote-helpers/server did not send all necessary\nobjects\n\n>> +# We sleep to give fast-export a chance to catch the SIGPIPE\n>\n> I'm not sure what this means; I don't see a sleep anywhere.\n\nThat's right, I'll remove the comment. It was cruft from the next\npatch (see below).\n\n>> +test_expect_failure 'proper failure checks for pushing' '\n>> + (GIT_REMOTE_TESTGIT_FAILURE=1 &&\n>> + export GIT_REMOTE_TESTGIT_FAILURE &&\n>> + cd local &&\n>> + test_must_fail git push --all 2> error &&\n>> + grep \"Error while running helper\" error\n>> + )\n>> +'\n>\n> I wondered why this one is marked for failure.\n\nBecause it fails to report the right error.\n\n> Even without check_command, the push produces:\n>\n> error: fast-export died of signal 13\n> fatal: Error while running fast-export\n>\n> which explains why the test fails: it does not even make it to the\n> check_command call at all. Why do we need check_command here, then? Is\n> it racy/non-deterministic for fast-export to die due to the pipe?\n\nIt does not make it to check_command because the wait is missing. I\nadded that in a separate patch:\nhttp://article.gmane.org/gmane.comp.version-control.git/219715\n\nWith that patch the check_command happens reliably, the error is\nreported correctly, and the test passes. However, the way to trigger\nit is ugly.\n\nIn a real use-case the chances of check_command catching the issue are\nmuch higher..\n\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>> if (finish_command(&fastimport))\n>> die(\"Error while running fast-import\");\n>> +\n>> + if (!check_command(data->helper))\n>> + die(\"Error while running helper\");\n>> +\n>> argv_array_free_detached(fastimport.argv);\n>\n> I'm still not very excited about this approach, as it is racy to detect\n> errors. E.g., there is nothing to say that the helper does not close the\n> pipe to fast-import prematurely and then die afterwards, leaving the\n> refs unwritten, fast-import happy, but a failed exit code from the\n> helper.\n\nThat's right, and that would leave us in the situation we are right\nnow, even with \"done\" check.\n\n> The import test still passes even without the check_command part of your\n> patch because some of the refs in refs/testgit do not exist prior to the\n> failed fetch, and therefore also do not exist afterwards. When fetch\n> tries to feed their sha1s to check_everything_connected, you get the\n> funny:\n>\n> fatal: bad object 0000000000000000000000000000000000000000\n> error: testgit::/home/peff/compile/git/t/trash\n> directory.t5801-remote-helpers/server did not send all necessary\n> objects\n\nAnd you think that is desirable? User-friendly?\n\n> we can see that without your check_command, the failure in the second\n> fetch is not noticed. Adding in your patch does detect this. _But_ it is\n> only checking a specific failure mode of the remote-helper: process\n> death that results in closing the fast-import pipe, which is how your\n> GIT_REMOTE_TESTGIT_FAILURE is implemented (closing the pipe first and\n> then dying is racy).\n>\n> If we then add this on top of your series (fixing the \"status\" bug in\n> patch 1 that Junio mentioned), the test will start failing (both the\n> original, and the more robust one I showed above):\n>\n> diff --git a/git-remote-testgit b/git-remote-testgit\n> index ca0cf09..41b0780 100755\n> --- a/git-remote-testgit\n> +++ b/git-remote-testgit\n> @@ -64,6 +64,11 @@ do\n>\n> if test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n> then\n> + # close stdout\n> + exec >/dev/null\n> + # now sleep to make sure fast-import\n> + # has time to die before we exit\n> + sleep 1\n> exit -1\n> fi\n\nYeah, again:\nhttp://article.gmane.org/gmane.comp.version-control.git/219715\n\n> I agree that the failure mode from your patch is probably the most\n> common order for helpers to fail in (i.e., they just die and that's what\n> kills the pipe). But I wonder if we can just cover all cases.\n> Something like:\n>\n> diff --git a/transport-helper.c b/transport-helper.c\n> index dfdfa7a..8562df0 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -461,7 +461,31 @@ static int fetch_with_import(struct transport *transport,\n> if (finish_command(&fastimport))\n> die(\"Error while running fast-import\");\n>\n> - if (!check_command(data->helper))\n> + /*\n> + * We must disconnect from the helper at this point, because even\n> + * though fast-import may have succeeded, it may only be because the\n> + * helper was not able to feed fast-import all of the data, and what\n> + * fast-import got looked OK (e.g., it may have got nothing if the\n> + * helper died early). We still need to check the return code of the\n> + * helper to make sure it is happy with what it sent.\n> + *\n> + * Since the import command does not require the helper to ever report\n> + * success/failure of the import, we have no mechanism to check for\n> + * problems except to check its exit status.\n> + *\n> + * Callers of the transport code are allowed to make more requests\n> + * of our helper, so we may be disconnecting before they expect in that\n> + * case. However:\n> + *\n> + * 1. Current callers don't do that; after fetching refs, there\n> + * is nothing left for the helper to do.\n> + *\n> + * 2. We transparently start the helper as necessary, so if we were\n> + * to get another request (e.g., to import more refs), we would\n> + * simply start a new instantiation of the helper.\n> + *\n> + */\n\nThat's a comprehensive essay, unfortunately, it's not correct. The\nexit status of the remote-helper is not important, it's the one of\nfast-import that we really care about.\n\n> + if (disconnect_helper(transport) != 0)\n> die(\"Error while running helper\");\n\nYeah, that's good, *if* the remote-helper is implemented correctly,\nbut try this:\n\n if test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n then\n exit 0\n fi\n\n> argv_array_free_detached(fastimport.argv);\n>\n> which passes both your original test and the more strict one above. Of\n> course adding a done-import capability would be nice to fix the protocol\n> deficiency, but it is more code.\n\nThe done-import capability *is already there*. The remote helper fails\nto say \"done\", fast-import detects that there was a problem and exits\nwith -1 (or whatever), but it doesn't matter, because we\n(transport-helper) are oblivious of that return status until it's too\nlate.\n\n>> @@ -818,6 +822,10 @@ static int push_refs_with_export(struct transport *transport,\n>>\n>> if (finish_command(&exporter))\n>> die(\"Error while running fast-export\");\n>> +\n>> + if (!check_command(data->helper))\n>> + die(\"Error while running helper\");\n>> +\n>> push_update_refs_status(data, remote_refs);\n>> return 0;\n>> }\n>\n> And this one is even more likely to race than the import case, I think,\n> as the exporter may send all of its data, exit, and then the helper\n> chugs on it for a bit (converting, sending across the network, etc). If\n> it is still chugging when we run check_command, we will not notice\n> anything here.\n>\n> E.g., if we instrument the failure like this:\n>\n> diff --git a/git-remote-testgit b/git-remote-testgit\n> index ca0cf09..a912bc1 100755\n> --- a/git-remote-testgit\n> +++ b/git-remote-testgit\n> @@ -75,6 +75,12 @@ do\n> export)\n> if test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n> then\n> + # imagine we read all of fast-export's data\n> + # first, and then die while trying to convert\n> + # it\n> + while read line; do\n> + test \"$line\" = \"done\" && break\n> + done\n> exit -1\n> fi\n>\n>\n> we do not get the sigpipe from fast-export, and depending on the timing,\n> your check_command may or may not do anything (in my tests, it did not).\n>\n> But unlike the import side, the export command _does_ give us a status\n> report back from the helper: it prints an ok/error line for each ref\n> that was exported, followed by a blank line. So we should be able to\n> confirm that we get the blank line at all, and then that each ref was\n> present, which would determine whether the export failed or not without\n> being subject to race conditions.\n>\n> And we seem to do those checks already; the only problem I see is that\n> recvline does not print a message before dying. Would the patch below be\n> sufficient?\n>\n> diff --git a/transport-helper.c b/transport-helper.c\n> index dfdfa7a..cce2062 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer)\n> if (strbuf_getline(buffer, helper, '\\n') == EOF) {\n> if (debug)\n> fprintf(stderr, \"Debug: Remote helper quit.\\n\");\n> - exit(128);\n> + die(\"remote helper died unexpectedly\");\n> }\n>\n> if (debug)\n\nYeah, I already explored this option, and I said it was possible on\npushing, but now the problem is fetching.\n\nhttp://article.gmane.org/gmane.comp.version-control.git/219760\n\nAnd to be frank, I'm tired of this. I keep repeating the same things\nover and over, when I ask for input on different ways to achieve this\nI get nothing, and when the patch is getting closer to be merged, then\nI receive criticism, only so I can repeat the same again.\n\nI don't like the option to die right in recvline(), but it would work.\nWe would need something else for import though. It would be possible\nto tell fast-import to ping the remote-helper, but I ran into a\nSIGPIPE, and I don't have the patience to find out if perhaps there's\na way to solve that. This option seems rather hacky and asymmetric to\nme.\n\nIf this current patch series is not good enough, I think it's best to\nleave things as they are for me.\n\nCheers.\n\n--\nFelipe Contreras\n"},{"id":"213493","messageId":"20130408023351.GA32558@sigill.intra.peff.net","threadId":"33413","inReplyTo":"CAMP44s0PYARrCQxyBOiGzCAKTXqGp5yrKyfVwLjE1CyuxVSU2A@mail.gmail.com","subject":"Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-08T02:33:51Z","receivedAt":"2013-04-08T02:33:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 07, 2013 at 09:03:25PM -0500, Felipe Contreras wrote:\n\n> > I'm still not very excited about this approach, as it is racy to detect\n> > errors. E.g., there is nothing to say that the helper does not close the\n> > pipe to fast-import prematurely and then die afterwards, leaving the\n> > refs unwritten, fast-import happy, but a failed exit code from the\n> > helper.\n> \n> That's right, and that would leave us in the situation we are right\n> now, even with \"done\" check.\n\nYes. I do not think your patch makes anything worse. But it also does\nnot fix the problem completely.\n\n> > The import test still passes even without the check_command part of your\n> > patch because some of the refs in refs/testgit do not exist prior to the\n> > failed fetch, and therefore also do not exist afterwards. When fetch\n> > tries to feed their sha1s to check_everything_connected, you get the\n> > funny:\n> >\n> > fatal: bad object 0000000000000000000000000000000000000000\n> > error: testgit::/home/peff/compile/git/t/trash\n> > directory.t5801-remote-helpers/server did not send all necessary\n> > objects\n> \n> And you think that is desirable? User-friendly?\n\nNo, as you probably realized if you read the rest of my email. My point\nin bringing it up was to try to explain exactly what is going on in each\ncase. Your commit message provided zero explanation of what the current\nproblem is, nor why your fix is the right thing to do.\n\n> > - if (!check_command(data->helper))\n> > + /*\n> > + * We must disconnect from the helper at this point, because even\n> > + * though fast-import may have succeeded, it may only be because the\n> > + * helper was not able to feed fast-import all of the data, and what\n> > + * fast-import got looked OK (e.g., it may have got nothing if the\n> > + * helper died early). We still need to check the return code of the\n> > + * helper to make sure it is happy with what it sent.\n> > + *\n> > + * Since the import command does not require the helper to ever report\n> > + * success/failure of the import, we have no mechanism to check for\n> > + * problems except to check its exit status.\n> > + *\n> > + * Callers of the transport code are allowed to make more requests\n> > + * of our helper, so we may be disconnecting before they expect in that\n> > + * case. However:\n> > + *\n> > + * 1. Current callers don't do that; after fetching refs, there\n> > + * is nothing left for the helper to do.\n> > + *\n> > + * 2. We transparently start the helper as necessary, so if we were\n> > + * to get another request (e.g., to import more refs), we would\n> > + * simply start a new instantiation of the helper.\n> > + *\n> > + */\n> \n> That's a comprehensive essay, unfortunately, it's not correct. The\n> exit status of the remote-helper is not important, it's the one of\n> fast-import that we really care about.\n\nNowhere does it say that we should not check fast-import's exit value,\nand indeed, the patch does not replace that check at all. It comes\nimmediately after. It replaces the WNOHANG check of the helper's exit\ncode (i.e., check_command) with a synchronous check.\n\nThe argument that the exit status of remote-helper is not important\nwould mean your series is pointless, too, no?\n\n> > + if (disconnect_helper(transport) != 0)\n> > die(\"Error while running helper\");\n> \n> Yeah, that's good, *if* the remote-helper is implemented correctly,\n> but try this:\n> \n> if test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n> then\n> exit 0\n> fi\n\nI don't see your point. If a helper reports success when it has failed,\nand doesn't feed anything to fast-import, we can do nothing. That case\nis indistinguishable from a helper which has nothing to fetch from the\nremote, and exits. My patch doesn't help with such a broken helper, but\nneither does yours, because they check the exact same thing.\n\n> > argv_array_free_detached(fastimport.argv);\n> >\n> > which passes both your original test and the more strict one above. Of\n> > course adding a done-import capability would be nice to fix the protocol\n> > deficiency, but it is more code.\n> \n> The done-import capability *is already there*. The remote helper fails\n> to say \"done\", fast-import detects that there was a problem and exits\n> with -1 (or whatever), but it doesn't matter, because we\n> (transport-helper) are oblivious of that return status until it's too\n> late.\n\nWait, then why is this a problem at all? We check the exit code of\nfast-import right before the code in question.\n\n> Yeah, I already explored this option, and I said it was possible on\n> pushing, but now the problem is fetching.\n> \n> http://article.gmane.org/gmane.comp.version-control.git/219760\n> \n> And to be frank, I'm tired of this. I keep repeating the same things\n> over and over, when I ask for input on different ways to achieve this\n> I get nothing, and when the patch is getting closer to be merged, then\n> I receive criticism, only so I can repeat the same again.\n\nI don't even know what to say to this. I reviewed your original patch,\nraised some concerns. You responded. I had not yet responded back,\nbecause I have many other things to work on. In the meantime you\nsubmitted another round. Now I have the opportunity to respond and did\nso, having read both threads.\n\nWhy is your patch to recvline so much more complicated than my\none-liner? Why do you go to lengths to pass out an error code from it\njust so that we can end up dying anyway? Why do you need a done\ncapability for push when the blank line already signals the end of the\nstatus?\n\nI did not respond to 219760 separately because those are all questions\nthat _don't matter_ if you follow the analysis in my last email that\nlead to my one-liner. Maybe there are interesting answers to those\nquestions, and my one-liner isn't sufficient. But I'll never know\nbecause instead of pointing them out, you are complaining that I didn't\nrespond fast enough.\n\n> I don't like the option to die right in recvline(), but it would work.\n> We would need something else for import though. It would be possible\n> to tell fast-import to ping the remote-helper, but I ran into a\n> SIGPIPE, and I don't have the patience to find out if perhaps there's\n> a way to solve that. This option seems rather hacky and asymmetric to\n> me.\n\nA ping-check would work. You just need:\n\n sigchain_push(SIGPIPE, SIG_IGN);\n /*\n * send ping; write can get EPIPE here, which is fine; we\n * would want to treat write errors as a failed ping\n */\n sigchain_pop(SIGPIPE);\n\nbut we would have to add a ping command, no?\n\n> If this current patch series is not good enough, I think it's best to\n> leave things as they are for me.\n\nThat's certainly your choice. It just seems like we can be more robust\nwhile adding less code, as I tried to show.\n\n-Peff\n"},{"id":"213517","messageId":"CAMP44s1cphJ5wojt2OeZjWJVOi5AMQJR2jN_BCkQw0B0BxpUfA@mail.gmail.com","threadId":"33413","inReplyTo":"20130408023351.GA32558@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-08T14:38:28Z","receivedAt":"2013-04-08T14:38:28Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Apr 7, 2013 at 9:33 PM, Jeff King wrote:\n> On Sun, Apr 07, 2013 at 09:03:25PM -0500, Felipe Contreras wrote:\n\n>> And you think that is desirable? User-friendly?\n>\n> No, as you probably realized if you read the rest of my email. My point\n> in bringing it up was to try to explain exactly what is going on in each\n> case. Your commit message provided zero explanation of what the current\n> problem is, nor why your fix is the right thing to do.\n\nI have explained this many times, I wonder why when the patch is close\nto be merged does this become important, and not before.\n\n>> That's a comprehensive essay, unfortunately, it's not correct. The\n>> exit status of the remote-helper is not important, it's the one of\n>> fast-import that we really care about.\n>\n> Nowhere does it say that we should not check fast-import's exit value,\n> and indeed, the patch does not replace that check at all. It comes\n> immediately after. It replaces the WNOHANG check of the helper's exit\n> code (i.e., check_command) with a synchronous check.\n\nYeah, and in the process it's changing the design of transport-helper,\nwhere the pipes are closed only at the very end.\n\n> The argument that the exit status of remote-helper is not important\n> would mean your series is pointless, too, no?\n\nThe main point of my patch was not to add a check for import, but for\nexport. Adding such a check here is not important, but it doesn't hurt\neither, as it would improve the situation if the remote-helper is not\nusing the \"done\" feature, which at the moment is a possibility.\n\n>> > + if (disconnect_helper(transport) != 0)\n>> > die(\"Error while running helper\");\n>>\n>> Yeah, that's good, *if* the remote-helper is implemented correctly,\n>> but try this:\n>>\n>> if test -n \"$GIT_REMOTE_TESTGIT_FAILURE\"\n>> then\n>> exit 0\n>> fi\n>\n> I don't see your point. If a helper reports success when it has failed,\n> and doesn't feed anything to fast-import, we can do nothing. That case\n> is indistinguishable from a helper which has nothing to fetch from the\n> remote, and exits. My patch doesn't help with such a broken helper, but\n> neither does yours, because they check the exact same thing.\n\nNo, my code checks that the remote-helper is *still running*, you code\ndoesn't. And after all, that is the design of transport-helper.\n\n>> > argv_array_free_detached(fastimport.argv);\n>> >\n>> > which passes both your original test and the more strict one above. Of\n>> > course adding a done-import capability would be nice to fix the protocol\n>> > deficiency, but it is more code.\n>>\n>> The done-import capability *is already there*. The remote helper fails\n>> to say \"done\", fast-import detects that there was a problem and exits\n>> with -1 (or whatever), but it doesn't matter, because we\n>> (transport-helper) are oblivious of that return status until it's too\n>> late.\n>\n> Wait, then why is this a problem at all? We check the exit code of\n> fast-import right before the code in question.\n\nIt's not, *If* the remote-helper is using \"done\", and a small one if it's not.\n\n>> Yeah, I already explored this option, and I said it was possible on\n>> pushing, but now the problem is fetching.\n>>\n>> http://article.gmane.org/gmane.comp.version-control.git/219760\n>>\n>> And to be frank, I'm tired of this. I keep repeating the same things\n>> over and over, when I ask for input on different ways to achieve this\n>> I get nothing, and when the patch is getting closer to be merged, then\n>> I receive criticism, only so I can repeat the same again.\n>\n> I don't even know what to say to this. I reviewed your original patch,\n> raised some concerns. You responded. I had not yet responded back,\n> because I have many other things to work on. In the meantime you\n> submitted another round. Now I have the opportunity to respond and did\n> so, having read both threads.\n\nI sent the first version of this patch series on October of last year.\n\n> Why is your patch to recvline so much more complicated than my\n> one-liner? Why do you go to lengths to pass out an error code from it\n> just so that we can end up dying anyway?\n\nBecause I think dying inside a generic function is bad design. I think\nthe error should be returned, and let the caller decide what to do\nwith it. In the current version of the code we seem to receive lines\nonly from the remote-helper, so I guess your patch is fine and works.\nBut I still think the current code is not good.\n\n> Why do you need a done\n> capability for push when the blank line already signals the end of the\n> status?\n\nI suppose we don't.\n\n> I did not respond to 219760 separately because those are all questions\n> that _don't matter_ if you follow the analysis in my last email that\n> lead to my one-liner. Maybe there are interesting answers to those\n> questions, and my one-liner isn't sufficient. But I'll never know\n> because instead of pointing them out, you are complaining that I didn't\n> respond fast enough.\n\nThat's not what I said. My complaint is why _now_, and not six months ago.\n\n>> If this current patch series is not good enough, I think it's best to\n>> leave things as they are for me.\n>\n> That's certainly your choice. It just seems like we can be more robust\n> while adding less code, as I tried to show.\n\nAfter reading back all the mails and checking all the different\nsituations, I think my main problem was that my test was not for the\nright thing. The test for import should be using the 'done' feature,\nand we should mostly ignore the rest.\n\nWhile my patch does improve the situation when the remote-helper is\nnot using the 'done' feature, it's probably not worth the effort, and\nwe might want to enforce the remote helpers to implement this somehow.\n\nIn the meantime, your patch should probably deal with all the real\nworld situations, so I've updated the tests and I'll resend.\n\nCheers.\n\n--\nFelipe Contreras\n"},{"id":"213564","messageId":"7vtxngyl30.fsf@alter.siamese.dyndns.org","threadId":"33413","inReplyTo":"CAMP44s1cphJ5wojt2OeZjWJVOi5AMQJR2jN_BCkQw0B0BxpUfA@mail.gmail.com","subject":"Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-08T17:43:31Z","receivedAt":"2013-04-08T17:43:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Sun, Apr 7, 2013 at 9:33 PM, Jeff King wrote:\n>> On Sun, Apr 07, 2013 at 09:03:25PM -0500, Felipe Contreras wrote:\n>\n>>> And you think that is desirable? User-friendly?\n>>\n>> No, as you probably realized if you read the rest of my email. My point\n>> in bringing it up was to try to explain exactly what is going on in each\n>> case. Your commit message provided zero explanation of what the current\n>> problem is, nor why your fix is the right thing to do.\n>\n> I have explained this many times, I wonder why when the patch is close\n> to be merged does this become important, and not before.\n\nThere are at least a few reasons I can think of off the top of my\nhead.\n\nReviewers have limited time allocated for each individual topic. To\nan original patch with say 3 problems, the review may only point out\nissues in 2 and suggest a concrete improvement for only 1 and that\nis sufficient to suggest a reroll. That is not being unhelpful by\ndeliberately withholding 1 and half reviews in the initial round.\n\nReviewers will see the same patch with fresh eyes after 6 months and\nwill notice different issues. That is not being unhelpful by\ndeliberately withholding a crucial point in the initial round of the\nreview.\n\nI would not be surprised if one ends up repeating oneself in\nmultiple review threads; the log message of a rerolled patch is a\nbetter place to avoid the need for the repetition.\n\n>>> That's a comprehensive essay, unfortunately, it's not correct. The\n>>> exit status of the remote-helper is not important, it's the one of\n>>> fast-import that we really care about.\n>>\n>> Nowhere does it say that we should not check fast-import's exit value,\n>> and indeed, the patch does not replace that check at all. It comes\n>> immediately after. It replaces the WNOHANG check of the helper's exit\n>> code (i.e., check_command) with a synchronous check.\n>\n> Yeah, and in the process it's changing the design of transport-helper,\n> where the pipes are closed only at the very end.\n\nI agree that the disconnection here closes the pipes a lot earlier\nthan we used to. But I am not sure what the practical consequences\nof doing so are. We know the fast-import process has successfully\nread everything from the helper (we called finish_command() on it).\nWe expect at this point the helper is still running or successfully\nexited (the other alternative, the helper died with non-zero status,\nis an error check_command() patch wanted to detect). But if we keep\nhelper running, who will be communicating with it via these open\npipes? The process that is calling finish_command() on fast-import\nand disconnecting from the helper won't be, as read/write to the\npipe, even if we do not disconnect from here, will result in errors\nif the helper has already exited at this point.\n\nWhat I am trying to get at is if \"changing the design\", which I\nagree is a change, is an improvement, or a backward incompatible\npossible breakage.\n"},{"id":"213575","messageId":"CAMP44s2PJGJQBM86CGsA5BOn1RLi4mqNsq+d0oyY0jA=XHQGDA@mail.gmail.com","threadId":"33413","inReplyTo":"7vtxngyl30.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-08T18:31:43Z","receivedAt":"2013-04-08T18:31:43Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Apr 8, 2013 at 12:43 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> On Sun, Apr 7, 2013 at 9:33 PM, Jeff King wrote:\n>>> On Sun, Apr 07, 2013 at 09:03:25PM -0500, Felipe Contreras wrote:\n>>\n>>>> And you think that is desirable? User-friendly?\n>>>\n>>> No, as you probably realized if you read the rest of my email. My point\n>>> in bringing it up was to try to explain exactly what is going on in each\n>>> case. Your commit message provided zero explanation of what the current\n>>> problem is, nor why your fix is the right thing to do.\n>>\n>> I have explained this many times, I wonder why when the patch is close\n>> to be merged does this become important, and not before.\n>\n> There are at least a few reasons I can think of off the top of my\n> head.\n>\n> Reviewers have limited time allocated for each individual topic. To\n> an original patch with say 3 problems, the review may only point out\n> issues in 2 and suggest a concrete improvement for only 1 and that\n> is sufficient to suggest a reroll. That is not being unhelpful by\n> deliberately withholding 1 and half reviews in the initial round.\n\nI'm not talking about the time it took to come up with the criticism\nbelow, I'm talking about the comment regarding the commit message. If\nthe commit message did indeed provide *zero* explanation, that's\ncertainly something that should be immediately visible, no? It could\nhave been mentioned six months ago.\n\n>>>> That's a comprehensive essay, unfortunately, it's not correct. The\n>>>> exit status of the remote-helper is not important, it's the one of\n>>>> fast-import that we really care about.\n>>>\n>>> Nowhere does it say that we should not check fast-import's exit value,\n>>> and indeed, the patch does not replace that check at all. It comes\n>>> immediately after. It replaces the WNOHANG check of the helper's exit\n>>> code (i.e., check_command) with a synchronous check.\n>>\n>> Yeah, and in the process it's changing the design of transport-helper,\n>> where the pipes are closed only at the very end.\n>\n> I agree that the disconnection here closes the pipes a lot earlier\n> than we used to. But I am not sure what the practical consequences\n> of doing so are. We know the fast-import process has successfully\n> read everything from the helper (we called finish_command() on it).\n> We expect at this point the helper is still running or successfully\n> exited (the other alternative, the helper died with non-zero status,\n> is an error check_command() patch wanted to detect). But if we keep\n> helper running, who will be communicating with it via these open\n> pipes? The process that is calling finish_command() on fast-import\n> and disconnecting from the helper won't be, as read/write to the\n> pipe, even if we do not disconnect from here, will result in errors\n> if the helper has already exited at this point.\n\nNobody will send any further input, but in theory we could redirect\nthe pipe and send more commands. That's how it was designed.\n\nAnd in fact, I'm thinking doing exactly that, so we can send another\ncommand to fetch the foreign commit ids and append notes with them.\n\n> What I am trying to get at is if \"changing the design\", which I\n> agree is a change, is an improvement, or a backward incompatible\n> possible breakage.\n\nIt's an improvement I guess, but it's \"changing the design\" only in\none part of the code, while the rest follows a different design.\n\n--\nFelipe Contreras\n"},{"id":"213581","messageId":"7vwqscx3l5.fsf@alter.siamese.dyndns.org","threadId":"33413","inReplyTo":"CAMP44s2PJGJQBM86CGsA5BOn1RLi4mqNsq+d0oyY0jA=XHQGDA@mail.gmail.com","subject":"Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-08T18:46:46Z","receivedAt":"2013-04-08T18:46:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> I'm not talking about the time it took to come up with the criticism\n> below, I'm talking about the comment regarding the commit message. If\n> the commit message did indeed provide *zero* explanation, that's\n> certainly something that should be immediately visible, no? It could\n> have been mentioned six months ago.\n\nGiven that all of your log messages tend to be missing or terser\nthan necessary, I wouldn't be surprised Peff stopped bothering about\nit.\n\nBut now we know this needs to be better explained, and your v4 has\nbetter explanation that will help you avoid having to repeat\nyourself in the discussion, so let's not dwell on it too much.\n\n>> ... But if we keep\n>> helper running, who will be communicating with it via these open\n>> pipes? The process that is calling finish_command() on fast-import\n>> and disconnecting from the helper won't be, as read/write to the\n>> pipe, even if we do not disconnect from here, will result in errors\n>> if the helper has already exited at this point.\n>\n> Nobody will send any further input, but in theory we could redirect\n> the pipe and send more commands. That's how it was designed.\n\nWho does the redirection to whom? How would the process tree and\npiping constructed around the current system?\n\nI am not trying to say it is just theoretical mental exercise (which\nI have seen you do not do at all on this list). I am trying to find\nout what the practical use case is that you have in mind, because\ndisconnecting will prove to be not an improvement but a regression\nfor that use case. But you do not have to answer this question\ndirectly, because...\n\n> And in fact, I'm thinking doing exactly that, so we can send another\n> command to fetch the foreign commit ids and append notes with them.\n\n...we will see the answer in that change anyway.\n"},{"id":"213582","messageId":"20130408184933.GA7337@sigill.intra.peff.net","threadId":"33413","inReplyTo":"CAMP44s2PJGJQBM86CGsA5BOn1RLi4mqNsq+d0oyY0jA=XHQGDA@mail.gmail.com","subject":"Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-08T18:49:33Z","receivedAt":"2013-04-08T18:49:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 08, 2013 at 01:31:43PM -0500, Felipe Contreras wrote:\n\n> > Reviewers have limited time allocated for each individual topic. To\n> > an original patch with say 3 problems, the review may only point out\n> > issues in 2 and suggest a concrete improvement for only 1 and that\n> > is sufficient to suggest a reroll. That is not being unhelpful by\n> > deliberately withholding 1 and half reviews in the initial round.\n> \n> I'm not talking about the time it took to come up with the criticism\n> below, I'm talking about the comment regarding the commit message. If\n> the commit message did indeed provide *zero* explanation, that's\n> certainly something that should be immediately visible, no? It could\n> have been mentioned six months ago.\n\nI do not recall this series at all from six months ago. I reviewed a\nseries (with no version markers, nor any mention of the previous round)\nposted on April 1st, and assumed it was a new series. I raised the same\nissues there that I did in v3, though in less detail (because I had not\nyet looked into it as thoroughly).\n\nI searched the archive after reading this mail and found the original\nseries. Yes, it existed. I didn't review it. I guess I had something\nmore important to do that day.\n\nBut is all of this even important? If there are technical issues with\nthis series, does it matter when we find out about them? They still\ndeserve to be fixed, no? Yes, it is nice if things get reviewed\npromptly, but the limited bandwidth and attention of reviewers is a fact\nof life.\n\n-Peff\n"},{"id":"213588","messageId":"CAMP44s0DjJL44uavM44sxgSyrgbTLpUarEPB6=-imvw1rewOOA@mail.gmail.com","threadId":"33413","inReplyTo":"7vwqscx3l5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-08T19:08:02Z","receivedAt":"2013-04-08T19:08:02Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Mon, Apr 8, 2013 at 1:46 PM, Junio C Hamano wrote:\n\n>>> ... But if we keep\n>>> helper running, who will be communicating with it via these open\n>>> pipes? The process that is calling finish_command() on fast-import\n>>> and disconnecting from the helper won't be, as read/write to the\n>>> pipe, even if we do not disconnect from here, will result in errors\n>>> if the helper has already exited at this point.\n>>\n>> Nobody will send any further input, but in theory we could redirect\n>> the pipe and send more commands. That's how it was designed.\n>\n> Who does the redirection to whom?\n\nThe one that is doing all the redirections, transport-helper.\n\n> How would the process tree and\n> piping constructed around the current system?\n\nI cannot parse that correctly, but transport-helper is already\nreceiving the output from the remote-helper.\n\n> I am not trying to say it is just theoretical mental exercise (which\n> I have seen you do not do at all on this list). I am trying to find\n> out what the practical use case is that you have in mind, because\n> disconnecting will prove to be not an improvement but a regression\n> for that use case. But you do not have to answer this question\n> directly, because...\n\nAs the gitifyhg developers effusively pointed out, it's useful to see\nwhich mercurial revision corresponds to certain git revision, and this\nis best achieved through notes. Implementing this in the remote-helper\ndoesn't make much sense (I won't go into details).\n\n>> And in fact, I'm thinking doing exactly that, so we can send another\n>> command to fetch the foreign commit ids and append notes with them.\n>\n> ...we will see the answer in that change anyway.\n\nThat doesn't change the fact that transport-helper would end up with\nmixed and matched design. Maybe the use-case above wouldn't be\nprevented by this change, but I think further changes would need to be\ndone to the file to not end up in this weird state.\n\nCheers.\n\n--\nFelipe Contreras\n"},{"id":"213590","messageId":"20130408191549.GB7337@sigill.intra.peff.net","threadId":"33413","inReplyTo":"CAMP44s1cphJ5wojt2OeZjWJVOi5AMQJR2jN_BCkQw0B0BxpUfA@mail.gmail.com","subject":"Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-08T19:15:49Z","receivedAt":"2013-04-08T19:15:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 08, 2013 at 09:38:28AM -0500, Felipe Contreras wrote:\n\n> > Nowhere does it say that we should not check fast-import's exit value,\n> > and indeed, the patch does not replace that check at all. It comes\n> > immediately after. It replaces the WNOHANG check of the helper's exit\n> > code (i.e., check_command) with a synchronous check.\n> \n> Yeah, and in the process it's changing the design of transport-helper,\n> where the pipes are closed only at the very end.\n\nYes. And I tried to explain why this is acceptable in the \"comprehensive\nessay\" of a comment. If you think there are reasons not to do so, please\nelaborate. But talking about fast-import's exit value is irrelevant.\n\n> > I don't see your point. If a helper reports success when it has failed,\n> > and doesn't feed anything to fast-import, we can do nothing. That case\n> > is indistinguishable from a helper which has nothing to fetch from the\n> > remote, and exits. My patch doesn't help with such a broken helper, but\n> > neither does yours, because they check the exact same thing.\n> \n> No, my code checks that the remote-helper is *still running*, you code\n> doesn't. And after all, that is the design of transport-helper.\n\nI see. That would have been a very helpful thing to put in the commit\nmessage, no?\n\nBut the issue still remains: a \"still running\" check is subject to race\nconditions. A disconnect solution should fail not only on a failed exit\ncode, but also if we are unable to send the disconnect command (i.e., it\nshould be checking EPIPE rather than ignoring it). So my solution was\nnot sufficient, either.\n\nBut I think it is moot, as I agree with your statements below and the\nfollow-up patch.\n\n> >> The done-import capability *is already there*. The remote helper fails\n> >> to say \"done\", fast-import detects that there was a problem and exits\n> >> with -1 (or whatever), but it doesn't matter, because we\n> >> (transport-helper) are oblivious of that return status until it's too\n> >> late.\n> >\n> > Wait, then why is this a problem at all? We check the exit code of\n> > fast-import right before the code in question.\n> \n> It's not, *If* the remote-helper is using \"done\", and a small one if it's not.\n\nSomething like this might have been helpful in the commit message to\nmake it more clear what the point of the patch is:\n\n If the remote-helper is using the \"done\" feature in the export stream,\n then it will signal to fast-import that it correctly output all of the\n data to the stream. We can trust that the exit code of fast-import\n will report such a problem to it, and do not have to worry.\n\n But for helpers which do not use the \"done\" feature and exit early,\n fast-import has no idea whether all of the data was written or not.\n The helper does not report any status to us, so we are left to guess\n at whether it worked successfully. We can make such a guess by seeing\n whether the process is still alive.\n\n This does not cover all cases, as it is possible for us to see the\n process still present just before it exits. However, it covers the\n common case that the helper dies with an error, closing the pipe to\n fast-import at the same time. Since we reap fast-import first, by the\n time we check whether the helper is still alive, it will have exited,\n too.\n\n That may miss some cases, but it is the best we can do for such a\n helper, given the current protocol. A remote helper which wants to be\n robust should use the \"done\" feature.\n\nMaybe you thought about all of those issues. Maybe you even communicated\nthem somewhere on the list. But I did not see them in the commit message\nnor in a comment, which is the main item I look at while reviewing. If\nyou did not realize it until after the discussion, that is fine, too.\nThat is why we have the discussions. Now we can perhaps improve the\npatch (either by explaining there what is happening, or deciding that\nthis case is not worth caring about).\n\n> > Why is your patch to recvline so much more complicated than my\n> > one-liner? Why do you go to lengths to pass out an error code from it\n> > just so that we can end up dying anyway?\n> \n> Because I think dying inside a generic function is bad design. I think\n> the error should be returned, and let the caller decide what to do\n> with it.\n\nWe already die inside the function; my patch only added a message. If you\nwant to change the dying, fine, but that cleanup is a separate issue.\n\n> In the current version of the code we seem to receive lines\n> only from the remote-helper, so I guess your patch is fine and works.\n> But I still think the current code is not good.\n\nIt's a static inside transport-helper.c. So I think it is OK for it to\nassume that the transport-helper code will be the only caller. In such a\nsituation, it _may_ be worth changing later if it becomes a more generic\nand widely accessible function, but I do not think that is the case\nhere. The function does nothing except report remote-helper debugging\ninformation and ask strbuf_getline to do the heavy lifting.\n\n> > I did not respond to 219760 separately because those are all questions\n> > that _don't matter_ if you follow the analysis in my last email that\n> > lead to my one-liner. Maybe there are interesting answers to those\n> > questions, and my one-liner isn't sufficient. But I'll never know\n> > because instead of pointing them out, you are complaining that I didn't\n> > respond fast enough.\n> \n> That's not what I said. My complaint is why _now_, and not six months ago.\n\nBecause I did not read your series 6 months ago, nor did you make any\nmention of it in your more recent cover letter.\n\n> After reading back all the mails and checking all the different\n> situations, I think my main problem was that my test was not for the\n> right thing. The test for import should be using the 'done' feature,\n> and we should mostly ignore the rest.\n> \n> While my patch does improve the situation when the remote-helper is\n> not using the 'done' feature, it's probably not worth the effort, and\n> we might want to enforce the remote helpers to implement this somehow.\n\nYes, that makes sense to me, too. I am inclined to say that failing to\nuse \"done\" is a quality-of-implementation issue for the remote-helper,\nand the right fix is for them to use the proper signal.\n\n-Peff\n"},{"id":"213607","messageId":"7vy5csvih8.fsf@alter.siamese.dyndns.org","threadId":"33413","inReplyTo":"CAMP44s0DjJL44uavM44sxgSyrgbTLpUarEPB6=-imvw1rewOOA@mail.gmail.com","subject":"Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-08T21:08:03Z","receivedAt":"2013-04-08T21:08:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Mon, Apr 8, 2013 at 1:46 PM, Junio C Hamano wrote:\n>\n>>>> ... But if we keep\n>>>> helper running, who will be communicating with it via these open\n>>>> pipes? The process that is calling finish_command() on fast-import\n>>>> and disconnecting from the helper won't be, as read/write to the\n>>>> pipe, even if we do not disconnect from here, will result in errors\n>>>> if the helper has already exited at this point.\n>>>\n>>> Nobody will send any further input, but in theory we could redirect\n>>> the pipe and send more commands. That's how it was designed.\n>>\n>> Who does the redirection to whom?\n>\n> The one that is doing all the redirections, transport-helper.\n>\n>> How would the process tree and\n>> piping constructed around the current system?\n>\n> I cannot parse that correctly,\n\nSorry, s/the current system/& look like/;\n\n> but transport-helper is already\n> receiving the output from the remote-helper.\n\nOK, so you are envisioning that transport-helper would read from the\nhelper after importer is done? If so, perhaps it is a prudent\nsolution to disconnect in this version (to fix), and then in a\nseparate patch that adds such an extension (I imagine it would\ninvolve that the helper advertising a capability or being invoked\nwith an option to let transport-helper somehow know that it should\ncontinue the conversation once fast-import is done) to disable the\ndisconnect here when that extension is in use?\n"},{"id":"213609","messageId":"20130408211154.GA10310@sigill.intra.peff.net","threadId":"33413","inReplyTo":"7vy5csvih8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-08T21:11:54Z","receivedAt":"2013-04-08T21:11:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 08, 2013 at 02:08:03PM -0700, Junio C Hamano wrote:\n\n> OK, so you are envisioning that transport-helper would read from the\n> helper after importer is done? If so, perhaps it is a prudent\n> solution to disconnect in this version (to fix), and then in a\n> separate patch that adds such an extension (I imagine it would\n> involve that the helper advertising a capability or being invoked\n> with an option to let transport-helper somehow know that it should\n> continue the conversation once fast-import is done) to disable the\n> disconnect here when that extension is in use?\n\nAt this point, I am of the opinion that it's OK to just do nothing;\nmodern helpers should be using the \"done\" flag, and if they aren't, then\nthat is the right place for the fix. Then we don't have to worry about\nany side effects of disconnecting, or adding a new capability flag.\n\n-Peff\n"},{"id":"213610","messageId":"7vtxngvhv0.fsf@alter.siamese.dyndns.org","threadId":"33413","inReplyTo":"20130408211154.GA10310@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-08T21:21:23Z","receivedAt":"2013-04-08T21:21:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Apr 08, 2013 at 02:08:03PM -0700, Junio C Hamano wrote:\n>\n>> OK, so you are envisioning that transport-helper would read from the\n>> helper after importer is done? If so, perhaps it is a prudent\n>> solution to disconnect in this version (to fix), and then in a\n>> separate patch that adds such an extension (I imagine it would\n>> involve that the helper advertising a capability or being invoked\n>> with an option to let transport-helper somehow know that it should\n>> continue the conversation once fast-import is done) to disable the\n>> disconnect here when that extension is in use?\n>\n> At this point, I am of the opinion that it's OK to just do nothing;\n> modern helpers should be using the \"done\" flag, and if they aren't, then\n> that is the right place for the fix. Then we don't have to worry about\n> any side effects of disconnecting, or adding a new capability flag.\n\nSounds sane. Thanks for unconfusing me ;-)\n"}]}