Re: [PATCH 4/8] connect: split ssh command line options into separate function

[Brandon Williams <bmwill@google.com>]

On 11/20, Jonathan Nieder wrote:
> The git_connect function is growing long.  Split the portion that
> discovers an ssh command and options it accepts before the service
> name and path 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 | 116 +++++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 61 insertions(+), 55 deletions(-)
> 
> diff --git a/connect.c b/connect.c
> index 9425229206..a9dc493db2 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -919,6 +919,65 @@ static struct child_process *git_connect_git(int fd[2], char *hostandport,
>  	return conn;
>  }
>  
> +/* Prepare a child_process for use by Git's SSH-tunneled transport. */
> +static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
> +			  const char *port, int flags)
> +{
> +	const char *ssh;
> +	enum ssh_variant variant;
> +
> +	if (looks_like_command_line_option(ssh_host))
> +		die("strange hostname '%s' blocked", ssh_host);
> +
> +	ssh = get_ssh_command();
> +	if (ssh) {
> +		variant = determine_ssh_variant(ssh, 1);
> +	} else {
> +		/*
> +		 * GIT_SSH is the no-shell version of
> +		 * GIT_SSH_COMMAND (and must remain so for
> +		 * historical compatibility).
> +		 */
> +		conn->use_shell = 0;
> +
> +		ssh = getenv("GIT_SSH");
> +		if (!ssh)
> +			ssh = "ssh";
> +		variant = determine_ssh_variant(ssh, 0);
> +	}
> +
> +	argv_array_push(&conn->args, ssh);
> +
> +	if (variant == VARIANT_SSH &&
> +	    get_protocol_version_config() > 0) {
> +		argv_array_push(&conn->args, "-o");
> +		argv_array_push(&conn->args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT);
> +		argv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
> +				 get_protocol_version_config());
> +	}
> +
> +	if (variant != VARIANT_SIMPLE) {
> +		if (flags & CONNECT_IPV4)
> +			argv_array_push(&conn->args, "-4");
> +		else if (flags & CONNECT_IPV6)
> +			argv_array_push(&conn->args, "-6");
> +	}
> +
> +	if (variant == VARIANT_TORTOISEPLINK)
> +		argv_array_push(&conn->args, "-batch");
> +
> +	if (port && variant != VARIANT_SIMPLE) {
> +		if (variant == VARIANT_SSH)
> +			argv_array_push(&conn->args, "-p");
> +		else
> +			argv_array_push(&conn->args, "-P");
> +
> +		argv_array_push(&conn->args, port);
> +	}
> +
> +	argv_array_push(&conn->args, ssh_host);
> +}
> +
>  /*
>   * 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
> @@ -972,16 +1031,13 @@ struct child_process *git_connect(int fd[2], const char *url,
>  		conn->use_shell = 1;
>  		conn->in = conn->out = -1;
>  		if (protocol == PROTO_SSH) {
> -			const char *ssh;
> -			enum ssh_variant variant;
>  			char *ssh_host = hostandport;
>  			const char *port = NULL;
> +
>  			transport_check_allowed("ssh");
>  			get_host_and_port(&ssh_host, &port);
> -
>  			if (!port)
>  				port = get_port(ssh_host);
> -

Are these random additions and deletions intentional?

>  			if (flags & CONNECT_DIAG_URL) {
>  				printf("Diag: url=%s\n", url ? url : "NULL");
>  				printf("Diag: protocol=%s\n", prot_name(protocol));
> @@ -995,57 +1051,7 @@ struct child_process *git_connect(int fd[2], const char *url,
>  				strbuf_release(&cmd);
>  				return NULL;
>  			}
> -
> -			if (looks_like_command_line_option(ssh_host))
> -				die("strange hostname '%s' blocked", ssh_host);
> -
> -			ssh = get_ssh_command();
> -			if (ssh) {
> -				variant = determine_ssh_variant(ssh, 1);
> -			} else {
> -				/*
> -				 * GIT_SSH is the no-shell version of
> -				 * GIT_SSH_COMMAND (and must remain so for
> -				 * historical compatibility).
> -				 */
> -				conn->use_shell = 0;
> -
> -				ssh = getenv("GIT_SSH");
> -				if (!ssh)
> -					ssh = "ssh";
> -				variant = determine_ssh_variant(ssh, 0);
> -			}
> -
> -			argv_array_push(&conn->args, ssh);
> -
> -			if (variant == VARIANT_SSH &&
> -			    get_protocol_version_config() > 0) {
> -				argv_array_push(&conn->args, "-o");
> -				argv_array_push(&conn->args, "SendEnv=" GIT_PROTOCOL_ENVIRONMENT);
> -				argv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
> -						 get_protocol_version_config());
> -			}
> -
> -			if (variant != VARIANT_SIMPLE) {
> -				if (flags & CONNECT_IPV4)
> -					argv_array_push(&conn->args, "-4");
> -				else if (flags & CONNECT_IPV6)
> -					argv_array_push(&conn->args, "-6");
> -			}
> -
> -			if (variant == VARIANT_TORTOISEPLINK)
> -				argv_array_push(&conn->args, "-batch");
> -
> -			if (port && variant != VARIANT_SIMPLE) {
> -				if (variant == VARIANT_SSH)
> -					argv_array_push(&conn->args, "-p");
> -				else
> -					argv_array_push(&conn->args, "-P");
> -
> -				argv_array_push(&conn->args, port);
> -			}
> -
> -			argv_array_push(&conn->args, ssh_host);
> +			fill_ssh_args(conn, ssh_host, port, flags);
>  		} else {
>  			transport_check_allowed("file");
>  			if (get_protocol_version_config() > 0) {
> -- 
> 2.15.0.448.gf294e3d99a
> 

-- 
Brandon Williams