[PATCH v2 0/4] run-command: new check_command helper

[PATCH v2 0/4] run-command: new check_command helper

[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

[PATCH v2 1/4] run-command: add new check_command helper

[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

Re: [PATCH v2 1/4] run-command: add new check_command helper

[Johannes Sixt]

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);
 

[PATCH v2 2/4] transport-helper: check if remote helper is alive

[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

Re: [PATCH v2 2/4] transport-helper: check if remote helper is alive

[Junio C Hamano]

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;
>  }

[PATCH v2 3/4] tmp: remote-helper: add timers to catch errors

[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

[PATCH v2 4/4] tmp: run-command: code to exercise check_command

[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