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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 8, 2013, 17:43 UTC
Message-ID: <7vtxngyl30.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAMP44s1cphJ5wojt2OeZjWJVOi5AMQJR2jN_BCkQw0B0BxpUfA@mail.gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> writes:

Show 12 quoted lines

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

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

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.