Re: [PATCH v6 1/9] connect: call get_host_and_port() earlier

From: Junio C Hamano <gitster@pobox.com>
Date: May 20, 2016, 20:58 UTC
Message-ID: <xmqqfutccuq5.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160517013554.22578-2-mh@glandium.org>

Mike Hommey <mh@glandium.org> writes:

Show 10 quoted lines

> Currently, get_host_and_port() is called in git_connect() for the ssh
> protocol, and in git_tcp_connect_sock() for the git protocol. Instead
> of doing this, just call it from a single place, right after
> parse_connect_url(), and pass the host and port separately to
> git_*_connect() functions.
>
> We however preserve hostandport, at least for now.
>
> Note that in git_tcp_connect_sock, the port was set to "<none>" in a
> case that never can happen, so that code path was removed.

Can never happen because?

  !*port means get_host_and_port() made the "port" pointer point at
  a NUL byte.  That does not happen because the only case port is
  moved by that function is to have it point at a byte after we
  found ':', and the "port" string is a decimal integer whose value
  is between 0 and 65535, so there is no way port points at an empty
  string.

OK.

Show 25 quoted lines

>  struct child_process *git_connect(int fd[2], const char *url,
>  				  const char *prog, int flags)
> ...
> @@ -683,6 +681,8 @@ struct child_process *git_connect(int fd[2], const char *url,
>  	signal(SIGCHLD, SIG_DFL);
>  
>  	protocol = parse_connect_url(url, &hostandport, &path);
> +	host = xstrdup(hostandport);
> +	get_host_and_port(&host, &port);
>  	if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
>  		printf("Diag: url=%s\n", url ? url : "NULL");
>  		printf("Diag: protocol=%s\n", prot_name(protocol));
> ...
> @@ -737,22 +737,20 @@ struct child_process *git_connect(int fd[2], const char *url,
>  		if (protocol == PROTO_SSH) {
>  			const char *ssh;
>  			int putty = 0, tortoiseplink = 0;
> -			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);
> +				port = get_port(host);

This looks strange. We asked get_host_and_port() to split hostandport into host and port already, and learned that port is empty, i.e. it wasn't <host>:<port> where <port> is a decimal integer between 0 and 65535. It could have been "<host>:" in which case get_host_and_port() would have turned that colon into NUL.

Would there be a case where this get_port() call does anthing useful?