* [PATCH v2 0/4] run-command: new check_command helper
@ 2013-04-02 10:31 Felipe Contreras
2013-04-02 10:31 ` [PATCH v2 1/4] run-command: add " Felipe Contreras
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Felipe Contreras @ 2013-04-02 10:31 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Sixt, Aaron Schrab,
Clemens Buchacher, David Michael Barr, Florian Achleitner,
Felipe Contreras
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/4] run-command: add new check_command helper
2013-04-02 10:31 [PATCH v2 0/4] run-command: new check_command helper Felipe Contreras
@ 2013-04-02 10:31 ` Felipe Contreras
2013-04-02 19:16 ` Johannes Sixt
2013-04-02 10:31 ` [PATCH v2 2/4] transport-helper: check if remote helper is alive Felipe Contreras
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Felipe Contreras @ 2013-04-02 10:31 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Sixt, Aaron Schrab,
Clemens Buchacher, David Michael Barr, Florian Achleitner,
Felipe Contreras
And persistent_waitpid() to recover the information from the last run.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/4] transport-helper: check if remote helper is alive
2013-04-02 10:31 [PATCH v2 0/4] run-command: new check_command helper Felipe Contreras
2013-04-02 10:31 ` [PATCH v2 1/4] run-command: add " Felipe Contreras
@ 2013-04-02 10:31 ` Felipe Contreras
2013-04-03 0:35 ` Junio C Hamano
2013-04-02 10:31 ` [PATCH v2 3/4] tmp: remote-helper: add timers to catch errors Felipe Contreras
2013-04-02 10:31 ` [PATCH v2 4/4] tmp: run-command: code to exercise check_command Felipe Contreras
3 siblings, 1 reply; 7+ messages in thread
From: Felipe Contreras @ 2013-04-02 10:31 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Sixt, Aaron Schrab,
Clemens Buchacher, David Michael Barr, Florian Achleitner,
Felipe Contreras
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 3/4] tmp: remote-helper: add timers to catch errors
2013-04-02 10:31 [PATCH v2 0/4] run-command: new check_command helper Felipe Contreras
2013-04-02 10:31 ` [PATCH v2 1/4] run-command: add " Felipe Contreras
2013-04-02 10:31 ` [PATCH v2 2/4] transport-helper: check if remote helper is alive Felipe Contreras
@ 2013-04-02 10:31 ` Felipe Contreras
2013-04-02 10:31 ` [PATCH v2 4/4] tmp: run-command: code to exercise check_command Felipe Contreras
3 siblings, 0 replies; 7+ messages in thread
From: Felipe Contreras @ 2013-04-02 10:31 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Sixt, Aaron Schrab,
Clemens Buchacher, David Michael Barr, Florian Achleitner,
Felipe Contreras
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 <felipe.contreras@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 4/4] tmp: run-command: code to exercise check_command
2013-04-02 10:31 [PATCH v2 0/4] run-command: new check_command helper Felipe Contreras
` (2 preceding siblings ...)
2013-04-02 10:31 ` [PATCH v2 3/4] tmp: remote-helper: add timers to catch errors Felipe Contreras
@ 2013-04-02 10:31 ` Felipe Contreras
3 siblings, 0 replies; 7+ messages in thread
From: Felipe Contreras @ 2013-04-02 10:31 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Johannes Sixt, Aaron Schrab,
Clemens Buchacher, David Michael Barr, Florian Achleitner,
Felipe Contreras
Do not merge!
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/4] run-command: add new check_command helper
2013-04-02 10:31 ` [PATCH v2 1/4] run-command: add " Felipe Contreras
@ 2013-04-02 19:16 ` Johannes Sixt
0 siblings, 0 replies; 7+ messages in thread
From: Johannes Sixt @ 2013-04-02 19:16 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Junio C Hamano, Jeff King, Aaron Schrab, Clemens Buchacher,
David Michael Barr, Florian Achleitner
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);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/4] transport-helper: check if remote helper is alive
2013-04-02 10:31 ` [PATCH v2 2/4] transport-helper: check if remote helper is alive Felipe Contreras
@ 2013-04-03 0:35 ` Junio C Hamano
0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2013-04-03 0:35 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Jeff King, Johannes Sixt, Aaron Schrab, Clemens Buchacher,
David Michael Barr, Florian Achleitner
Felipe Contreras <felipe.contreras@gmail.com> 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;
> }
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-04-03 0:36 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-04-02 10:31 [PATCH v2 0/4] run-command: new check_command helper Felipe Contreras
2013-04-02 10:31 ` [PATCH v2 1/4] run-command: add " Felipe Contreras
2013-04-02 19:16 ` Johannes Sixt
2013-04-02 10:31 ` [PATCH v2 2/4] transport-helper: check if remote helper is alive Felipe Contreras
2013-04-03 0:35 ` Junio C Hamano
2013-04-02 10:31 ` [PATCH v2 3/4] tmp: remote-helper: add timers to catch errors Felipe Contreras
2013-04-02 10:31 ` [PATCH v2 4/4] tmp: run-command: code to exercise check_command Felipe Contreras
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.