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

[Junio C Hamano <gitster@pobox.com>]

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.

Reviewers will see the same patch with fresh eyes after 6 months and
will notice different issues. That is not being unhelpful by
deliberately withholding a crucial point in the initial round of the
review.

I would not be surprised if one ends up repeating oneself in
multiple review threads; the log message of a rerolled patch is a
better place to avoid the need for the repetition.

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

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.