Re: [PATCH v3 2/2] transport-helper: check if remote helper is alive

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Apr 8, 2013, 18:31 UTC
Message-ID: <CAMP44s2PJGJQBM86CGsA5BOn1RLi4mqNsq+d0oyY0jA=XHQGDA@mail.gmail.com>
In-Reply-To: <7vtxngyl30.fsf@alter.siamese.dyndns.org>

On Mon, Apr 8, 2013 at 12:43 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 23 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Sun, Apr 7, 2013 at 9:33 PM, Jeff King <peff@peff.net> wrote:
>>> On Sun, Apr 07, 2013 at 09:03:25PM -0500, Felipe Contreras wrote:
>>
>>>> And you think that is desirable? User-friendly?
>>>
>>> No, as you probably realized if you read the rest of my email. My point
>>> in bringing it up was to try to explain exactly what is going on in each
>>> case. Your commit message provided zero explanation of what the current
>>> problem is, nor why your fix is the right thing to do.
>>
>> I have explained this many times, I wonder why when the patch is close
>> to be merged does this become important, and not before.
>
> There are at least a few reasons I can think of off the top of my
> head.
>
> Reviewers have limited time allocated for each individual topic.  To
> an original patch with say 3 problems, the review may only point out
> issues in 2 and suggest a concrete improvement for only 1 and that
> is sufficient to suggest a reroll.  That is not being unhelpful by
> deliberately withholding 1 and half reviews in the initial round.

I'm not talking about the time it took to come up with the criticism below, I'm talking about the comment regarding the commit message. If the commit message did indeed provide *zero* explanation, that's certainly something that should be immediately visible, no? It could have been mentioned six months ago.

Show 24 quoted lines

>>>> That's a comprehensive essay, unfortunately, it's not correct. The
>>>> exit status of the remote-helper is not important, it's the one of
>>>> fast-import that we really care about.
>>>
>>> Nowhere does it say that we should not check fast-import's exit value,
>>> and indeed, the patch does not replace that check at all. It comes
>>> immediately after. It replaces the WNOHANG check of the helper's exit
>>> code (i.e., check_command) with a synchronous check.
>>
>> Yeah, and in the process it's changing the design of transport-helper,
>> where the pipes are closed only at the very end.
>
> I agree that the disconnection here closes the pipes a lot earlier
> than we used to.  But I am not sure what the practical consequences
> of doing so are.  We know the fast-import process has successfully
> read everything from the helper (we called finish_command() on it).
> We expect at this point the helper is still running or successfully
> exited (the other alternative, the helper died with non-zero status,
> is an error check_command() patch wanted to detect).  But if we keep
> helper running, who will be communicating with it via these open
> pipes?  The process that is calling finish_command() on fast-import
> and disconnecting from the helper won't be, as read/write to the
> pipe, even if we do not disconnect from here, will result in errors
> if the helper has already exited at this point.

Nobody will send any further input, but in theory we could redirect the pipe and send more commands. That's how it was designed.

And in fact, I'm thinking doing exactly that, so we can send another command to fetch the foreign commit ids and append notes with them.

> What I am trying to get at is if "changing the design", which I
> agree is a change, is an improvement, or a backward incompatible
> possible breakage.

It's an improvement I guess, but it's "changing the design" only in one part of the code, while the rest follows a different design.

-- Felipe Contreras