Re: [PATCH] remote-curl: do not complain on EOF from parent git

2014-07-09 Thread Jeff King
On Wed, Jul 09, 2014 at 09:25:18PM +, Keller, Jacob E wrote:

> On Wed, 2014-07-09 at 17:20 -0400, Jeff King wrote:
> > The parent git process is supposed to send us an empty line
> > to indicate that the conversation is over. However, the
> > parent process may die() if there is a problem with the
> > operaiton (e.g., we try to fetch a ref that does not exist). 
> 
> Nitpick, but you probably meant operation here.

I did (and I probably meant to turn on spellcheck, too...).

I'll repost in a minute, as I have some follow-on patches. Thanks.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] remote-curl: do not complain on EOF from parent git

2014-07-09 Thread Keller, Jacob E
On Wed, 2014-07-09 at 17:20 -0400, Jeff King wrote:
> The parent git process is supposed to send us an empty line
> to indicate that the conversation is over. However, the
> parent process may die() if there is a problem with the
> operaiton (e.g., we try to fetch a ref that does not exist). 

Nitpick, but you probably meant operation here.

Thanks,
Jake

> In this case, it produces a useful message, but then
> remote-curl _also_ produces an unhelpful message:
> 
>   $ git pull origin matser
>   fatal: couldn't find remote ref matser
>   Unexpected end of command stream
> 
> The "right" way to fix this is to teach the parent git to
> always cleanly close the connection to the helper, letting
> it know that we are done. Implementing that is rather
> clunky, though, as it would involve either replacing die()
> operations with returning errors up the stack (until we
> disconnect the transport), or adding an atexit handler to
> clean up any transport helpers left open.
> 
> It's much simpler to just suppress the EOF message in
> remote-curl. It was not added to address any real-world
> situation in the first place, but rather a "we should
> probably report unexpected things" suggestion[1].
> 
> It is the parent git which drives the operation, and whose
> exit value actually matters. If the parent dies, then the
> helper has no need to complain (except as a debugging aid).
> In the off chance that the pipe is closed without the parent
> dying, the parent can still notice the non-zero exit code.
> 
> [1] http://article.gmane.org/gmane.comp.version-control.git/176036
> 
> Reported-by: Dmitry 
> Signed-off-by: Jeff King 
> ---
> The original discussion that led to this code being implemented was due
> to us checking the helper's exit code in the first place. However, we
> seem to be inconsistent about doing so. I'm not inclined to pursue it
> further, though, as these subtle details of the transport helper code
> usually turn into a can of worms, and more importantly, I don't think it
> hurts anything in the real world. Either the parent git gets the
> expected protocol output from the helper or it doesn't, and we report
> errors on that. An error from a helper after the operation completes is
> not really important to the parent git either way.
> 
>  remote-curl.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/remote-curl.c b/remote-curl.c
> index 4493b38..0454ffc 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -971,8 +971,6 @@ int main(int argc, const char **argv)
>   if (strbuf_getline(&buf, stdin, '\n') == EOF) {
>   if (ferror(stdin))
>   fprintf(stderr, "Error reading command 
> stream\n");
> - else
> - fprintf(stderr, "Unexpected end of command 
> stream\n");
>   return 1;
>   }
>   if (buf.len == 0)