threads / patch / 42351

v6, 9 partsconnect: group CONNECT_DIAG_URL handling code

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

## tl;dr

17 messages between May 17, 2016 and May 20, 2016. Diffs are folded; open one to read it.

replies: 16people: 2as markdown or json

Mike Hommey· May 17, 2016, 01:35 UTC · lore

[PATCH v6 0/9] connect: various cleanups

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
Mike Hommey· May 17, 2016, 01:35 UTC · re: Mike Hommey · lore

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

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(-)
Show changes to connect.c +23 −24
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
Junio C Hamano· May 20, 2016, 20:58 UTC · re: Mike Hommey · lore

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

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?

Mike Hommey· May 20, 2016, 22:14 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· May 20, 2016, 22:20 UTC · re: Mike Hommey · lore

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

Mike Hommey <mh@glandium.org> writes:
Show 12 quoted lines
>> 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 ;-)

Show 10 quoted lines
>> 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?

Mike Hommey· May 20, 2016, 22:28 UTC · re: Junio C Hamano · lore

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

On Fri, May 20, 2016 at 03:20:23PM -0700, Junio C Hamano wrote:
Show 33 quoted lines
> 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
Mike Hommey· May 17, 2016, 01:35 UTC · re: Mike Hommey · lore

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

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(-)
Show changes to 2 files +24 −4

connect.c, t/t5532-fetch-proxy.sh

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
Junio C Hamano· May 20, 2016, 21:48 UTC · re: Mike Hommey · lore

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

Mike Hommey <mh@glandium.org> writes:
Show 6 quoted lines
> 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.
Mike Hommey· May 20, 2016, 22:30 UTC · re: Junio C Hamano · lore

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

On Fri, May 20, 2016 at 02:48:28PM -0700, Junio C Hamano wrote:
Show 5 quoted lines
> 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
Junio C Hamano· May 20, 2016, 22:56 UTC · re: Mike Hommey · lore

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

Mike Hommey <mh@glandium.org> writes:
Show 11 quoted lines
> 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.
Mike Hommey· May 17, 2016, 01:35 UTC · re: Mike Hommey · lore

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

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(-)
Show changes to connect.c +20 −7
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
Mike Hommey· May 17, 2016, 01:35 UTC · re: Mike Hommey · lore

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

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(-)
Show changes to 2 files +42 −23

connect.c, t/t5500-fetch-pack.sh

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
Mike Hommey· May 17, 2016, 01:35 UTC · re: Mike Hommey · lore

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(-)
Show changes to connect.c +1 −15
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
Mike Hommey· May 17, 2016, 01:35 UTC · re: Mike Hommey · lore

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

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 connect.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)
Show changes to connect.c +25 −8
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
Mike Hommey· May 17, 2016, 01:35 UTC · re: Mike Hommey · lore

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

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

connect.c, t/t5500-fetch-pack.sh

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
Mike Hommey· May 17, 2016, 01:35 UTC · re: Mike Hommey · lore

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

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(+)
Show changes to connect.c +3 −0
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
Mike Hommey· May 17, 2016, 01:35 UTC · re: Mike Hommey · lore

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

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 connect.c | 108 +++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 58 insertions(+), 50 deletions(-)
Show changes to connect.c +58 −50
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

← back to recent threads