On Sun, Oct 13, 2013 at 10:00:12PM +0200, Torsten Bögershausen wrote: > On 05.10.13 21:48, Torsten Bögershausen wrote: > > On 2013-10-03 03.31, Jeff King wrote: > >> > >> 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. > 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. > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html