[PATCH v6 0/9] connect: various cleanups

[PATCH v6 0/9] connect: various cleanups

[Mike Hommey]

The main difference here is patch 2/9 now throwing an error in user's face
when they have a core.gitProxy configuration with a port number. There might
be some bikeshedding to do on the content of the error message.

Mike Hommey (9):
  connect: call get_host_and_port() earlier
  connect: only match the host with core.gitProxy
  connect: fill the host header in the git protocol with the host and
    port variables
  connect: make parse_connect_url() return separated host and port
  connect: group CONNECT_DIAG_URL handling code
  connect: make parse_connect_url() return the user part of the url as a
    separate value
  connect: change the --diag-url output to separate user and host
  connect: actively reject git:// urls with a user part
  connect: move ssh command line preparation to a separate function

 connect.c              | 243 +++++++++++++++++++++++++++++--------------------
 t/t5500-fetch-pack.sh  |  42 ++++++---
 t/t5532-fetch-proxy.sh |   8 +-
 3 files changed, 183 insertions(+), 110 deletions(-)

-- 
2.8.2.411.ga570dec.dirty

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

[Mike Hommey]

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.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 connect.c | 47 +++++++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/connect.c b/connect.c
index c53f3f1..d3448c2 100644
--- a/connect.c
+++ b/connect.c
@@ -343,18 +343,16 @@ static const char *ai_name(const struct addrinfo *ai)
 /*
  * Returns a connected socket() fd, or else die()s.
  */
-static int git_tcp_connect_sock(char *host, int flags)
+static int git_tcp_connect_sock(const char *host, const char *port, int flags)
 {
 	struct strbuf error_message = STRBUF_INIT;
 	int sockfd = -1;
-	const char *port = STR(DEFAULT_GIT_PORT);
 	struct addrinfo hints, *ai0, *ai;
 	int gai;
 	int cnt = 0;
 
-	get_host_and_port(&host, &port);
-	if (!*port)
-		port = "<none>";
+	if (!port)
+		port = STR(DEFAULT_GIT_PORT);
 
 	memset(&hints, 0, sizeof(hints));
 	if (flags & CONNECT_IPV4)
@@ -411,11 +409,10 @@ static int git_tcp_connect_sock(char *host, int flags)
 /*
  * Returns a connected socket() fd, or else die()s.
  */
-static int git_tcp_connect_sock(char *host, int flags)
+static int git_tcp_connect_sock(const char *host, const char *port, int flags)
 {
 	struct strbuf error_message = STRBUF_INIT;
 	int sockfd = -1;
-	const char *port = STR(DEFAULT_GIT_PORT);
 	char *ep;
 	struct hostent *he;
 	struct sockaddr_in sa;
@@ -423,7 +420,8 @@ static int git_tcp_connect_sock(char *host, int flags)
 	unsigned int nport;
 	int cnt;
 
-	get_host_and_port(&host, &port);
+	if (!port)
+		port = STR(DEFAULT_GIT_PORT);
 
 	if (flags & CONNECT_VERBOSE)
 		fprintf(stderr, "Looking up %s ... ", host);
@@ -482,9 +480,10 @@ static int git_tcp_connect_sock(char *host, int flags)
 #endif /* NO_IPV6 */
 
 
-static void git_tcp_connect(int fd[2], char *host, int flags)
+static void git_tcp_connect(int fd[2], const char *host, const char *port,
+			    int flags)
 {
-	int sockfd = git_tcp_connect_sock(host, flags);
+	int sockfd = git_tcp_connect_sock(host, port, flags);
 
 	fd[0] = sockfd;
 	fd[1] = dup(sockfd);
@@ -550,18 +549,16 @@ static int git_use_proxy(const char *host)
 	return (git_proxy_command && *git_proxy_command);
 }
 
-static struct child_process *git_proxy_connect(int fd[2], char *host)
+static struct child_process *git_proxy_connect(int fd[2], const char *host,
+					       const char *port)
 {
-	const char *port = STR(DEFAULT_GIT_PORT);
 	struct child_process *proxy;
 
-	get_host_and_port(&host, &port);
-
 	proxy = xmalloc(sizeof(*proxy));
 	child_process_init(proxy);
 	argv_array_push(&proxy->args, git_proxy_command);
 	argv_array_push(&proxy->args, host);
-	argv_array_push(&proxy->args, port);
+	argv_array_push(&proxy->args, port ? port : STR(DEFAULT_GIT_PORT));
 	proxy->in = -1;
 	proxy->out = -1;
 	if (start_command(proxy))
@@ -672,7 +669,8 @@ static struct child_process no_fork = CHILD_PROCESS_INIT;
 struct child_process *git_connect(int fd[2], const char *url,
 				  const char *prog, int flags)
 {
-	char *hostandport, *path;
+	char *hostandport, *path, *host;
+	const char *port = NULL;
 	struct child_process *conn = &no_fork;
 	enum protocol protocol;
 	struct strbuf cmd = STRBUF_INIT;
@@ -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));
@@ -707,9 +707,9 @@ struct child_process *git_connect(int fd[2], const char *url,
 		 * cannot connect.
 		 */
 		if (git_use_proxy(hostandport))
-			conn = git_proxy_connect(fd, hostandport);
+			conn = git_proxy_connect(fd, host, port);
 		else
-			git_tcp_connect(fd, hostandport, flags);
+			git_tcp_connect(fd, host, port, flags);
 		/*
 		 * Separate original protocol components prog and path
 		 * from extended host header with a NUL byte.
@@ -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);
 
 			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: userandhost=%s\n", host ? host : "NULL");
 				printf("Diag: port=%s\n", port ? port : "NONE");
 				printf("Diag: path=%s\n", path ? path : "NULL");
 
 				free(hostandport);
+				free(host);
 				free(path);
 				free(conn);
 				return NULL;
@@ -798,7 +796,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 				argv_array_push(&conn->args, putty ? "-P" : "-p");
 				argv_array_push(&conn->args, port);
 			}
-			argv_array_push(&conn->args, ssh_host);
+			argv_array_push(&conn->args, host);
 		} else {
 			transport_check_allowed("file");
 		}
@@ -812,6 +810,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 		strbuf_release(&cmd);
 	}
 	free(hostandport);
+	free(host);
 	free(path);
 	return conn;
 }
-- 
2.8.2.411.ga570dec.dirty

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

[Junio C Hamano]

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.

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

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

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

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

[Junio C Hamano]

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

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 <mh@glandium.org> 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

[PATCH v6 2/9] connect: only match the host with core.gitProxy

[Mike Hommey]

Currently, core.gitProxy doesn't actually match purely on domain names
as documented: it also matches ports.

So a core.gitProxy value like "script for kernel.org" doesn't make the
script called for an url like git://kernel.org:port/path, while it is
called for git://kernel.org/path.

This per-port behavior seems like an oversight rather than a deliberate
choice, so, make git://kernel.org:port/path call the gitProxy script in
the case described above.

However, if people have been relying on this behavior, git now fails
with an error message inviting for a configuration change.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 connect.c              | 20 +++++++++++++++++---
 t/t5532-fetch-proxy.sh |  8 +++++++-
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/connect.c b/connect.c
index d3448c2..0f48cde 100644
--- a/connect.c
+++ b/connect.c
@@ -490,6 +490,8 @@ static void git_tcp_connect(int fd[2], const char *host, const char *port,
 }
 
 
+static char *get_port(char *host);
+
 static char *git_proxy_command;
 
 static int git_proxy_command_options(const char *var, const char *value,
@@ -517,10 +519,14 @@ static int git_proxy_command_options(const char *var, const char *value,
 			/* matches everybody */
 			matchlen = strlen(value);
 		else {
-			hostlen = strlen(for_pos + 5);
+			struct strbuf host = STRBUF_INIT;
+			char *port;
+			strbuf_addstr(&host, for_pos + 5);
+			port = get_port(host.buf);
+			hostlen = strlen(host.buf);
 			if (rhost_len < hostlen)
 				matchlen = -1;
-			else if (!strncmp(for_pos + 5,
+			else if (!strncmp(host.buf,
 					  rhost_name + rhost_len - hostlen,
 					  hostlen) &&
 				 ((rhost_len == hostlen) ||
@@ -528,6 +534,14 @@ static int git_proxy_command_options(const char *var, const char *value,
 				matchlen = for_pos - value;
 			else
 				matchlen = -1;
+			if (matchlen > 0 && port)
+				die("core.gitProxy only supports \"for "
+				    "host/domain\", not \"for host:port\".\n"
+				    "Please change your configuration "
+				    "accordingly.\nPlease note that the port "
+				    "is given as second argument to the proxy "
+				    "script.\n");
+			strbuf_release(&host);
 		}
 		if (0 <= matchlen) {
 			/* core.gitproxy = none for kernel.org */
@@ -706,7 +720,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 		/* These underlying connection commands die() if they
 		 * cannot connect.
 		 */
-		if (git_use_proxy(hostandport))
+		if (git_use_proxy(host))
 			conn = git_proxy_connect(fd, host, port);
 		else
 			git_tcp_connect(fd, host, port, flags);
diff --git a/t/t5532-fetch-proxy.sh b/t/t5532-fetch-proxy.sh
index 51c9669..efa80f8 100755
--- a/t/t5532-fetch-proxy.sh
+++ b/t/t5532-fetch-proxy.sh
@@ -33,7 +33,9 @@ test_expect_success 'setup proxy script' '
 
 test_expect_success 'setup local repo' '
 	git remote add fake git://example.com/remote &&
-	git config core.gitproxy ./proxy
+	git remote add fake_b git://example.org/remote &&
+	git config core.gitproxy "./proxy for example.com" &&
+	git config --add core.gitproxy "./proxy for example.org:9418"
 '
 
 test_expect_success 'fetch through proxy works' '
@@ -43,4 +45,8 @@ test_expect_success 'fetch through proxy works' '
 	test_cmp expect actual
 '
 
+test_expect_success 'core.gitProxy with port fails' '
+	test_must_fail git fetch fake_b
+'
+
 test_done
-- 
2.8.2.411.ga570dec.dirty

Re: [PATCH v6 2/9] connect: only match the host with core.gitProxy

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> Currently, core.gitProxy doesn't actually match purely on domain names
> as documented: it also matches ports.
>
> So a core.gitProxy value like "script for kernel.org" doesn't make the
> script called for an url like git://kernel.org:port/path, while it is
> called for git://kernel.org/path.

Let's recap.  When you are accessing "git://kernel.org:9418/path"

 - "script1 for kernel.org" does not match, which may be
   counter-intuitive;

 - "script2 for kernel.org:9418" matches and gets used.

Isn't that what the current code does?  If so, I suspect that
allowing the first one to match may be an improvement with slight
backward incompatibility that we agreed that we do not mind.

	Note for those other than Mike: The reason why we do not
	care about the breakage of the backward compatibility is
	because it would only matter when you configured both
	"script1 for kernel.org" and "script2 for kernel.org:9418",
	so that URL can be used to differenciate which proxy script
	to use.  In such a configuration, we used to call script2,
	but "fixing" it would make it call script1.  Given that the
	proxy script takes the port number as its parameter, this
	can be worked around easily by introducing a new script0
	that switches between script1 and script2 based on the port
	number parameter.  IOW, we accept the compatibility breakage
	because adjusting is easy.

We of course need to warn the user if we do this.  That is

 - If the URL we access has :<port>, and we apply <host>-only
   core.gitProxy entry to it, and

 - If the user has core.gitProxy entry for <host>:<port>,

we definitely need to warn so that the user is told that the "easy
adjustment" is necessary.

Thinking aloud a bit more, if the user has "script1 for kernel.org"
without "script2 for kernel.org:9418", and relied on the fact that
having :9418 that is not necessary (as it is the default port) in
the URL lets her bypass the proxy script, the user also needs to be
told that we have changed the rule of the game and her configuration
needs to be updated.  So the above condition to warn would need to
be loosened, i.e. "if the URL being accessed has :<port>, and if we
apply gitProxy specified with only <host>" we need to warn.

A proper transition plan is necessary; if the long term ideal is to
use "script1 for kernel.org" for "git://kernel.org:9418/path", we do
not want to keep giving this warning forever.

And if we need a transition plan _anyway_, we probably should have a
period during which those who have been relying on the fact that
"script2 for kernel.org:9418" was a supported way to specify proxy
for "git://kernel.org:9418/path" would get a warning but will still
use "script2 for kernel.org:9418" as a valid core.gitProxy
specification.

> This per-port behavior seems like an oversight rather than a deliberate
> choice, so, make git://kernel.org:port/path call the gitProxy script in
> the case described above.

So, even if we agree that per-port behaviour is not something we
would use if we were building the system without any existing users
today, I do not think we want "git now fails with an error" at all.
It goes against the approach Git takes to give smooth transtion to
users when we must break backward compatibility.

> However, if people have been relying on this behavior, git now fails
> with an error message inviting for a configuration change.

Re: [PATCH v6 2/9] connect: only match the host with core.gitProxy

[Mike Hommey]

On Fri, May 20, 2016 at 02:48:28PM -0700, Junio C Hamano wrote:
> So, even if we agree that per-port behaviour is not something we
> would use if we were building the system without any existing users
> today, I do not think we want "git now fails with an error" at all.
> It goes against the approach Git takes to give smooth transtion to
> users when we must break backward compatibility.

I don't disagree. I went with a hard fail because it was easier. I'm
not too keen blocking this series on this transition happening. So I'll
try to finish this series without this change, and we can separate this
transition discussion from the rest of the connect.c changes.

Mike

Re: [PATCH v6 2/9] connect: only match the host with core.gitProxy

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> On Fri, May 20, 2016 at 02:48:28PM -0700, Junio C Hamano wrote:
>> So, even if we agree that per-port behaviour is not something we
>> would use if we were building the system without any existing users
>> today, I do not think we want "git now fails with an error" at all.
>> It goes against the approach Git takes to give smooth transtion to
>> users when we must break backward compatibility.
>
> I don't disagree. I went with a hard fail because it was easier. I'm
> not too keen blocking this series on this transition happening. So I'll
> try to finish this series without this change, and we can separate this
> transition discussion from the rest of the connect.c changes.

Yeah, I was thinking about the same thing as a short-term
direction.

Thanks.

[PATCH v6 3/9] connect: fill the host header in the git protocol with the host and port variables

[Mike Hommey]

The last use of the hostandport variable, besides being strdup'ed before
being split into host and port, is to fill the host header in the git
protocol.

Instead of relying on parse_connect_url() to return a host:port string
that makes sense there, construct one from the host and port variables.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 connect.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/connect.c b/connect.c
index 0f48cde..0676ee0 100644
--- a/connect.c
+++ b/connect.c
@@ -709,11 +709,24 @@ struct child_process *git_connect(int fd[2], const char *url,
 		 * connect, unless the user has overridden us in
 		 * the environment.
 		 */
-		char *target_host = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
-		if (target_host)
-			target_host = xstrdup(target_host);
-		else
-			target_host = xstrdup(hostandport);
+		struct strbuf target_host = STRBUF_INIT;
+		char *override_vhost = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
+		if (override_vhost)
+			strbuf_addstr(&target_host, override_vhost);
+		else {
+			/* If the host contains a colon (ipv6 address), it
+			 * needs to be enclosed with square brackets. */
+			const char *colon = strchr(host, ':');
+			if (colon)
+				strbuf_addch(&target_host, '[');
+			strbuf_addstr(&target_host, host);
+			if (colon)
+				strbuf_addch(&target_host, ']');
+			if (port) {
+				strbuf_addch(&target_host, ':');
+				strbuf_addstr(&target_host, port);
+			}
+		}
 
 		transport_check_allowed("git");
 
@@ -734,8 +747,8 @@ struct child_process *git_connect(int fd[2], const char *url,
 		packet_write(fd[1],
 			     "%s %s%chost=%s%c",
 			     prog, path, 0,
-			     target_host, 0);
-		free(target_host);
+			     target_host.buf, 0);
+		strbuf_release(&target_host);
 	} else {
 		conn = xmalloc(sizeof(*conn));
 		child_process_init(conn);
-- 
2.8.2.411.ga570dec.dirty

[PATCH v6 4/9] connect: make parse_connect_url() return separated host and port

[Mike Hommey]

Now that nothing besides CONNECT_DIAG_URL is using hostandport, we can
have parse_connect_url() itself do the host and port splitting.

This still leaves "user@" part of the host, if there is one, which will
be addressed in a subsequent change. This however does add /some/
handling of the "user@" part of the host, in order to pick the port
properly.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 connect.c             | 33 +++++++++++++++++++++------------
 t/t5500-fetch-pack.sh | 32 +++++++++++++++++++++-----------
 2 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/connect.c b/connect.c
index 0676ee0..4ce83f7 100644
--- a/connect.c
+++ b/connect.c
@@ -603,10 +603,11 @@ static char *get_port(char *host)
  * The caller must free() the returned strings.
  */
 static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
-				       char **ret_path)
+				       char **ret_port, char **ret_path)
 {
 	char *url;
 	char *host, *path;
+	const char *port = NULL;
 	char *end;
 	int separator = '/';
 	enum protocol protocol = PROTO_LOCAL;
@@ -661,7 +662,20 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
 	path = xstrdup(path);
 	*end = '\0';
 
+	get_host_and_port(&host, &port);
+
+	if (*host && !port) {
+		/* At this point, the host part may look like user@host:port.
+		 * As the `user` portion tends to be less strict than
+		 * `host:port`, we first put it out of the equation: since a
+		 * hostname cannot contain a '@', we start from the last '@'
+		 * in the string. */
+		char *end_user = strrchr(host, '@');
+		port = get_port(end_user ? end_user : host);
+	}
+
 	*ret_host = xstrdup(host);
+	*ret_port = port ? xstrdup(port) : NULL;
 	*ret_path = path;
 	free(url);
 	return protocol;
@@ -683,8 +697,7 @@ static struct child_process no_fork = CHILD_PROCESS_INIT;
 struct child_process *git_connect(int fd[2], const char *url,
 				  const char *prog, int flags)
 {
-	char *hostandport, *path, *host;
-	const char *port = NULL;
+	char *host, *port, *path;
 	struct child_process *conn = &no_fork;
 	enum protocol protocol;
 	struct strbuf cmd = STRBUF_INIT;
@@ -694,13 +707,12 @@ 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);
+	protocol = parse_connect_url(url, &host, &port, &path);
 	if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
 		printf("Diag: url=%s\n", url ? url : "NULL");
 		printf("Diag: protocol=%s\n", prot_name(protocol));
-		printf("Diag: hostandport=%s\n", hostandport ? hostandport : "NULL");
+		printf("Diag: userandhost=%s\n", host ? host : "NULL");
+		printf("Diag: port=%s\n", port ? port : "NONE");
 		printf("Diag: path=%s\n", path ? path : "NULL");
 		conn = NULL;
 	} else if (protocol == PROTO_GIT) {
@@ -766,9 +778,6 @@ struct child_process *git_connect(int fd[2], const char *url,
 			int putty = 0, tortoiseplink = 0;
 			transport_check_allowed("ssh");
 
-			if (!port)
-				port = get_port(host);
-
 			if (flags & CONNECT_DIAG_URL) {
 				printf("Diag: url=%s\n", url ? url : "NULL");
 				printf("Diag: protocol=%s\n", prot_name(protocol));
@@ -776,8 +785,8 @@ struct child_process *git_connect(int fd[2], const char *url,
 				printf("Diag: port=%s\n", port ? port : "NONE");
 				printf("Diag: path=%s\n", path ? path : "NULL");
 
-				free(hostandport);
 				free(host);
+				free(port);
 				free(path);
 				free(conn);
 				return NULL;
@@ -836,8 +845,8 @@ struct child_process *git_connect(int fd[2], const char *url,
 		fd[1] = conn->in;  /* write to child's stdin */
 		strbuf_release(&cmd);
 	}
-	free(hostandport);
 	free(host);
+	free(port);
 	free(path);
 	return conn;
 }
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 91a69fc..739c6b1 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -553,7 +553,7 @@ check_prot_path () {
 	Diag: protocol=$2
 	Diag: path=$3
 	EOF
-	git fetch-pack --diag-url "$1" | grep -v hostandport= >actual &&
+	git fetch-pack --diag-url "$1" | egrep -v '(host|port)=' >actual &&
 	test_cmp expected actual
 }
 
@@ -562,22 +562,17 @@ check_prot_host_port_path () {
 	case "$2" in
 		*ssh*)
 		pp=ssh
-		uah=userandhost
-		ehost=$(echo $3 | tr -d "[]")
-		diagport="Diag: port=$4"
 		;;
 		*)
-		pp=$p
-		uah=hostandport
-		ehost=$(echo $3$4 | sed -e "s/22$/:22/" -e "s/NONE//")
-		diagport=""
+		pp=$2
 		;;
 	esac
+	ehost=$(echo $3 | tr -d "[]")
 	cat >exp <<-EOF &&
 	Diag: url=$1
 	Diag: protocol=$pp
-	Diag: $uah=$ehost
-	$diagport
+	Diag: userandhost=$ehost
+	Diag: port=$4
 	Diag: path=$5
 	EOF
 	grep -v "^$" exp >expected
@@ -585,7 +580,7 @@ check_prot_host_port_path () {
 	test_cmp expected actual
 }
 
-for r in repo re:po re/po
+for r in repo re:po re/po re@po
 do
 	# git or ssh with scheme
 	for p in "ssh+git" "git+ssh" git ssh
@@ -608,6 +603,9 @@ do
 			test_expect_success "fetch-pack --diag-url $p://$h:22/$r" '
 				check_prot_host_port_path $p://$h:22/$r $p "$h" 22 "/$r"
 			'
+			test_expect_success "fetch-pack --diag-url $p://$h:22/$r" '
+				check_prot_host_port_path $p://$h:22/$r $p "$h" 22 "/$r"
+			'
 		done
 	done
 	# file with scheme
@@ -644,6 +642,18 @@ do
 			check_prot_host_port_path $h:/~$r $p "$h" NONE "~$r"
 		'
 	done
+	#ssh without scheme with port
+	p=ssh
+	for h in host user@host
+	do
+		test_expect_success "fetch-pack --diag-url [$h:22]:$r" '
+			check_prot_host_port_path [$h:22]:$r $p $h 22 "$r"
+		'
+		# Do "/~" -> "~" conversion
+		test_expect_success "fetch-pack --diag-url [$h:22]:/~$r" '
+			check_prot_host_port_path [$h:22]:/~$r $p $h 22 "~$r"
+		'
+	done
 done
 
 test_expect_success MINGW 'fetch-pack --diag-url file://c:/repo' '
-- 
2.8.2.411.ga570dec.dirty

[PATCH v6 5/9] connect: group CONNECT_DIAG_URL handling code

[Mike Hommey]

Previous changes made both branches handling CONNECT_DIAG_URL identical.
We can now remove one of those branches and have CONNECT_DIAG_URL be
handled in one place.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 connect.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/connect.c b/connect.c
index 4ce83f7..3a77b2f 100644
--- a/connect.c
+++ b/connect.c
@@ -708,7 +708,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 	signal(SIGCHLD, SIG_DFL);
 
 	protocol = parse_connect_url(url, &host, &port, &path);
-	if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
+	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", host ? host : "NULL");
@@ -778,20 +778,6 @@ struct child_process *git_connect(int fd[2], const char *url,
 			int putty = 0, tortoiseplink = 0;
 			transport_check_allowed("ssh");
 
-			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", host ? host : "NULL");
-				printf("Diag: port=%s\n", port ? port : "NONE");
-				printf("Diag: path=%s\n", path ? path : "NULL");
-
-				free(host);
-				free(port);
-				free(path);
-				free(conn);
-				return NULL;
-			}
-
 			ssh = getenv("GIT_SSH_COMMAND");
 			if (!ssh) {
 				const char *base;
-- 
2.8.2.411.ga570dec.dirty

[PATCH v6 6/9] connect: make parse_connect_url() return the user part of the url as a separate value

[Mike Hommey]

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 connect.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/connect.c b/connect.c
index 3a77b2f..2659b40 100644
--- a/connect.c
+++ b/connect.c
@@ -602,11 +602,13 @@ static char *get_port(char *host)
  * Extract protocol and relevant parts from the specified connection URL.
  * The caller must free() the returned strings.
  */
-static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
-				       char **ret_port, char **ret_path)
+static enum protocol parse_connect_url(const char *url_orig, char **ret_user,
+				       char **ret_host, char **ret_port,
+				       char **ret_path)
 {
 	char *url;
 	char *host, *path;
+	const char *user = NULL;
 	const char *port = NULL;
 	char *end;
 	int separator = '/';
@@ -664,16 +666,23 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
 
 	get_host_and_port(&host, &port);
 
-	if (*host && !port) {
+	if (*host) {
 		/* At this point, the host part may look like user@host:port.
 		 * As the `user` portion tends to be less strict than
 		 * `host:port`, we first put it out of the equation: since a
 		 * hostname cannot contain a '@', we start from the last '@'
 		 * in the string. */
 		char *end_user = strrchr(host, '@');
-		port = get_port(end_user ? end_user : host);
+		if (end_user) {
+			*end_user = '\0';
+			user = host;
+			host = end_user + 1;
+		}
 	}
+	if (!port)
+		port = get_port(host);
 
+	*ret_user = user ? xstrdup(user) : NULL;
 	*ret_host = xstrdup(host);
 	*ret_port = port ? xstrdup(port) : NULL;
 	*ret_path = path;
@@ -697,7 +706,7 @@ static struct child_process no_fork = CHILD_PROCESS_INIT;
 struct child_process *git_connect(int fd[2], const char *url,
 				  const char *prog, int flags)
 {
-	char *host, *port, *path;
+	char *user, *host, *port, *path;
 	struct child_process *conn = &no_fork;
 	enum protocol protocol;
 	struct strbuf cmd = STRBUF_INIT;
@@ -707,11 +716,14 @@ struct child_process *git_connect(int fd[2], const char *url,
 	 */
 	signal(SIGCHLD, SIG_DFL);
 
-	protocol = parse_connect_url(url, &host, &port, &path);
+	protocol = parse_connect_url(url, &user, &host, &port, &path);
 	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", host ? host : "NULL");
+		if (user)
+			printf("Diag: userandhost=%s@%s\n", user, host);
+		else
+			printf("Diag: userandhost=%s\n", host ? host : "NULL");
 		printf("Diag: port=%s\n", port ? port : "NONE");
 		printf("Diag: path=%s\n", path ? path : "NULL");
 		conn = NULL;
@@ -818,7 +830,11 @@ struct child_process *git_connect(int fd[2], const char *url,
 				argv_array_push(&conn->args, putty ? "-P" : "-p");
 				argv_array_push(&conn->args, port);
 			}
-			argv_array_push(&conn->args, host);
+			if (user)
+				argv_array_pushf(&conn->args, "%s@%s",
+				                 user, host);
+			else
+				argv_array_push(&conn->args, host);
 		} else {
 			transport_check_allowed("file");
 		}
@@ -831,6 +847,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 		fd[1] = conn->in;  /* write to child's stdin */
 		strbuf_release(&cmd);
 	}
+	free(user);
 	free(host);
 	free(port);
 	free(path);
-- 
2.8.2.411.ga570dec.dirty

[PATCH v6 7/9] connect: change the --diag-url output to separate user and host

[Mike Hommey]

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 connect.c             |  6 ++----
 t/t5500-fetch-pack.sh | 14 ++++++++++++--
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/connect.c b/connect.c
index 2659b40..dcaf32f 100644
--- a/connect.c
+++ b/connect.c
@@ -720,10 +720,8 @@ struct child_process *git_connect(int fd[2], const char *url,
 	if (flags & CONNECT_DIAG_URL) {
 		printf("Diag: url=%s\n", url ? url : "NULL");
 		printf("Diag: protocol=%s\n", prot_name(protocol));
-		if (user)
-			printf("Diag: userandhost=%s@%s\n", user, host);
-		else
-			printf("Diag: userandhost=%s\n", host ? host : "NULL");
+		printf("Diag: user=%s\n", user ? user : "NULL");
+		printf("Diag: host=%s\n", host ? host : "NULL");
 		printf("Diag: port=%s\n", port ? port : "NONE");
 		printf("Diag: path=%s\n", path ? path : "NULL");
 		conn = NULL;
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 739c6b1..2d9c4be 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -553,7 +553,7 @@ check_prot_path () {
 	Diag: protocol=$2
 	Diag: path=$3
 	EOF
-	git fetch-pack --diag-url "$1" | egrep -v '(host|port)=' >actual &&
+	git fetch-pack --diag-url "$1" | egrep -v "(user|host|port)=" >actual &&
 	test_cmp expected actual
 }
 
@@ -568,10 +568,20 @@ check_prot_host_port_path () {
 		;;
 	esac
 	ehost=$(echo $3 | tr -d "[]")
+	case "$ehost" in
+		*@*)
+		user=${ehost%@*}
+		ehost=${ehost#$user@}
+		;;
+		*)
+		user=NULL
+		;;
+	esac
 	cat >exp <<-EOF &&
 	Diag: url=$1
 	Diag: protocol=$pp
-	Diag: userandhost=$ehost
+	Diag: user=$user
+	Diag: host=$ehost
 	Diag: port=$4
 	Diag: path=$5
 	EOF
-- 
2.8.2.411.ga570dec.dirty

[PATCH v6 8/9] connect: actively reject git:// urls with a user part

[Mike Hommey]

Currently, urls of the for git://user@host don't work because user@host
is not resolving at the DNS level, but we shouldn't be relying on it
being an invalid host name, and actively reject it for containing a
username in the first place.

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 connect.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/connect.c b/connect.c
index dcaf32f..7360d57 100644
--- a/connect.c
+++ b/connect.c
@@ -733,6 +733,9 @@ struct child_process *git_connect(int fd[2], const char *url,
 		 */
 		struct strbuf target_host = STRBUF_INIT;
 		char *override_vhost = getenv("GIT_OVERRIDE_VIRTUAL_HOST");
+		if (user)
+			die("user@host is not allowed in git:// urls");
+
 		if (override_vhost)
 			strbuf_addstr(&target_host, override_vhost);
 		else {
-- 
2.8.2.411.ga570dec.dirty

[PATCH v6 9/9] connect: move ssh command line preparation to a separate function

[Mike Hommey]

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 connect.c | 108 +++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 58 insertions(+), 50 deletions(-)

diff --git a/connect.c b/connect.c
index 7360d57..38cdffc 100644
--- a/connect.c
+++ b/connect.c
@@ -690,6 +690,61 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_user,
 	return protocol;
 }
 
+static int prepare_ssh_command(struct argv_array *cmd, const char *user,
+			       const char *host, const char *port, int flags)
+{
+	const char *ssh;
+	int putty = 0, tortoiseplink = 0, use_shell = 1;
+	transport_check_allowed("ssh");
+
+	ssh = getenv("GIT_SSH_COMMAND");
+	if (!ssh) {
+		const char *base;
+		char *ssh_dup;
+
+		/*
+		 * GIT_SSH is the no-shell version of
+		 * GIT_SSH_COMMAND (and must remain so for
+		 * historical compatibility).
+		 */
+		use_shell = 0;
+
+		ssh = getenv("GIT_SSH");
+		if (!ssh)
+			ssh = "ssh";
+
+		ssh_dup = xstrdup(ssh);
+		base = basename(ssh_dup);
+
+		tortoiseplink = !strcasecmp(base, "tortoiseplink") ||
+			!strcasecmp(base, "tortoiseplink.exe");
+		putty = tortoiseplink ||
+			!strcasecmp(base, "plink") ||
+			!strcasecmp(base, "plink.exe");
+
+		free(ssh_dup);
+	}
+
+	argv_array_push(cmd, ssh);
+	if (flags & CONNECT_IPV4)
+		argv_array_push(cmd, "-4");
+	else if (flags & CONNECT_IPV6)
+		argv_array_push(cmd, "-6");
+	if (tortoiseplink)
+		argv_array_push(cmd, "-batch");
+	if (port) {
+		/* P is for PuTTY, p is for OpenSSH */
+		argv_array_push(cmd, putty ? "-P" : "-p");
+		argv_array_push(cmd, port);
+	}
+	if (user)
+		argv_array_pushf(cmd, "%s@%s", user, host);
+	else
+		argv_array_push(cmd, host);
+
+	return use_shell;
+}
+
 static struct child_process no_fork = CHILD_PROCESS_INIT;
 
 /*
@@ -784,59 +839,12 @@ struct child_process *git_connect(int fd[2], const char *url,
 
 		/* remove repo-local variables from the environment */
 		conn->env = local_repo_env;
-		conn->use_shell = 1;
 		conn->in = conn->out = -1;
 		if (protocol == PROTO_SSH) {
-			const char *ssh;
-			int putty = 0, tortoiseplink = 0;
-			transport_check_allowed("ssh");
-
-			ssh = getenv("GIT_SSH_COMMAND");
-			if (!ssh) {
-				const char *base;
-				char *ssh_dup;
-
-				/*
-				 * GIT_SSH is the no-shell version of
-				 * GIT_SSH_COMMAND (and must remain so for
-				 * historical compatibility).
-				 */
-				conn->use_shell = 0;
-
-				ssh = getenv("GIT_SSH");
-				if (!ssh)
-					ssh = "ssh";
-
-				ssh_dup = xstrdup(ssh);
-				base = basename(ssh_dup);
-
-				tortoiseplink = !strcasecmp(base, "tortoiseplink") ||
-					!strcasecmp(base, "tortoiseplink.exe");
-				putty = tortoiseplink ||
-					!strcasecmp(base, "plink") ||
-					!strcasecmp(base, "plink.exe");
-
-				free(ssh_dup);
-			}
-
-			argv_array_push(&conn->args, ssh);
-			if (flags & CONNECT_IPV4)
-				argv_array_push(&conn->args, "-4");
-			else if (flags & CONNECT_IPV6)
-				argv_array_push(&conn->args, "-6");
-			if (tortoiseplink)
-				argv_array_push(&conn->args, "-batch");
-			if (port) {
-				/* P is for PuTTY, p is for OpenSSH */
-				argv_array_push(&conn->args, putty ? "-P" : "-p");
-				argv_array_push(&conn->args, port);
-			}
-			if (user)
-				argv_array_pushf(&conn->args, "%s@%s",
-				                 user, host);
-			else
-				argv_array_push(&conn->args, host);
+			conn->use_shell = prepare_ssh_command(
+				&conn->args, user, host, port, flags);
 		} else {
+			conn->use_shell = 1;
 			transport_check_allowed("file");
 		}
 		argv_array_push(&conn->args, cmd.buf);
-- 
2.8.2.411.ga570dec.dirty