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

Remote repository URL expressed in scp-style host:path notation are
parsed more carefully (e.g. "foo/bar:baz" is local, "[::1]:/~user" asks
to connect to user's home directory on host at address ::1.

* 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

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,24 +551,95 @@ 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;
 }
 
+/*
+ * 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_path)
+{
+	char *url;
+	char *host, *path;
+	char *end;
+	int separator = '/';
+	enum protocol protocol = PROTO_LOCAL;
+
+	if (is_url(url_orig))
+		url = url_decode(url_orig);
+	else
+		url = xstrdup(url_orig);
+
+	host = strstr(url, "://");
+	if (host) {
+		*host = '\0';
+		protocol = get_protocol(url);
+		host += 3;
+	} else {
+		host = url;
+		if (!url_is_local_not_ssh(url)) {
+			protocol = PROTO_SSH;
+			separator = ':';
+		}
+	}
+
+	/*
+	 * Don't do destructive transforms as protocol code does
+	 * '[]' unwrapping in get_host_and_port()
+	 */
+	if (host[0] == '[') {
+		end = strchr(host + 1, ']');
+		if (end) {
+			end++;
+		} else
+			end = host;
+	} else
+		end = host;
+
+	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");
+
+	/*
+	 * null-terminate hostname and point path to ~ for URL's like this:
+	 *    ssh://host.xz/~user/repo
+	 */
+
+	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++;
+	}
+
+	path = xstrdup(path);
+	*end = '\0';
+
+	*ret_host = xstrdup(host);
+	*ret_path = path;
+	free(url);
+	return protocol;
+}
+
 static struct child_process no_fork;
 
 /*
@@ -558,104 +653,36 @@ static struct child_process no_fork;
  * 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_orig,
+struct child_process *git_connect(int fd[2], const char *url,
 				  const char *prog, int flags)
 {
-	char *url;
-	char *host, *path;
-	char *end;
-	int c;
+	char *hostandport, *path;
 	struct child_process *conn = &no_fork;
-	enum protocol protocol = PROTO_LOCAL;
-	int free_path = 0;
-	char *port = NULL;
+	enum protocol protocol;
 	const char **arg;
-	struct strbuf cmd;
+	struct strbuf cmd = STRBUF_INIT;
 
 	/* 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);
-	else
-		url = xstrdup(url_orig);
-
-	host = strstr(url, "://");
-	if (host) {
-		*host = '\0';
-		protocol = get_protocol(url);
-		host += 3;
-		c = '/';
-	} else {
-		host = url;
-		c = ':';
-	}
-
-	/*
-	 * Don't do destructive transforms with git:// as that
-	 * protocol code does '[]' unwrapping of its own.
-	 */
-	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
-		path = end;
-
-	if (!path || !*path)
-		die("No path specified. See 'man git-pull' for valid url syntax");
-
-	/*
-	 * 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;
-		if (path[1] == '~')
-			path++;
-		else {
-			path = xstrdup(ptr);
-			free_path = 1;
-		}
-
-		*ptr = '\0';
-	}
-
-	/*
-	 * Add support for ssh port: ssh://host.xy:<port>/...
-	 */
-	if (protocol == PROTO_SSH && host != url)
-		port = get_port(end);
-
-	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;
-	}
+	} else {
+		conn = xcalloc(1, sizeof(*conn));
 
-	conn = xcalloc(1, sizeof(*conn));
+		strbuf_addstr(&cmd, prog);
+		strbuf_addch(&cmd, ' ');
+		sq_quote_buf(&cmd, path);
 
-	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");
+			char *ssh_host = hostandport;
+			const char *port = NULL;
+			get_host_and_port(&ssh_host, &port);
+			port = get_port_numeric(port);
 
-	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";
+			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;
+			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

fetch-pack.h

@@ -14,6 +14,7 @@ struct fetch_pack_args {
 		use_thin_pack:1,
 		fetch_all:1,
 		stdin_refs:1,
+		diag_url:1,
 		verbose:1,
 		no_progress:1,
 		include_tag:1,

t/t5500-fetch-pack.sh

@@ -531,5 +531,92 @@ test_expect_success 'shallow fetch with tags does not break the repository' '
 		git fsck
 	)
 '
+check_prot_path () {
+	cat >expected <<-EOF &&
+	Diag: url=$1
+	Diag: protocol=$2
+	Diag: path=$3
+	EOF
+	git fetch-pack --diag-url "$1" | grep -v hostandport= >actual &&
+	test_cmp expected actual
+}
+
+check_prot_host_path () {
+	cat >expected <<-EOF &&
+	Diag: url=$1
+	Diag: protocol=$2
+	Diag: hostandport=$3
+	Diag: path=$4
+	EOF
+	git fetch-pack --diag-url "$1" >actual &&
+	test_cmp expected actual
+}
+
+for r in repo re:po re/po
+do
+	# git or ssh with scheme
+	for p in "ssh+git" "git+ssh" git ssh
+	do
+		for h in host host:12 [::1] [::1]:23
+		do
+			case "$p" in
+			*ssh*)
+				pp=ssh
+				;;
+			*)
+				pp=$p
+			;;
+			esac
+			test_expect_success "fetch-pack --diag-url $p://$h/$r" '
+				check_prot_host_path $p://$h/$r $pp "$h" "/$r"
+			'
+			# "/~" -> "~" conversion
+			test_expect_success "fetch-pack --diag-url $p://$h/~$r" '
+				check_prot_host_path $p://$h/~$r $pp "$h" "~$r"
+			'
+		done
+	done
+	# file with scheme
+	for p in file
+	do
+		test_expect_success "fetch-pack --diag-url $p://$h/$r" '
+			check_prot_path $p://$h/$r $p "/$r"
+		'
+		# No "/~" -> "~" conversion for file
+		test_expect_success "fetch-pack --diag-url $p://$h/~$r" '
+			check_prot_path $p://$h/~$r $p "/~$r"
+		'
+	done
+	# file without scheme
+	for h in nohost nohost:12 [::1] [::1]:23 [ [:aa
+	do
+		test_expect_success "fetch-pack --diag-url ./$h:$r" '
+			check_prot_path ./$h:$r $p "./$h:$r"
+		'
+		# No "/~" -> "~" conversion for file
+		test_expect_success "fetch-pack --diag-url ./$p:$h/~$r" '
+		check_prot_path ./$p:$h/~$r $p "./$p:$h/~$r"
+		'
+	done
+	#ssh without scheme
+	p=ssh
+	for h in host [::1]
+	do
+		test_expect_success "fetch-pack --diag-url $h:$r" '
+			check_prot_path $h:$r $p "$r"
+		'
+		# Do "/~" -> "~" conversion
+		test_expect_success "fetch-pack --diag-url $h:/~$r" '
+			check_prot_host_path $h:/~$r $p "$h" "~$r"
+		'
+	done
+done
+
+test_expect_success MINGW 'fetch-pack --diag-url file://c:/repo' '
+	check_prot_path file://c:/repo file c:/repo
+'
+test_expect_success MINGW 'fetch-pack --diag-url c:repo' '
+	check_prot_path c:repo file c:repo
+'
 
 test_done

t/t5601-clone.sh

@@ -280,25 +280,26 @@ test_expect_success 'clone checking out a tag' '
 	test_cmp fetch.expected fetch.actual
 '
 
-test_expect_success 'setup ssh wrapper' '
-	write_script "$TRASH_DIRECTORY/ssh-wrapper" <<-\EOF &&
-	echo >>"$TRASH_DIRECTORY/ssh-output" "ssh: $*" &&
-	# throw away all but the last argument, which should be the
-	# command
-	while test $# -gt 1; do shift; done
-	eval "$1"
-	EOF
-
-	GIT_SSH="$TRASH_DIRECTORY/ssh-wrapper" &&
-	export GIT_SSH &&
-	export TRASH_DIRECTORY
-'
-
-clear_ssh () {
-	>"$TRASH_DIRECTORY/ssh-output"
+setup_ssh_wrapper () {
+	test_expect_success 'setup ssh wrapper' '
+		write_script "$TRASH_DIRECTORY/ssh-wrapper" <<-\EOF &&
+		echo >>"$TRASH_DIRECTORY/ssh-output" "ssh: $*" &&
+		# throw away all but the last argument, which should be the
+		# command
+		while test $# -gt 1; do shift; done
+		eval "$1"
+		EOF
+		GIT_SSH="$TRASH_DIRECTORY/ssh-wrapper" &&
+		export GIT_SSH &&
+		export TRASH_DIRECTORY &&
+		>"$TRASH_DIRECTORY"/ssh-output
+	'
 }
 
 expect_ssh () {
+	test_when_finished '
+		(cd "$TRASH_DIRECTORY" && rm -f ssh-expect && >ssh-output)
+	' &&
 	{
 		case "$1" in
 		none)
@@ -310,25 +311,114 @@ expect_ssh () {
 	(cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output)
 }
 
-test_expect_success 'cloning myhost:src uses ssh' '
-	clear_ssh &&
+setup_ssh_wrapper
+
+test_expect_success 'clone myhost:src uses ssh' '
 	git clone myhost:src ssh-clone &&
 	expect_ssh myhost src
 '
 
 test_expect_success NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '
-	clear_ssh &&
 	cp -R src "foo:bar" &&
-	git clone "./foo:bar" foobar &&
+	git clone "foo:bar" foobar &&
 	expect_ssh none
 '
 
 test_expect_success 'bracketed hostnames are still ssh' '
-	clear_ssh &&
 	git clone "[myhost:123]:src" ssh-bracket-clone &&
 	expect_ssh myhost:123 src
 '
 
+counter=0
+# $1 url
+# $2 none|host
+# $3 path
+test_clone_url () {
+	counter=$(($counter + 1))
+	test_might_fail git clone "$1" tmp$counter &&
+	expect_ssh "$2" "$3"
+}
+
+test_expect_success NOT_MINGW 'clone c:temp is ssl' '
+	test_clone_url c:temp c temp
+'
+
+test_expect_success MINGW 'clone c:temp is dos drive' '
+	test_clone_url c:temp none
+'
+
+#ip v4
+for repo in rep rep/home/project 123
+do
+	test_expect_success "clone host:$repo" '
+		test_clone_url host:$repo host $repo
+	'
+done
+
+#ipv6
+for repo in rep rep/home/project 123
+do
+	test_expect_success "clone [::1]:$repo" '
+		test_clone_url [::1]:$repo ::1 $repo
+	'
+done
+#home directory
+test_expect_success "clone host:/~repo" '
+	test_clone_url host:/~repo host "~repo"
+'
+
+test_expect_success "clone [::1]:/~repo" '
+	test_clone_url [::1]:/~repo ::1 "~repo"
+'
+
+# Corner cases
+for url in foo/bar:baz [foo]bar/baz:qux [foo/bar]:baz
+do
+	test_expect_success "clone $url is not ssh" '
+		test_clone_url $url none
+	'
+done
+
+#with ssh:// scheme
+test_expect_success 'clone ssh://host.xz/home/user/repo' '
+	test_clone_url "ssh://host.xz/home/user/repo" host.xz "/home/user/repo"
+'
+
+# from home directory
+test_expect_success 'clone ssh://host.xz/~repo' '
+	test_clone_url "ssh://host.xz/~repo" host.xz "~repo"
+'
+
+# with port number
+test_expect_success 'clone ssh://host.xz:22/home/user/repo' '
+	test_clone_url "ssh://host.xz:22/home/user/repo" "-p 22 host.xz" "/home/user/repo"
+'
+
+# from home directory with port number
+test_expect_success 'clone ssh://host.xz:22/~repo' '
+	test_clone_url "ssh://host.xz:22/~repo" "-p 22 host.xz" "~repo"
+'
+
+#IPv6
+test_expect_success 'clone ssh://[::1]/home/user/repo' '
+	test_clone_url "ssh://[::1]/home/user/repo" "::1" "/home/user/repo"
+'
+
+#IPv6 from home directory
+test_expect_success 'clone ssh://[::1]/~repo' '
+	test_clone_url "ssh://[::1]/~repo" "::1" "~repo"
+'
+
+#IPv6 with port number
+test_expect_success 'clone ssh://[::1]:22/home/user/repo' '
+	test_clone_url "ssh://[::1]:22/home/user/repo" "-p 22 ::1" "/home/user/repo"
+'
+
+#IPv6 from home directory with port number
+test_expect_success 'clone ssh://[::1]:22/~repo' '
+	test_clone_url "ssh://[::1]:22/~repo" "-p 22 ::1" "~repo"
+'
+
 test_expect_success 'clone from a repository with two identical branches' '
 
 	(

transport.c

@@ -885,14 +885,6 @@ void transport_take_over(struct transport *transport,
 	transport->cannot_reuse = 1;
 }
 
-static int is_local(const char *url)
-{
-	const char *colon = strchr(url, ':');
-	const char *slash = strchr(url, '/');
-	return !colon || (slash && slash < colon) ||
-		has_dos_drive_prefix(url);
-}
-
 static int is_file(const char *url)
 {
 	struct stat buf;
@@ -941,7 +933,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		ret->fetch = fetch_objs_via_rsync;
 		ret->push = rsync_transport_push;
 		ret->smart_options = NULL;
-	} else if (is_local(url) && is_file(url) && is_bundle(url, 1)) {
+	} else if (url_is_local_not_ssh(url) && is_file(url) && is_bundle(url, 1)) {
 		struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
 		ret->data = data;
 		ret->get_refs_list = get_refs_from_bundle;
@@ -1297,7 +1289,7 @@ char *transport_anonymize_url(const char *url)
 	size_t anon_len, prefix_len = 0;
 
 	anon_part = strchr(url, '@');
-	if (is_local(url) || !anon_part)
+	if (url_is_local_not_ssh(url) || !anon_part)
 		goto literal_copy;
 
 	anon_len = strlen(++anon_part);