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

[PATCH V2] 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>
---
(This does apply on pu, not on master.
 I'm almost sure there are more corner cases, but the
 most important things should be covered)
 Changes since V1: Comments from Eric Sunshine (thanks)
 connect.c        | 47 +++++++++++++++++----------
 connect.h        |  1 +
 t/t5601-clone.sh | 98 ++++++++++++++++++++++++++++++++++++++++++++------------
 transport.c      |  8 -----
 4 files changed, 108 insertions(+), 46 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..a126f08 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -294,39 +294,95 @@ test_expect_success 'setup ssh wrapper' '
 	export TRASH_DIRECTORY
 '
 
-clear_ssh () {
-	>"$TRASH_DIRECTORY/ssh-output"
-}
-
-expect_ssh () {
+i5601=0
+# $1 url
+# $2 none|host
+# $3 path
+test_clone_url () {
+	i5601=$(($i5601 + 1))
+	>"$TRASH_DIRECTORY/ssh-output" &&
+	test_might_fail git clone "$1" tmp$i5601 &&
 	{
-		case "$1" in
+		case "$2" in
 		none)
 			;;
 		*)
-			echo "ssh: $1 git-upload-pack '$2'"
+			echo "ssh: $2 git-upload-pack '$3'"
 		esac
 	} >"$TRASH_DIRECTORY/ssh-expect" &&
-	(cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output)
+	(
+		cd "$TRASH_DIRECTORY" &&
+		test_cmp ssh-expect ssh-output &&
+		rm -rf ssh-expect ssh-output
+	)
 }
 
-test_expect_success 'cloning myhost:src uses ssh' '
-	clear_ssh &&
-	git clone myhost:src ssh-clone &&
-	expect_ssh myhost src
-'
-
+# url looks ssh like, and is on disc: should be local
 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
+	test_clone_url "foo:bar" none &&
+	( cd tmp$i5601 && git log)
+'
+#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"
 '
 
-test_expect_success 'bracketed hostnames are still ssh' '
-	clear_ssh &&
-	git clone "[myhost:123]:src" ssh-bracket-clone &&
-	expect_ssh myhost:123 src
+# 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.457.g424cb08

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

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> (This does apply on pu, not on master.

Hmph.  At least for me, it applies down to cabb411f (Merge branch
'nd/clone-local-with-colon', 2013-10-14) just fine.  Puzzled.

> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 1d1c875..a126f08 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -294,39 +294,95 @@ test_expect_success 'setup ssh wrapper' '
>  	export TRASH_DIRECTORY
>  '
>  
> -clear_ssh () {
> -	>"$TRASH_DIRECTORY/ssh-output"
> -}
> -
> -expect_ssh () {
> +i5601=0
> +# $1 url
> +# $2 none|host
> +# $3 path
> +test_clone_url () {
> +	i5601=$(($i5601 + 1))
> +	>"$TRASH_DIRECTORY/ssh-output" &&
> +	test_might_fail git clone "$1" tmp$i5601 &&
>  	{
> -		case "$1" in
> +		case "$2" in
>  		none)
>  			;;
>  		*)
> -			echo "ssh: $1 git-upload-pack '$2'"
> +			echo "ssh: $2 git-upload-pack '$3'"
>  		esac
>  	} >"$TRASH_DIRECTORY/ssh-expect" &&

This looks like a strange use of {} (not an issue this patch
introduced, though).  Shouldn't this suffice?

	case ... in
        ...
        esac >"$TRASH_DIRECTORY/ssh-expect"

> +	(
> +		cd "$TRASH_DIRECTORY" &&
> +		test_cmp ssh-expect ssh-output &&
> +		rm -rf ssh-expect ssh-output

Drop "r", please, when you know these are supposed to be files.

> +	)
>  }
>  
> +# url looks ssh like, and is on disc: should be local
>  test_expect_success NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '
>  	cp -R src "foo:bar" &&
> +	test_clone_url "foo:bar" none &&
> +	( cd tmp$i5601 && git log)

Hmph.  What is this "git log" about?  Leftover from an earlier
debugging session?

The code change to connect.c part seemed to be OK from a cursory
look.

Thanks.

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

[Jeff King]

On Mon, Oct 28, 2013 at 09:16:19PM +0100, Torsten Bögershausen wrote:

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

IMHO, this would have been a little easier to follow as separate
patches, as there are a few things going on. I think the changes to the
code are fine, though.

> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 1d1c875..a126f08 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -294,39 +294,95 @@ test_expect_success 'setup ssh wrapper' '
>  	export TRASH_DIRECTORY
>  '
>  
> -clear_ssh () {
> -	>"$TRASH_DIRECTORY/ssh-output"
> -}
> -
> -expect_ssh () {

I'd prefer if you left the expect_ssh interface intact, as it is
potentially useful outside of t5601 (but your patch ties it very closely
to a particular set of clone tests). Can you call out to expect_ssh
instead from test_clone_url instead?

> +i5601=0

The name of this variable confused me for a bit. Maybe "counter" or
something would be a little more descriptive?

> +# $1 url
> +# $2 none|host
> +# $3 path
> +test_clone_url () {
> +	i5601=$(($i5601 + 1))
> +	>"$TRASH_DIRECTORY/ssh-output" &&
> +	test_might_fail git clone "$1" tmp$i5601 &&

Do we always want test_might_fail in these tests? I really would prefer
that the test actually accomplish the clone, as then we know that
nothing funny is going on (e.g., multiple invocations of ssh, one of
which is good and one of which is not). I think I gave more specific
examples in the last round of review.

> -	(cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output)
> +	(
> +		cd "$TRASH_DIRECTORY" &&
> +		test_cmp ssh-expect ssh-output &&
> +		rm -rf ssh-expect ssh-output
> +	)

Here we clean up at the end of the test, which is a good improvement on
my existing functions. But I notice that you also clear ssh-output at
the beginning of the test, still (which you must do to not leave a
polluted state after a failing test). I think this might be better as:

  test_when_finished '
    (cd "$TRASH_DIRECTORY" && rm -f ssh-expect ssh-output)
  '

and then the "clear_ssh" can just go away (and your reset of ssh-output,
which is the analogous line), as tests can expect a clean state.

-Peff

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

[Jeff King]

On Mon, Oct 28, 2013 at 01:57:13PM -0700, Junio C Hamano wrote:

> > +i5601=0
> > +# $1 url
> > +# $2 none|host
> > +# $3 path
> > +test_clone_url () {
> > +	i5601=$(($i5601 + 1))
> > +	>"$TRASH_DIRECTORY/ssh-output" &&
> > +	test_might_fail git clone "$1" tmp$i5601 &&
> >  	{
> > -		case "$1" in
> > +		case "$2" in
> >  		none)
> >  			;;
> >  		*)
> > -			echo "ssh: $1 git-upload-pack '$2'"
> > +			echo "ssh: $2 git-upload-pack '$3'"
> >  		esac
> >  	} >"$TRASH_DIRECTORY/ssh-expect" &&
> 
> This looks like a strange use of {} (not an issue this patch
> introduced, though).  Shouldn't this suffice?
> 
> 	case ... in
>         ...
>         esac >"$TRASH_DIRECTORY/ssh-expect"

Yeah, I think you are right. This one is my fault from 8d3d28f; I think
in an earlier draft I had more complex logic that needed the {}, and
then never dropped them when it got simplified.

-Peff