On Mon, Apr 1, 2013 at 5:33 PM, Jeff King <peff@xxxxxxxx> wrote:
> On Mon, Apr 01, 2013 at 03:46:42PM -0600, Felipe Contreras wrote:
>
>> Otherwise transport-helper will continue checking for refs and other
>> things what will confuse the user more.
>> [...]
>> diff --git a/transport-helper.c b/transport-helper.c
>> index cb3ef7d..dfdfa7a 100644
>> --- a/transport-helper.c
>> +++ b/transport-helper.c
>> @@ -460,6 +460,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");
>> +
>> argv_array_free_detached(fastimport.argv);
>
> Can you be more specific about what happens when we miss the death here,
> what happens next, etc?
I have seen problems sporadically, like git trying to update refs to
object that don't exist. I also remember seeing mismatches between the
marks and the remote branches refs.
> Checking asynchronously for death like this is subject to a rac
> condition; the helper may be about to die but not have died yet. In
> practice we may catch some cases, but this seems like an indication that
> the protocol is not well thought-out. Usually we would wait for a
> confirmation over the read pipe from a child, and know that the child
> failed when either:
>
> 1. It tells us so on the pipe.
>
> 2. The pipe closes (at which point we know it is time to reap the
> child).
>
> Why doesn't that scheme work here? I am not doubting you that it does
> not; the import helper protocol is a bit of a mess, and I can easily
> believe it has such a problem. But I'm wondering if it's possible to
> improve it in a more robust way.
The pipe is between fast-export and the remote-helper, "we"
(transport-helper) are not part of the pipe any more. That's the
problem.
Cheers.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html