From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 08 Dec 2009 23:38:41 GMT
Subject: Re: [REROLL PATCH 6/8] Support remote helpers implementing smart transports
Message-ID: <7vein5594u.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1260278177-9029-7-git-send-email-ilari.liusvaara@elisanet.fi>

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:

> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
> ---
>  Documentation/git-remote-helpers.txt |   25 +++++++-
>  transport-helper.c                   |  126 ++++++++++++++++++++++++++++++++--
>  2 files changed, 144 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
> index 20a05fe..b957813 100644
> --- a/Documentation/git-remote-helpers.txt
> +++ b/Documentation/git-remote-helpers.txt
> @@ -93,6 +93,20 @@ Supported if the helper has the "push" capability.
>  +
>  Supported if the helper has the "import" capability.
>  
> +'connect' <service>::
> +	Connects to given service. Stdin and stdout of helper are

A minor point, but in prose, unless it explains how to use "stdin" and
"stdout" as a variable, keyword, etc. in code, I'd prefer to see these
spelled out, like so:

	The standard input and output of helpers are connected to ...

> @@ -34,12 +36,12 @@ static void sendline(struct helper_data *helper, struct strbuf *buffer)
>  		die_errno("Full write to remote helper failed");
>  }
>  
> -static int recvline(struct helper_data *helper, struct strbuf *buffer)
> +static int _recvline(FILE *helper, struct strbuf *buffer)
>  {

Not a good naming for two reasons.

 - Somebody new to the code wouldn't be able to tell which of recvline()
   and _recvline() take helper_data and FILE easily.

 - A function name that starts with an underscore, even if it is file
   scoped static, is something we tend to avoid.  This applies to
   _process_connect() in the earlier patch as well.

recvline_fh() vs revline() might be better as most of the interaction in
this layer are done on helper_data, which makes the name recvline() pair
nicely with sendline that also takes helper_data; and the oddball one that
is _not_ an implementation detail (i.e. you have calls outside recvline()
implementation that call _recvline()) hints that it takes filehandle
instead.