{"thread":{"id":"35041","subject":"[PATCH] clone: local URLs are not for ssh","startedAt":"2013-09-28T19:37:21Z","lastAt":"2013-10-15T07:22:46Z","messageCount":10,"participants":["Torsten Bögershausen","Duy Nguyen","Jeff King","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"228362","messageId":"201309282137.21802.tboegi@web.de","threadId":"35041","inReplyTo":null,"subject":"[PATCH] clone: local URLs are not for ssh","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-09-28T19:37:21Z","receivedAt":"2013-09-28T19:37:21Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\"git clone /foo/bar:baz\" or \"git clone ../foo/bar:baz\"\nare meant to clone from the local file system, and not to clone\nfrom a remote server over git-over-ssh.\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\n---\n connect.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/connect.c b/connect.c\nindex a80ebd3..b382032 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -550,7 +550,8 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t\tend = host;\n \n \tpath = strchr(end, c);\n-\tif (path && !has_dos_drive_prefix(end)) {\n+\tif (path && !has_dos_drive_prefix(end) &&\n+\t    url[0] != '/' && url[0] != '.' ) {\n \t\tif (c == ':') {\n \t\t\tif (path < strchrnul(host, '/')) {\n \t\t\t\tprotocol = PROTO_SSH;\n-- \n1.8.4.457.g424cb08\n"},{"id":"228429","messageId":"CACsJy8B-wA=bX6+E6O6UvX2vEtOwnR1PCMZNoi-q0x_jacB89Q@mail.gmail.com","threadId":"35041","inReplyTo":"201309282137.21802.tboegi@web.de","subject":"Re: [PATCH] clone: local URLs are not for ssh","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-29T00:33:38Z","receivedAt":"2013-09-29T00:33:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Sep 29, 2013 at 2:37 AM, Torsten Bögershausen <tboegi@web.de> wrote:\n> \"git clone /foo/bar:baz\" or \"git clone ../foo/bar:baz\"\n> are meant to clone from the local file system, and not to clone\n> from a remote server over git-over-ssh.\n\nI don't think this is necessary. Commit 6000334 should detect both\ncases fine because both have a slash before the first colon.\n\n>\n> Signed-off-by: Torsten Bögershausen <tboegi@web.de>\n> ---\n>  connect.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/connect.c b/connect.c\n> index a80ebd3..b382032 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -550,7 +550,8 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n>                 end = host;\n>\n>         path = strchr(end, c);\n> -       if (path && !has_dos_drive_prefix(end)) {\n> +       if (path && !has_dos_drive_prefix(end) &&\n> +           url[0] != '/' && url[0] != '.' ) {\n>                 if (c == ':') {\n>                         if (path < strchrnul(host, '/')) {\n>                                 protocol = PROTO_SSH;\n> --\n> 1.8.4.457.g424cb08\n>\n\n\n\n-- \nDuy\n"},{"id":"228519","messageId":"524C6885.8020602@web.de","threadId":"35041","inReplyTo":"CACsJy8B-wA=bX6+E6O6UvX2vEtOwnR1PCMZNoi-q0x_jacB89Q@mail.gmail.com","subject":"Re: [PATCH] clone: local URLs are not for ssh","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-10-02T18:40:05Z","receivedAt":"2013-10-02T18:40:05Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2013-09-29 02.33, Duy Nguyen wrote:\n> On Sun, Sep 29, 2013 at 2:37 AM, Torsten Bögershausen <tboegi@web.de> wrote:\n>> \"git clone /foo/bar:baz\" or \"git clone ../foo/bar:baz\"\n>> are meant to clone from the local file system, and not to clone\n>> from a remote server over git-over-ssh.\n> \n> I don't think this is necessary. Commit 6000334 should detect both\n> cases fine because both have a slash before the first colon.\nSorry for the noise, I noticed it when I was trying to construct test cases.\n\nWhat do we think about adding this at the end of t5505:\n\n\n\ntest_expect_success 'fetch fail [noexistinghost0:2223]:blink.git' '\n\t(\n\t\t! git fetch [noexistinghost0:2223]:blink.git 2>err &&\n\t\tgrep ssh err &&\n\t\trm err\n\t)\n'\n\ntest_expect_success 'fetch fail noexistinghost1:2223:blink.git' '\n\t(\n\t\t! git fetch \"noexistinghost1:2223:blink.git\" 2>err &&\n\t\tgrep ssh err &&\n\t\trm err\n\t)\n'\n\ntest_expect_success 'fetch fail noexistinghost2:2223' '\n\t(\n\t\t! git fetch \"noexistinghost2:2223\" 2>err &&\n\t\tgrep ssh err &&\n\t\trm err\n\t)\n'\ntest_expect_success 'fetch fail ./noexistinghost4:2223\"' '\n\t(\n\t\t! git fetch \"./noexistinghost4:2223\" 2>err &&\n\t\tgrep \"does not appear to be a git repository\" err &&\n\t\trm err\n\t)\n'\n"},{"id":"228528","messageId":"CACsJy8DjPiwpMvLe2p+dGBdpU6iTg2my7tddBsQwLnHiHdG8wQ@mail.gmail.com","threadId":"35041","inReplyTo":"524C6885.8020602@web.de","subject":"Re: [PATCH] clone: local URLs are not for ssh","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-10-03T01:01:23Z","receivedAt":"2013-10-03T01:01:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Oct 3, 2013 at 1:40 AM, Torsten Bögershausen <tboegi@web.de> wrote:\n> On 2013-09-29 02.33, Duy Nguyen wrote:\n>> On Sun, Sep 29, 2013 at 2:37 AM, Torsten Bögershausen <tboegi@web.de> wrote:\n>>> \"git clone /foo/bar:baz\" or \"git clone ../foo/bar:baz\"\n>>> are meant to clone from the local file system, and not to clone\n>>> from a remote server over git-over-ssh.\n>>\n>> I don't think this is necessary. Commit 6000334 should detect both\n>> cases fine because both have a slash before the first colon.\n> Sorry for the noise, I noticed it when I was trying to construct test cases.\n>\n> What do we think about adding this at the end of t5505:\n\nAs usual more tests are usually better. But is t5505-remote.sh the\nbest place? That file seems about \"git remote\"..\n\n>\n>\n>\n> test_expect_success 'fetch fail [noexistinghost0:2223]:blink.git' '\n>         (\n>                 ! git fetch [noexistinghost0:2223]:blink.git 2>err &&\n>                 grep ssh err &&\n>                 rm err\n>         )\n> '\n>\n> test_expect_success 'fetch fail noexistinghost1:2223:blink.git' '\n>         (\n>                 ! git fetch \"noexistinghost1:2223:blink.git\" 2>err &&\n>                 grep ssh err &&\n>                 rm err\n>         )\n> '\n>\n> test_expect_success 'fetch fail noexistinghost2:2223' '\n>         (\n>                 ! git fetch \"noexistinghost2:2223\" 2>err &&\n>                 grep ssh err &&\n>                 rm err\n>         )\n> '\n> test_expect_success 'fetch fail ./noexistinghost4:2223\"' '\n>         (\n>                 ! git fetch \"./noexistinghost4:2223\" 2>err &&\n>                 grep \"does not appear to be a git repository\" err &&\n>                 rm err\n>         )\n> '\n>\n>\n>\n>\n>\n>\n>\n>\n>\n>\n>\n>\n\n\n\n-- \nDuy\n"},{"id":"228529","messageId":"20131003013127.GA7917@sigill.intra.peff.net","threadId":"35041","inReplyTo":"CACsJy8DjPiwpMvLe2p+dGBdpU6iTg2my7tddBsQwLnHiHdG8wQ@mail.gmail.com","subject":"Re: [PATCH] clone: local URLs are not for ssh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-03T01:31:27Z","receivedAt":"2013-10-03T01:31:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 03, 2013 at 08:01:23AM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> > Sorry for the noise, I noticed it when I was trying to construct test cases.\n> >\n> > What do we think about adding this at the end of t5505:\n> \n> As usual more tests are usually better. But is t5505-remote.sh the\n> best place? That file seems about \"git remote\"..\n\nYeah, agreed. How about at the end of t5601, after the ssh wrapper I set\nup here:\n\n  http://article.gmane.org/gmane.comp.version-control.git/235473\n\nI don't know of Jonathan squashed those in to your commit...neither is\nin his 'pu' yet.\n\n> > test_expect_success 'fetch fail [noexistinghost0:2223]:blink.git' '\n> >         (\n> >                 ! git fetch [noexistinghost0:2223]:blink.git 2>err &&\n> >                 grep ssh err &&\n> >                 rm err\n> >         )\n> > '\n\nThis one looks like basically the same test I added in the message\nabove (except because of the ssh wrapper, we can check that it did\nindeed try to ssh to noexistinghost0:2223).\n\nThe other tests can check that we fed ssh various host/port/path\ncombinations. I'm not clear on what we're expecting, though...\n\n> > test_expect_success 'fetch fail noexistinghost1:2223:blink.git' '\n> >         (\n> >                 ! git fetch \"noexistinghost1:2223:blink.git\" 2>err &&\n> >                 grep ssh err &&\n> >                 rm err\n> >         )\n> > '\n\nWe are expecting this to be host=noexistinghost1, and\npath=2223:blink.git?\n\n> > test_expect_success 'fetch fail noexistinghost2:2223' '\n> >         (\n> >                 ! git fetch \"noexistinghost2:2223\" 2>err &&\n> >                 grep ssh err &&\n> >                 rm err\n> >         )\n> > '\n\nAnd this is host=noexistinghost2, path=2223?\n\n> > test_expect_success 'fetch fail ./noexistinghost4:2223\"' '\n> >         (\n> >                 ! git fetch \"./noexistinghost4:2223\" 2>err &&\n> >                 grep \"does not appear to be a git repository\" err &&\n> >                 rm err\n> >         )\n> > '\n\nAnd this one we would be checking that ssh is _not_ used. It seems\nredundant with the \"./foo:bar\" test already in t5601, but perhaps it is\nworth double-checking the numeric path. It would be more robust if we\nactually had a repo called \"noexistinghost4:2223\" and checked that we\ndid clone it, as the existing test does (maybe that test can just\n\"s/bar/2223/\" ?).\n\n-Peff\n"},{"id":"228570","messageId":"52506D15.9040206@web.de","threadId":"35041","inReplyTo":"20131003013127.GA7917@sigill.intra.peff.net","subject":"Re: [PATCH] clone: local URLs are not for ssh","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-10-05T19:48:37Z","receivedAt":"2013-10-05T19:48:37Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2013-10-03 03.31, Jeff King wrote:\n> On Thu, Oct 03, 2013 at 08:01:23AM +0700, Nguyen Thai Ngoc Duy wrote:\n> \n>>> Sorry for the noise, I noticed it when I was trying to construct test cases.\n>>>\n>>> What do we think about adding this at the end of t5505:\n>>\n>> As usual more tests are usually better. But is t5505-remote.sh the\n>> best place? That file seems about \"git remote\"..\n> \n> Yeah, agreed. How about at the end of t5601, after the ssh wrapper I set\n> up here:\n> \n>   http://article.gmane.org/gmane.comp.version-control.git/235473\n\nThanks for the review & pointer.\nTo get it working, a little tweak was needed here, please see below.\n\ndiff --git a/t/t5602-clone-remote-exec.sh b/t/t5602-clone-remote-exec.sh\nindex d232e94..37464aa 100755\n--- a/t/t5602-clone-remote-exec.sh\n+++ b/t/t5602-clone-remote-exec.sh\n@@ -62,21 +62,20 @@ expect_ssh () {\n \n test_expect_success 'cloning myhost:src uses ssh' '\n        clear_ssh &&\n-       git clone myhost:src ssh-clone &&\n+       ! git clone myhost:src ssh-clone &&\n        expect_ssh myhost src\n '\n \n-test_expect_success NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '\n+test_expect_success SYMLINKS,NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '\n        clear_ssh &&\n-       cp -R src \"foo:bar\" &&\n-       git clone \"./foo:bar\" foobar\n-       git clone \"./foo:bar\" foobar &&\n+       ln -s src \"foo:bar\" &&\n+       ! git clone \"./foo:bar\" foobar &&\n        expect_ssh none\n '\n \n test_expect_success 'bracketed hostnames are still ssh' '\n        clear_ssh &&\n-       git clone \"[myhost:123]:src\" ssh-bracket-clone &&\n+       ! git clone \"[myhost:123]:src\" ssh-bracket-clone &&\n        expect_ssh myhost:123 src\n '\n"},{"id":"228571","messageId":"vpqfvsfbgn2.fsf@anie.imag.fr","threadId":"35041","inReplyTo":"52506D15.9040206@web.de","subject":"Re: [PATCH] clone: local URLs are not for ssh","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-10-05T20:35:13Z","receivedAt":"2013-10-05T20:35:13Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n>  test_expect_success 'cloning myhost:src uses ssh' '\n>         clear_ssh &&\n> -       git clone myhost:src ssh-clone &&\n> +       ! git clone myhost:src ssh-clone &&\n\nThis succeeds if Git segfaults for example. Please, use test_must_fail\ninstead of \"!\".\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"228907","messageId":"525AFBCC.4080303@web.de","threadId":"35041","inReplyTo":"52506D15.9040206@web.de","subject":"Re: [PATCH] clone: local URLs are not for ssh","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-10-13T20:00:12Z","receivedAt":"2013-10-13T20:00:12Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 05.10.13 21:48, Torsten Bögershausen wrote:\n> On 2013-10-03 03.31, Jeff King wrote:\n>>\n>>   http://article.gmane.org/gmane.comp.version-control.git/235473\nWhat do we think about extending the test a little bit:\n\n\ngit diff 771cf1dab9303bab3c8198b8b6 -- t5602-clone-remote-exec.sh \n\ndiff --git a/t/t5602-clone-remote-exec.sh b/t/t5602-clone-remote-exec.sh\nindex 3f353d9..790896f 100755\n--- a/t/t5602-clone-remote-exec.sh\n+++ b/t/t5602-clone-remote-exec.sh\n@@ -30,5 +30,124 @@ test_expect_success 'clone calls specified git upload-pack with -u option' '\n \techo \"localhost ./something/bin/git-upload-pack '\\''/path/to/repo'\\''\" >expected &&\n \ttest_cmp expected not_ssh_output\n '\n+test_expect_success 'setup ssh wrapper' '\n+\twrite_script \"$TRASH_DIRECTORY/ssh-wrapper\" <<-\\EOF &&\n+\techo >>\"$TRASH_DIRECTORY/ssh-output\" \"ssh: $*\" &&\n+\t# throw away all but the last argument, which should be the\n+\t# command\n+\twhile test $# -gt 1; do shift; done\n+\teval \"$1\"\n+\tEOF\n+\n+\tGIT_SSH=\"$TRASH_DIRECTORY/ssh-wrapper\" &&\n+\texport GIT_SSH &&\n+\texport TRASH_DIRECTORY\n+'\n+\n+clear_ssh () {\n+\t>\"$TRASH_DIRECTORY/ssh-output\"\n+}\n+\n+expect_ssh () {\n+\t{\n+\t\tcase \"$1\" in\n+\t\tnone)\n+\t\t\t;;\n+\t\t*)\n+\t\t\techo \"ssh: $1 git-upload-pack '$2'\"\n+\t\tesac\n+\t} >\"$TRASH_DIRECTORY/ssh-expect\" &&\n+\t(cd \"$TRASH_DIRECTORY\" && test_cmp ssh-expect ssh-output)\n+}\n+\n+test_expect_success 'create src.git' '\n+\tmkdir src.git &&\n+\t( \n+\t\tcd src.git &&\n+\t\tgit init &&\n+\t\t>file &&\n+\t\tgit add file &&\n+\t\tgit commit -m \"add file\"\n+\t)\n+'\n+\n+# git clone could fail, so break the && chain and ignore the exit code\n+# clone local\n+test_expect_success './foo:bar is not ssh' '\n+\tclear_ssh &&\n+\tgit clone \"./foo:bar\" foobar\n+\texpect_ssh none\n+'\n+\n+test_expect_success './[nohost:123]:src is not ssh' '\n+\tclear_ssh &&\n+\tgit clone \"./[nohost:123]:src\" 1_2_3\n+\texpect_ssh none\n+'\n+\n+test_expect_success '[nohost:234] is not ssh' '\n+\tclear_ssh &&\n+\tgit clone \"[nohost:234]\" 2_3_4\n+\texpect_ssh none\n+'\n+\n+test_expect_success ':345 is not ssh' '\n+\tclear_ssh &&\n+\tgit clone \":345\" 3_4_5 \n+\texpect_ssh none\n+'\n+\n+test_expect_success '456: is not ssh' '\n+\tclear_ssh &&\n+\tgit clone \"456:\" 4_5_6 \n+\texpect_ssh none\n+'\n+\n+# clone via ssh\n+# the expect_ssh checks that git clone tried to use ssh\n+test_expect_success 'myhost:567 is ssh' '\n+\tclear_ssh &&\n+\tgit clone myhost:567 myhost_567\n+\texpect_ssh myhost 567\n+'\n+\n+test_expect_success '[myhost:678]:src is ssh' '\n+\tclear_ssh &&\n+\tgit clone \"[myhost:678]:src\" myhost_678\n+\texpect_ssh myhost:678 src\n+'\n+\n+#clone url looks like ssh, but is on disk\n+test_expect_success SYMLINKS 'dir:123 on disk' '\n+\tclear_ssh &&\n+\tln -s src.git dir:123 &&\n+\tgit clone dir:123 dir_123 &&\n+\texpect_ssh none\n+'\n+\n+test_expect_success SYMLINKS '[dir:234]:src on disk' '\n+\tclear_ssh &&\n+\tln -s src.git [dir:234]:src &&\n+\tgit clone [dir:234]:src dir_234_src &&\n+\texpect_ssh none\n+'\n+\n+test_expect_success 'ssh://host.xz/~user/repo' '\n+\tclear_ssh &&\n+\tgit clone \"ssh://host.xz/~user/repo\" user-repo\n+\texpect_ssh host.xz \"~user/repo\"\n+'\n+\n+test_expect_success 'ssh://host.xz:22/~user/repo' '\n+\tclear_ssh &&\n+\tgit clone \"ssh://host.xz:22/~user/repo\" user-repo\n+\texpect_ssh \"-p 22 host.xz\" \"~user/repo\"\n+'\n+\n+test_expect_success 'ssh://[::1]:22/~user/repo' '\n+\tclear_ssh &&\n+\tgit clone \"ssh://[::1]:22/~user/repo\" user-repo6\n+\texpect_ssh \"-p 22 ::1\" \"~user/repo\"\n+'\n \n test_done\n\n==============\nAnd we need this on top of Duys patch:\n\ndiff --git a/connect.c b/connect.c\nindex e8473f3..09be2b3 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -611,7 +611,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n                end = host;\n \n        path = strchr(end, c);\n-       if (path && !has_dos_drive_prefix(end)) {\n+       if (path && host != path && !has_dos_drive_prefix(end)) {\n                if (c == ':') {\n       if (host != url || path < strchrnul(host, '/')) {\n                                protocol = PROTO_SSH;\n"},{"id":"228982","messageId":"20131015001259.GB10415@sigill.intra.peff.net","threadId":"35041","inReplyTo":"525AFBCC.4080303@web.de","subject":"Re: [PATCH] clone: local URLs are not for ssh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-15T00:12:59Z","receivedAt":"2013-10-15T00:12:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 13, 2013 at 10:00:12PM +0200, Torsten Bögershausen wrote:\n\n> On 05.10.13 21:48, Torsten Bögershausen wrote:\n> > On 2013-10-03 03.31, Jeff King wrote:\n> >>\n> >>   http://article.gmane.org/gmane.comp.version-control.git/235473\n> What do we think about extending the test a little bit:\n\nI never mind more tests, but note that my tests are now part of Duy's\n8d3d28f, so you would want to build on top.\n\n> diff --git a/t/t5602-clone-remote-exec.sh b/t/t5602-clone-remote-exec.sh\n> index 3f353d9..790896f 100755\n> --- a/t/t5602-clone-remote-exec.sh\n> +++ b/t/t5602-clone-remote-exec.sh\n\nMine were in t5601...should these go there, too, or is there a reason to\ndo it in t5602?\n\n> +test_expect_success 'setup ssh wrapper' '\n> [...]\n> +clear_ssh () {\n> [...]\n> +expect_ssh () {\n\nIf we're going to put these in multiple spots, it may be time to factor\nthem out to lib-ssh.sh or similar (I _almost_ did in my initial patch,\nbut since there was only one caller, I refrained).\n\n> +# git clone could fail, so break the && chain and ignore the exit code\n> +# clone local\n> +test_expect_success './foo:bar is not ssh' '\n> +\tclear_ssh &&\n> +\tgit clone \"./foo:bar\" foobar\n> +\texpect_ssh none\n> +'\n\nPlease use test_might_fail instead of breaking the &&-chaining. I'm not\nsure I understand why it might fail, though. If it is because foo:bar\ndoes not exist, then please create it (and guard it appropriately with\n\"NOT_MINGW,NOT_CYGWIN\" as the test in t5601 does). Or are we trying to\ntest the behavior when the path does not exist? In that case, I think we\nwould want test_must_fail, along with expect_ssh (to make sure that we\ncouldn't proceed, but that we didn't try to use ssh).\n\n> +test_expect_success './[nohost:123]:src is not ssh' '\n> [...]\n> +test_expect_success '[nohost:234] is not ssh' '\n> [...]\n> +test_expect_success ':345 is not ssh' '\n> [...]\n> +test_expect_success '456: is not ssh' '\n\nThese all make sense from to me (though I admit I did not even know\nabout the []-syntax until this thread, so there may be something I am\nmissing).\n\n> +test_expect_success 'myhost:567 is ssh' '\n> [...]\n> +test_expect_success '[myhost:678]:src is ssh' '\n\nThese two are redundant with what's in t5601 already.\n\n> +#clone url looks like ssh, but is on disk\n> +test_expect_success SYMLINKS 'dir:123 on disk' '\n> +\tclear_ssh &&\n> +\tln -s src.git dir:123 &&\n> +\tgit clone dir:123 dir_123 &&\n> +\texpect_ssh none\n> +'\n> +\n> +test_expect_success SYMLINKS '[dir:234]:src on disk' '\n> +\tclear_ssh &&\n> +\tln -s src.git [dir:234]:src &&\n> +\tgit clone [dir:234]:src dir_234_src &&\n> +\texpect_ssh none\n> +'\n\nI think you may need extra prerequisites here for systems that support\nsymlinks, but can't handle colons in paths (cygwin on a sane\nfilesystem?). Also, this first is redundant with what's in t5601 now, I\nthink.\n\n> +test_expect_success 'ssh://host.xz/~user/repo' '\n> [...]\n> +test_expect_success 'ssh://host.xz:22/~user/repo' '\n> [...]\n> +test_expect_success 'ssh://[::1]:22/~user/repo' '\n\nLooks sensible.\n\n> And we need this on top of Duys patch:\n> \n> diff --git a/connect.c b/connect.c\n> index e8473f3..09be2b3 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -611,7 +611,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n>                 end = host;\n>  \n>         path = strchr(end, c);\n> -       if (path && !has_dos_drive_prefix(end)) {\n> +       if (path && host != path && !has_dos_drive_prefix(end)) {\n>                 if (c == ':') {\n>        if (host != url || path < strchrnul(host, '/')) {\n>                                 protocol = PROTO_SSH;\n\nI'm not clear on which case this was meant to affect. When you write a\ncommit message, it should be more obvious. ;) But you may also want to\nintroduce the battery of tests (most of which pass) in one commit, and\nthen have a follow-up which adds the new test (or flips it from\nexpect_failure to expect_success).\n\n-Peff\n"},{"id":"228995","messageId":"525CED46.3050006@web.de","threadId":"35041","inReplyTo":"20131015001259.GB10415@sigill.intra.peff.net","subject":"Re: [PATCH] clone: local URLs are not for ssh","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-10-15T07:22:46Z","receivedAt":"2013-10-15T07:22:46Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 15.10.13 02:12, Jeff King wrote:\n> On Sun, Oct 13, 2013 at 10:00:12PM +0200, Torsten Bögershausen wrote:\n> \n>> On 05.10.13 21:48, Torsten Bögershausen wrote:\n>>> On 2013-10-03 03.31, Jeff King wrote:\n>>>>\n>>>>   http://article.gmane.org/gmane.comp.version-control.git/235473\n>> What do we think about extending the test a little bit:\n> \n> I never mind more tests, but note that my tests are now part of Duy's\n> 8d3d28f, so you would want to build on top.\n> \n>> diff --git a/t/t5602-clone-remote-exec.sh b/t/t5602-clone-remote-exec.sh\n>> index 3f353d9..790896f 100755\n>> --- a/t/t5602-clone-remote-exec.sh\n>> +++ b/t/t5602-clone-remote-exec.sh\n> \n> Mine were in t5601...should these go there, too, or is there a reason to\n> do it in t5602?\nI confused 5601 with 5602, started from there, ended up in a mess.\nThanks for saving me.\nUsing the comments, I'll send a real patch on top of pu.\n/Torsten\n"}]}