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

[Felipe Contreras <felipe.contreras@gmail.com>]

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