Re: [RFC/PATCH 3/5] connect.c: connect to a remote service with some flags

From: Torsten Bögershausen <tboegi@web.de>
Date: Feb 28, 2015, 11:11 UTC
Message-ID: <54F1A272.80401@web.de>
In-Reply-To: <1425085318-30537-4-git-send-email-sbeller@google.com>

On 2015-02-28 02.01, Stefan Beller wrote:

Show 18 quoted lines

> If this is over git protocol, the flags is appended as the next
> parameter after host=. If it's ssh, a new argument is appended to the
> command line.
> 
> None of the callers use this now though.
> 
> [sb: originally by pclouds, rebased as jk implemented 1823bea10,
> (git_connect: use argv_array), so any error is mine]
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/fetch-pack.c |  2 +-
>  builtin/send-pack.c  |  2 +-
>  connect.c            | 18 ++++++++++++------
>  connect.h            |  2 +-
>  transport.c          |  3 ++-
>  5 files changed, 17 insertions(+), 10 deletions(-)
>

[]

Show 11 quoted lines

> diff --git a/connect.c b/connect.c
> index 062e133..7b6b241 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -650,7 +650,9 @@ static struct child_process no_fork = CHILD_PROCESS_INIT;
>   * the connection failed).
>   */
>  struct child_process *git_connect(int fd[2], const char *url,
> -				  const char *prog, int flags)
> +				  const char *prog,
> +				  const char *service_flags,

The name "service_flags" reminds me on a service: We connect to a service, but we are not a service.

And "flags" is often used for different bits, collected in an int, but not a "string"

Options are used at the command line, or arguments. How about "extra_arg", or "extra_option", or "option_str", or simply "options" (or "option") ?

Show 19 quoted lines

> +				  int flags)
>  {
>  	char *hostandport, *path;
>  	struct child_process *conn = &no_fork;
> @@ -685,10 +687,13 @@ struct child_process *git_connect(int fd[2], const char *url,
>  		 * Note: Do not add any other headers here!  Doing so
>  		 * will cause older git-daemon servers to crash.
>  		 */
> -		packet_write(fd[1],
> -			     "%s %s%chost=%s%c",
> -			     prog, path, 0,
> -			     target_host, 0);
> +		if (!service_flags)
> +			packet_write(fd[1], "%s %s%chost=%s%c",
> +				     prog, path, 0, target_host, 0);
> +		else
> +			packet_write(fd[1], "%s %s%chost=%s%c%s%c",
> +				     prog, path, 0, target_host, 0,
> +				     service_flags, 0);

We don't need this "big if" here, a simple "?" will do:
 
			packet_write(fd[1], "%s %s%chost=%s%c%s%c",
				     prog, path, 0, target_host, 0,
				     options ? options : "", /* old service_flasgs */ 
                                     0);