patchtransport-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 <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(+)

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

Fun.

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?

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.

On Sun, Oct 21, 2012 at 10:33 PM, Junio C Hamano <gitster@pobox.com> wrote:

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.

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

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

LGTM.

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 <j.sixt@viscovery.net> wrote:

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.

Right.

How is that violating the protocol?

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

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 <j.sixt@viscovery.net> wrote:

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.

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

Cheers.

On Mon, Oct 22, 2012 at 4:31 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

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.

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

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:

On Mon, Oct 22, 2012 at 7:12 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

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.