Re: [BUG] Filenames with single colon being treated as remote repository

Jeff King [off-list ref] writes:

On Sat, Apr 20, 2013 at 11:05:39PM -0700, Jonathan Nieder wrote:

quoted

quoted

    Cloning into 'new-baz'...
    ssh: Could not resolve hostname /tmp/foo: Success
    fatal: Could not read from remote repository.

Here's a toy patch.  I haven't thought carefully about whether it's a
good idea, but maybe it can be useful for thinking about that.

Still needs documentation and tests.

My main worry is that the proposed rule for when an argument is
treated as a local path is hard to explain.  There's some precedent in
handling of bundles, though.  What do you think?

I think the rule could be something like:

  1. If it looks like a URL ("^scheme://"), it is.

  2. Otherwise, if it is a path in the filesystem, it is.

  3. Otherwise, if it has a colon, it's host:path

  4. Otherwise, barf.

where the interesting bit is the ordering of 2 and 3.  It seems like
"git clone" follows the order above with get_repo_path. But we do not
seem to follow it in git_connect, where we prefer 3 over 2.

At least for a string whose "host" part does not have any slash,
switching the rules 2 and 3 in git_connect() would be a regression,
no?  "frotz:/srv/git/git.git" has been the way to talk to host frotz
for a long time, and if you want to talk to a local directory that
is a subdirectory of "frotz:/" directory you have in your $cwd, you
can disambiguate by saying "./frotz:/srv/git/git.git" or something.

On Sun, Apr 21, 2013 at 11:01:58AM -0700, Junio C Hamano wrote:

quoted

I think the rule could be something like:

  1. If it looks like a URL ("^scheme://"), it is.

  2. Otherwise, if it is a path in the filesystem, it is.

  3. Otherwise, if it has a colon, it's host:path

  4. Otherwise, barf.

where the interesting bit is the ordering of 2 and 3.  It seems like
"git clone" follows the order above with get_repo_path. But we do not
seem to follow it in git_connect, where we prefer 3 over 2.

At least for a string whose "host" part does not have any slash,
switching the rules 2 and 3 in git_connect() would be a regression,
no?  "frotz:/srv/git/git.git" has been the way to talk to host frotz
for a long time, and if you want to talk to a local directory that
is a subdirectory of "frotz:/" directory you have in your $cwd, you
can disambiguate by saying "./frotz:/srv/git/git.git" or something.

Yeah, it would be a regression for fetch, though "git clone frotz:/srv"
is already broken if that file exists; it turns into `realpath
frotz:/srv` before we even feed it into the fetch machinery.

So I think one reasonable path would be:

  1. Do not treat "host:path" as ssh if "host" has a slash, which should
     not regress anybody. It does not allow unadorned relative paths
     with colons, but it lets you use absolute paths or "./" to
     disambiguate.

  2. Teach git-clone to ask the transport code to parse the source repo
     spec, and decide from that whether it is local or not. That would
     harmonize the implementations and avoid errors when you _did_ mean
     to use ssh, but "host:path" happens to exist in your filesystem. I
     also would not be surprised if there are problems with
     URL-encoding, but maybe clone handles that properly (I didn't
     check).

And the "host contains slash" rule is pretty easy to explain in the
documentation, which is good.

-Peff

Usually "foo:bar" is interpreted as an ssh url. This patch allows to
clone from such paths by putting at least one slash before the colon
(i.e. /path/to/foo:bar or just ./foo:bar).

file://foo:bar should also work, but local optimizations are off in
that case, which may be unwanted. While at there, warn the users about
--local being ignored in this case.

Reported-by: William Giokas <redacted>
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 On Mon, Apr 22, 2013 at 10:35 PM, Jeff King [off-list ref] wrote:
 > So I think one reasonable path would be:
 >
 >   1. Do not treat "host:path" as ssh if "host" has a slash, which should
 >      not regress anybody. It does not allow unadorned relative paths
 >      with colons, but it lets you use absolute paths or "./" to
 >      disambiguate.
 >
 >   2. Teach git-clone to ask the transport code to parse the source repo
 >      spec, and decide from that whether it is local or not. That would
 >      harmonize the implementations and avoid errors when you _did_ mean
 >      to use ssh, but "host:path" happens to exist in your filesystem. I
 >      also would not be surprised if there are problems with
 >      URL-encoding, but maybe clone handles that properly (I didn't
 >      check).
 >
 > And the "host contains slash" rule is pretty easy to explain in the
 > documentation, which is good.

 I totally agree with this. But doing #2 seems to require a bit of
 code reorganization. How about just this for now?

 Documentation/urls.txt | 6 ++++++
 builtin/clone.c        | 2 ++
 connect.c              | 7 +++++--
 t/t5601-clone.sh       | 5 +++++
 4 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/Documentation/urls.txt b/Documentation/urls.txt
index 3ca122f..476e338 100644
--- a/Documentation/urls.txt
+++ b/Documentation/urls.txt

@@ -23,6 +23,12 @@ An alternative scp-like syntax may also be used with the ssh protocol:
 
 - {startsb}user@{endsb}host.xz:path/to/repo.git/
 
+This syntax is only recognized if there are no slashes before the
+first colon. This helps differentiate a local path that contains a
+colon. For example the local path `foo:bar` could be specified as an
+absolute path or `./foo:bar` to avoid being misinterpreted as an ssh
+url.
+
 The ssh and git protocols additionally support ~username expansion:
 
 - ssh://{startsb}user@{endsb}host.xz{startsb}:port{endsb}/~{startsb}user{endsb}/path/to/repo.git/

diff --git a/builtin/clone.c b/builtin/clone.c
index 58fee98..e13da4d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c

@@ -783,6 +783,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	is_local = option_local != 0 && path && !is_bundle;
 	if (is_local && option_depth)
 		warning(_("--depth is ignored in local clones; use file:// instead."));
+	if (option_local > 0 && !is_local)
+		warning(_("--local is ignored"));
 
 	if (argc == 2)
 		dir = xstrdup(argv[1]);

diff --git a/connect.c b/connect.c
index f57efd0..b568f10 100644
--- a/connect.c
+++ b/connect.c

@@ -551,8 +551,11 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 	path = strchr(end, c);
 	if (path && !has_dos_drive_prefix(end)) {
 		if (c == ':') {
-			protocol = PROTO_SSH;
-			*path++ = '\0';
+			if (!strchr(url, '/') || strchr(url, '/') >= path) {
+				protocol = PROTO_SSH;
+				*path++ = '\0';
+			} else
+				path = end;
 		}
 	} else
 		path = end;

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 67869b4..0629149 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh

@@ -280,4 +280,9 @@ test_expect_success 'clone checking out a tag' '
 	test_cmp fetch.expected fetch.actual
 '
 
+test_expect_success NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '
+	cp -R src "foo:bar" &&
+	git clone "./foo:bar" foobar
+'
+
 test_done

-- 
1.8.2.83.gc99314b

On Sat, Apr 27, 2013 at 10:36:18AM +0700, Nguyễn Thái Ngọc Duy wrote:

Usually "foo:bar" is interpreted as an ssh url. This patch allows to
clone from such paths by putting at least one slash before the colon
(i.e. /path/to/foo:bar or just ./foo:bar).

file://foo:bar should also work, but local optimizations are off in
that case, which may be unwanted. While at there, warn the users about
--local being ignored in this case.

Reported-by: William Giokas <redacted>
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---

Working fine at the moment for the local clones (thank you). It looks
nice and clean, to me, and doesn't break any existing functionality I
have. 

Though I did notice that if it is a local file, then you don't actually
need a `/` anywhere at all, because I think git looks to see that it is
a local file first. (This is totally fine, though.)

 On Mon, Apr 22, 2013 at 10:35 PM, Jeff King [off-list ref] wrote:
 > So I think one reasonable path would be:
 >
 >   1. Do not treat "host:path" as ssh if "host" has a slash, which should
 >      not regress anybody. It does not allow unadorned relative paths
 >      with colons, but it lets you use absolute paths or "./" to
 >      disambiguate.
 >
 >   2. Teach git-clone to ask the transport code to parse the source repo
 >      spec, and decide from that whether it is local or not. That would
 >      harmonize the implementations and avoid errors when you _did_ mean
 >      to use ssh, but "host:path" happens to exist in your filesystem. I
 >      also would not be surprised if there are problems with
 >      URL-encoding, but maybe clone handles that properly (I didn't
 >      check).
 >
 > And the "host contains slash" rule is pretty easy to explain in the
 > documentation, which is good.

 I totally agree with this. But doing #2 seems to require a bit of
 code reorganization. How about just this for now?

 Documentation/urls.txt | 6 ++++++
 builtin/clone.c        | 2 ++
 connect.c              | 7 +++++--
 t/t5601-clone.sh       | 5 +++++
 4 files changed, 18 insertions(+), 2 deletions(-)

Thank you,
-- 
William Giokas | KaiSforza
GnuPG Key: 0x73CD09CF
Fingerprint: F73F 50EF BBE2 9846 8306  E6B8 6902 06D8 73CD 09CF

Usually "foo:bar" is interpreted as an ssh url. This patch allows to
clone from such paths by putting at least one slash before the colon
(i.e. /path/to/foo:bar or just ./foo:bar).

file://foo:bar should also work, but local optimizations are off in
that case, which may be unwanted. While at there, warn the users about
--local being ignored in this case.

Reported-by: William Giokas <redacted>
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 Changes from v1: replace strchr with strchrnul.

 Documentation/urls.txt | 6 ++++++
 builtin/clone.c        | 2 ++
 connect.c              | 7 +++++--
 t/t5601-clone.sh       | 5 +++++
 4 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/Documentation/urls.txt b/Documentation/urls.txt
index 3ca122f..476e338 100644
--- a/Documentation/urls.txt
+++ b/Documentation/urls.txt

@@ -23,6 +23,12 @@ An alternative scp-like syntax may also be used with the ssh protocol:
 
 - {startsb}user@{endsb}host.xz:path/to/repo.git/
 
+This syntax is only recognized if there are no slashes before the
+first colon. This helps differentiate a local path that contains a
+colon. For example the local path `foo:bar` could be specified as an
+absolute path or `./foo:bar` to avoid being misinterpreted as an ssh
+url.
+
 The ssh and git protocols additionally support ~username expansion:
 
 - ssh://{startsb}user@{endsb}host.xz{startsb}:port{endsb}/~{startsb}user{endsb}/path/to/repo.git/

diff --git a/builtin/clone.c b/builtin/clone.c
index 58fee98..e13da4d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c

@@ -783,6 +783,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	is_local = option_local != 0 && path && !is_bundle;
 	if (is_local && option_depth)
 		warning(_("--depth is ignored in local clones; use file:// instead."));
+	if (option_local > 0 && !is_local)
+		warning(_("--local is ignored"));
 
 	if (argc == 2)
 		dir = xstrdup(argv[1]);

diff --git a/connect.c b/connect.c
index f57efd0..a0783d4 100644
--- a/connect.c
+++ b/connect.c

@@ -551,8 +551,11 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 	path = strchr(end, c);
 	if (path && !has_dos_drive_prefix(end)) {
 		if (c == ':') {
-			protocol = PROTO_SSH;
-			*path++ = '\0';
+			if (path < strchrnul(host, '/')) {
+				protocol = PROTO_SSH;
+				*path++ = '\0';
+			} else /* '/' in the host part, assume local path */
+				path = end;
 		}
 	} else
 		path = end;

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 67869b4..0629149 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh

@@ -280,4 +280,9 @@ test_expect_success 'clone checking out a tag' '
 	test_cmp fetch.expected fetch.actual
 '
 
+test_expect_success NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '
+	cp -R src "foo:bar" &&
+	git clone "./foo:bar" foobar
+'
+
 test_done

-- 
1.8.2.83.gc99314b