[PATCH v3 0/2] run-command: new check_command helper

[PATCH v3 0/2] run-command: new check_command helper

[Felipe Contreras]

Hi,

This is a reroll to fix the build with NO_PTHREADS, and also a bit of cleanup.

The interdiff:

--- b/run-command.c
+++ a/run-command.c
@@ -226,27 +226,20 @@ static inline void set_cloexec(int fd)
 		fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
 }
 
-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;
 
-	waiting = persistent_waitpid(cmd, pid, &status);
+	/* First try the last status from check_command() */
+	if (cmd && cmd->last_status.valid) {
+		status = cmd->last_status.status;
+		waiting = pid;
+	} else {
+		while ((waiting = waitpid(pid, status, 0)) < 0 && errno == EINTR)
+			;	/* nothing */
+	}
 
 	if (waiting < 0) {
 		failed_errno = errno;
@@ -770,7 +763,7 @@ error:
 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);
 
Felipe Contreras (2):
  run-command: add new check_command helper
  transport-helper: check if remote helper is alive

 git-remote-testgit        | 11 +++++++++++
 run-command.c             | 46 ++++++++++++++++++++++++++++++++++++++++------
 run-command.h             |  5 +++++
 t/t5801-remote-helpers.sh | 19 +++++++++++++++++++
 transport-helper.c        |  8 ++++++++
 5 files changed, 83 insertions(+), 6 deletions(-)

-- 
1.8.2

[PATCH v3 1/2] run-command: add new check_command helper

[Felipe Contreras]

And to recover the information from the last run when running
wait_or_whine().

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 run-command.c | 46 ++++++++++++++++++++++++++++++++++++++++------
 run-command.h |  5 +++++
 2 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/run-command.c b/run-command.c
index 07e27ff..5cb5114 100644
--- a/run-command.c
+++ b/run-command.c
@@ -226,14 +226,20 @@ 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 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 */
+	/* First try the last status from check_command() */
+	if (cmd && cmd->last_status.valid) {
+		status = cmd->last_status.status;
+		waiting = pid;
+	} else {
+		while ((waiting = waitpid(pid, status, 0)) < 0 && errno == EINTR)
+			;	/* nothing */
+	}
 
 	if (waiting < 0) {
 		failed_errno = errno;
@@ -276,6 +282,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 +445,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 +550,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 +561,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 +763,7 @@ error:
 int finish_async(struct async *async)
 {
 #ifdef NO_PTHREADS
-	return wait_or_whine(async->pid, "child process");
+	return wait_or_whine(NULL, 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 v3 1/2] run-command: add new check_command helper

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> And to recover the information from the last run when running
> wait_or_whine().
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---

The above says what the updated wait_or_whine() does (it returns the
state an earlier call to check_command() has already polled to
determine), but lacks what the added check_command() does and why it
is needed.  I am guessing:

	In a codeflow like _this_ calls _that_ calls _that-other_,
	and finally the original caller culls the subprocesses by
	wait-or-whine, there is no good way for an intermediate
	level to detect dead child and abort the process early.
	They can now poll the status with check_command() without
	blocking in order to do so.

	wait_or_while() is adjusted to correctly report the status
	of the child that was already culled by an earlier call to
	check_command().  check_command() itself uses the same
	mechanism to indicate that the child was already culled, so
	that it is safe to call it more than once on the same child
	process.

might be a more understandable description for this change, but
these _this_, _that_ and _that-other_ blanks need to be filled.

>  run-command.c | 46 ++++++++++++++++++++++++++++++++++++++++------
>  run-command.h |  5 +++++
>  2 files changed, 45 insertions(+), 6 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 07e27ff..5cb5114 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -226,14 +226,20 @@ 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 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 */
> +	/* First try the last status from check_command() */
> +	if (cmd && cmd->last_status.valid) {
> +		status = cmd->last_status.status;
> +		waiting = pid;
> +	} else {
> +		while ((waiting = waitpid(pid, status, 0)) < 0 && errno == EINTR)
> +			;	/* nothing */
> +	}
>  
>  	if (waiting < 0) {
>  		failed_errno = errno;
> @@ -276,6 +282,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 +445,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 +550,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 +561,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 +763,7 @@ error:
>  int finish_async(struct async *async)
>  {
>  #ifdef NO_PTHREADS
> -	return wait_or_whine(async->pid, "child process");
> +	return wait_or_whine(NULL, 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, ...);

Re: [PATCH v3 1/2] run-command: add new check_command helper

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> -	while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
> -		;	/* nothing */
> +	/* First try the last status from check_command() */
> +	if (cmd && cmd->last_status.valid) {
> +		status = cmd->last_status.status;
> +		waiting = pid;
> +	} else {
> +		while ((waiting = waitpid(pid, status, 0)) < 0 && errno == EINTR)
> +			;	/* nothing */

waitpid(pid, &status, 0), I presume.

[PATCH v3 2/2] 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 v3 2/2] 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

It is rather unusual to see a negative return value returned from a
shell script.  Shouldn't this be something like 127 (or 1)?

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

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

[Jeff King]

On Sun, Apr 07, 2013 at 01:45:06AM -0600, Felipe Contreras wrote:

> 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
> +	)
> +'

Hmm.  If you drop the final "grep" (which is looking for the error
message you add elsewhere in this patch), this test passes even without
the addition of the check_command calls added by your patch. Which made
me wonder if we should be checking something else (repo state, error
messages, etc), since we seem to otherwise be detecting the error. More
analysis below on exactly what is going on there.

> +# We sleep to give fast-export a chance to catch the SIGPIPE

I'm not sure what this means; I don't see a sleep anywhere.

> +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
> +	)
> +'

I wondered why this one is marked for failure.

Even without check_command, the push produces:

  error: fast-export died of signal 13
  fatal: Error while running fast-export

which explains why the test fails: it does not even make it to the
check_command call at all. Why do we need check_command here, then? Is
it racy/non-deterministic for fast-export to die due to the pipe?

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

I'm still not very excited about this approach, as it is racy to detect
errors. E.g., there is nothing to say that the helper does not close the
pipe to fast-import prematurely and then die afterwards, leaving the
refs unwritten, fast-import happy, but a failed exit code from the
helper.

The import test still passes even without the check_command part of your
patch because some of the refs in refs/testgit do not exist prior to the
failed fetch, and therefore also do not exist afterwards. When fetch
tries to feed their sha1s to check_everything_connected, you get the
funny:

  fatal: bad object 0000000000000000000000000000000000000000
  error: testgit::/home/peff/compile/git/t/trash
   directory.t5801-remote-helpers/server did not send all necessary
   objects

But if we change the test like this:

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index efe67e2..e968ea9 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -167,11 +167,11 @@ test_expect_success 'proper failure checks for fetching' '
 '
 
 test_expect_success 'proper failure checks for fetching' '
-	(GIT_REMOTE_TESTGIT_FAILURE=1 &&
+	(cd local &&
+	git fetch &&
+	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
+	test_must_fail git fetch 2> error
 	)
 '
 
we can see that without your check_command, the failure in the second
fetch is not noticed. Adding in your patch does detect this. _But_ it is
only checking a specific failure mode of the remote-helper: process
death that results in closing the fast-import pipe, which is how your
GIT_REMOTE_TESTGIT_FAILURE is implemented (closing the pipe first and
then dying is racy).

If we then add this on top of your series (fixing the "status" bug in
patch 1 that Junio mentioned), the test will start failing (both the
original, and the more robust one I showed above):

diff --git a/git-remote-testgit b/git-remote-testgit
index ca0cf09..41b0780 100755
--- a/git-remote-testgit
+++ b/git-remote-testgit
@@ -64,6 +64,11 @@ do
 
 		if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
 		then
+			# close stdout
+			exec >/dev/null
+			# now sleep to make sure fast-import
+			# has time to die before we exit
+			sleep 1
 			exit -1
 		fi
 

I agree that the failure mode from your patch is probably the most
common order for helpers to fail in (i.e., they just die and that's what
kills the pipe).  But I wonder if we can just cover all cases.
Something like:

diff --git a/transport-helper.c b/transport-helper.c
index dfdfa7a..8562df0 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -461,7 +461,31 @@ static int fetch_with_import(struct transport *transport,
 	if (finish_command(&fastimport))
 		die("Error while running fast-import");
 
-	if (!check_command(data->helper))
+	/*
+	 * We must disconnect from the helper at this point, because even
+	 * though fast-import may have succeeded, it may only be because the
+	 * helper was not able to feed fast-import all of the data, and what
+	 * fast-import got looked OK (e.g., it may have got nothing if the
+	 * helper died early). We still need to check the return code of the
+	 * helper to make sure it is happy with what it sent.
+	 *
+	 * Since the import command does not require the helper to ever report
+	 * success/failure of the import, we have no mechanism to check for
+	 * problems except to check its exit status.
+	 *
+	 * Callers of the transport code are allowed to make more requests
+	 * of our helper, so we may be disconnecting before they expect in that
+	 * case. However:
+	 *
+	 *   1. Current callers don't do that; after fetching refs, there
+	 *      is nothing left for the helper to do.
+	 *
+	 *   2. We transparently start the helper as necessary, so if we were
+	 *      to get another request (e.g., to import more refs), we would
+	 *      simply start a new instantiation of the helper.
+	 *
+	 */
+	if (disconnect_helper(transport) != 0)
 		die("Error while running helper");
 
 	argv_array_free_detached(fastimport.argv);

which passes both your original test and the more strict one above. Of
course adding a done-import capability would be nice to fix the protocol
deficiency, but it is more code.

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

And this one is even more likely to race than the import case, I think,
as the exporter may send all of its data, exit, and then the helper
chugs on it for a bit (converting, sending across the network, etc). If
it is still chugging when we run check_command, we will not notice
anything here.

E.g., if we instrument the failure like this:

diff --git a/git-remote-testgit b/git-remote-testgit
index ca0cf09..a912bc1 100755
--- a/git-remote-testgit
+++ b/git-remote-testgit
@@ -75,6 +75,12 @@ do
 	export)
 		if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
 		then
+			# imagine we read all of fast-export's data
+			# first, and then die while trying to convert
+			# it
+			while read line; do
+				test "$line" = "done" && break
+			done
 			exit -1
 		fi
 

we do not get the sigpipe from fast-export, and depending on the timing,
your check_command may or may not do anything (in my tests, it did not).

But unlike the import side, the export command _does_ give us a status
report back from the helper: it prints an ok/error line for each ref
that was exported, followed by a blank line. So we should be able to
confirm that we get the blank line at all, and then that each ref was
present, which would determine whether the export failed or not without
being subject to race conditions.

And we seem to do those checks already; the only problem I see is that
recvline does not print a message before dying. Would the patch below be
sufficient?

diff --git a/transport-helper.c b/transport-helper.c
index dfdfa7a..cce2062 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer)
 	if (strbuf_getline(buffer, helper, '\n') == EOF) {
 		if (debug)
 			fprintf(stderr, "Debug: Remote helper quit.\n");
-		exit(128);
+		die("remote helper died unexpectedly");
 	}
 
 	if (debug)

-Peff

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

[Felipe Contreras]

On Sun, Apr 7, 2013 at 7:51 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Apr 07, 2013 at 01:45:06AM -0600, Felipe Contreras wrote:
>
>> 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
>> +     )
>> +'
>
> Hmm.  If you drop the final "grep" (which is looking for the error
> message you add elsewhere in this patch), this test passes even without
> the addition of the check_command calls added by your patch. Which made
> me wonder if we should be checking something else (repo state, error
> messages, etc), since we seem to otherwise be detecting the error. More
> analysis below on exactly what is going on there.

It fails all right, because the fetch process assumes the
remote-helper succeeded and only later eventually fails with an error
that is anything but user-friendly:

fatal: bad object 0000000000000000000000000000000000000000
error: testgit::/home/felipec/dev/git/t/trash
directory.t5801-remote-helpers/server did not send all necessary
objects

>> +# We sleep to give fast-export a chance to catch the SIGPIPE
>
> I'm not sure what this means; I don't see a sleep anywhere.

That's right, I'll remove the comment. It was cruft from the next
patch (see below).

>> +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
>> +     )
>> +'
>
> I wondered why this one is marked for failure.

Because it fails to report the right error.

> Even without check_command, the push produces:
>
>   error: fast-export died of signal 13
>   fatal: Error while running fast-export
>
> which explains why the test fails: it does not even make it to the
> check_command call at all. Why do we need check_command here, then? Is
> it racy/non-deterministic for fast-export to die due to the pipe?

It does not make it to check_command because the wait is missing. I
added that in a separate patch:
http://article.gmane.org/gmane.comp.version-control.git/219715

With that patch the check_command happens reliably, the error is
reported correctly, and the test passes. However, the way to trigger
it is ugly.

In a real use-case the chances of check_command catching the issue are
much higher..

>> 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);
>
> I'm still not very excited about this approach, as it is racy to detect
> errors. E.g., there is nothing to say that the helper does not close the
> pipe to fast-import prematurely and then die afterwards, leaving the
> refs unwritten, fast-import happy, but a failed exit code from the
> helper.

That's right, and that would leave us in the situation we are right
now, even with "done" check.

> The import test still passes even without the check_command part of your
> patch because some of the refs in refs/testgit do not exist prior to the
> failed fetch, and therefore also do not exist afterwards. When fetch
> tries to feed their sha1s to check_everything_connected, you get the
> funny:
>
>   fatal: bad object 0000000000000000000000000000000000000000
>   error: testgit::/home/peff/compile/git/t/trash
>    directory.t5801-remote-helpers/server did not send all necessary
>    objects

And you think that is desirable? User-friendly?

> we can see that without your check_command, the failure in the second
> fetch is not noticed. Adding in your patch does detect this. _But_ it is
> only checking a specific failure mode of the remote-helper: process
> death that results in closing the fast-import pipe, which is how your
> GIT_REMOTE_TESTGIT_FAILURE is implemented (closing the pipe first and
> then dying is racy).
>
> If we then add this on top of your series (fixing the "status" bug in
> patch 1 that Junio mentioned), the test will start failing (both the
> original, and the more robust one I showed above):
>
> diff --git a/git-remote-testgit b/git-remote-testgit
> index ca0cf09..41b0780 100755
> --- a/git-remote-testgit
> +++ b/git-remote-testgit
> @@ -64,6 +64,11 @@ do
>
>                 if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
>                 then
> +                       # close stdout
> +                       exec >/dev/null
> +                       # now sleep to make sure fast-import
> +                       # has time to die before we exit
> +                       sleep 1
>                         exit -1
>                 fi

Yeah, again:
http://article.gmane.org/gmane.comp.version-control.git/219715

> I agree that the failure mode from your patch is probably the most
> common order for helpers to fail in (i.e., they just die and that's what
> kills the pipe).  But I wonder if we can just cover all cases.
> Something like:
>
> diff --git a/transport-helper.c b/transport-helper.c
> index dfdfa7a..8562df0 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -461,7 +461,31 @@ static int fetch_with_import(struct transport *transport,
>         if (finish_command(&fastimport))
>                 die("Error while running fast-import");
>
> -       if (!check_command(data->helper))
> +       /*
> +        * We must disconnect from the helper at this point, because even
> +        * though fast-import may have succeeded, it may only be because the
> +        * helper was not able to feed fast-import all of the data, and what
> +        * fast-import got looked OK (e.g., it may have got nothing if the
> +        * helper died early). We still need to check the return code of the
> +        * helper to make sure it is happy with what it sent.
> +        *
> +        * Since the import command does not require the helper to ever report
> +        * success/failure of the import, we have no mechanism to check for
> +        * problems except to check its exit status.
> +        *
> +        * Callers of the transport code are allowed to make more requests
> +        * of our helper, so we may be disconnecting before they expect in that
> +        * case. However:
> +        *
> +        *   1. Current callers don't do that; after fetching refs, there
> +        *      is nothing left for the helper to do.
> +        *
> +        *   2. We transparently start the helper as necessary, so if we were
> +        *      to get another request (e.g., to import more refs), we would
> +        *      simply start a new instantiation of the helper.
> +        *
> +        */

That's a comprehensive essay, unfortunately, it's not correct. The
exit status of the remote-helper is not important, it's the one of
fast-import that we really care about.

> +       if (disconnect_helper(transport) != 0)
>                 die("Error while running helper");

Yeah, that's good, *if* the remote-helper is implemented correctly,
but try this:

               if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
               then
                       exit 0
               fi

>         argv_array_free_detached(fastimport.argv);
>
> which passes both your original test and the more strict one above. Of
> course adding a done-import capability would be nice to fix the protocol
> deficiency, but it is more code.

The done-import capability *is already there*. The remote helper fails
to say "done", fast-import detects that there was a problem and exits
with -1 (or whatever), but it doesn't matter, because we
(transport-helper) are oblivious of that return status until it's too
late.

>> @@ -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;
>>  }
>
> And this one is even more likely to race than the import case, I think,
> as the exporter may send all of its data, exit, and then the helper
> chugs on it for a bit (converting, sending across the network, etc). If
> it is still chugging when we run check_command, we will not notice
> anything here.
>
> E.g., if we instrument the failure like this:
>
> diff --git a/git-remote-testgit b/git-remote-testgit
> index ca0cf09..a912bc1 100755
> --- a/git-remote-testgit
> +++ b/git-remote-testgit
> @@ -75,6 +75,12 @@ do
>         export)
>                 if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
>                 then
> +                       # imagine we read all of fast-export's data
> +                       # first, and then die while trying to convert
> +                       # it
> +                       while read line; do
> +                               test "$line" = "done" && break
> +                       done
>                         exit -1
>                 fi
>
>
> we do not get the sigpipe from fast-export, and depending on the timing,
> your check_command may or may not do anything (in my tests, it did not).
>
> But unlike the import side, the export command _does_ give us a status
> report back from the helper: it prints an ok/error line for each ref
> that was exported, followed by a blank line. So we should be able to
> confirm that we get the blank line at all, and then that each ref was
> present, which would determine whether the export failed or not without
> being subject to race conditions.
>
> And we seem to do those checks already; the only problem I see is that
> recvline does not print a message before dying. Would the patch below be
> sufficient?
>
> diff --git a/transport-helper.c b/transport-helper.c
> index dfdfa7a..cce2062 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer)
>         if (strbuf_getline(buffer, helper, '\n') == EOF) {
>                 if (debug)
>                         fprintf(stderr, "Debug: Remote helper quit.\n");
> -               exit(128);
> +               die("remote helper died unexpectedly");
>         }
>
>         if (debug)

Yeah, I already explored this option, and I said it was possible on
pushing, but now the problem is fetching.

http://article.gmane.org/gmane.comp.version-control.git/219760

And to be frank, I'm tired of this. I keep repeating the same things
over and over, when I ask for input on different ways to achieve this
I get nothing, and when the patch is getting closer to be merged, then
I receive criticism, only so I can repeat the same again.

I don't like the option to die right in recvline(), but it would work.
We would need something else for import though. It would be possible
to tell fast-import to ping the remote-helper, but I ran into a
SIGPIPE, and I don't have the patience to find out if perhaps there's
a way to solve that. This option seems rather hacky and asymmetric to
me.

If this current patch series is not good enough, I think it's best to
leave things as they are for me.

Cheers.

--
Felipe Contreras

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

[Jeff King]

On Sun, Apr 07, 2013 at 09:03:25PM -0500, Felipe Contreras wrote:

> > I'm still not very excited about this approach, as it is racy to detect
> > errors. E.g., there is nothing to say that the helper does not close the
> > pipe to fast-import prematurely and then die afterwards, leaving the
> > refs unwritten, fast-import happy, but a failed exit code from the
> > helper.
> 
> That's right, and that would leave us in the situation we are right
> now, even with "done" check.

Yes. I do not think your patch makes anything worse. But it also does
not fix the problem completely.

> > The import test still passes even without the check_command part of your
> > patch because some of the refs in refs/testgit do not exist prior to the
> > failed fetch, and therefore also do not exist afterwards. When fetch
> > tries to feed their sha1s to check_everything_connected, you get the
> > funny:
> >
> >   fatal: bad object 0000000000000000000000000000000000000000
> >   error: testgit::/home/peff/compile/git/t/trash
> >    directory.t5801-remote-helpers/server did not send all necessary
> >    objects
> 
> And you think that is desirable? User-friendly?

No, as you probably realized if you read the rest of my email. My point
in bringing it up was to try to explain exactly what is going on in each
case. Your commit message provided zero explanation of what the current
problem is, nor why your fix is the right thing to do.

> > -       if (!check_command(data->helper))
> > +       /*
> > +        * We must disconnect from the helper at this point, because even
> > +        * though fast-import may have succeeded, it may only be because the
> > +        * helper was not able to feed fast-import all of the data, and what
> > +        * fast-import got looked OK (e.g., it may have got nothing if the
> > +        * helper died early). We still need to check the return code of the
> > +        * helper to make sure it is happy with what it sent.
> > +        *
> > +        * Since the import command does not require the helper to ever report
> > +        * success/failure of the import, we have no mechanism to check for
> > +        * problems except to check its exit status.
> > +        *
> > +        * Callers of the transport code are allowed to make more requests
> > +        * of our helper, so we may be disconnecting before they expect in that
> > +        * case. However:
> > +        *
> > +        *   1. Current callers don't do that; after fetching refs, there
> > +        *      is nothing left for the helper to do.
> > +        *
> > +        *   2. We transparently start the helper as necessary, so if we were
> > +        *      to get another request (e.g., to import more refs), we would
> > +        *      simply start a new instantiation of the helper.
> > +        *
> > +        */
> 
> That's a comprehensive essay, unfortunately, it's not correct. The
> exit status of the remote-helper is not important, it's the one of
> fast-import that we really care about.

Nowhere does it say that we should not check fast-import's exit value,
and indeed, the patch does not replace that check at all. It comes
immediately after. It replaces the WNOHANG check of the helper's exit
code (i.e., check_command) with a synchronous check.

The argument that the exit status of remote-helper is not important
would mean your series is pointless, too, no?

> > +       if (disconnect_helper(transport) != 0)
> >                 die("Error while running helper");
> 
> Yeah, that's good, *if* the remote-helper is implemented correctly,
> but try this:
> 
>                if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
>                then
>                        exit 0
>                fi

I don't see your point. If a helper reports success when it has failed,
and doesn't feed anything to fast-import, we can do nothing. That case
is indistinguishable from a helper which has nothing to fetch from the
remote, and exits. My patch doesn't help with such a broken helper, but
neither does yours, because they check the exact same thing.

> >         argv_array_free_detached(fastimport.argv);
> >
> > which passes both your original test and the more strict one above. Of
> > course adding a done-import capability would be nice to fix the protocol
> > deficiency, but it is more code.
> 
> The done-import capability *is already there*. The remote helper fails
> to say "done", fast-import detects that there was a problem and exits
> with -1 (or whatever), but it doesn't matter, because we
> (transport-helper) are oblivious of that return status until it's too
> late.

Wait, then why is this a problem at all? We check the exit code of
fast-import right before the code in question.

> Yeah, I already explored this option, and I said it was possible on
> pushing, but now the problem is fetching.
> 
> http://article.gmane.org/gmane.comp.version-control.git/219760
> 
> And to be frank, I'm tired of this. I keep repeating the same things
> over and over, when I ask for input on different ways to achieve this
> I get nothing, and when the patch is getting closer to be merged, then
> I receive criticism, only so I can repeat the same again.

I don't even know what to say to this. I reviewed your original patch,
raised some concerns. You responded. I had not yet responded back,
because I have many other things to work on. In the meantime you
submitted another round. Now I have the opportunity to respond and did
so, having read both threads.

Why is your patch to recvline so much more complicated than my
one-liner? Why do you go to lengths to pass out an error code from it
just so that we can end up dying anyway? Why do you need a done
capability for push when the blank line already signals the end of the
status?

I did not respond to 219760 separately because those are all questions
that _don't matter_ if you follow the analysis in my last email that
lead to my one-liner. Maybe there are interesting answers to those
questions, and my one-liner isn't sufficient. But I'll never know
because instead of pointing them out, you are complaining that I didn't
respond fast enough.

> I don't like the option to die right in recvline(), but it would work.
> We would need something else for import though. It would be possible
> to tell fast-import to ping the remote-helper, but I ran into a
> SIGPIPE, and I don't have the patience to find out if perhaps there's
> a way to solve that. This option seems rather hacky and asymmetric to
> me.

A ping-check would work. You just need:

  sigchain_push(SIGPIPE, SIG_IGN);
  /*
   * send ping; write can get EPIPE here, which is fine; we
   * would want to treat write errors as a failed ping
   */
  sigchain_pop(SIGPIPE);

but we would have to add a ping command, no?

> If this current patch series is not good enough, I think it's best to
> leave things as they are for me.

That's certainly your choice. It just seems like we can be more robust
while adding less code, as I tried to show.

-Peff

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

[Felipe Contreras]

On Sun, Apr 7, 2013 at 9:33 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Apr 07, 2013 at 09:03:25PM -0500, Felipe Contreras wrote:

>> And you think that is desirable? User-friendly?
>
> No, as you probably realized if you read the rest of my email. My point
> in bringing it up was to try to explain exactly what is going on in each
> case. Your commit message provided zero explanation of what the current
> problem is, nor why your fix is the right thing to do.

I have explained this many times, I wonder why when the patch is close
to be merged does this become important, and not before.

>> That's a comprehensive essay, unfortunately, it's not correct. The
>> exit status of the remote-helper is not important, it's the one of
>> fast-import that we really care about.
>
> Nowhere does it say that we should not check fast-import's exit value,
> and indeed, the patch does not replace that check at all. It comes
> immediately after. It replaces the WNOHANG check of the helper's exit
> code (i.e., check_command) with a synchronous check.

Yeah, and in the process it's changing the design of transport-helper,
where the pipes are closed only at the very end.

> The argument that the exit status of remote-helper is not important
> would mean your series is pointless, too, no?

The main point of my patch was not to add a check for import, but for
export. Adding such a check here is not important, but it doesn't hurt
either, as it would improve the situation if the remote-helper is not
using the "done" feature, which at the moment is a possibility.

>> > +       if (disconnect_helper(transport) != 0)
>> >                 die("Error while running helper");
>>
>> Yeah, that's good, *if* the remote-helper is implemented correctly,
>> but try this:
>>
>>                if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
>>                then
>>                        exit 0
>>                fi
>
> I don't see your point. If a helper reports success when it has failed,
> and doesn't feed anything to fast-import, we can do nothing. That case
> is indistinguishable from a helper which has nothing to fetch from the
> remote, and exits. My patch doesn't help with such a broken helper, but
> neither does yours, because they check the exact same thing.

No, my code checks that the remote-helper is *still running*, you code
doesn't. And after all, that is the design of transport-helper.

>> >         argv_array_free_detached(fastimport.argv);
>> >
>> > which passes both your original test and the more strict one above. Of
>> > course adding a done-import capability would be nice to fix the protocol
>> > deficiency, but it is more code.
>>
>> The done-import capability *is already there*. The remote helper fails
>> to say "done", fast-import detects that there was a problem and exits
>> with -1 (or whatever), but it doesn't matter, because we
>> (transport-helper) are oblivious of that return status until it's too
>> late.
>
> Wait, then why is this a problem at all? We check the exit code of
> fast-import right before the code in question.

It's not, *If* the remote-helper is using "done", and a small one if it's not.

>> Yeah, I already explored this option, and I said it was possible on
>> pushing, but now the problem is fetching.
>>
>> http://article.gmane.org/gmane.comp.version-control.git/219760
>>
>> And to be frank, I'm tired of this. I keep repeating the same things
>> over and over, when I ask for input on different ways to achieve this
>> I get nothing, and when the patch is getting closer to be merged, then
>> I receive criticism, only so I can repeat the same again.
>
> I don't even know what to say to this. I reviewed your original patch,
> raised some concerns. You responded. I had not yet responded back,
> because I have many other things to work on. In the meantime you
> submitted another round. Now I have the opportunity to respond and did
> so, having read both threads.

I sent the first version of this patch series on October of last year.

> Why is your patch to recvline so much more complicated than my
> one-liner? Why do you go to lengths to pass out an error code from it
> just so that we can end up dying anyway?

Because I think dying inside a generic function is bad design. I think
the error should be returned, and let the caller decide what to do
with it. In the current version of the code we seem to receive lines
only from the remote-helper, so I guess your patch is fine and works.
But I still think the current code is not good.

> Why do you need a done
> capability for push when the blank line already signals the end of the
> status?

I suppose we don't.

> I did not respond to 219760 separately because those are all questions
> that _don't matter_ if you follow the analysis in my last email that
> lead to my one-liner. Maybe there are interesting answers to those
> questions, and my one-liner isn't sufficient. But I'll never know
> because instead of pointing them out, you are complaining that I didn't
> respond fast enough.

That's not what I said. My complaint is why _now_, and not six months ago.

>> If this current patch series is not good enough, I think it's best to
>> leave things as they are for me.
>
> That's certainly your choice. It just seems like we can be more robust
> while adding less code, as I tried to show.

After reading back all the mails and checking all the different
situations, I think my main problem was that my test was not for the
right thing. The test for import should be using the 'done' feature,
and we should mostly ignore the rest.

While my patch does improve the situation when the remote-helper is
not using the 'done' feature, it's probably not worth the effort, and
we might want to enforce the remote helpers to implement this somehow.

In the meantime, your patch should probably deal with all the real
world situations, so I've updated the tests and I'll resend.

Cheers.

--
Felipe Contreras

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Sun, Apr 7, 2013 at 9:33 PM, Jeff King <peff@peff.net> wrote:
>> On Sun, Apr 07, 2013 at 09:03:25PM -0500, Felipe Contreras wrote:
>
>>> And you think that is desirable? User-friendly?
>>
>> No, as you probably realized if you read the rest of my email. My point
>> in bringing it up was to try to explain exactly what is going on in each
>> case. Your commit message provided zero explanation of what the current
>> problem is, nor why your fix is the right thing to do.
>
> I have explained this many times, I wonder why when the patch is close
> to be merged does this become important, and not before.

There are at least a few reasons I can think of off the top of my
head.

Reviewers have limited time allocated for each individual topic.  To
an original patch with say 3 problems, the review may only point out
issues in 2 and suggest a concrete improvement for only 1 and that
is sufficient to suggest a reroll.  That is not being unhelpful by
deliberately withholding 1 and half reviews in the initial round.

Reviewers will see the same patch with fresh eyes after 6 months and
will notice different issues. That is not being unhelpful by
deliberately withholding a crucial point in the initial round of the
review.

I would not be surprised if one ends up repeating oneself in
multiple review threads; the log message of a rerolled patch is a
better place to avoid the need for the repetition.

>>> That's a comprehensive essay, unfortunately, it's not correct. The
>>> exit status of the remote-helper is not important, it's the one of
>>> fast-import that we really care about.
>>
>> Nowhere does it say that we should not check fast-import's exit value,
>> and indeed, the patch does not replace that check at all. It comes
>> immediately after. It replaces the WNOHANG check of the helper's exit
>> code (i.e., check_command) with a synchronous check.
>
> Yeah, and in the process it's changing the design of transport-helper,
> where the pipes are closed only at the very end.

I agree that the disconnection here closes the pipes a lot earlier
than we used to.  But I am not sure what the practical consequences
of doing so are.  We know the fast-import process has successfully
read everything from the helper (we called finish_command() on it).
We expect at this point the helper is still running or successfully
exited (the other alternative, the helper died with non-zero status,
is an error check_command() patch wanted to detect).  But if we keep
helper running, who will be communicating with it via these open
pipes?  The process that is calling finish_command() on fast-import
and disconnecting from the helper won't be, as read/write to the
pipe, even if we do not disconnect from here, will result in errors
if the helper has already exited at this point.

What I am trying to get at is if "changing the design", which I
agree is a change, is an improvement, or a backward incompatible
possible breakage.

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

[Felipe Contreras]

On Mon, Apr 8, 2013 at 12:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Sun, Apr 7, 2013 at 9:33 PM, Jeff King <peff@peff.net> wrote:
>>> On Sun, Apr 07, 2013 at 09:03:25PM -0500, Felipe Contreras wrote:
>>
>>>> And you think that is desirable? User-friendly?
>>>
>>> No, as you probably realized if you read the rest of my email. My point
>>> in bringing it up was to try to explain exactly what is going on in each
>>> case. Your commit message provided zero explanation of what the current
>>> problem is, nor why your fix is the right thing to do.
>>
>> I have explained this many times, I wonder why when the patch is close
>> to be merged does this become important, and not before.
>
> There are at least a few reasons I can think of off the top of my
> head.
>
> Reviewers have limited time allocated for each individual topic.  To
> an original patch with say 3 problems, the review may only point out
> issues in 2 and suggest a concrete improvement for only 1 and that
> is sufficient to suggest a reroll.  That is not being unhelpful by
> deliberately withholding 1 and half reviews in the initial round.

I'm not talking about the time it took to come up with the criticism
below, I'm talking about the comment regarding the commit message. If
the commit message did indeed provide *zero* explanation, that's
certainly something that should be immediately visible, no? It could
have been mentioned six months ago.

>>>> That's a comprehensive essay, unfortunately, it's not correct. The
>>>> exit status of the remote-helper is not important, it's the one of
>>>> fast-import that we really care about.
>>>
>>> Nowhere does it say that we should not check fast-import's exit value,
>>> and indeed, the patch does not replace that check at all. It comes
>>> immediately after. It replaces the WNOHANG check of the helper's exit
>>> code (i.e., check_command) with a synchronous check.
>>
>> Yeah, and in the process it's changing the design of transport-helper,
>> where the pipes are closed only at the very end.
>
> I agree that the disconnection here closes the pipes a lot earlier
> than we used to.  But I am not sure what the practical consequences
> of doing so are.  We know the fast-import process has successfully
> read everything from the helper (we called finish_command() on it).
> We expect at this point the helper is still running or successfully
> exited (the other alternative, the helper died with non-zero status,
> is an error check_command() patch wanted to detect).  But if we keep
> helper running, who will be communicating with it via these open
> pipes?  The process that is calling finish_command() on fast-import
> and disconnecting from the helper won't be, as read/write to the
> pipe, even if we do not disconnect from here, will result in errors
> if the helper has already exited at this point.

Nobody will send any further input, but in theory we could redirect
the pipe and send more commands. That's how it was designed.

And in fact, I'm thinking doing exactly that, so we can send another
command to fetch the foreign commit ids and append notes with them.

> What I am trying to get at is if "changing the design", which I
> agree is a change, is an improvement, or a backward incompatible
> possible breakage.

It's an improvement I guess, but it's "changing the design" only in
one part of the code, while the rest follows a different design.

--
Felipe Contreras

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> I'm not talking about the time it took to come up with the criticism
> below, I'm talking about the comment regarding the commit message. If
> the commit message did indeed provide *zero* explanation, that's
> certainly something that should be immediately visible, no? It could
> have been mentioned six months ago.

Given that all of your log messages tend to be missing or terser
than necessary, I wouldn't be surprised Peff stopped bothering about
it.

But now we know this needs to be better explained, and your v4 has
better explanation that will help you avoid having to repeat
yourself in the discussion, so let's not dwell on it too much.

>> ...  But if we keep
>> helper running, who will be communicating with it via these open
>> pipes?  The process that is calling finish_command() on fast-import
>> and disconnecting from the helper won't be, as read/write to the
>> pipe, even if we do not disconnect from here, will result in errors
>> if the helper has already exited at this point.
>
> Nobody will send any further input, but in theory we could redirect
> the pipe and send more commands. That's how it was designed.

Who does the redirection to whom?  How would the process tree and
piping constructed around the current system?

I am not trying to say it is just theoretical mental exercise (which
I have seen you do not do at all on this list). I am trying to find
out what the practical use case is that you have in mind, because
disconnecting will prove to be not an improvement but a regression
for that use case.  But you do not have to answer this question
directly, because...

> And in fact, I'm thinking doing exactly that, so we can send another
> command to fetch the foreign commit ids and append notes with them.

...we will see the answer in that change anyway.

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

[Felipe Contreras]

On Mon, Apr 8, 2013 at 1:46 PM, Junio C Hamano <gitster@pobox.com> wrote:

>>> ...  But if we keep
>>> helper running, who will be communicating with it via these open
>>> pipes?  The process that is calling finish_command() on fast-import
>>> and disconnecting from the helper won't be, as read/write to the
>>> pipe, even if we do not disconnect from here, will result in errors
>>> if the helper has already exited at this point.
>>
>> Nobody will send any further input, but in theory we could redirect
>> the pipe and send more commands. That's how it was designed.
>
> Who does the redirection to whom?

The one that is doing all the redirections, transport-helper.

> How would the process tree and
> piping constructed around the current system?

I cannot parse that correctly, but transport-helper is already
receiving the output from the remote-helper.

> I am not trying to say it is just theoretical mental exercise (which
> I have seen you do not do at all on this list). I am trying to find
> out what the practical use case is that you have in mind, because
> disconnecting will prove to be not an improvement but a regression
> for that use case.  But you do not have to answer this question
> directly, because...

As the gitifyhg developers effusively pointed out, it's useful to see
which mercurial revision corresponds to certain git revision, and this
is best achieved through notes. Implementing this in the remote-helper
doesn't make much sense (I won't go into details).

>> And in fact, I'm thinking doing exactly that, so we can send another
>> command to fetch the foreign commit ids and append notes with them.
>
> ...we will see the answer in that change anyway.

That doesn't change the fact that transport-helper would end up with
mixed and matched design. Maybe the use-case above wouldn't be
prevented by this change, but I think further changes would need to be
done to the file to not end up in this weird state.

Cheers.

--
Felipe Contreras

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Mon, Apr 8, 2013 at 1:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>>> ...  But if we keep
>>>> helper running, who will be communicating with it via these open
>>>> pipes?  The process that is calling finish_command() on fast-import
>>>> and disconnecting from the helper won't be, as read/write to the
>>>> pipe, even if we do not disconnect from here, will result in errors
>>>> if the helper has already exited at this point.
>>>
>>> Nobody will send any further input, but in theory we could redirect
>>> the pipe and send more commands. That's how it was designed.
>>
>> Who does the redirection to whom?
>
> The one that is doing all the redirections, transport-helper.
>
>> How would the process tree and
>> piping constructed around the current system?
>
> I cannot parse that correctly,

Sorry, s/the current system/& look like/;

> but transport-helper is already
> receiving the output from the remote-helper.

OK, so you are envisioning that transport-helper would read from the
helper after importer is done?  If so, perhaps it is a prudent
solution to disconnect in this version (to fix), and then in a
separate patch that adds such an extension (I imagine it would
involve that the helper advertising a capability or being invoked
with an option to let transport-helper somehow know that it should
continue the conversation once fast-import is done) to disable the
disconnect here when that extension is in use?

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

[Jeff King]

On Mon, Apr 08, 2013 at 02:08:03PM -0700, Junio C Hamano wrote:

> OK, so you are envisioning that transport-helper would read from the
> helper after importer is done?  If so, perhaps it is a prudent
> solution to disconnect in this version (to fix), and then in a
> separate patch that adds such an extension (I imagine it would
> involve that the helper advertising a capability or being invoked
> with an option to let transport-helper somehow know that it should
> continue the conversation once fast-import is done) to disable the
> disconnect here when that extension is in use?

At this point, I am of the opinion that it's OK to just do nothing;
modern helpers should be using the "done" flag, and if they aren't, then
that is the right place for the fix. Then we don't have to worry about
any side effects of disconnecting, or adding a new capability flag.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Apr 08, 2013 at 02:08:03PM -0700, Junio C Hamano wrote:
>
>> OK, so you are envisioning that transport-helper would read from the
>> helper after importer is done?  If so, perhaps it is a prudent
>> solution to disconnect in this version (to fix), and then in a
>> separate patch that adds such an extension (I imagine it would
>> involve that the helper advertising a capability or being invoked
>> with an option to let transport-helper somehow know that it should
>> continue the conversation once fast-import is done) to disable the
>> disconnect here when that extension is in use?
>
> At this point, I am of the opinion that it's OK to just do nothing;
> modern helpers should be using the "done" flag, and if they aren't, then
> that is the right place for the fix. Then we don't have to worry about
> any side effects of disconnecting, or adding a new capability flag.

Sounds sane.  Thanks for unconfusing me ;-)

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

[Jeff King]

On Mon, Apr 08, 2013 at 01:31:43PM -0500, Felipe Contreras wrote:

> > Reviewers have limited time allocated for each individual topic.  To
> > an original patch with say 3 problems, the review may only point out
> > issues in 2 and suggest a concrete improvement for only 1 and that
> > is sufficient to suggest a reroll.  That is not being unhelpful by
> > deliberately withholding 1 and half reviews in the initial round.
> 
> I'm not talking about the time it took to come up with the criticism
> below, I'm talking about the comment regarding the commit message. If
> the commit message did indeed provide *zero* explanation, that's
> certainly something that should be immediately visible, no? It could
> have been mentioned six months ago.

I do not recall this series at all from six months ago. I reviewed a
series (with no version markers, nor any mention of the previous round)
posted on April 1st, and assumed it was a new series. I raised the same
issues there that I did in v3, though in less detail (because I had not
yet looked into it as thoroughly).

I searched the archive after reading this mail and found the original
series. Yes, it existed. I didn't review it. I guess I had something
more important to do that day.

But is all of this even important? If there are technical issues with
this series, does it matter when we find out about them? They still
deserve to be fixed, no?  Yes, it is nice if things get reviewed
promptly, but the limited bandwidth and attention of reviewers is a fact
of life.

-Peff

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

[Jeff King]

On Mon, Apr 08, 2013 at 09:38:28AM -0500, Felipe Contreras wrote:

> > Nowhere does it say that we should not check fast-import's exit value,
> > and indeed, the patch does not replace that check at all. It comes
> > immediately after. It replaces the WNOHANG check of the helper's exit
> > code (i.e., check_command) with a synchronous check.
> 
> Yeah, and in the process it's changing the design of transport-helper,
> where the pipes are closed only at the very end.

Yes. And I tried to explain why this is acceptable in the "comprehensive
essay" of a comment. If you think there are reasons not to do so, please
elaborate. But talking about fast-import's exit value is irrelevant.

> > I don't see your point. If a helper reports success when it has failed,
> > and doesn't feed anything to fast-import, we can do nothing. That case
> > is indistinguishable from a helper which has nothing to fetch from the
> > remote, and exits. My patch doesn't help with such a broken helper, but
> > neither does yours, because they check the exact same thing.
> 
> No, my code checks that the remote-helper is *still running*, you code
> doesn't. And after all, that is the design of transport-helper.

I see. That would have been a very helpful thing to put in the commit
message, no?

But the issue still remains: a "still running" check is subject to race
conditions. A disconnect solution should fail not only on a failed exit
code, but also if we are unable to send the disconnect command (i.e., it
should be checking EPIPE rather than ignoring it). So my solution was
not sufficient, either.

But I think it is moot, as I agree with your statements below and the
follow-up patch.

> >> The done-import capability *is already there*. The remote helper fails
> >> to say "done", fast-import detects that there was a problem and exits
> >> with -1 (or whatever), but it doesn't matter, because we
> >> (transport-helper) are oblivious of that return status until it's too
> >> late.
> >
> > Wait, then why is this a problem at all? We check the exit code of
> > fast-import right before the code in question.
> 
> It's not, *If* the remote-helper is using "done", and a small one if it's not.

Something like this might have been helpful in the commit message to
make it more clear what the point of the patch is:

  If the remote-helper is using the "done" feature in the export stream,
  then it will signal to fast-import that it correctly output all of the
  data to the stream. We can trust that the exit code of fast-import
  will report such a problem to it, and do not have to worry.

  But for helpers which do not use the "done" feature and exit early,
  fast-import has no idea whether all of the data was written or not.
  The helper does not report any status to us, so we are left to guess
  at whether it worked successfully. We can make such a guess by seeing
  whether the process is still alive.

  This does not cover all cases, as it is possible for us to see the
  process still present just before it exits. However, it covers the
  common case that the helper dies with an error, closing the pipe to
  fast-import at the same time. Since we reap fast-import first, by the
  time we check whether the helper is still alive, it will have exited,
  too.

  That may miss some cases, but it is the best we can do for such a
  helper, given the current protocol. A remote helper which wants to be
  robust should use the "done" feature.

Maybe you thought about all of those issues. Maybe you even communicated
them somewhere on the list. But I did not see them in the commit message
nor in a comment, which is the main item I look at while reviewing. If
you did not realize it until after the discussion, that is fine, too.
That is why we have the discussions. Now we can perhaps improve the
patch (either by explaining there what is happening, or deciding that
this case is not worth caring about).

> > Why is your patch to recvline so much more complicated than my
> > one-liner? Why do you go to lengths to pass out an error code from it
> > just so that we can end up dying anyway?
> 
> Because I think dying inside a generic function is bad design. I think
> the error should be returned, and let the caller decide what to do
> with it.

We already die inside the function; my patch only added a message. If you
want to change the dying, fine, but that cleanup is a separate issue.

> In the current version of the code we seem to receive lines
> only from the remote-helper, so I guess your patch is fine and works.
> But I still think the current code is not good.

It's a static inside transport-helper.c. So I think it is OK for it to
assume that the transport-helper code will be the only caller. In such a
situation, it _may_ be worth changing later if it becomes a more generic
and widely accessible function, but I do not think that is the case
here. The function does nothing except report remote-helper debugging
information and ask strbuf_getline to do the heavy lifting.

> > I did not respond to 219760 separately because those are all questions
> > that _don't matter_ if you follow the analysis in my last email that
> > lead to my one-liner. Maybe there are interesting answers to those
> > questions, and my one-liner isn't sufficient. But I'll never know
> > because instead of pointing them out, you are complaining that I didn't
> > respond fast enough.
> 
> That's not what I said. My complaint is why _now_, and not six months ago.

Because I did not read your series 6 months ago, nor did you make any
mention of it in your more recent cover letter.

> After reading back all the mails and checking all the different
> situations, I think my main problem was that my test was not for the
> right thing. The test for import should be using the 'done' feature,
> and we should mostly ignore the rest.
> 
> While my patch does improve the situation when the remote-helper is
> not using the 'done' feature, it's probably not worth the effort, and
> we might want to enforce the remote helpers to implement this somehow.

Yes, that makes sense to me, too. I am inclined to say that failing to
use "done" is a quality-of-implementation issue for the remote-helper,
and the right fix is for them to use the proper signal.

-Peff