Re: [PATCH] clone: local URLs are not for ssh
From: Jeff King <hidden>
Date: 2016-06-15 22:59:00
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: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 hunk ↗ jump to hunk
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?
+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.
quoted hunk ↗ jump to hunk
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;
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