Re: [PATCH 3/8] connect: split git:// setup into a separate function

[Brandon Williams <bmwill@google.com>]

On 11/20, Jonathan Nieder wrote:
> The git_connect function is growing long.  Split the
> PROTO_GIT-specific portion to a separate function to make it easier to
> read.
> 
> No functional change intended.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Reviewed-by: Stefan Beller <sbeller@google.com>
> ---
> As before.
> 
>  connect.c | 103 +++++++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 59 insertions(+), 44 deletions(-)
> 
> diff --git a/connect.c b/connect.c
> index aa994d1518..9425229206 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -861,6 +861,64 @@ static enum ssh_variant determine_ssh_variant(const char *ssh_command,
>  	return ssh_variant;
>  }
>  
> +/*
> + * Open a connection using Git's native protocol.
> + *
> + * The caller is responsible for freeing hostandport, but this function may
> + * modify it (for example, to truncate it to remove the port part).
> + */
> +static struct child_process *git_connect_git(int fd[2], char *hostandport,
> +					     const char *path, const char *prog,
> +					     int flags)
> +{
> +	struct child_process *conn;
> +	struct strbuf request = STRBUF_INIT;
> +	/*
> +	 * Set up virtual host information based on where we will
> +	 * connect, unless the user has overridden us in
> +	 * the environment.
> +	 */
> +	char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
> +	if (target_host)
> +		target_host = xstrdup(target_host);
> +	else
> +		target_host = xstrdup(hostandport);
> +
> +	transport_check_allowed("git");
> +
> +	/* These underlying connection commands die() if they
> +	 * cannot connect.
> +	 */

I know this is really just code motion but maybe we can fix the style of
the comment here?

> +	if (git_use_proxy(hostandport))
> +		conn = git_proxy_connect(fd, hostandport);
> +	else
> +		conn = git_tcp_connect(fd, hostandport, flags);
> +	/*
> +	 * Separate original protocol components prog and path
> +	 * from extended host header with a NUL byte.
> +	 *
> +	 * Note: Do not add any other headers here!  Doing so
> +	 * will cause older git-daemon servers to crash.
> +	 */
> +	strbuf_addf(&request,
> +		    "%s %s%chost=%s%c",
> +		    prog, path, 0,
> +		    target_host, 0);
> +
> +	/* If using a new version put that stuff here after a second null byte */
> +	if (get_protocol_version_config() > 0) {
> +		strbuf_addch(&request, '\0');
> +		strbuf_addf(&request, "version=%d%c",
> +			    get_protocol_version_config(), '\0');
> +	}
> +
> +	packet_write(fd[1], request.buf, request.len);
> +
> +	free(target_host);
> +	strbuf_release(&request);
> +	return conn;
> +}
> +
>  /*
>   * This returns the dummy child_process `no_fork` if the transport protocol
>   * does not need fork(2), or a struct child_process object if it does.  Once
> @@ -892,50 +950,7 @@ struct child_process *git_connect(int fd[2], const char *url,
>  		printf("Diag: path=%s\n", path ? path : "NULL");
>  		conn = NULL;
>  	} else if (protocol == PROTO_GIT) {
> -		struct strbuf request = STRBUF_INIT;
> -		/*
> -		 * Set up virtual host information based on where we will
> -		 * connect, unless the user has overridden us in
> -		 * the environment.
> -		 */
> -		char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
> -		if (target_host)
> -			target_host = xstrdup(target_host);
> -		else
> -			target_host = xstrdup(hostandport);
> -
> -		transport_check_allowed("git");
> -
> -		/* These underlying connection commands die() if they
> -		 * cannot connect.
> -		 */
> -		if (git_use_proxy(hostandport))
> -			conn = git_proxy_connect(fd, hostandport);
> -		else
> -			conn = git_tcp_connect(fd, hostandport, flags);
> -		/*
> -		 * Separate original protocol components prog and path
> -		 * from extended host header with a NUL byte.
> -		 *
> -		 * Note: Do not add any other headers here!  Doing so
> -		 * will cause older git-daemon servers to crash.
> -		 */
> -		strbuf_addf(&request,
> -			    "%s %s%chost=%s%c",
> -			    prog, path, 0,
> -			    target_host, 0);
> -
> -		/* If using a new version put that stuff here after a second null byte */
> -		if (get_protocol_version_config() > 0) {
> -			strbuf_addch(&request, '\0');
> -			strbuf_addf(&request, "version=%d%c",
> -				    get_protocol_version_config(), '\0');
> -		}
> -
> -		packet_write(fd[1], request.buf, request.len);
> -
> -		free(target_host);
> -		strbuf_release(&request);
> +		conn = git_connect_git(fd, hostandport, path, prog, flags);
>  	} else {
>  		struct strbuf cmd = STRBUF_INIT;
>  		const char *const *var;
> -- 
> 2.15.0.448.gf294e3d99a
> 

-- 
Brandon Williams