Re: [PATCH] clone: allow cloning local paths with colons in them
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 1007...@gmail.com Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- 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 p...@peff.net 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 pgp6OyxPsvqVk.pgp Description: PGP signature
Re: [PATCH] clone: allow cloning local paths with colons in them
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
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;
}
That was fairly hard to grok. Is that equivalent to this?
if (c == ':' path strchrnul(host, '/')) {
/* is the first slash past the colon? */
protocol = PROTO_SSH;
*path++ = '\0';
} else {
path = end;
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clone: allow cloning local paths with colons in them
On Sun, Apr 28, 2013 at 4:16 AM, Junio C Hamano gits...@pobox.com wrote:
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
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
}
That was fairly hard to grok. Is that equivalent to this?
if (c == ':' path strchrnul(host, '/')) {
/* is the first slash past the colon? */
protocol = PROTO_SSH;
*path++ = '\0';
} else {
path = end;
}
The original code is already hard to grok so I may be mistaken here.
But I think it's not the same. For the case when c == '/', it will do
path = end;, which is unintended. It should keep the current path
value (i.e. == strchr(end, '/')). The use of strchrnul(host, '/') is
good though.
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clone: allow cloning local paths with colons in them
On Sat, Apr 27, 2013 at 8:19 PM, Duy Nguyen pclo...@gmail.com wrote:
On Sun, Apr 28, 2013 at 4:16 AM, Junio C Hamano gits...@pobox.com wrote:
That was fairly hard to grok. Is that equivalent to this?
if (c == ':' path strchrnul(host, '/')) {
/* is the first slash past the colon? */
protocol = PROTO_SSH;
*path++ = '\0';
} else {
path = end;
}
The original code is already hard to grok so I may be mistaken here.
But I think it's not the same. For the case when c == '/', it will do
path = end;, which is unintended. It should keep the current path
value (i.e. == strchr(end, '/')). The use of strchrnul(host, '/') is
good though.
Do you want to take Windows '\' into account also?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clone: allow cloning local paths with colons in them
On Sun, Apr 28, 2013 at 8:48 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Sat, Apr 27, 2013 at 8:19 PM, Duy Nguyen pclo...@gmail.com wrote:
On Sun, Apr 28, 2013 at 4:16 AM, Junio C Hamano gits...@pobox.com wrote:
That was fairly hard to grok. Is that equivalent to this?
if (c == ':' path strchrnul(host, '/')) {
/* is the first slash past the colon? */
protocol = PROTO_SSH;
*path++ = '\0';
} else {
path = end;
}
The original code is already hard to grok so I may be mistaken here.
But I think it's not the same. For the case when c == '/', it will do
path = end;, which is unintended. It should keep the current path
value (i.e. == strchr(end, '/')). The use of strchrnul(host, '/') is
good though.
Do you want to take Windows '\' into account also?
Windows path does not allow ':' so it's a non-issue in the first place.
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] clone: allow cloning local paths with colons in them
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 1007...@gmail.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
On Mon, Apr 22, 2013 at 10:35 PM, Jeff King p...@peff.net 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
