[PATCH] clone: local URLs are not for ssh

"git clone /foo/bar:baz" or "git clone ../foo/bar:baz"
are meant to clone from the local file system, and not to clone
from a remote server over git-over-ssh.

Signed-off-by: Torsten Bögershausen <redacted>
---
 connect.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index a80ebd3..b382032 100644
--- a/connect.c
+++ b/connect.c

@@ -550,7 +550,8 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 		end = host;
 
 	path = strchr(end, c);
-	if (path && !has_dos_drive_prefix(end)) {
+	if (path && !has_dos_drive_prefix(end) &&
+	    url[0] != '/' && url[0] != '.' ) {
 		if (c == ':') {
 			if (path < strchrnul(host, '/')) {
 				protocol = PROTO_SSH;

-- 
1.8.4.457.g424cb08

On Sun, Sep 29, 2013 at 2:37 AM, Torsten Bögershausen [off-list ref] wrote:

"git clone /foo/bar:baz" or "git clone ../foo/bar:baz"
are meant to clone from the local file system, and not to clone
from a remote server over git-over-ssh.

I don't think this is necessary. Commit 6000334 should detect both
cases fine because both have a slash before the first colon.

-- 
Duy

On 2013-09-29 02.33, Duy Nguyen wrote:

On Sun, Sep 29, 2013 at 2:37 AM, Torsten Bögershausen [off-list ref] wrote:

quoted

"git clone /foo/bar:baz" or "git clone ../foo/bar:baz"
are meant to clone from the local file system, and not to clone
from a remote server over git-over-ssh.

I don't think this is necessary. Commit 6000334 should detect both
cases fine because both have a slash before the first colon.

Sorry for the noise, I noticed it when I was trying to construct test cases.

What do we think about adding this at the end of t5505:



test_expect_success 'fetch fail [noexistinghost0:2223]:blink.git' '
	(
		! git fetch [noexistinghost0:2223]:blink.git 2>err &&
		grep ssh err &&
		rm err
	)
'

test_expect_success 'fetch fail noexistinghost1:2223:blink.git' '
	(
		! git fetch "noexistinghost1:2223:blink.git" 2>err &&
		grep ssh err &&
		rm err
	)
'

test_expect_success 'fetch fail noexistinghost2:2223' '
	(
		! git fetch "noexistinghost2:2223" 2>err &&
		grep ssh err &&
		rm err
	)
'
test_expect_success 'fetch fail ./noexistinghost4:2223"' '
	(
		! git fetch "./noexistinghost4:2223" 2>err &&
		grep "does not appear to be a git repository" err &&
		rm err
	)
'

On Thu, Oct 3, 2013 at 1:40 AM, Torsten Bögershausen [off-list ref] wrote:

On 2013-09-29 02.33, Duy Nguyen wrote:

quoted

On Sun, Sep 29, 2013 at 2:37 AM, Torsten Bögershausen [off-list ref] wrote:

quoted

"git clone /foo/bar:baz" or "git clone ../foo/bar:baz"
are meant to clone from the local file system, and not to clone
from a remote server over git-over-ssh.

I don't think this is necessary. Commit 6000334 should detect both
cases fine because both have a slash before the first colon.

Sorry for the noise, I noticed it when I was trying to construct test cases.

What do we think about adding this at the end of t5505:

As usual more tests are usually better. But is t5505-remote.sh the
best place? That file seems about "git remote"..

test_expect_success 'fetch fail [noexistinghost0:2223]:blink.git' '
        (
                ! git fetch [noexistinghost0:2223]:blink.git 2>err &&
                grep ssh err &&
                rm err
        )
'

test_expect_success 'fetch fail noexistinghost1:2223:blink.git' '
        (
                ! git fetch "noexistinghost1:2223:blink.git" 2>err &&
                grep ssh err &&
                rm err
        )
'

test_expect_success 'fetch fail noexistinghost2:2223' '
        (
                ! git fetch "noexistinghost2:2223" 2>err &&
                grep ssh err &&
                rm err
        )
'
test_expect_success 'fetch fail ./noexistinghost4:2223"' '
        (
                ! git fetch "./noexistinghost4:2223" 2>err &&
                grep "does not appear to be a git repository" err &&
                rm err
        )
'

-- 
Duy

On Thu, Oct 03, 2013 at 08:01:23AM +0700, Nguyen Thai Ngoc Duy wrote:

quoted

Sorry for the noise, I noticed it when I was trying to construct test cases.

What do we think about adding this at the end of t5505:

As usual more tests are usually better. But is t5505-remote.sh the
best place? That file seems about "git remote"..

Yeah, agreed. How about at the end of t5601, after the ssh wrapper I set
up here:

  http://article.gmane.org/gmane.comp.version-control.git/235473

I don't know of Jonathan squashed those in to your commit...neither is
in his 'pu' yet.

quoted

test_expect_success 'fetch fail [noexistinghost0:2223]:blink.git' '
        (
                ! git fetch [noexistinghost0:2223]:blink.git 2>err &&
                grep ssh err &&
                rm err
        )
'

This one looks like basically the same test I added in the message
above (except because of the ssh wrapper, we can check that it did
indeed try to ssh to noexistinghost0:2223).

The other tests can check that we fed ssh various host/port/path
combinations. I'm not clear on what we're expecting, though...

quoted

test_expect_success 'fetch fail noexistinghost1:2223:blink.git' '
        (
                ! git fetch "noexistinghost1:2223:blink.git" 2>err &&
                grep ssh err &&
                rm err
        )
'

We are expecting this to be host=noexistinghost1, and
path=2223:blink.git?

quoted

test_expect_success 'fetch fail noexistinghost2:2223' '
        (
                ! git fetch "noexistinghost2:2223" 2>err &&
                grep ssh err &&
                rm err
        )
'

And this is host=noexistinghost2, path=2223?

quoted

test_expect_success 'fetch fail ./noexistinghost4:2223"' '
        (
                ! git fetch "./noexistinghost4:2223" 2>err &&
                grep "does not appear to be a git repository" err &&
                rm err
        )
'

And this one we would be checking that ssh is _not_ used. It seems
redundant with the "./foo:bar" test already in t5601, but perhaps it is
worth double-checking the numeric path. It would be more robust if we
actually had a repo called "noexistinghost4:2223" and checked that we
did clone it, as the existing test does (maybe that test can just
"s/bar/2223/" ?).

-Peff

On 2013-10-03 03.31, Jeff King wrote:

On Thu, Oct 03, 2013 at 08:01:23AM +0700, Nguyen Thai Ngoc Duy wrote:

quoted

quoted

Sorry for the noise, I noticed it when I was trying to construct test cases.

What do we think about adding this at the end of t5505:

As usual more tests are usually better. But is t5505-remote.sh the
best place? That file seems about "git remote"..

Yeah, agreed. How about at the end of t5601, after the ssh wrapper I set
up here:

  http://article.gmane.org/gmane.comp.version-control.git/235473

Thanks for the review & pointer.
To get it working, a little tweak was needed here, please see below.

diff --git a/t/t5602-clone-remote-exec.sh b/t/t5602-clone-remote-exec.sh
index d232e94..37464aa 100755
--- a/t/t5602-clone-remote-exec.sh
+++ b/t/t5602-clone-remote-exec.sh

@@ -62,21 +62,20 @@ expect_ssh () {
 
 test_expect_success 'cloning myhost:src uses ssh' '
        clear_ssh &&
-       git clone myhost:src ssh-clone &&
+       ! git clone myhost:src ssh-clone &&
        expect_ssh myhost src
 '
 
-test_expect_success NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '
+test_expect_success SYMLINKS,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 &&
+       ln -s src "foo:bar" &&
+       ! 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 &&
+       ! git clone "[myhost:123]:src" ssh-bracket-clone &&
        expect_ssh myhost:123 src
 '

On 05.10.13 21:48, Torsten Bögershausen wrote:

On 2013-10-03 03.31, Jeff King wrote:

quoted

  http://article.gmane.org/gmane.comp.version-control.git/235473

What do we think about extending the test a little bit:


git diff 771cf1dab9303bab3c8198b8b6 -- t5602-clone-remote-exec.sh 

diff --git a/t/t5602-clone-remote-exec.sh b/t/t5602-clone-remote-exec.sh
index 3f353d9..790896f 100755
--- a/t/t5602-clone-remote-exec.sh
+++ b/t/t5602-clone-remote-exec.sh

@@ -30,5 +30,124 @@ test_expect_success 'clone calls specified git upload-pack with -u option' '
 	echo "localhost ./something/bin/git-upload-pack '\''/path/to/repo'\''" >expected &&
 	test_cmp expected not_ssh_output
 '
+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"
+}
+
+expect_ssh () {
+	{
+		case "$1" in
+		none)
+			;;
+		*)
+			echo "ssh: $1 git-upload-pack '$2'"
+		esac
+	} >"$TRASH_DIRECTORY/ssh-expect" &&
+	(cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output)
+}
+
+test_expect_success 'create src.git' '
+	mkdir src.git &&
+	( 
+		cd src.git &&
+		git init &&
+		>file &&
+		git add file &&
+		git commit -m "add file"
+	)
+'
+
+# git clone could fail, so break the && chain and ignore the exit code
+# clone local
+test_expect_success './foo:bar is not ssh' '
+	clear_ssh &&
+	git clone "./foo:bar" foobar
+	expect_ssh none
+'
+
+test_expect_success './[nohost:123]:src is not ssh' '
+	clear_ssh &&
+	git clone "./[nohost:123]:src" 1_2_3
+	expect_ssh none
+'
+
+test_expect_success '[nohost:234] is not ssh' '
+	clear_ssh &&
+	git clone "[nohost:234]" 2_3_4
+	expect_ssh none
+'
+
+test_expect_success ':345 is not ssh' '
+	clear_ssh &&
+	git clone ":345" 3_4_5 
+	expect_ssh none
+'
+
+test_expect_success '456: is not ssh' '
+	clear_ssh &&
+	git clone "456:" 4_5_6 
+	expect_ssh none
+'
+
+# clone via ssh
+# the expect_ssh checks that git clone tried to use ssh
+test_expect_success 'myhost:567 is ssh' '
+	clear_ssh &&
+	git clone myhost:567 myhost_567
+	expect_ssh myhost 567
+'
+
+test_expect_success '[myhost:678]:src is ssh' '
+	clear_ssh &&
+	git clone "[myhost:678]:src" myhost_678
+	expect_ssh myhost:678 src
+'
+
+#clone url looks like ssh, but is on disk
+test_expect_success SYMLINKS 'dir:123 on disk' '
+	clear_ssh &&
+	ln -s src.git dir:123 &&
+	git clone dir:123 dir_123 &&
+	expect_ssh none
+'
+
+test_expect_success SYMLINKS '[dir:234]:src on disk' '
+	clear_ssh &&
+	ln -s src.git [dir:234]:src &&
+	git clone [dir:234]:src dir_234_src &&
+	expect_ssh none
+'
+
+test_expect_success 'ssh://host.xz/~user/repo' '
+	clear_ssh &&
+	git clone "ssh://host.xz/~user/repo" user-repo
+	expect_ssh host.xz "~user/repo"
+'
+
+test_expect_success 'ssh://host.xz:22/~user/repo' '
+	clear_ssh &&
+	git clone "ssh://host.xz:22/~user/repo" user-repo
+	expect_ssh "-p 22 host.xz" "~user/repo"
+'
+
+test_expect_success 'ssh://[::1]:22/~user/repo' '
+	clear_ssh &&
+	git clone "ssh://[::1]:22/~user/repo" user-repo6
+	expect_ssh "-p 22 ::1" "~user/repo"
+'
 
 test_done

==============
And we need this on top of Duys patch:

diff --git a/connect.c b/connect.c
index e8473f3..09be2b3 100644
--- a/connect.c
+++ b/connect.c

@@ -611,7 +611,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
                end = host;
 
        path = strchr(end, c);
-       if (path && !has_dos_drive_prefix(end)) {
+       if (path && host != path && !has_dos_drive_prefix(end)) {
                if (c == ':') {
       if (host != url || path < strchrnul(host, '/')) {
                                protocol = PROTO_SSH;

On Sun, Oct 13, 2013 at 10:00:12PM +0200, Torsten Bögershausen wrote:

On 05.10.13 21:48, Torsten Bögershausen wrote:

quoted

On 2013-10-03 03.31, Jeff King wrote:

quoted

  http://article.gmane.org/gmane.comp.version-control.git/235473

What do we think about extending the test a little bit:

I never mind more tests, but note that my tests are now part of Duy's
8d3d28f, so you would want to build on top.

Mine were in t5601...should these go there, too, or is there a reason to
do it in t5602?

+test_expect_success 'setup ssh wrapper' '
[...]
+clear_ssh () {
[...]
+expect_ssh () {

If we're going to put these in multiple spots, it may be time to factor
them out to lib-ssh.sh or similar (I _almost_ did in my initial patch,
but since there was only one caller, I refrained).

+# git clone could fail, so break the && chain and ignore the exit code
+# clone local
+test_expect_success './foo:bar is not ssh' '
+	clear_ssh &&
+	git clone "./foo:bar" foobar
+	expect_ssh none
+'

Please use test_might_fail instead of breaking the &&-chaining. I'm not
sure I understand why it might fail, though. If it is because foo:bar
does not exist, then please create it (and guard it appropriately with
"NOT_MINGW,NOT_CYGWIN" as the test in t5601 does). Or are we trying to
test the behavior when the path does not exist? In that case, I think we
would want test_must_fail, along with expect_ssh (to make sure that we
couldn't proceed, but that we didn't try to use ssh).

+test_expect_success './[nohost:123]:src is not ssh' '
[...]
+test_expect_success '[nohost:234] is not ssh' '
[...]
+test_expect_success ':345 is not ssh' '
[...]
+test_expect_success '456: is not ssh' '

These all make sense from to me (though I admit I did not even know
about the []-syntax until this thread, so there may be something I am
missing).

+test_expect_success 'myhost:567 is ssh' '
[...]
+test_expect_success '[myhost:678]:src is ssh' '

These two are redundant with what's in t5601 already.

+#clone url looks like ssh, but is on disk
+test_expect_success SYMLINKS 'dir:123 on disk' '
+	clear_ssh &&
+	ln -s src.git dir:123 &&
+	git clone dir:123 dir_123 &&
+	expect_ssh none
+'
+
+test_expect_success SYMLINKS '[dir:234]:src on disk' '
+	clear_ssh &&
+	ln -s src.git [dir:234]:src &&
+	git clone [dir:234]:src dir_234_src &&
+	expect_ssh none
+'

I think you may need extra prerequisites here for systems that support
symlinks, but can't handle colons in paths (cygwin on a sane
filesystem?). Also, this first is redundant with what's in t5601 now, I
think.

+test_expect_success 'ssh://host.xz/~user/repo' '
[...]
+test_expect_success 'ssh://host.xz:22/~user/repo' '
[...]
+test_expect_success 'ssh://[::1]:22/~user/repo' '

Looks sensible.

I'm not clear on which case this was meant to affect. When you write a
commit message, it should be more obvious. ;) But you may also want to
introduce the battery of tests (most of which pass) in one commit, and
then have a follow-up which adds the new test (or flips it from
expect_failure to expect_success).

-Peff

On 15.10.13 02:12, Jeff King wrote:

On Sun, Oct 13, 2013 at 10:00:12PM +0200, Torsten Bögershausen wrote:

quoted

On 05.10.13 21:48, Torsten Bögershausen wrote:

quoted

On 2013-10-03 03.31, Jeff King wrote:

quoted

  http://article.gmane.org/gmane.comp.version-control.git/235473

What do we think about extending the test a little bit:

I never mind more tests, but note that my tests are now part of Duy's
8d3d28f, so you would want to build on top.

quoted

diff --git a/t/t5602-clone-remote-exec.sh b/t/t5602-clone-remote-exec.sh
index 3f353d9..790896f 100755
--- a/t/t5602-clone-remote-exec.sh
+++ b/t/t5602-clone-remote-exec.sh

Mine were in t5601...should these go there, too, or is there a reason to
do it in t5602?

I confused 5601 with 5602, started from there, ended up in a mess.
Thanks for saving me.
Using the comments, I'll send a real patch on top of pu.
/Torsten