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

[Mike Hommey]

On Fri, May 20, 2016 at 03:20:23PM -0700, Junio C Hamano wrote:
> Mike Hommey  writes:
> 
> >> 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?
> 
> No.
> 
> I was merely thinking aloud to see if "in a case that never can
> happen" is sufficient decsription.  I think it is ;-)
> 
> >> This looks strange
> > 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.
> 
> Doesn't that indicate that this codepath deserves some in-code
> comment?

What would be the usual way you'd do this? separate patch? or just doing
it in one of the patches that touches the surrounding code?

Mike
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Mike Hommey  writes:

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

No.

I was merely thinking aloud to see if "in a case that never can
happen" is sufficient decsription.  I think it is ;-)

>> This looks strange
> 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.

Doesn't that indicate that this codepath deserves some in-code
comment?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mike Hommey]

On Fri, May 20, 2016 at 01:58:26PM -0700, Junio C Hamano wrote:
> Mike Hommey  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 "" 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?

> >  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, , );
> > +   host = xstrdup(hostandport);
> > +   get_host_and_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(_host, );
> >  
> > 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 : where  is a decimal
> integer between 0 and 65535.  It could have been ":" 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Mike Hommey  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 "" 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.

>  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, , );
> + host = xstrdup(hostandport);
> + get_host_and_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(_host, );
>  
>   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 : where  is a decimal
integer between 0 and 65535.  It could have been ":" 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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html