Merge branch 'tb/clone-ssh-with-colon-for-port'

Be more careful when parsing remote repository URL given in the
scp-style host:path notation.

* tb/clone-ssh-with-colon-for-port:
  git_connect(): use common return point
  connect.c: refactor url parsing
  git_connect(): refactor the port handling for ssh
  git fetch: support host:/~repo
  t5500: add test cases for diag-url
  git fetch-pack: add --diag-url
  git_connect: factor out discovery of the protocol and its parts
  git_connect: remove artificial limit of a remote command
  t5601: add tests for ssh
  t5601: remove clear_ssh, refactor setup_ssh_wrapper

Author: gitster

builtin/fetch-pack.c

@@ -7,7 +7,7 @@
 static const char fetch_pack_usage[] =
 "git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin] "
 "[--include-tag] [--upload-pack=<git-upload-pack>] [--depth=<n>] "
-"[--no-progress] [-v] [<host>:]<directory> [<refs>...]";
+"[--no-progress] [--diag-url] [-v] [<host>:]<directory> [<refs>...]";
 
 static void add_sought_entry_mem(struct ref ***sought, int *nr, int *alloc,
 				 const char *name, int namelen)
@@ -81,6 +81,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			args.stdin_refs = 1;
 			continue;
 		}
+		if (!strcmp("--diag-url", arg)) {
+			args.diag_url = 1;
+			continue;
+		}
 		if (!strcmp("-v", arg)) {
 			args.verbose = 1;
 			continue;
@@ -146,10 +150,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 		fd[0] = 0;
 		fd[1] = 1;
 	} else {
+		int flags = args.verbose ? CONNECT_VERBOSE : 0;
+		if (args.diag_url)
+			flags |= CONNECT_DIAG_URL;
 		conn = git_connect(fd, dest, args.uploadpack,
-				   args.verbose ? CONNECT_VERBOSE : 0);
+				   flags);
+		if (!conn)
+			return args.diag_url ? 0 : 1;
 	}
-
 	get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL);
 
 	ref = fetch_pack(&args, fd, conn, ref, dest,

connect.c

@@ -232,10 +232,34 @@ int server_supports(const char *feature)
 
 enum protocol {
 	PROTO_LOCAL = 1,
+	PROTO_FILE,
 	PROTO_SSH,
 	PROTO_GIT
 };
 
+int url_is_local_not_ssh(const char *url)
+{
+	const char *colon = strchr(url, ':');
+	const char *slash = strchr(url, '/');
+	return !colon || (slash && slash < colon) ||
+		has_dos_drive_prefix(url);
+}
+
+static const char *prot_name(enum protocol protocol)
+{
+	switch (protocol) {
+		case PROTO_LOCAL:
+		case PROTO_FILE:
+			return "file";
+		case PROTO_SSH:
+			return "ssh";
+		case PROTO_GIT:
+			return "git";
+		default:
+			return "unkown protocol";
+	}
+}
+
 static enum protocol get_protocol(const char *name)
 {
 	if (!strcmp(name, "ssh"))
@@ -247,7 +271,7 @@ static enum protocol get_protocol(const char *name)
 	if (!strcmp(name, "ssh+git"))
 		return PROTO_SSH;
 	if (!strcmp(name, "file"))
-		return PROTO_LOCAL;
+		return PROTO_FILE;
 	die("I don't handle protocol '%s'", name);
 }
 
@@ -527,55 +551,31 @@ static struct child_process *git_proxy_connect(int fd[2], char *host)
 	return proxy;
 }
 
-#define MAX_CMD_LEN 1024
-
-static char *get_port(char *host)
+static const char *get_port_numeric(const char *p)
 {
 	char *end;
-	char *p = strchr(host, ':');
-
 	if (p) {
 		long port = strtol(p + 1, &end, 10);
 		if (end != p + 1 && *end == '\0' && 0 <= port && port < 65536) {
-			*p = '\0';
-			return p+1;
+			return p;
 		}
 	}
 
 	return NULL;
 }
 
-static struct child_process no_fork;
-
 /*
- * This returns a dummy child_process if the transport protocol does not
- * need fork(2), or a struct child_process object if it does.  Once done,
- * finish the connection with finish_connect() with the value returned from
- * this function (it is safe to call finish_connect() with NULL to support
- * the former case).
- *
- * If it returns, the connect is successful; it just dies on errors (this
- * will hopefully be changed in a libification effort, to return NULL when
- * the connection failed).
+ * Extract protocol and relevant parts from the specified connection URL.
+ * The caller must free() the returned strings.
  */
-struct child_process *git_connect(int fd[2], const char *url_orig,
-				  const char *prog, int flags)
+static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
+				       char **ret_path)
 {
 	char *url;
 	char *host, *path;
 	char *end;
-	int c;
-	struct child_process *conn = &no_fork;
+	int separator = '/';
 	enum protocol protocol = PROTO_LOCAL;
-	int free_path = 0;
-	char *port = NULL;
-	const char **arg;
-	struct strbuf cmd;
-
-	/* Without this we cannot rely on waitpid() to tell
-	 * what happened to our children.
-	 */
-	signal(SIGCHLD, SIG_DFL);
 
 	if (is_url(url_orig))
 		url = url_decode(url_orig);
@@ -587,40 +587,33 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 		*host = '\0';
 		protocol = get_protocol(url);
 		host += 3;
-		c = '/';
 	} else {
 		host = url;
-		c = ':';
+		if (!url_is_local_not_ssh(url)) {
+			protocol = PROTO_SSH;
+			separator = ':';
+		}
 	}
 
 	/*
-	 * Don't do destructive transforms with git:// as that
-	 * protocol code does '[]' unwrapping of its own.
+	 * Don't do destructive transforms as protocol code does
+	 * '[]' unwrapping in get_host_and_port()
 	 */
 	if (host[0] == '[') {
 		end = strchr(host + 1, ']');
 		if (end) {
-			if (protocol != PROTO_GIT) {
-				*end = 0;
-				host++;
-			}
 			end++;
 		} else
 			end = host;
 	} else
 		end = host;
 
-	path = strchr(end, c);
-	if (path && !has_dos_drive_prefix(end)) {
-		if (c == ':') {
-			if (host != url || path < strchrnul(host, '/')) {
-				protocol = PROTO_SSH;
-				*path++ = '\0';
-			} else /* '/' in the host part, assume local path */
-				path = end;
-		}
-	} else
+	if (protocol == PROTO_LOCAL)
 		path = end;
+	else if (protocol == PROTO_FILE && has_dos_drive_prefix(end))
+		path = end; /* "file://$(pwd)" may be "file://C:/projects/repo" */
+	else
+		path = strchr(end, separator);
 
 	if (!path || !*path)
 		die("No path specified. See 'man git-pull' for valid url syntax");
@@ -629,33 +622,67 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 	 * null-terminate hostname and point path to ~ for URL's like this:
 	 *    ssh://host.xz/~user/repo
 	 */
-	if (protocol != PROTO_LOCAL && host != url) {
-		char *ptr = path;
+
+	end = path; /* Need to \0 terminate host here */
+	if (separator == ':')
+		path++; /* path starts after ':' */
+	if (protocol == PROTO_GIT || protocol == PROTO_SSH) {
 		if (path[1] == '~')
 			path++;
-		else {
-			path = xstrdup(ptr);
-			free_path = 1;
-		}
-
-		*ptr = '\0';
 	}
 
-	/*
-	 * Add support for ssh port: ssh://host.xy:<port>/...
+	path = xstrdup(path);
+	*end = '\0';
+
+	*ret_host = xstrdup(host);
+	*ret_path = path;
+	free(url);
+	return protocol;
+}
+
+static struct child_process no_fork;
+
+/*
+ * This returns a dummy child_process if the transport protocol does not
+ * need fork(2), or a struct child_process object if it does.  Once done,
+ * finish the connection with finish_connect() with the value returned from
+ * this function (it is safe to call finish_connect() with NULL to support
+ * the former case).
+ *
+ * If it returns, the connect is successful; it just dies on errors (this
+ * will hopefully be changed in a libification effort, to return NULL when
+ * the connection failed).
+ */
+struct child_process *git_connect(int fd[2], const char *url,
+				  const char *prog, int flags)
+{
+	char *hostandport, *path;
+	struct child_process *conn = &no_fork;
+	enum protocol protocol;
+	const char **arg;
+	struct strbuf cmd = STRBUF_INIT;
+
+	/* Without this we cannot rely on waitpid() to tell
+	 * what happened to our children.
 	 */
-	if (protocol == PROTO_SSH && host != url)
-		port = get_port(end);
+	signal(SIGCHLD, SIG_DFL);
 
-	if (protocol == PROTO_GIT) {
+	protocol = parse_connect_url(url, &hostandport, &path);
+	if (flags & CONNECT_DIAG_URL) {
+		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: path=%s\n", path ? path : "NULL");
+		conn = NULL;
+	} else if (protocol == PROTO_GIT) {
 		/* These underlying connection commands die() if they
 		 * cannot connect.
 		 */
-		char *target_host = xstrdup(host);
-		if (git_use_proxy(host))
-			conn = git_proxy_connect(fd, host);
+		char *target_host = xstrdup(hostandport);
+		if (git_use_proxy(hostandport))
+			conn = git_proxy_connect(fd, hostandport);
 		else
-			git_tcp_connect(fd, host, flags);
+			git_tcp_connect(fd, hostandport, flags);
 		/*
 		 * Separate original protocol components prog and path
 		 * from extended host header with a NUL byte.
@@ -668,55 +695,51 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 			     prog, path, 0,
 			     target_host, 0);
 		free(target_host);
-		free(url);
-		if (free_path)
-			free(path);
-		return conn;
-	}
-
-	conn = xcalloc(1, sizeof(*conn));
-
-	strbuf_init(&cmd, MAX_CMD_LEN);
-	strbuf_addstr(&cmd, prog);
-	strbuf_addch(&cmd, ' ');
-	sq_quote_buf(&cmd, path);
-	if (cmd.len >= MAX_CMD_LEN)
-		die("command line too long");
-
-	conn->in = conn->out = -1;
-	conn->argv = arg = xcalloc(7, sizeof(*arg));
-	if (protocol == PROTO_SSH) {
-		const char *ssh = getenv("GIT_SSH");
-		int putty = ssh && strcasestr(ssh, "plink");
-		if (!ssh) ssh = "ssh";
-
-		*arg++ = ssh;
-		if (putty && !strcasestr(ssh, "tortoiseplink"))
-			*arg++ = "-batch";
-		if (port) {
-			/* P is for PuTTY, p is for OpenSSH */
-			*arg++ = putty ? "-P" : "-p";
-			*arg++ = port;
+	} else {
+		conn = xcalloc(1, sizeof(*conn));
+
+		strbuf_addstr(&cmd, prog);
+		strbuf_addch(&cmd, ' ');
+		sq_quote_buf(&cmd, path);
+
+		conn->in = conn->out = -1;
+		conn->argv = arg = xcalloc(7, sizeof(*arg));
+		if (protocol == PROTO_SSH) {
+			const char *ssh = getenv("GIT_SSH");
+			int putty = ssh && strcasestr(ssh, "plink");
+			char *ssh_host = hostandport;
+			const char *port = NULL;
+			get_host_and_port(&ssh_host, &port);
+			port = get_port_numeric(port);
+
+			if (!ssh) ssh = "ssh";
+
+			*arg++ = ssh;
+			if (putty && !strcasestr(ssh, "tortoiseplink"))
+				*arg++ = "-batch";
+			if (port) {
+				/* P is for PuTTY, p is for OpenSSH */
+				*arg++ = putty ? "-P" : "-p";
+				*arg++ = port;
+			}
+			*arg++ = ssh_host;
+		}	else {
+			/* remove repo-local variables from the environment */
+			conn->env = local_repo_env;
+			conn->use_shell = 1;
 		}
-		*arg++ = host;
-	}
-	else {
-		/* remove repo-local variables from the environment */
-		conn->env = local_repo_env;
-		conn->use_shell = 1;
-	}
-	*arg++ = cmd.buf;
-	*arg = NULL;
+		*arg++ = cmd.buf;
+		*arg = NULL;
 
-	if (start_command(conn))
-		die("unable to fork");
+		if (start_command(conn))
+			die("unable to fork");
 
-	fd[0] = conn->out; /* read from child's stdout */
-	fd[1] = conn->in;  /* write to child's stdin */
-	strbuf_release(&cmd);
-	free(url);
-	if (free_path)
-		free(path);
+		fd[0] = conn->out; /* read from child's stdout */
+		fd[1] = conn->in;  /* write to child's stdin */
+		strbuf_release(&cmd);
+	}
+	free(hostandport);
+	free(path);
 	return conn;
 }
 

connect.h

@@ -2,11 +2,13 @@
 #define CONNECT_H
 
 #define CONNECT_VERBOSE       (1u << 0)
+#define CONNECT_DIAG_URL      (1u << 1)
 extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);
 extern int finish_connect(struct child_process *conn);
 extern int git_connection_is_socket(struct child_process *conn);
 extern int server_supports(const char *feature);
 extern int parse_feature_request(const char *features, const char *feature);
 extern const char *server_feature_value(const char *feature, int *len_ret);
+extern int url_is_local_not_ssh(const char *url);
 
 #endif