[PATCH] transport-helper: check when helpers fail

Otherwise transport-helper will continue checking for refs and other
things what will confuse the user more.

Signed-off-by: Felipe Contreras <redacted>
---
 git-remote-testgit.py     |  3 +++
 run-command.c             | 17 +++++++++++++++++
 run-command.h             |  1 +
 t/t5800-remote-helpers.sh |  6 ++++++
 transport-helper.c        |  8 ++++++++
 5 files changed, 35 insertions(+)

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index 5f3ebd2..355e3f5 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py

@@ -159,6 +159,9 @@ def do_import(repo, args):
         ref = line[7:].strip()
         refs.append(ref)
 
+    if os.environ.get("GIT_REMOTE_TESTGIT_FAILURE"):
+        die('Told to fail')
+
     repo = update_local_repo(repo)
     repo.exporter.export_repo(repo.gitdir, refs)
 

diff --git a/run-command.c b/run-command.c
index 1101ef7..2852e9d 100644
--- a/run-command.c
+++ b/run-command.c

@@ -559,6 +559,23 @@ int run_command(struct child_process *cmd)
 	return finish_command(cmd);
 }
 
+int check_command(struct child_process *cmd)
+{
+	int status;
+	pid_t pid;
+
+	pid = waitpid(cmd->pid, &status, WNOHANG);
+
+	if (pid < 0)
+		return -1;
+	if (WIFSIGNALED(status))
+		return WTERMSIG(status);
+	if (WIFEXITED(status))
+		return WEXITSTATUS(status);
+
+	return 0;
+}
+
 static void prepare_run_command_v_opt(struct child_process *cmd,
 				      const char **argv,
 				      int opt)

diff --git a/run-command.h b/run-command.h
index 44f7d2b..9019e38 100644
--- a/run-command.h
+++ b/run-command.h

@@ -45,6 +45,7 @@ struct child_process {
 int start_command(struct child_process *);
 int finish_command(struct child_process *);
 int run_command(struct child_process *);
+int check_command(struct child_process *cmd);
 
 extern int run_hook(const char *index_file, const char *name, ...);
 

diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index e7dc668..d4b17ae 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh

@@ -145,4 +145,10 @@ test_expect_failure 'push new branch with old:new refspec' '
 	compare_refs clone HEAD server refs/heads/new-refspec
 '
 
+test_expect_success 'proper failure checks' '
+	export GIT_REMOTE_TESTGIT_FAILURE=1 &&
+	! git clone "testgit::$PWD/server" failure 2> errors &&
+	grep -q "Error while running helper" errors
+'
+
 test_done

diff --git a/transport-helper.c b/transport-helper.c
index cfe0988..fbd923d 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -441,6 +441,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");
+
 	free(fastimport.argv);
 	fastimport.argv = NULL;
 

@@ -784,6 +788,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.0.rc2

Am 10/21/2012 21:19, schrieb Felipe Contreras:

In this form, the function is not suitable as a public run-command API: If
the child did exit, it does not allow finish_command() to do its thing.
The only thing the caller of this function can do is to die() if it
returns non-zero. It doesn't report treat error cases in the same way as
wait_or_whine().

I would expect the function to be usable in this way:

	start_command(&proc);

	loop {
		if (check_command(&proc))
			break;
	}

	finish_command(&proc);

but it would require a bit more work because it would have to cache the
exit status in struct child_process.

BTW, you should check for return value 0 from waitpid() explicitly.

Another thought: In your use-case, isn't it so that it would be an error
that the process exited for whatever reason? I.e., even if it exited with
code 0 ("success"), it would be an error because it violated the protocol?

-- Hannes

On Mon, Oct 22, 2012 at 8:35 AM, Johannes Sixt [off-list ref] wrote:

Am 10/21/2012 21:19, schrieb Felipe Contreras:

I would expect the function to be usable in this way:

        start_command(&proc);

        loop {
                if (check_command(&proc))
                        break;
        }

        finish_command(&proc);

but it would require a bit more work because it would have to cache the
exit status in struct child_process.

Yes, I would expect that as well. I just noticed transport-helper also
fails with that, but some reason that's not enough to actually fail
the tests, so something weird is going on.

BTW, you should check for return value 0 from waitpid() explicitly.

Right.

Another thought: In your use-case, isn't it so that it would be an error
that the process exited for whatever reason? I.e., even if it exited with
code 0 ("success"), it would be an error because it violated the protocol?

How is that violating the protocol?

-- 
Felipe Contreras

Am 10/22/2012 13:50, schrieb Felipe Contreras:

On Mon, Oct 22, 2012 at 8:35 AM, Johannes Sixt [off-list ref] wrote:

quoted

Another thought: In your use-case, isn't it so that it would be an error
that the process exited for whatever reason? I.e., even if it exited with
code 0 ("success"), it would be an error because it violated the protocol?

How is that violating the protocol?

Because the helper stops talking too early. But as I said, I actually
don't know the protocol.

I was just infering what I saw in transport-helper.c: get_helper() dup's
the output of the helper process and stores it in data->out (after
fdopen()ing on it). (The original file descriptor is handed over to
fast-import or fast-export.)

Actually, I didn't find a spot where data->out was used except to fclose()
it. But I take it that there is a reason that it exists and infer that
further output from the helper is expected by something after fast-import
or fast-export have exited.

But I may be completely off...

-- Hannes

On Mon, Oct 22, 2012 at 3:46 PM, Johannes Sixt [off-list ref] wrote:

Am 10/22/2012 13:50, schrieb Felipe Contreras:

quoted

On Mon, Oct 22, 2012 at 8:35 AM, Johannes Sixt [off-list ref] wrote:

quoted

Another thought: In your use-case, isn't it so that it would be an error
that the process exited for whatever reason? I.e., even if it exited with
code 0 ("success"), it would be an error because it violated the protocol?

How is that violating the protocol?

Because the helper stops talking too early. But as I said, I actually
don't know the protocol.

We could use the 'feature done' of fast-import, but this causes
problems because of the way transport-helper uses it:

-> import refs/heads/master
<- exported stuff
<- done
-> import refs/heads/devel
<- exported stuff
<- done

'done' will terminate the fast-import process, so the second exported
stuff won't be processed; the fast-import process is reused.

For some reason remote-testgit doesn't exercise this multiple import
stuff properly, but my remote-hg certainly does, so I can't just say
'done'.

It would be much better if the transport-helper protocol was something
like this:

-> import-begin
<- feature X
<- feature Y
-> import refs/heads/master
<- exported stuff
-> import refs/heads/devel
<- exported stuff
-> import-end
<- done

This would certainly makes things easier for transport-helpers that
support multiple ref selections (like my remote-hg). Maybe I should
add code that does this if certain feature is specified (so it doesn't
break other helpers)

But at least on my tests, even with 'feature done' the crash is not
detected properly, either by the transport-helper, or fast-import.

And also, the msysgit branch does the same check for fast-export,
which actually uses the 'done' feature always, so it should work fine,
but perhaps because of the strange issue with fast-import I just
mentioned, it's not actually detected. I should add tests for this
too.

I was just infering what I saw in transport-helper.c: get_helper() dup's
the output of the helper process and stores it in data->out (after
fdopen()ing on it). (The original file descriptor is handed over to
fast-import or fast-export.)

Actually, I didn't find a spot where data->out was used except to fclose()
it. But I take it that there is a reason that it exists and infer that
further output from the helper is expected by something after fast-import
or fast-export have exited.

But I may be completely off...

Yes, further output is expected, or at least in theory.

Cheers.

-- 
Felipe Contreras

On Mon, Oct 22, 2012 at 4:31 PM, Felipe Contreras
[off-list ref] wrote:

-> import-begin
<- feature X
<- feature Y
-> import refs/heads/master
<- exported stuff
-> import refs/heads/devel
<- exported stuff
-> import-end
<- done

This would certainly makes things easier for transport-helpers that
support multiple ref selections (like my remote-hg). Maybe I should
add code that does this if certain feature is specified (so it doesn't
break other helpers)

Never mind this, it's possible to do the same by assuming that all the
imports will be together, and finished by a line feed, so the code can
do:

if import
  do import-begin stuff
  while import
    import stuff
  do import-end stuff

Of course, this could break if for some reason transport-helper
changes, but that seems unlikely.

But at least on my tests, even with 'feature done' the crash is not
detected properly, either by the transport-helper, or fast-import.

Never mind this either; I was forcing the error before exporting that
feature. See the code at the end.

And also, the msysgit branch does the same check for fast-export,
which actually uses the 'done' feature always, so it should work fine,
but perhaps because of the strange issue with fast-import I just
mentioned, it's not actually detected. I should add tests for this
too.

I have added tests for this, and the failure is detected reliably...
but only with remote-testgit, not with my remote-hg, and I've no idea
what is different.

I've tried everything, and yet a SIGPIPE is detected only with
remote-testgit, not with my code, and they both exit the same way, and
at the same time, and fast-export exits the main function (apparently
a process can finish with SIGPIPE after main?)

I have no idea what's going on, so I don't know if we need any extra
code in transport-helper at all.

Any ideas?

Here is what I have so far:

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index 5f3ebd2..b8707e6 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py

@@ -159,6 +159,11 @@ def do_import(repo, args):
         ref = line[7:].strip()
         refs.append(ref)

+    print "feature done"
+
+    if os.getenv("GIT_REMOTE_TESTGIT_FAILURE"):
+        die('Told to fail')
+
     repo = update_local_repo(repo)
     repo.exporter.export_repo(repo.gitdir, refs)

@@ -172,6 +177,9 @@ def do_export(repo, args):
     if not repo.gitdir:
         die("Need gitdir to export")

+    if os.getenv("GIT_REMOTE_TESTGIT_FAILURE"):
+        die('Told to fail')
+
     update_local_repo(repo)
     changed = repo.importer.do_import(repo.gitdir)

diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index e7dc668..00b0cd3 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh

@@ -145,4 +145,16 @@ test_expect_failure 'push new branch with old:new

refspec' '
 	compare_refs clone HEAD server refs/heads/new-refspec
 '

+test_expect_success 'proper failure checks for fetching' '
+	export GIT_REMOTE_TESTGIT_FAILURE=1 &&
+	(cd localclone && ! git fetch) 2> errors &&
+	grep -q "Error while running fast-import" errors
+'
+
+test_expect_success 'proper failure checks for pushing' '
+	export GIT_REMOTE_TESTGIT_FAILURE=1 &&
+	(cd localclone && ! git push --all) 2> errors &&
+	grep -q "Error while running fast-export" errors
+'
+
 test_done

-- 
Felipe Contreras

On Mon, Oct 22, 2012 at 7:12 PM, Felipe Contreras
[off-list ref] wrote:

On Mon, Oct 22, 2012 at 4:31 PM, Felipe Contreras

I've tried everything, and yet a SIGPIPE is detected only with
remote-testgit, not with my code, and they both exit the same way, and
at the same time, and fast-export exits the main function (apparently
a process can finish with SIGPIPE after main?)

I have no idea what's going on, so I don't know if we need any extra
code in transport-helper at all.

Any ideas?

Must be a timing issue:

sh -c 'echo hello' | sh -c 'exit 1' -> no signal
sh -c 'echo hello' | /usr/bin/false -> SIGPIPE

I can trigger it by adding an extra delay:

This works:

test_expect_success 'proper failure checks for pushing 1' '
	export GIT_REMOTE_TESTGIT_FAILURE=1 &&
	(cd localclone && ! git push --all) 2> errors &&
	grep -q "Error while running fast-export" errors
'

This doesn't:

test_expect_success 'proper failure checks for pushing 2' '
	export GIT_REMOTE_TESTGIT_FAILURE=1 &&
	export GIT_REMOTE_TESTGIT_SLEEPY=1 &&
	(cd localclone && ! git push --all) 2> errors &&
	grep -q "Error while running fast-export" errors
'

This does:

test_expect_success 'proper failure checks for pushing 3' '
	export GIT_REMOTE_TESTGIT_FAILURE=1 &&
	export GIT_REMOTE_TESTGIT_SLEEPY=1 &&
	(cd localclone && ! git push --all) 2> errors &&
	grep -q "Told to fail" errors
'

So, depending on your luck, transport-helper might or might display an
error, it will exit at the right place nonetheless, because of:

	if (strbuf_getline(buffer, helper, '\n') == EOF) {
		if (debug)
			fprintf(stderr, "Debug: Remote helper quit.\n");
		exit(128);
	}

Not ideal, but I guess it's not a big deal.

Cheers.

-- 
Felipe Contreras