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

From: Brandon Williams <bmwill@google.com>
Date: Nov 20, 2017, 22:28 UTC
Message-ID: <20171120222820.GE92506@google.com>
In-Reply-To: <20171120220950.f5ipri354x6squck@aiede.mtv.corp.google.com>

On 11/20, Jonathan Nieder wrote:

Show 27 quoted lines

> 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]
> >> @@ -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.

If its for better code readability I'm all for the additions and deletions. I just wanted to make sure they were intentional :)

So I'm fine with either the original or the modified patch.

Show 34 quoted lines

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

-- 
Brandon Williams