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

From: Mike Hommey <mh@glandium.org>
Date: May 20, 2016, 22:14 UTC
Message-ID: <20160520221430.GA28273@glandium.org>
In-Reply-To: <xmqqfutccuq5.fsf@gitster.mtv.corp.google.com>

On Fri, May 20, 2016 at 01:58:26PM -0700, Junio C Hamano wrote:

Show 23 quoted lines

> Mike Hommey <mh@glandium.org> writes:
> 
> > 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.

Do you want me to add this to the commit message in a possible v7?

Show 34 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?

v3 of this series did remove this get_port(), and broke the '[host:port]:path' syntax as a consequence. The reason this happens is that get_host_and_port, in that case, is called with [host:port], sees the square brackets, and searches the port *after* the closing bracket, because the usual case where square brackets appear is ipv6 addresses, which contain colons, and the brackets in that case are used to separate the host and the port.

In that case, get_host_and_port returns "host:port" and null.

Mike