Felipe Contreras <felipe.contreras@xxxxxxxxx> writes: > On Sun, Apr 7, 2013 at 9:33 PM, Jeff King <peff@xxxxxxxx> 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. -- 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