[PATCH V3] git clone: is an URL local or ssh

[PATCH V3] git clone: is an URL local or ssh

[Torsten Bögershausen]

Commit 8d3d28f5 added test cases for URLs which should be ssh.

Add more tests testing all the combinations:
 -IPv4 or IPv6
 -path starting with "/" or with "/~"
 -with and without the ssh:// scheme

Add tests for ssh:// with port number.

When a git repository "foo:bar" exist, git clone will call
absolute_path() and git_connect() will be called with
something like "/home/user/projects/foo:bar"

Tighten the test and use "foo:bar" instead of "./foo:bar",
refactor clear_ssh() and expect_ssh() into test_clone_url().

"git clone foo/bar:baz" should not be ssh:
  Make the rule
  "if there is a slash before the first colon, it is not ssh"
  more strict by using the same is_local() in both connect.c
  and transport.c. Add a test case.

Bug fixes for corner cases:
- Handle clone of [::1]:/~repo correctly (2e7766655):
  Git should call "ssh ::1 ~repo", not ssh ::1 /~repo
  This was caused by looking at (host != url), which never
  worked for host names with literal IPv6 adresses, embedded by []
  Support for the [] URLs was introduced in 356bece0a.

- Do not tamper local URLs starting with '[' which have a ']'

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 (Thanks for the reviews)
 Changes since V2:
 clear_ssh and expect_ssh did come back
 And I couldn't get it working without the
 >"$TRASH_DIRECTORY/ssh-output"

 test_when_finished:
 I could not get that to work. Probably because of the
 battle between the quotings: '"' "'" '"'
 
 Other note about test_might_fail:
 The first version did not need it, git clone did
 always succeed.
 After a while it always failed.
 According to my understanding, git clone ssh://xxx.yy
 should fail (as we can not clone) ??
 But it seems to be a timing problem, anybody wants to comment
 on this ?
 

 connect.c        | 47 ++++++++++++++++++++------------
 connect.h        |  1 +
 t/t5601-clone.sh | 81 +++++++++++++++++++++++++++++++++++++++++++++++++-------
 transport.c      |  8 ------
 4 files changed, 102 insertions(+), 35 deletions(-)

diff --git a/connect.c b/connect.c
index 06e88b0..d61adc9 100644
--- a/connect.c
+++ b/connect.c
@@ -231,6 +231,7 @@ int server_supports(const char *feature)
 }
 
 enum protocol {
+	PROTO_LOCAL_OR_SSH = 0,
 	PROTO_LOCAL = 1,
 	PROTO_SSH,
 	PROTO_GIT
@@ -547,6 +548,15 @@ static char *get_port(char *host)
 
 static struct child_process no_fork;
 
+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);
+}
+
+
 /*
  * 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,
@@ -564,9 +574,9 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 	char *url;
 	char *host, *path;
 	char *end;
-	int c;
+	int separator;
 	struct child_process *conn = &no_fork;
-	enum protocol protocol = PROTO_LOCAL;
+	enum protocol protocol = PROTO_LOCAL_OR_SSH;
 	int free_path = 0;
 	char *port = NULL;
 	const char **arg;
@@ -587,20 +597,23 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 		*host = '\0';
 		protocol = get_protocol(url);
 		host += 3;
-		c = '/';
+		separator = '/';
 	} else {
 		host = url;
-		c = ':';
+		separator = ':';
+		if (is_local(url))
+			protocol = PROTO_LOCAL;
 	}
 
 	/*
 	 * Don't do destructive transforms with git:// as that
 	 * protocol code does '[]' unwrapping of its own.
+	 * Don't change local URLs
 	 */
 	if (host[0] == '[') {
 		end = strchr(host + 1, ']');
 		if (end) {
-			if (protocol != PROTO_GIT) {
+			if (protocol != PROTO_GIT && protocol != PROTO_LOCAL) {
 				*end = 0;
 				host++;
 			}
@@ -610,17 +623,17 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 	} 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;
+	path = strchr(end, separator);
+	if (separator == ':') {
+		if (path && protocol == PROTO_LOCAL_OR_SSH) {
+			/* We have a ':' */
+			protocol = PROTO_SSH;
+			*path++ = '\0';
+		} else {/* assume local path */
+			protocol = PROTO_LOCAL;
+			path = end;
 		}
-	} else
-		path = end;
+	}
 
 	if (!path || !*path)
 		die("No path specified. See 'man git-pull' for valid url syntax");
@@ -629,7 +642,7 @@ 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) {
+	if (protocol != PROTO_LOCAL && separator == '/') {
 		char *ptr = path;
 		if (path[1] == '~')
 			path++;
@@ -644,7 +657,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 	/*
 	 * Add support for ssh port: ssh://host.xy:<port>/...
 	 */
-	if (protocol == PROTO_SSH && host != url)
+	if (protocol == PROTO_SSH && separator == '/')
 		port = get_port(end);
 
 	if (protocol == PROTO_GIT) {
diff --git a/connect.h b/connect.h
index 64fb7db..fb2de9b 100644
--- a/connect.h
+++ b/connect.h
@@ -5,6 +5,7 @@
 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 is_local(const char *url);
 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);
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 1d1c875..3aa2ce2 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -310,23 +310,84 @@ expect_ssh () {
 	(cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output)
 }
 
-test_expect_success 'cloning myhost:src uses ssh' '
-	clear_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' '
+counter=0
+# $1 url
+# $2 none|host
+# $3 path
+test_clone_url () {
+	counter=$(($counter + 1))
 	clear_ssh &&
-	git clone "[myhost:123]:src" ssh-bracket-clone &&
-	expect_ssh myhost:123 src
+	test_might_fail git clone "$1" tmp$counter &&
+	expect_ssh "$2" "$3"
+}
+
+#ip v4
+for repo in rep rep/home/project /~proj 123
+do
+	test_expect_success "cloning host:$repo" '
+		test_clone_url host:$repo host $repo
+	'
+done
+
+#ipv6
+for repo in rep rep/home/project /~proj 123
+do
+	test_expect_success "cloning [::1]:$repo" '
+		test_clone_url [::1]:$repo ::1 $repo
+	'
+done
+
+# Corner cases
+for url in [foo]bar/baz:qux [foo/bar]:baz foo/bar:baz
+do
+	test_expect_success "cloning $url is not ssh" '
+			test_clone_url $url none
+	'
+done
+
+#with ssh:// scheme
+test_expect_success '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 'ssh://host.xz/~repo' '
+	test_clone_url "ssh://host.xz/~repo" host.xz "~repo"
+'
+# with port number
+test_expect_success '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 'ssh://host.xz:22/~repo' '
+	test_clone_url "ssh://host.xz:22/~repo" "-p 22 host.xz" "~repo"
+'
+
+#IPv6
+test_expect_success 'ssh://[::1]/home/user/repo' '
+	test_clone_url "ssh://[::1]/home/user/repo" "::1" "/home/user/repo"
+'
+
+#IPv6 from home directory
+test_expect_success 'ssh://[::1]/~repo' '
+	test_clone_url "ssh://[::1]/~repo" "::1" "~repo"
+'
+
+#IPv6 with port number
+test_expect_success '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 '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' '
diff --git a/transport.c b/transport.c
index 7202b77..a09ba95 100644
--- a/transport.c
+++ b/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;
-- 
1.8.4.499.g8798c92

Re: [PATCH V3] git clone: is an URL local or ssh

[Jeff King]

On Tue, Oct 29, 2013 at 10:07:50PM +0100, Torsten Bögershausen wrote:

>  Changes since V2:
>  clear_ssh and expect_ssh did come back
>  And I couldn't get it working without the
>  >"$TRASH_DIRECTORY/ssh-output"

Thanks.

>  test_when_finished:
>  I could not get that to work. Probably because of the
>  battle between the quotings: '"' "'" '"'

The quoting should be straight-forward, since you can do it in
expect_ssh, outside of the regular test eval. But what is tricky is that
you do not actually want "ssh-output" to disappear, but rather you want
it to be an empty file, so that tests which do not trigger ssh can
compare against it using test_cmp.

The patch below makes it work, but I'm thinking that it does not
actually improve readability. While the "clear_ssh" call is something
each test needs to remember, at least it is obvious there that the test
is clearing the state before running the clone.

>  Other note about test_might_fail:
>  The first version did not need it, git clone did
>  always succeed.
>  After a while it always failed.
>  According to my understanding, git clone ssh://xxx.yy
>  should fail (as we can not clone) ??

Cloning over ssh via our fake wrapper should work, as the wrapper finds
the shell command and execs it.  So ssh://host/path should end up
running:

  ssh host 'git-upload-pack path'

and the ssh wrapper converts that to:

  git-upload-pack path

If we want to make it more robust, we could cd into a "hosts/$host"
directory that simulates the remote host, but I don't know if that is
necessary.

---
This is the test_when_finished patch.

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 7db7f48..05afe5a 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -291,14 +291,14 @@ test_expect_success 'setup ssh wrapper' '
 
 	GIT_SSH="$TRASH_DIRECTORY/ssh-wrapper" &&
 	export GIT_SSH &&
-	export TRASH_DIRECTORY
+	export TRASH_DIRECTORY &&
+	>"$TRASH_DIRECTORY"/ssh-output
 '
 
-clear_ssh () {
-	>"$TRASH_DIRECTORY/ssh-output"
-}
-
 expect_ssh () {
+	test_when_finished '
+	  (cd "$TRASH_DIRECTORY" && rm -f ssh-expect && >ssh-output)
+	' &&
 	{
 		case "$1" in
 		none)
@@ -311,7 +311,6 @@ expect_ssh () {
 }
 
 test_expect_success NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '
-	clear_ssh &&
 	cp -R src "foo:bar" &&
 	git clone "foo:bar" foobar &&
 	expect_ssh none
@@ -323,7 +322,6 @@ counter=0
 # $3 path
 test_clone_url () {
 	counter=$(($counter + 1))
-	clear_ssh &&
 	test_might_fail git clone "$1" tmp$counter &&
 	expect_ssh "$2" "$3"
 }

Re: [PATCH V3] git clone: is an URL local or ssh

[Johannes Sixt]

Just a heads-up: This patch breaks t5601 totally on Windows. Test #4, a
local clone via file: protocol, fails already. I'm investigating now.

-- Hannes

Re: [PATCH V3] git clone: is an URL local or ssh

[Johannes Sixt]

Am 10/29/2013 22:07, schrieb Torsten Bögershausen:
> @@ -610,17 +623,17 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
>  	} 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;
> +	path = strchr(end, separator);
> +	if (separator == ':') {
> +		if (path && protocol == PROTO_LOCAL_OR_SSH) {
> +			/* We have a ':' */
> +			protocol = PROTO_SSH;
> +			*path++ = '\0';
> +		} else {/* assume local path */
> +			protocol = PROTO_LOCAL;
> +			path = end;
>  		}
> -	} else
> -		path = end;
> +	}
>  
>  	if (!path || !*path)
>  		die("No path specified. See 'man git-pull' for valid url syntax");

This hunk breaks on Windows. You removed the has_dos_drive_prefix check.

The check for has_dos_drive_prefix check must happen *before* you further
investigate the path/url/host for ssh protocol, and if it returns true,
then the path is local, no matter what follows.

-- Hannes