Re: [PATCH] clone: local URLs are not for ssh
On Sun, Oct 13, 2013 at 10:00:12PM +0200, Torsten Bögershausen wrote:
Show 5 quoted lines
> 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?
Show 5 quoted lines
> +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).
Show 7 quoted lines
> +# 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).
Show 7 quoted lines
> +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.
Show 14 quoted lines
> +#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.
Show 5 quoted lines
> +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' '
Show 15 quoted lines
> 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
Torsten Bögershausen
