[PATCH] transport-helper: check when helpers fail

[PATCH] transport-helper: check when helpers fail

[Felipe Contreras]

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

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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

Re: [PATCH] transport-helper: check when helpers fail

[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.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  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')
> +

Fun.

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

It is nice to have a separate helper that would theoretically be
useful by anybody who runs a child process, but I have to wonder:

 - How other codepaths that run child process check their results?
   Do they ignore the result altogether?  Do they check the results
   in an ad-hoc way without a good reason?  Do they check the
   results differently because their error handling need to be
   different?

   With this, I am not requesting to port these other codepaths to
   use this helper in this patch series.  But designing the helper
   with potential others' use in mind is within the scope of this
   patch.

 - How does the caller use the return value from this helper?  I can
   see that "zero is success, non-zero is failure", unless there is
   a platform what defines a signum 0 for some signal that can kill
   the child process.  But I am not sure if the caller can tell more
   than that from the return value ("did it die with a signal, or
   did it exit with non-zero status"?)

   It looks to me that this helper loses information; does it belong
   to the public part of API in run-command.c?

> 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

Please be nicer to people who come *after* you are done with this
change.  Forcing failure will propagate to any new test added after
this piece with this patch.

Perhaps like this:

        test_expect_success 'proper failure checks' '
		(
	                GIT_REMOTE_TESTGIT_FAILURE=1 &&
	                export GIT_REMOTE_TESTGIT_FAILURE &&
	                test_must_fail git clone "testgit::$PWD/server" failure
		) 2>errors &&
                grep "Error while running helper" errors
        '

Points to note:

 - The environment does not propagate outside this test this way.

 - Avoid "export VAR=VAL" to help other people's shell; cf. 69ae92b
   (shell portability: no "export VAR=VAL", 2010-10-13).

 - Detect uncontrolled exit of "git clone" by using test_must_fail.

 - Avoid "grep -q"; cf. the latter half of aadbe44 (grep portability
   fix: don't use "-e" or "-q", 2008-03-12).

Thanks.

Re: [PATCH] transport-helper: check when helpers fail

[Felipe Contreras]

On Sun, Oct 21, 2012 at 10:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:

>> +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;
>> +}
>
> It is nice to have a separate helper that would theoretically be
> useful by anybody who runs a child process, but I have to wonder:
>
>  - How other codepaths that run child process check their results?
>    Do they ignore the result altogether?  Do they check the results
>    in an ad-hoc way without a good reason?  Do they check the
>    results differently because their error handling need to be
>    different?

They probably check at the end, truly waiting for the process to
finish, that's not what we want, thus the WNOHANG that apparently
nobody else in git is using.

The reason is that the transport-helpers are special; they are
re-used, so we can't wait for the process to finish after running a
certain command.

>    With this, I am not requesting to port these other codepaths to
>    use this helper in this patch series.  But designing the helper
>    with potential others' use in mind is within the scope of this
>    patch.

That's what I did. If somebody else needs to check the status of the
command mid-stream, they can do that.

>  - How does the caller use the return value from this helper?  I can
>    see that "zero is success, non-zero is failure", unless there is
>    a platform what defines a signum 0 for some signal that can kill
>    the child process.  But I am not sure if the caller can tell more
>    than that from the return value ("did it die with a signal, or
>    did it exit with non-zero status"?)

I don't really care, I'm fine returning -1 on all errors.

>> 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
>
> Please be nicer to people who come *after* you are done with this
> change.  Forcing failure will propagate to any new test added after
> this piece with this patch.
>
> Perhaps like this:
>
>         test_expect_success 'proper failure checks' '
>                 (
>                         GIT_REMOTE_TESTGIT_FAILURE=1 &&
>                         export GIT_REMOTE_TESTGIT_FAILURE &&
>                         test_must_fail git clone "testgit::$PWD/server" failure
>                 ) 2>errors &&
>                 grep "Error while running helper" errors
>         '

LGTM.

-- 
Felipe Contreras

Re: [PATCH] transport-helper: check when helpers fail

[Johannes Sixt]

Am 10/21/2012 21:19, schrieb Felipe Contreras:
> 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;
> +}
> +

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

Re: [PATCH] transport-helper: check when helpers fail

[Felipe Contreras]

On Mon, Oct 22, 2012 at 8:35 AM, Johannes Sixt <j.sixt@viscovery.net> 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

Re: [PATCH] transport-helper: check when helpers fail

[Johannes Sixt]

Am 10/22/2012 13:50, schrieb Felipe Contreras:
> On Mon, Oct 22, 2012 at 8:35 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>> 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

Re: [PATCH] transport-helper: check when helpers fail

[Felipe Contreras]

On Mon, Oct 22, 2012 at 3:46 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Am 10/22/2012 13:50, schrieb Felipe Contreras:
>> On Mon, Oct 22, 2012 at 8:35 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>> 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

Re: [PATCH] transport-helper: check when helpers fail

[Felipe Contreras]

On Mon, Oct 22, 2012 at 4:31 PM, Felipe Contreras
<felipe.contreras@gmail.com> 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

Re: [PATCH] transport-helper: check when helpers fail

[Felipe Contreras]

On Mon, Oct 22, 2012 at 7:12 PM, Felipe Contreras
<felipe.contreras@gmail.com> 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