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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Nov 20, 2017, 22:09 UTC
Message-ID: <20171120220950.f5ipri354x6squck@aiede.mtv.corp.google.com>
In-Reply-To: <20171120215404.GC92506@google.com>

Hi,

Brandon Williams wrote:

> On 11/20, Jonathan Nieder wrote:

[long stream of quoted context snipped; please cut down the quoted
 text to what you are replying to in the future]

Show 17 quoted lines

>> @@ -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?

Sorry about that. It was to make the code easier to read, but I can see how it's jarring during review. I can resend without the removed blank lines if you like.

For context, here's the code in question after the current patch:

	if (protocol == PROTO_SSH) {
		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);
		if (flags & CONNECT_DIAG_URL) {
			printf("Diag: url=%s\n", url ? url : "NULL");
			printf("Diag: protocol=%s\n", prot_name(protocol));
			printf("Diag: userandhost=%s\n", ssh_host ? ssh_host : "NULL");
			printf("Diag: port=%s\n", port ? port : "NONE");
			printf("Diag: path=%s\n", path ? path : "NULL");

			free(hostandport);
			free(path);
			free(conn);
			strbuf_release(&cmd);
			return NULL;
		}
		fill_ssh_args(conn, ssh_host, port, flags);
	} else {
		transport_check_allowed("file");
		if (get_protocol_version_config() > 0) {
			argv_array_pushf(&conn->env_array, GIT_PROTOCOL_ENVIRONMENT "=version=%d",
					 get_protocol_version_config());
		}
	}

Jonathan