# [PATCH v2 0/4] run-command: new check_command helper 7 messages from 2013-04-02 to 2013-04-03. Participants: Felipe Contreras, Johannes Sixt, Junio C Hamano. Thread: https://gitlist.dev/t/33349 ## Felipe Contreras, 2013-04-02 10:31 Subject: [PATCH v2 0/4] run-command: new check_command helper Message-ID: <1364898709-21583-1-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1364898709-21583-1-git-send-email-felipe.contreras%40gmail.com ``` Hi, Here's the second version of the patches incorporating input from Junio and Peff. The first patch does all the work, the second patch uses it; basically, this is needed so the transport-helper code is able to check if the remote-helper child is stilll running. Without this support, the status of the remote-helper files and configuration can end up very badly when errors occur, to the point where the user is unable to use it any more. The rest of the patches are for testing purposes only. I ran all the tests with these, and I didn't see any problems. Felipe Contreras (4): run-command: add new check_command helper transport-helper: check if remote helper is alive tmp: remote-helper: add timers to catch errors tmp: run-command: code to exercise check_command git-remote-testgit | 12 ++++++++++ run-command.c | 58 ++++++++++++++++++++++++++++++++++++++++++----- run-command.h | 5 ++++ t/t5801-remote-helpers.sh | 19 ++++++++++++++++ transport-helper.c | 11 +++++++++ 5 files changed, 99 insertions(+), 6 deletions(-) -- 1.8.2 ``` ## Felipe Contreras, 2013-04-02 10:31 Subject: [PATCH v2 1/4] run-command: add new check_command helper Message-ID: <1364898709-21583-2-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1364898709-21583-2-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1364898709-21583-1-git-send-email-felipe.contreras@gmail.com> ``` And persistent_waitpid() to recover the information from the last run. Signed-off-by: Felipe Contreras --- run-command.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++------ run-command.h | 5 +++++ 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/run-command.c b/run-command.c index 07e27ff..b900c6e 100644 --- a/run-command.c +++ b/run-command.c @@ -226,14 +226,27 @@ static inline void set_cloexec(int fd) fcntl(fd, F_SETFD, flags | FD_CLOEXEC); } -static int wait_or_whine(pid_t pid, const char *argv0) +static pid_t persistent_waitpid(struct child_process *cmd, pid_t pid, int *status) +{ + pid_t waiting; + + if (cmd->last_status.valid) { + *status = cmd->last_status.status; + return pid; + } + + while ((waiting = waitpid(pid, status, 0)) < 0 && errno == EINTR) + ; /* nothing */ + return waiting; +} + +static int wait_or_whine(struct child_process *cmd, pid_t pid, const char *argv0) { int status, code = -1; pid_t waiting; int failed_errno = 0; - while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR) - ; /* nothing */ + waiting = persistent_waitpid(cmd, pid, &status); if (waiting < 0) { failed_errno = errno; @@ -276,6 +289,8 @@ int start_command(struct child_process *cmd) int failed_errno = failed_errno; char *str; + cmd->last_status.valid = 0; + /* * In case of errors we must keep the promise to close FDs * that have been passed in via ->in and ->out. @@ -437,7 +452,7 @@ fail_pipe: * At this point we know that fork() succeeded, but execvp() * failed. Errors have been reported to our stderr. */ - wait_or_whine(cmd->pid, cmd->argv[0]); + wait_or_whine(cmd, cmd->pid, cmd->argv[0]); failed_errno = errno; cmd->pid = -1; } @@ -542,7 +557,7 @@ fail_pipe: int finish_command(struct child_process *cmd) { - return wait_or_whine(cmd->pid, cmd->argv[0]); + return wait_or_whine(cmd, cmd->pid, cmd->argv[0]); } int run_command(struct child_process *cmd) @@ -553,6 +568,32 @@ int run_command(struct child_process *cmd) return finish_command(cmd); } +int check_command(struct child_process *cmd) +{ + int status; + pid_t waiting; + + if (cmd->last_status.valid) + return 0; + + while ((waiting = waitpid(cmd->pid, &status, WNOHANG)) < 0 && errno == EINTR) + ; /* nothing */ + + if (!waiting) + return 1; + + if (waiting == cmd->pid) { + cmd->last_status.valid = 1; + cmd->last_status.status = status; + return 0; + } + + if (waiting > 0) + die("BUG: waitpid reported a random pid?"); + + return 0; +} + static void prepare_run_command_v_opt(struct child_process *cmd, const char **argv, int opt) @@ -729,7 +770,7 @@ error: int finish_async(struct async *async) { #ifdef NO_PTHREADS - return wait_or_whine(async->pid, "child process"); + return wait_or_whine(cmd, async->pid, "child process"); #else void *ret = (void *)(intptr_t)(-1); diff --git a/run-command.h b/run-command.h index 221ce33..74a733d 100644 --- a/run-command.h +++ b/run-command.h @@ -39,11 +39,16 @@ struct child_process { unsigned stdout_to_stderr:1; unsigned use_shell:1; unsigned clean_on_exit:1; + struct last_status { + unsigned valid:1; + int status; + } last_status; }; int start_command(struct child_process *); int finish_command(struct child_process *); int run_command(struct child_process *); +int check_command(struct child_process *); extern char *find_hook(const char *name); extern int run_hook(const char *index_file, const char *name, ...); -- 1.8.2 ``` ## Felipe Contreras, 2013-04-02 10:31 Subject: [PATCH v2 2/4] transport-helper: check if remote helper is alive Message-ID: <1364898709-21583-3-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1364898709-21583-3-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1364898709-21583-1-git-send-email-felipe.contreras@gmail.com> ``` Otherwise transport-helper will continue checking for refs and other things what will confuse the user more. --- git-remote-testgit | 11 +++++++++++ t/t5801-remote-helpers.sh | 19 +++++++++++++++++++ transport-helper.c | 8 ++++++++ 3 files changed, 38 insertions(+) diff --git a/git-remote-testgit b/git-remote-testgit index b395c8d..ca0cf09 100755 --- a/git-remote-testgit +++ b/git-remote-testgit @@ -61,12 +61,23 @@ do echo "feature import-marks=$gitmarks" echo "feature export-marks=$gitmarks" fi + + if test -n "$GIT_REMOTE_TESTGIT_FAILURE" + then + exit -1 + fi + echo "feature done" git fast-export "${testgitmarks_args[@]}" $refs | sed -e "s#refs/heads/#${prefix}/heads/#g" echo "done" ;; export) + if test -n "$GIT_REMOTE_TESTGIT_FAILURE" + then + exit -1 + fi + before=$(git for-each-ref --format='%(refname) %(objectname)') git fast-import "${testgitmarks_args[@]}" --quiet diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index f387027..efe67e2 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -166,4 +166,23 @@ test_expect_success 'push ref with existing object' ' compare_refs local dup server dup ' +test_expect_success 'proper failure checks for fetching' ' + (GIT_REMOTE_TESTGIT_FAILURE=1 && + export GIT_REMOTE_TESTGIT_FAILURE && + cd local && + test_must_fail git fetch 2> error && + grep "Error while running helper" error + ) +' + +# We sleep to give fast-export a chance to catch the SIGPIPE +test_expect_failure 'proper failure checks for pushing' ' + (GIT_REMOTE_TESTGIT_FAILURE=1 && + export GIT_REMOTE_TESTGIT_FAILURE && + cd local && + test_must_fail git push --all 2> error && + grep "Error while running helper" error + ) +' + test_done diff --git a/transport-helper.c b/transport-helper.c index cb3ef7d..dfdfa7a 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -460,6 +460,10 @@ static int fetch_with_import(struct transport *transport, if (finish_command(&fastimport)) die("Error while running fast-import"); + + if (!check_command(data->helper)) + die("Error while running helper"); + argv_array_free_detached(fastimport.argv); /* @@ -818,6 +822,10 @@ static int push_refs_with_export(struct transport *transport, if (finish_command(&exporter)) die("Error while running fast-export"); + + if (!check_command(data->helper)) + die("Error while running helper"); + push_update_refs_status(data, remote_refs); return 0; } -- 1.8.2 ``` ## Felipe Contreras, 2013-04-02 10:31 Subject: [PATCH v2 3/4] tmp: remote-helper: add timers to catch errors Message-ID: <1364898709-21583-4-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1364898709-21583-4-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1364898709-21583-1-git-send-email-felipe.contreras@gmail.com> ``` This way the test reliably succeeds (in catching the failure). Not sure what's the proper way to do this, but here it is for the record. Signed-off-by: Felipe Contreras --- git-remote-testgit | 1 + t/t5801-remote-helpers.sh | 2 +- transport-helper.c | 3 +++ 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/git-remote-testgit b/git-remote-testgit index ca0cf09..6ae1b7f 100755 --- a/git-remote-testgit +++ b/git-remote-testgit @@ -75,6 +75,7 @@ do export) if test -n "$GIT_REMOTE_TESTGIT_FAILURE" then + sleep 1 # don't let fast-export get SIGPIPE exit -1 fi diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index efe67e2..d6702b4 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -176,7 +176,7 @@ test_expect_success 'proper failure checks for fetching' ' ' # We sleep to give fast-export a chance to catch the SIGPIPE -test_expect_failure 'proper failure checks for pushing' ' +test_expect_success 'proper failure checks for pushing' ' (GIT_REMOTE_TESTGIT_FAILURE=1 && export GIT_REMOTE_TESTGIT_FAILURE && cd local && diff --git a/transport-helper.c b/transport-helper.c index dfdfa7a..f0d28aa 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -823,6 +823,9 @@ static int push_refs_with_export(struct transport *transport, if (finish_command(&exporter)) die("Error while running fast-export"); + if (getenv("GIT_REMOTE_TESTGIT_FAILURE")) + sleep(2); + if (!check_command(data->helper)) die("Error while running helper"); -- 1.8.2 ``` ## Felipe Contreras, 2013-04-02 10:31 Subject: [PATCH v2 4/4] tmp: run-command: code to exercise check_command Message-ID: <1364898709-21583-5-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1364898709-21583-5-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1364898709-21583-1-git-send-email-felipe.contreras@gmail.com> ``` Do not merge! Signed-off-by: Felipe Contreras --- run-command.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/run-command.c b/run-command.c index b900c6e..2bb3dcf 100644 --- a/run-command.c +++ b/run-command.c @@ -246,6 +246,11 @@ static int wait_or_whine(struct child_process *cmd, pid_t pid, const char *argv0 pid_t waiting; int failed_errno = 0; + do { + if (!check_command(cmd)) + break; + } while(1); + waiting = persistent_waitpid(cmd, pid, &status); if (waiting < 0) { -- 1.8.2 ``` ## Johannes Sixt, 2013-04-02 19:16 Subject: Re: [PATCH v2 1/4] run-command: add new check_command helper Message-ID: <515B2EAB.10402@kdbg.org> URL: https://gitlist.dev/e/515B2EAB.10402%40kdbg.org In-Reply-To: <1364898709-21583-2-git-send-email-felipe.contreras@gmail.com> ``` Am 02.04.2013 12:31, schrieb Felipe Contreras: > And persistent_waitpid() to recover the information from the last run. I'm not a fan of this new API, because it looks like a workaround for a problem that should have been solved in a cleaner way. But if we can't avoid it, please also add a paragraph to Documentation/technical/api-run-command.txt > +int check_command(struct child_process *cmd) > +{ > + int status; > + pid_t waiting; > + > + if (cmd->last_status.valid) > + return 0; > + > + while ((waiting = waitpid(cmd->pid, &status, WNOHANG)) < 0 && errno == EINTR) > + ; /* nothing */ > + > + if (!waiting) > + return 1; > + > + if (waiting == cmd->pid) { > + cmd->last_status.valid = 1; > + cmd->last_status.status = status; > + return 0; > + } > + > + if (waiting > 0) > + die("BUG: waitpid reported a random pid?"); > + > + return 0; > +} > + > static void prepare_run_command_v_opt(struct child_process *cmd, > const char **argv, > int opt) > @@ -729,7 +770,7 @@ error: > int finish_async(struct async *async) > { > #ifdef NO_PTHREADS > - return wait_or_whine(async->pid, "child process"); > + return wait_or_whine(cmd, async->pid, "child process"); This breaks the NO_PTHREADS build because cmd is undeclared. Perhaps this on top: diff --git a/run-command.c b/run-command.c index a9fa779..a02ef62 100644 --- a/run-command.c +++ b/run-command.c @@ -230,7 +230,7 @@ static pid_t persistent_waitpid(struct child_process *cmd, pid_t pid, int *statu { pid_t waiting; - if (cmd->last_status.valid) { + if (cmd && cmd->last_status.valid) { *status = cmd->last_status.status; return pid; } @@ -771,7 +771,7 @@ int start_async(struct async *async) int finish_async(struct async *async) { #ifdef NO_PTHREADS - return wait_or_whine(cmd, async->pid, "child process"); + return wait_or_whine(NULL, async->pid, "child process"); #else void *ret = (void *)(intptr_t)(-1); ``` ## Junio C Hamano, 2013-04-03 00:35 Subject: Re: [PATCH v2 2/4] transport-helper: check if remote helper is alive Message-ID: <7v1uas8n88.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1uas8n88.fsf%40alter.siamese.dyndns.org In-Reply-To: <1364898709-21583-3-git-send-email-felipe.contreras@gmail.com> ``` Felipe Contreras writes: > Otherwise transport-helper will continue checking for refs and other > things what will confuse the user more. > --- Sign-off? > git-remote-testgit | 11 +++++++++++ > t/t5801-remote-helpers.sh | 19 +++++++++++++++++++ > transport-helper.c | 8 ++++++++ > 3 files changed, 38 insertions(+) > > diff --git a/git-remote-testgit b/git-remote-testgit > index b395c8d..ca0cf09 100755 > --- a/git-remote-testgit > +++ b/git-remote-testgit > @@ -61,12 +61,23 @@ do > echo "feature import-marks=$gitmarks" > echo "feature export-marks=$gitmarks" > fi > + > + if test -n "$GIT_REMOTE_TESTGIT_FAILURE" > + then > + exit -1 That looks somewhat funny. Why not exit 1 or exit 127? > + fi > + > echo "feature done" > git fast-export "${testgitmarks_args[@]}" $refs | > sed -e "s#refs/heads/#${prefix}/heads/#g" > echo "done" > ;; > export) > + if test -n "$GIT_REMOTE_TESTGIT_FAILURE" > + then > + exit -1 Ditto. > + fi > + > before=$(git for-each-ref --format='%(refname) %(objectname)') > > git fast-import "${testgitmarks_args[@]}" --quiet > diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh > index f387027..efe67e2 100755 > --- a/t/t5801-remote-helpers.sh > +++ b/t/t5801-remote-helpers.sh > @@ -166,4 +166,23 @@ test_expect_success 'push ref with existing object' ' > compare_refs local dup server dup > ' > > +test_expect_success 'proper failure checks for fetching' ' > + (GIT_REMOTE_TESTGIT_FAILURE=1 && > + export GIT_REMOTE_TESTGIT_FAILURE && > + cd local && > + test_must_fail git fetch 2> error && > + grep "Error while running helper" error > + ) > +' > + > +# We sleep to give fast-export a chance to catch the SIGPIPE > +test_expect_failure 'proper failure checks for pushing' ' > + (GIT_REMOTE_TESTGIT_FAILURE=1 && > + export GIT_REMOTE_TESTGIT_FAILURE && > + cd local && > + test_must_fail git push --all 2> error && > + grep "Error while running helper" error > + ) > +' > + > test_done > diff --git a/transport-helper.c b/transport-helper.c > index cb3ef7d..dfdfa7a 100644 > --- a/transport-helper.c > +++ b/transport-helper.c > @@ -460,6 +460,10 @@ static int fetch_with_import(struct transport *transport, > > if (finish_command(&fastimport)) > die("Error while running fast-import"); > + > + if (!check_command(data->helper)) > + die("Error while running helper"); > + > argv_array_free_detached(fastimport.argv); > > /* > @@ -818,6 +822,10 @@ static int push_refs_with_export(struct transport *transport, > > if (finish_command(&exporter)) > die("Error while running fast-export"); > + > + if (!check_command(data->helper)) > + die("Error while running helper"); > + > push_update_refs_status(data, remote_refs); > return 0; > } ```