* [PATCH] clone: local URLs are not for ssh @ 2013-09-28 19:37 Torsten Bögershausen 2013-09-29 0:33 ` Duy Nguyen 0 siblings, 1 reply; 10+ messages in thread From: Torsten Bögershausen @ 2013-09-28 19:37 UTC (permalink / raw) To: git; +Cc: pclouds "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 <tboegi@web.de> --- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] clone: local URLs are not for ssh 2013-09-28 19:37 [PATCH] clone: local URLs are not for ssh Torsten Bögershausen @ 2013-09-29 0:33 ` Duy Nguyen 2013-10-02 18:40 ` Torsten Bögershausen 0 siblings, 1 reply; 10+ messages in thread From: Duy Nguyen @ 2013-09-29 0:33 UTC (permalink / raw) To: Torsten Bögershausen; +Cc: Git Mailing List On Sun, Sep 29, 2013 at 2:37 AM, Torsten Bögershausen <tboegi@web.de> 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. > > Signed-off-by: Torsten Bögershausen <tboegi@web.de> > --- > 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 > -- Duy ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] clone: local URLs are not for ssh 2013-09-29 0:33 ` Duy Nguyen @ 2013-10-02 18:40 ` Torsten Bögershausen 2013-10-03 1:01 ` Duy Nguyen 0 siblings, 1 reply; 10+ messages in thread From: Torsten Bögershausen @ 2013-10-02 18:40 UTC (permalink / raw) To: Duy Nguyen, Torsten Bögershausen; +Cc: Git Mailing List On 2013-09-29 02.33, Duy Nguyen wrote: > On Sun, Sep 29, 2013 at 2:37 AM, Torsten Bögershausen <tboegi@web.de> 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. 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 ) ' ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] clone: local URLs are not for ssh 2013-10-02 18:40 ` Torsten Bögershausen @ 2013-10-03 1:01 ` Duy Nguyen 2013-10-03 1:31 ` Jeff King 0 siblings, 1 reply; 10+ messages in thread From: Duy Nguyen @ 2013-10-03 1:01 UTC (permalink / raw) To: Torsten Bögershausen; +Cc: Git Mailing List On Thu, Oct 3, 2013 at 1:40 AM, Torsten Bögershausen <tboegi@web.de> wrote: > On 2013-09-29 02.33, Duy Nguyen wrote: >> On Sun, Sep 29, 2013 at 2:37 AM, Torsten Bögershausen <tboegi@web.de> 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. > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] clone: local URLs are not for ssh 2013-10-03 1:01 ` Duy Nguyen @ 2013-10-03 1:31 ` Jeff King 2013-10-05 19:48 ` Torsten Bögershausen 0 siblings, 1 reply; 10+ messages in thread From: Jeff King @ 2013-10-03 1:31 UTC (permalink / raw) To: Duy Nguyen; +Cc: Torsten Bögershausen, Git Mailing List On Thu, Oct 03, 2013 at 08:01:23AM +0700, Nguyen Thai Ngoc Duy wrote: > > 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. > > 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... > > 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? > > 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? > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] clone: local URLs are not for ssh 2013-10-03 1:31 ` Jeff King @ 2013-10-05 19:48 ` Torsten Bögershausen 2013-10-05 20:35 ` Matthieu Moy 2013-10-13 20:00 ` Torsten Bögershausen 0 siblings, 2 replies; 10+ messages in thread From: Torsten Bögershausen @ 2013-10-05 19:48 UTC (permalink / raw) To: Jeff King, Duy Nguyen; +Cc: Torsten Bögershausen, Git Mailing List On 2013-10-03 03.31, Jeff King wrote: > On Thu, Oct 03, 2013 at 08:01:23AM +0700, Nguyen Thai Ngoc Duy wrote: > >>> 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 ' ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] clone: local URLs are not for ssh 2013-10-05 19:48 ` Torsten Bögershausen @ 2013-10-05 20:35 ` Matthieu Moy 2013-10-13 20:00 ` Torsten Bögershausen 1 sibling, 0 replies; 10+ messages in thread From: Matthieu Moy @ 2013-10-05 20:35 UTC (permalink / raw) To: Torsten Bögershausen; +Cc: Jeff King, Duy Nguyen, Git Mailing List Torsten Bögershausen <tboegi@web.de> writes: > test_expect_success 'cloning myhost:src uses ssh' ' > clear_ssh && > - git clone myhost:src ssh-clone && > + ! git clone myhost:src ssh-clone && This succeeds if Git segfaults for example. Please, use test_must_fail instead of "!". -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] clone: local URLs are not for ssh 2013-10-05 19:48 ` Torsten Bögershausen 2013-10-05 20:35 ` Matthieu Moy @ 2013-10-13 20:00 ` Torsten Bögershausen 2013-10-15 0:12 ` Jeff King 1 sibling, 1 reply; 10+ messages in thread From: Torsten Bögershausen @ 2013-10-13 20:00 UTC (permalink / raw) To: Torsten Bögershausen, Jeff King, Duy Nguyen, Matthieu Moy Cc: Git Mailing List 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: 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; ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] clone: local URLs are not for ssh 2013-10-13 20:00 ` Torsten Bögershausen @ 2013-10-15 0:12 ` Jeff King 2013-10-15 7:22 ` Torsten Bögershausen 0 siblings, 1 reply; 10+ messages in thread From: Jeff King @ 2013-10-15 0:12 UTC (permalink / raw) To: Torsten Bögershausen; +Cc: Duy Nguyen, Matthieu Moy, Git Mailing List 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] clone: local URLs are not for ssh 2013-10-15 0:12 ` Jeff King @ 2013-10-15 7:22 ` Torsten Bögershausen 0 siblings, 0 replies; 10+ messages in thread From: Torsten Bögershausen @ 2013-10-15 7:22 UTC (permalink / raw) To: Jeff King, Torsten Bögershausen Cc: Duy Nguyen, Matthieu Moy, Git Mailing List On 15.10.13 02:12, Jeff King wrote: > 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? 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-10-15 7:23 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-09-28 19:37 [PATCH] clone: local URLs are not for ssh Torsten Bögershausen 2013-09-29 0:33 ` Duy Nguyen 2013-10-02 18:40 ` Torsten Bögershausen 2013-10-03 1:01 ` Duy Nguyen 2013-10-03 1:31 ` Jeff King 2013-10-05 19:48 ` Torsten Bögershausen 2013-10-05 20:35 ` Matthieu Moy 2013-10-13 20:00 ` Torsten Bögershausen 2013-10-15 0:12 ` Jeff King 2013-10-15 7:22 ` Torsten Bögershausen
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).