[PATCH V2] git clone: is an URL local or ssh
Commit 8d3d28f5 added test cases for URLs which should be ssh.
Add more tests testing all the combinations:
-IPv4 or IPv6
-path starting with / or with /~
-with and without the ssh:// scheme
Add tests for ssh:// with port number.
When a git repository foo:bar exist, git clone will call
absolute_path() and git_connect() will be called with
something like /home/user/projects/foo:bar
Tighten the test and use foo:bar instead of ./foo:bar,
refactor clear_ssh() and expect_ssh() into test_clone_url().
git clone foo/bar:baz should not be ssh:
Make the rule
if there is a slash before the first colon, it is not ssh
more strict by using the same is_local() in both connect.c
and transport.c. Add a test case.
Bug fixes for corner cases:
- Handle clone of [::1]:/~repo correctly (2e7766655):
Git should call ssh ::1 ~repo, not ssh ::1 /~repo
This was caused by looking at (host != url), which never
worked for host names with literal IPv6 adresses, embedded by []
Support for the [] URLs was introduced in 356bece0a.
- Do not tamper local URLs starting with '[' which have a ']'
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
(This does apply on pu, not on master.
I'm almost sure there are more corner cases, but the
most important things should be covered)
Changes since V1: Comments from Eric Sunshine (thanks)
connect.c| 47 +--
connect.h| 1 +
t/t5601-clone.sh | 98
transport.c | 8 -
4 files changed, 108 insertions(+), 46 deletions(-)
diff --git a/connect.c b/connect.c
index 06e88b0..d61adc9 100644
--- a/connect.c
+++ b/connect.c
@@ -231,6 +231,7 @@ int server_supports(const char *feature)
}
enum protocol {
+ PROTO_LOCAL_OR_SSH = 0,
PROTO_LOCAL = 1,
PROTO_SSH,
PROTO_GIT
@@ -547,6 +548,15 @@ static char *get_port(char *host)
static struct child_process no_fork;
+int is_local(const char *url)
+{
+ const char *colon = strchr(url, ':');
+ const char *slash = strchr(url, '/');
+ return !colon || (slash slash colon) ||
+ has_dos_drive_prefix(url);
+}
+
+
/*
* This returns a dummy child_process if the transport protocol does not
* need fork(2), or a struct child_process object if it does. Once done,
@@ -564,9 +574,9 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
char *url;
char *host, *path;
char *end;
- int c;
+ int separator;
struct child_process *conn = no_fork;
- enum protocol protocol = PROTO_LOCAL;
+ enum protocol protocol = PROTO_LOCAL_OR_SSH;
int free_path = 0;
char *port = NULL;
const char **arg;
@@ -587,20 +597,23 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
*host = '\0';
protocol = get_protocol(url);
host += 3;
- c = '/';
+ separator = '/';
} else {
host = url;
- c = ':';
+ separator = ':';
+ if (is_local(url))
+ protocol = PROTO_LOCAL;
}
/*
* Don't do destructive transforms with git:// as that
* protocol code does '[]' unwrapping of its own.
+* Don't change local URLs
*/
if (host[0] == '[') {
end = strchr(host + 1, ']');
if (end) {
- if (protocol != PROTO_GIT) {
+ if (protocol != PROTO_GIT protocol != PROTO_LOCAL) {
*end = 0;
host++;
}
@@ -610,17 +623,17 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
} else
end = host;
- path = strchr(end, c);
- if (path !has_dos_drive_prefix(end)) {
- if (c == ':') {
- if (host != url || path strchrnul(host, '/')) {
- protocol = PROTO_SSH;
- *path++ = '\0';
- } else /* '/' in the host part, assume local path */
- path = end;
+ path = strchr(end, separator);
+ if (separator == ':') {
+ if (path protocol == PROTO_LOCAL_OR_SSH) {
+ /* We have a ':' */
+ protocol = PROTO_SSH;
+ *path++ = '\0';
+ } else {/* assume local path */
+ protocol = PROTO_LOCAL;
+ path = end;
}
- } else
- path = end;
+ }
if (!path || !*path)
die(No path specified. See 'man git-pull' for valid url
syntax);
@@ -629,7 +642,7 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
* null-terminate hostname and point path to ~ for URL's like
Re: [PATCH V2] git clone: is an URL local or ssh
Torsten Bögershausen tbo...@web.de writes:
(This does apply on pu, not on master.
Hmph. At least for me, it applies down to cabb411f (Merge branch
'nd/clone-local-with-colon', 2013-10-14) just fine. Puzzled.
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 1d1c875..a126f08 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -294,39 +294,95 @@ test_expect_success 'setup ssh wrapper' '
export TRASH_DIRECTORY
'
-clear_ssh () {
- $TRASH_DIRECTORY/ssh-output
-}
-
-expect_ssh () {
+i5601=0
+# $1 url
+# $2 none|host
+# $3 path
+test_clone_url () {
+ i5601=$(($i5601 + 1))
+ $TRASH_DIRECTORY/ssh-output
+ test_might_fail git clone $1 tmp$i5601
{
- case $1 in
+ case $2 in
none)
;;
*)
- echo ssh: $1 git-upload-pack '$2'
+ echo ssh: $2 git-upload-pack '$3'
esac
} $TRASH_DIRECTORY/ssh-expect
This looks like a strange use of {} (not an issue this patch
introduced, though). Shouldn't this suffice?
case ... in
...
esac $TRASH_DIRECTORY/ssh-expect
+ (
+ cd $TRASH_DIRECTORY
+ test_cmp ssh-expect ssh-output
+ rm -rf ssh-expect ssh-output
Drop r, please, when you know these are supposed to be files.
+ )
}
+# url looks ssh like, and is on disc: should be local
test_expect_success NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '
cp -R src foo:bar
+ test_clone_url foo:bar none
+ ( cd tmp$i5601 git log)
Hmph. What is this git log about? Leftover from an earlier
debugging session?
The code change to connect.c part seemed to be OK from a cursory
look.
Thanks.
--
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 V2] git clone: is an URL local or ssh
On Mon, Oct 28, 2013 at 09:16:19PM +0100, Torsten Bögershausen wrote:
Commit 8d3d28f5 added test cases for URLs which should be ssh.
Add more tests testing all the combinations:
-IPv4 or IPv6
-path starting with / or with /~
-with and without the ssh:// scheme
Add tests for ssh:// with port number.
When a git repository foo:bar exist, git clone will call
absolute_path() and git_connect() will be called with
something like /home/user/projects/foo:bar
Tighten the test and use foo:bar instead of ./foo:bar,
refactor clear_ssh() and expect_ssh() into test_clone_url().
git clone foo/bar:baz should not be ssh:
Make the rule
if there is a slash before the first colon, it is not ssh
more strict by using the same is_local() in both connect.c
and transport.c. Add a test case.
Bug fixes for corner cases:
- Handle clone of [::1]:/~repo correctly (2e7766655):
Git should call ssh ::1 ~repo, not ssh ::1 /~repo
This was caused by looking at (host != url), which never
worked for host names with literal IPv6 adresses, embedded by []
Support for the [] URLs was introduced in 356bece0a.
- Do not tamper local URLs starting with '[' which have a ']'
IMHO, this would have been a little easier to follow as separate
patches, as there are a few things going on. I think the changes to the
code are fine, though.
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 1d1c875..a126f08 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -294,39 +294,95 @@ test_expect_success 'setup ssh wrapper' '
export TRASH_DIRECTORY
'
-clear_ssh () {
- $TRASH_DIRECTORY/ssh-output
-}
-
-expect_ssh () {
I'd prefer if you left the expect_ssh interface intact, as it is
potentially useful outside of t5601 (but your patch ties it very closely
to a particular set of clone tests). Can you call out to expect_ssh
instead from test_clone_url instead?
+i5601=0
The name of this variable confused me for a bit. Maybe counter or
something would be a little more descriptive?
+# $1 url
+# $2 none|host
+# $3 path
+test_clone_url () {
+ i5601=$(($i5601 + 1))
+ $TRASH_DIRECTORY/ssh-output
+ test_might_fail git clone $1 tmp$i5601
Do we always want test_might_fail in these tests? I really would prefer
that the test actually accomplish the clone, as then we know that
nothing funny is going on (e.g., multiple invocations of ssh, one of
which is good and one of which is not). I think I gave more specific
examples in the last round of review.
- (cd $TRASH_DIRECTORY test_cmp ssh-expect ssh-output)
+ (
+ cd $TRASH_DIRECTORY
+ test_cmp ssh-expect ssh-output
+ rm -rf ssh-expect ssh-output
+ )
Here we clean up at the end of the test, which is a good improvement on
my existing functions. But I notice that you also clear ssh-output at
the beginning of the test, still (which you must do to not leave a
polluted state after a failing test). I think this might be better as:
test_when_finished '
(cd $TRASH_DIRECTORY rm -f ssh-expect ssh-output)
'
and then the clear_ssh can just go away (and your reset of ssh-output,
which is the analogous line), as tests can expect a clean state.
-Peff
--
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 V2] git clone: is an URL local or ssh
On Mon, Oct 28, 2013 at 01:57:13PM -0700, Junio C Hamano wrote:
+i5601=0
+# $1 url
+# $2 none|host
+# $3 path
+test_clone_url () {
+ i5601=$(($i5601 + 1))
+ $TRASH_DIRECTORY/ssh-output
+ test_might_fail git clone $1 tmp$i5601
{
- case $1 in
+ case $2 in
none)
;;
*)
- echo ssh: $1 git-upload-pack '$2'
+ echo ssh: $2 git-upload-pack '$3'
esac
} $TRASH_DIRECTORY/ssh-expect
This looks like a strange use of {} (not an issue this patch
introduced, though). Shouldn't this suffice?
case ... in
...
esac $TRASH_DIRECTORY/ssh-expect
Yeah, I think you are right. This one is my fault from 8d3d28f; I think
in an earlier draft I had more complex logic that needed the {}, and
then never dropped them when it got simplified.
-Peff
--
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
