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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Oct 22, 2012, 14:31 UTC
Message-ID: <CAMP44s1m9eVqqrgJFuWOBa3DCZAzAqpVwG8Nxn-6MbXWbF_2fw@mail.gmail.com>
In-Reply-To: <50854E20.1040303@viscovery.net>

On Mon, Oct 22, 2012 at 3:46 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:

Show 10 quoted lines

> 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.

Show 11 quoted lines

> 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