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

2013-04-08 Thread Junio C Hamano
Jeff King writes: > On Mon, Apr 08, 2013 at 02:08:03PM -0700, Junio C Hamano wrote: > >> OK, so you are envisioning that transport-helper would read from the >> helper after importer is done? If so, perhaps it is a prudent >> solution to disconnect in this version (to fix), and then in a >> sepa

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

2013-04-08 Thread Jeff King
On Mon, Apr 08, 2013 at 02:08:03PM -0700, Junio C Hamano wrote: > OK, so you are envisioning that transport-helper would read from the > helper after importer is done? If so, perhaps it is a prudent > solution to disconnect in this version (to fix), and then in a > separate patch that adds such a

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

2013-04-08 Thread Junio C Hamano
Felipe Contreras writes: > On Mon, Apr 8, 2013 at 1:46 PM, Junio C Hamano wrote: > ... 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

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

2013-04-08 Thread Jeff King
On Mon, Apr 08, 2013 at 09:38:28AM -0500, Felipe Contreras wrote: > > 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.

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

2013-04-08 Thread Felipe Contreras
On Mon, Apr 8, 2013 at 1:46 PM, Junio C Hamano wrote: >>> ... 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,

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

2013-04-08 Thread Jeff King
On Mon, Apr 08, 2013 at 01:31:43PM -0500, Felipe Contreras wrote: > > 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 sufficie

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

2013-04-08 Thread Junio C Hamano
Felipe Contreras writes: > 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?

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

2013-04-08 Thread Felipe Contreras
On Mon, Apr 8, 2013 at 12:43 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> On Sun, Apr 7, 2013 at 9:33 PM, Jeff King 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 realiz

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

2013-04-08 Thread Junio C Hamano
Felipe Contreras writes: > On Sun, Apr 7, 2013 at 9:33 PM, Jeff King 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 u

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

2013-04-08 Thread Felipe Contreras
On Sun, Apr 7, 2013 at 9:33 PM, Jeff King 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 wha

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

2013-04-07 Thread Jeff King
On Sun, Apr 07, 2013 at 09:03:25PM -0500, Felipe Contreras wrote: > > I'm still not very excited about this approach, as it is racy to detect > > errors. E.g., there is nothing to say that the helper does not close the > > pipe to fast-import prematurely and then die afterwards, leaving the > > re

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

2013-04-07 Thread Felipe Contreras
On Sun, Apr 7, 2013 at 7:51 PM, Jeff King wrote: > On Sun, Apr 07, 2013 at 01:45:06AM -0600, Felipe Contreras wrote: > >> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh >> index f387027..efe67e2 100755 >> --- a/t/t5801-remote-helpers.sh >> +++ b/t/t5801-remote-helpers.sh >> @@

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

2013-04-07 Thread Jeff King
On Sun, Apr 07, 2013 at 01:45:06AM -0600, Felipe Contreras wrote: > diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh > index f387027..efe67e2 100755 > --- a/t/t5801-remote-helpers.sh > +++ b/t/t5801-remote-helpers.sh > @@ -166,4 +166,23 @@ test_expect_success 'push ref with exist

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

2013-04-07 Thread Junio C Hamano
Felipe Contreras writes: > Otherwise transport-helper will continue checking for refs and other > things what will confuse the user more. > --- Sign-off? > git-remote-testgit| 11 +++ > t/t5801-remote-helpers.sh | 19 +++ > transport-helper.c| 8 +++

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

2013-04-07 Thread Felipe Contreras
Otherwise transport-helper will continue checking for refs and other things what will confuse the user more. --- git-remote-testgit| 11 +++ t/t5801-remote-helpers.sh | 19 +++ transport-helper.c| 8 3 files changed, 38 insertions(+) diff --git a/