From 710eb3e22cfe5fb84f41a3ef1d3a91b5cf5d3463 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torsten=20B=C3=B6gershausen?= Date: Thu, 28 Nov 2013 20:53:47 +0100 Subject: [PATCH 01/10] t5601: remove clear_ssh, refactor setup_ssh_wrapper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 8d3d28f5 added test cases for URLs which should be ssh. Remove the function clear_ssh, use test_when_finished to clean up. Introduce the function setup_ssh_wrapper, which could be factored out together with expect_ssh. Tighten one test and use "foo:bar" instead of "./foo:bar", Helped-by: Jeff King Signed-off-by: Torsten Bögershausen Signed-off-by: Junio C Hamano --- t/t5601-clone.sh | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 1d1c8755ea..c634f776d7 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -280,25 +280,26 @@ test_expect_success 'clone checking out a tag' ' test_cmp fetch.expected fetch.actual ' -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" +setup_ssh_wrapper () { + 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 && + >"$TRASH_DIRECTORY"/ssh-output + ' } expect_ssh () { + test_when_finished ' + (cd "$TRASH_DIRECTORY" && rm -f ssh-expect && >ssh-output) + ' && { case "$1" in none) @@ -310,21 +311,20 @@ expect_ssh () { (cd "$TRASH_DIRECTORY" && test_cmp ssh-expect ssh-output) } +setup_ssh_wrapper + test_expect_success 'cloning myhost:src uses ssh' ' - clear_ssh && git clone myhost:src ssh-clone && expect_ssh myhost src ' test_expect_success 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 && expect_ssh none ' test_expect_success 'bracketed hostnames are still ssh' ' - clear_ssh && git clone "[myhost:123]:src" ssh-bracket-clone && expect_ssh myhost:123 src ' From 2171f3d2aa1412a744545c07b28003a7ff4af5df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torsten=20B=C3=B6gershausen?= Date: Thu, 28 Nov 2013 20:48:22 +0100 Subject: [PATCH 02/10] t5601: add tests for ssh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add more tests testing all the combinations: -IPv4 or IPv6 -path starting with "/" or with "/~" -with and without the ssh:// scheme Some tests fail; they need updates in connect.c Signed-off-by: Torsten Bögershausen Signed-off-by: Junio C Hamano --- t/t5601-clone.sh | 100 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 99 insertions(+), 1 deletion(-) diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index c634f776d7..ba99972d88 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -313,7 +313,7 @@ expect_ssh () { setup_ssh_wrapper -test_expect_success 'cloning myhost:src uses ssh' ' +test_expect_success 'clone myhost:src uses ssh' ' git clone myhost:src ssh-clone && expect_ssh myhost src ' @@ -329,6 +329,104 @@ test_expect_success 'bracketed hostnames are still ssh' ' expect_ssh myhost:123 src ' +counter=0 +# $1 url +# $2 none|host +# $3 path +test_clone_url () { + counter=$(($counter + 1)) + test_might_fail git clone "$1" tmp$counter && + expect_ssh "$2" "$3" +} + +test_expect_success NOT_MINGW 'clone c:temp is ssl' ' + test_clone_url c:temp c temp +' + +test_expect_success MINGW 'clone c:temp is dos drive' ' + test_clone_url c:temp none +' + +#ip v4 +for repo in rep rep/home/project /~proj 123 +do + test_expect_success "clone host:$repo" ' + test_clone_url host:$repo host $repo + ' +done + +#ipv6 +# failing +for repo in /~proj +do + test_expect_failure "clone [::1]:$repo" ' + test_clone_url [::1]:$repo ::1 $repo + ' +done + +for repo in rep rep/home/project 123 +do + test_expect_success "clone [::1]:$repo" ' + test_clone_url [::1]:$repo ::1 $repo + ' +done + +# Corner cases +# failing +for repo in [foo]bar/baz:qux [foo/bar]:baz +do + test_expect_failure "clone $url is not ssh" ' + test_clone_url $url none + ' +done + +for url in foo/bar:baz +do + test_expect_success "clone $url is not ssh" ' + test_clone_url $url none + ' +done + +#with ssh:// scheme +test_expect_success 'clone ssh://host.xz/home/user/repo' ' + test_clone_url "ssh://host.xz/home/user/repo" host.xz "/home/user/repo" +' + +# from home directory +test_expect_success 'clone ssh://host.xz/~repo' ' + test_clone_url "ssh://host.xz/~repo" host.xz "~repo" +' + +# with port number +test_expect_success 'clone ssh://host.xz:22/home/user/repo' ' + test_clone_url "ssh://host.xz:22/home/user/repo" "-p 22 host.xz" "/home/user/repo" +' + +# from home directory with port number +test_expect_success 'clone ssh://host.xz:22/~repo' ' + test_clone_url "ssh://host.xz:22/~repo" "-p 22 host.xz" "~repo" +' + +#IPv6 +test_expect_success 'clone ssh://[::1]/home/user/repo' ' + test_clone_url "ssh://[::1]/home/user/repo" "::1" "/home/user/repo" +' + +#IPv6 from home directory +test_expect_success 'clone ssh://[::1]/~repo' ' + test_clone_url "ssh://[::1]/~repo" "::1" "~repo" +' + +#IPv6 with port number +test_expect_success 'clone ssh://[::1]:22/home/user/repo' ' + test_clone_url "ssh://[::1]:22/home/user/repo" "-p 22 ::1" "/home/user/repo" +' + +#IPv6 from home directory with port number +test_expect_success 'clone ssh://[::1]:22/~repo' ' + test_clone_url "ssh://[::1]:22/~repo" "-p 22 ::1" "~repo" +' + test_expect_success 'clone from a repository with two identical branches' ' ( From d98d109979760bed1909fbf5e56cbb6239980095 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Thu, 28 Nov 2013 20:48:45 +0100 Subject: [PATCH 03/10] git_connect: remove artificial limit of a remote command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since day one, function git_connect() had a limit on the command line of the command that is invoked to make a connection. 7a33bcbe converted the code that constructs the command to strbuf. This would have been the right time to remove the limit, but it did not happen. Remove it now. git_connect() uses start_command() to invoke the command; consequently, the limits of the system still apply, but are diagnosed only at execve() time. But these limits are more lenient than the 1K that git_connect() imposed. Signed-off-by: Johannes Sixt Signed-off-by: Torsten Bögershausen Signed-off-by: Junio C Hamano --- connect.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/connect.c b/connect.c index 06e88b0705..6cc1f8dd38 100644 --- a/connect.c +++ b/connect.c @@ -527,8 +527,6 @@ static struct child_process *git_proxy_connect(int fd[2], char *host) return proxy; } -#define MAX_CMD_LEN 1024 - static char *get_port(char *host) { char *end; @@ -570,7 +568,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig, int free_path = 0; char *port = NULL; const char **arg; - struct strbuf cmd; + struct strbuf cmd = STRBUF_INIT; /* Without this we cannot rely on waitpid() to tell * what happened to our children. @@ -676,12 +674,9 @@ struct child_process *git_connect(int fd[2], const char *url_orig, conn = xcalloc(1, sizeof(*conn)); - strbuf_init(&cmd, MAX_CMD_LEN); strbuf_addstr(&cmd, prog); strbuf_addch(&cmd, ' '); sq_quote_buf(&cmd, path); - if (cmd.len >= MAX_CMD_LEN) - die("command line too long"); conn->in = conn->out = -1; conn->argv = arg = xcalloc(7, sizeof(*arg)); From cabc3c12e40f5556bb4de934ab783d53e908e401 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Thu, 28 Nov 2013 20:49:01 +0100 Subject: [PATCH 04/10] git_connect: factor out discovery of the protocol and its parts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit git_connect has grown large due to the many different protocols syntaxes that are supported. Move the part of the function that parses the URL to connect to into a separate function for readability. Signed-off-by: Johannes Sixt Signed-off-by: Torsten Bögershausen Signed-off-by: Junio C Hamano --- connect.c | 80 ++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 27 deletions(-) diff --git a/connect.c b/connect.c index 6cc1f8dd38..a6cf3454a2 100644 --- a/connect.c +++ b/connect.c @@ -543,37 +543,20 @@ static char *get_port(char *host) return NULL; } -static struct child_process no_fork; - /* - * 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, - * finish the connection with finish_connect() with the value returned from - * this function (it is safe to call finish_connect() with NULL to support - * the former case). - * - * If it returns, the connect is successful; it just dies on errors (this - * will hopefully be changed in a libification effort, to return NULL when - * the connection failed). + * Extract protocol and relevant parts from the specified connection URL. + * The caller must free() the returned strings. */ -struct child_process *git_connect(int fd[2], const char *url_orig, - const char *prog, int flags) +static enum protocol parse_connect_url(const char *url_orig, char **ret_host, + char **ret_port, char **ret_path) { char *url; char *host, *path; char *end; int c; - struct child_process *conn = &no_fork; enum protocol protocol = PROTO_LOCAL; int free_path = 0; char *port = NULL; - const char **arg; - struct strbuf cmd = STRBUF_INIT; - - /* Without this we cannot rely on waitpid() to tell - * what happened to our children. - */ - signal(SIGCHLD, SIG_DFL); if (is_url(url_orig)) url = url_decode(url_orig); @@ -645,6 +628,49 @@ struct child_process *git_connect(int fd[2], const char *url_orig, if (protocol == PROTO_SSH && host != url) port = get_port(end); + *ret_host = xstrdup(host); + if (port) + *ret_port = xstrdup(port); + else + *ret_port = NULL; + if (free_path) + *ret_path = path; + else + *ret_path = xstrdup(path); + free(url); + return protocol; +} + +static struct child_process no_fork; + +/* + * 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, + * finish the connection with finish_connect() with the value returned from + * this function (it is safe to call finish_connect() with NULL to support + * the former case). + * + * If it returns, the connect is successful; it just dies on errors (this + * will hopefully be changed in a libification effort, to return NULL when + * the connection failed). + */ +struct child_process *git_connect(int fd[2], const char *url, + const char *prog, int flags) +{ + char *host, *path; + struct child_process *conn = &no_fork; + enum protocol protocol; + char *port; + const char **arg; + struct strbuf cmd = STRBUF_INIT; + + /* Without this we cannot rely on waitpid() to tell + * what happened to our children. + */ + signal(SIGCHLD, SIG_DFL); + + protocol = parse_connect_url(url, &host, &port, &path); + if (protocol == PROTO_GIT) { /* These underlying connection commands die() if they * cannot connect. @@ -666,9 +692,9 @@ struct child_process *git_connect(int fd[2], const char *url_orig, prog, path, 0, target_host, 0); free(target_host); - free(url); - if (free_path) - free(path); + free(host); + free(port); + free(path); return conn; } @@ -709,9 +735,9 @@ struct child_process *git_connect(int fd[2], const char *url_orig, fd[0] = conn->out; /* read from child's stdout */ fd[1] = conn->in; /* write to child's stdin */ strbuf_release(&cmd); - free(url); - if (free_path) - free(path); + free(host); + free(port); + free(path); return conn; } From 5610b7c0c6957cf0b236b6fac087c1f4dc209376 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torsten=20B=C3=B6gershausen?= Date: Thu, 28 Nov 2013 20:49:17 +0100 Subject: [PATCH 05/10] git fetch-pack: add --diag-url MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The main purpose is to trace the URL parser called by git_connect() in connect.c The main features of the parser can be listed as this: - parse out host and path for URLs with a scheme (git:// file:// ssh://) - parse host names embedded by [] correctly - extract the port number, if present - separate URLs like "file" (which are local) from URLs like "host:repo" which should use ssh Add the new parameter "--diag-url" to "git fetch-pack", which prints the value for protocol, host and path to stderr and exits. Signed-off-by: Torsten Bögershausen Signed-off-by: Junio C Hamano --- builtin/fetch-pack.c | 14 +++++++++++--- connect.c | 28 ++++++++++++++++++++++++++++ connect.h | 1 + fetch-pack.h | 1 + 4 files changed, 41 insertions(+), 3 deletions(-) diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index c8e858232a..758b5acd55 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -7,7 +7,7 @@ static const char fetch_pack_usage[] = "git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin] " "[--include-tag] [--upload-pack=] [--depth=] " -"[--no-progress] [-v] [:] [...]"; +"[--no-progress] [--diag-url] [-v] [:] [...]"; static void add_sought_entry_mem(struct ref ***sought, int *nr, int *alloc, const char *name, int namelen) @@ -81,6 +81,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) args.stdin_refs = 1; continue; } + if (!strcmp("--diag-url", arg)) { + args.diag_url = 1; + continue; + } if (!strcmp("-v", arg)) { args.verbose = 1; continue; @@ -146,10 +150,14 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix) fd[0] = 0; fd[1] = 1; } else { + int flags = args.verbose ? CONNECT_VERBOSE : 0; + if (args.diag_url) + flags |= CONNECT_DIAG_URL; conn = git_connect(fd, dest, args.uploadpack, - args.verbose ? CONNECT_VERBOSE : 0); + flags); + if (!conn) + return args.diag_url ? 0 : 1; } - get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL); ref = fetch_pack(&args, fd, conn, ref, dest, diff --git a/connect.c b/connect.c index a6cf3454a2..a16bdaf0b7 100644 --- a/connect.c +++ b/connect.c @@ -236,6 +236,20 @@ enum protocol { PROTO_GIT }; +static const char *prot_name(enum protocol protocol) +{ + switch (protocol) { + case PROTO_LOCAL: + return "file"; + case PROTO_SSH: + return "ssh"; + case PROTO_GIT: + return "git"; + default: + return "unkown protocol"; + } +} + static enum protocol get_protocol(const char *name) { if (!strcmp(name, "ssh")) @@ -670,6 +684,20 @@ struct child_process *git_connect(int fd[2], const char *url, signal(SIGCHLD, SIG_DFL); protocol = parse_connect_url(url, &host, &port, &path); + if (flags & CONNECT_DIAG_URL) { + printf("Diag: url=%s\n", url ? url : "NULL"); + printf("Diag: protocol=%s\n", prot_name(protocol)); + printf("Diag: hostandport=%s", host ? host : "NULL"); + if (port) + printf(":%s\n", port); + else + printf("\n"); + printf("Diag: path=%s\n", path ? path : "NULL"); + free(host); + free(port); + free(path); + return NULL; + } if (protocol == PROTO_GIT) { /* These underlying connection commands die() if they diff --git a/connect.h b/connect.h index 64fb7dbbee..527d58a3b3 100644 --- a/connect.h +++ b/connect.h @@ -2,6 +2,7 @@ #define CONNECT_H #define CONNECT_VERBOSE (1u << 0) +#define CONNECT_DIAG_URL (1u << 1) extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags); extern int finish_connect(struct child_process *conn); extern int git_connection_is_socket(struct child_process *conn); diff --git a/fetch-pack.h b/fetch-pack.h index 461cbf39b2..20ccc12e57 100644 --- a/fetch-pack.h +++ b/fetch-pack.h @@ -14,6 +14,7 @@ struct fetch_pack_args { use_thin_pack:1, fetch_all:1, stdin_refs:1, + diag_url:1, verbose:1, no_progress:1, include_tag:1, From 854aeb7beb85a7a3f05fc11daea7c8d2fed3a22c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torsten=20B=C3=B6gershausen?= Date: Thu, 28 Nov 2013 20:49:29 +0100 Subject: [PATCH 06/10] t5500: add test cases for diag-url MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add test cases using git fetch-pack --diag-url: - parse out host and path for URLs with a scheme (git:// file:// ssh://) - parse host names embedded by [] correctly - extract the port number, if present - separate URLs like "file" (which are local) from URLs like "host:repo" which should use ssh Signed-off-by: Torsten Bögershausen Signed-off-by: Junio C Hamano --- t/t5500-fetch-pack.sh | 59 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index d87ddf73b7..545dd7b8e1 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -531,5 +531,64 @@ test_expect_success 'shallow fetch with tags does not break the repository' ' git fsck ) ' +check_prot_path () { + cat >expected <<-EOF && + Diag: url=$1 + Diag: protocol=$2 + Diag: path=$3 + EOF + git fetch-pack --diag-url "$1" | grep -v hostandport= >actual && + test_cmp expected actual +} + +check_prot_host_path () { + cat >expected <<-EOF && + Diag: url=$1 + Diag: protocol=$2 + Diag: hostandport=$3 + Diag: path=$4 + EOF + git fetch-pack --diag-url "$1" >actual && + test_cmp expected actual +} + +for r in repo re:po re/po +do + # git or ssh with scheme + for p in "ssh+git" "git+ssh" git ssh + do + for h in host host:12 [::1] [::1]:23 + do + case "$p" in + *ssh*) + hh=$(echo $h | tr -d "[]") + pp=ssh + ;; + *) + hh=$h + pp=$p + ;; + esac + test_expect_success "fetch-pack --diag-url $p://$h/$r" ' + check_prot_host_path $p://$h/$r $pp "$hh" "/$r" + ' + # "/~" -> "~" conversion + test_expect_success "fetch-pack --diag-url $p://$h/~$r" ' + check_prot_host_path $p://$h/~$r $pp "$hh" "~$r" + ' + done + done + # file with scheme + for p in file + do + test_expect_success "fetch-pack --diag-url $p://$h/$r" ' + check_prot_path $p://$h/$r $p "/$r" + ' + # No "/~" -> "~" conversion for file + test_expect_success "fetch-pack --diag-url $p://$h/~$r" ' + check_prot_path $p://$h/~$r $p "/~$r" + ' + done +done test_done From 6a59974869c0a6e9399101bc02223b4c00a8aff2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torsten=20B=C3=B6gershausen?= Date: Thu, 28 Nov 2013 20:49:38 +0100 Subject: [PATCH 07/10] git fetch: support host:/~repo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The documentation (in urls.txt) says that "ssh://host:/~repo", "host:/~repo" or "host:~repo" specify the repository "repo" in the home directory at "host". This has not been working for "host:/~repo". Before commit 356bec "Support [address] in URLs", the comparison "url != hostname" could be used to determine if the URL had a scheme or not: "ssh://host/host" != "host". However, after 356bec "[::1]" was converted into "::1", yielding url != hostname as well. To fix this regression, don't use "if (url != hostname)", but look at the separator instead. Rename the variable "c" into "separator" to make it easier to read. Signed-off-by: Torsten Bögershausen Signed-off-by: Junio C Hamano --- connect.c | 14 +++++++------- t/t5500-fetch-pack.sh | 24 ++++++++++++++++++++++++ t/t5601-clone.sh | 12 ++---------- 3 files changed, 33 insertions(+), 17 deletions(-) diff --git a/connect.c b/connect.c index a16bdaf0b7..7e5f608a2f 100644 --- a/connect.c +++ b/connect.c @@ -567,7 +567,7 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host, char *url; char *host, *path; char *end; - int c; + int separator; enum protocol protocol = PROTO_LOCAL; int free_path = 0; char *port = NULL; @@ -582,10 +582,10 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host, *host = '\0'; protocol = get_protocol(url); host += 3; - c = '/'; + separator = '/'; } else { host = url; - c = ':'; + separator = ':'; } /* @@ -605,9 +605,9 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host, } else end = host; - path = strchr(end, c); + path = strchr(end, separator); if (path && !has_dos_drive_prefix(end)) { - if (c == ':') { + if (separator == ':') { if (host != url || path < strchrnul(host, '/')) { protocol = PROTO_SSH; *path++ = '\0'; @@ -624,7 +624,7 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host, * null-terminate hostname and point path to ~ for URL's like this: * ssh://host.xz/~user/repo */ - if (protocol != PROTO_LOCAL && host != url) { + if (protocol != PROTO_LOCAL) { char *ptr = path; if (path[1] == '~') path++; @@ -639,7 +639,7 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host, /* * Add support for ssh port: ssh://host.xy:/... */ - if (protocol == PROTO_SSH && host != url) + if (protocol == PROTO_SSH && separator == '/') port = get_port(end); *ret_host = xstrdup(host); diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index 545dd7b8e1..df5baf8d6f 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -589,6 +589,30 @@ do check_prot_path $p://$h/~$r $p "/~$r" ' done + # file without scheme + for h in nohost nohost:12 [::1] [::1]:23 [ [:aa + do + test_expect_success "fetch-pack --diag-url ./$h:$r" ' + check_prot_path ./$h:$r $p "./$h:$r" + ' + # No "/~" -> "~" conversion for file + test_expect_success "fetch-pack --diag-url ./$p:$h/~$r" ' + check_prot_path ./$p:$h/~$r $p "./$p:$h/~$r" + ' + done + #ssh without scheme + p=ssh + for h in host [::1] + do + hh=$(echo $h | tr -d "[]") + test_expect_success "fetch-pack --diag-url $h:$r" ' + check_prot_path $h:$r $p "$r" + ' + # Do "/~" -> "~" conversion + test_expect_success "fetch-pack --diag-url $h:/~$r" ' + check_prot_host_path $h:/~$r $p "$hh" "~$r" + ' + done done test_done diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index ba99972d88..4db0c0b01e 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -348,7 +348,7 @@ test_expect_success MINGW 'clone c:temp is dos drive' ' ' #ip v4 -for repo in rep rep/home/project /~proj 123 +for repo in rep rep/home/project 123 do test_expect_success "clone host:$repo" ' test_clone_url host:$repo host $repo @@ -356,14 +356,6 @@ do done #ipv6 -# failing -for repo in /~proj -do - test_expect_failure "clone [::1]:$repo" ' - test_clone_url [::1]:$repo ::1 $repo - ' -done - for repo in rep rep/home/project 123 do test_expect_success "clone [::1]:$repo" ' @@ -373,7 +365,7 @@ done # Corner cases # failing -for repo in [foo]bar/baz:qux [foo/bar]:baz +for url in [foo]bar/baz:qux [foo/bar]:baz do test_expect_failure "clone $url is not ssh" ' test_clone_url $url none From 83b058752707a6ba4af51ebc98c47913bc7d2d25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torsten=20B=C3=B6gershausen?= Date: Thu, 28 Nov 2013 20:49:54 +0100 Subject: [PATCH 08/10] git_connect(): refactor the port handling for ssh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use get_host_and_port() even for ssh. Remove the variable port git_connect(), and simplify parse_connect_url() Use only one return point in git_connect(), doing the free() and return conn. t5601 had 2 corner test cases which now pass. Signed-off-by: Torsten Bögershausen Signed-off-by: Junio C Hamano --- connect.c | 47 ++++++++++++------------------------------- t/t5500-fetch-pack.sh | 9 +++------ t/t5601-clone.sh | 10 +-------- 3 files changed, 17 insertions(+), 49 deletions(-) diff --git a/connect.c b/connect.c index 7e5f608a2f..78745306cb 100644 --- a/connect.c +++ b/connect.c @@ -541,16 +541,13 @@ static struct child_process *git_proxy_connect(int fd[2], char *host) return proxy; } -static char *get_port(char *host) +static const char *get_port_numeric(const char *p) { char *end; - char *p = strchr(host, ':'); - if (p) { long port = strtol(p + 1, &end, 10); if (end != p + 1 && *end == '\0' && 0 <= port && port < 65536) { - *p = '\0'; - return p+1; + return p; } } @@ -562,7 +559,7 @@ static char *get_port(char *host) * The caller must free() the returned strings. */ static enum protocol parse_connect_url(const char *url_orig, char **ret_host, - char **ret_port, char **ret_path) + char **ret_path) { char *url; char *host, *path; @@ -570,7 +567,6 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host, int separator; enum protocol protocol = PROTO_LOCAL; int free_path = 0; - char *port = NULL; if (is_url(url_orig)) url = url_decode(url_orig); @@ -589,16 +585,12 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host, } /* - * Don't do destructive transforms with git:// as that - * protocol code does '[]' unwrapping of its own. + * Don't do destructive transforms as protocol code does + * '[]' unwrapping in get_host_and_port() */ if (host[0] == '[') { end = strchr(host + 1, ']'); if (end) { - if (protocol != PROTO_GIT) { - *end = 0; - host++; - } end++; } else end = host; @@ -636,17 +628,7 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host, *ptr = '\0'; } - /* - * Add support for ssh port: ssh://host.xy:/... - */ - if (protocol == PROTO_SSH && separator == '/') - port = get_port(end); - *ret_host = xstrdup(host); - if (port) - *ret_port = xstrdup(port); - else - *ret_port = NULL; if (free_path) *ret_path = path; else @@ -674,7 +656,6 @@ struct child_process *git_connect(int fd[2], const char *url, char *host, *path; struct child_process *conn = &no_fork; enum protocol protocol; - char *port; const char **arg; struct strbuf cmd = STRBUF_INIT; @@ -683,18 +664,13 @@ struct child_process *git_connect(int fd[2], const char *url, */ signal(SIGCHLD, SIG_DFL); - protocol = parse_connect_url(url, &host, &port, &path); + protocol = parse_connect_url(url, &host, &path); if (flags & CONNECT_DIAG_URL) { printf("Diag: url=%s\n", url ? url : "NULL"); printf("Diag: protocol=%s\n", prot_name(protocol)); - printf("Diag: hostandport=%s", host ? host : "NULL"); - if (port) - printf(":%s\n", port); - else - printf("\n"); + printf("Diag: hostandport=%s\n", host ? host : "NULL"); printf("Diag: path=%s\n", path ? path : "NULL"); free(host); - free(port); free(path); return NULL; } @@ -721,7 +697,6 @@ struct child_process *git_connect(int fd[2], const char *url, target_host, 0); free(target_host); free(host); - free(port); free(path); return conn; } @@ -737,6 +712,11 @@ struct child_process *git_connect(int fd[2], const char *url, if (protocol == PROTO_SSH) { const char *ssh = getenv("GIT_SSH"); int putty = ssh && strcasestr(ssh, "plink"); + char *ssh_host = host; /* keep host for the free() below */ + const char *port = NULL; + get_host_and_port(&ssh_host, &port); + port = get_port_numeric(port); + if (!ssh) ssh = "ssh"; *arg++ = ssh; @@ -747,7 +727,7 @@ struct child_process *git_connect(int fd[2], const char *url, *arg++ = putty ? "-P" : "-p"; *arg++ = port; } - *arg++ = host; + *arg++ = ssh_host; } else { /* remove repo-local variables from the environment */ @@ -764,7 +744,6 @@ struct child_process *git_connect(int fd[2], const char *url, fd[1] = conn->in; /* write to child's stdin */ strbuf_release(&cmd); free(host); - free(port); free(path); return conn; } diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index df5baf8d6f..b4866ea3a3 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -561,20 +561,18 @@ do do case "$p" in *ssh*) - hh=$(echo $h | tr -d "[]") pp=ssh ;; *) - hh=$h pp=$p ;; esac test_expect_success "fetch-pack --diag-url $p://$h/$r" ' - check_prot_host_path $p://$h/$r $pp "$hh" "/$r" + check_prot_host_path $p://$h/$r $pp "$h" "/$r" ' # "/~" -> "~" conversion test_expect_success "fetch-pack --diag-url $p://$h/~$r" ' - check_prot_host_path $p://$h/~$r $pp "$hh" "~$r" + check_prot_host_path $p://$h/~$r $pp "$h" "~$r" ' done done @@ -604,13 +602,12 @@ do p=ssh for h in host [::1] do - hh=$(echo $h | tr -d "[]") test_expect_success "fetch-pack --diag-url $h:$r" ' check_prot_path $h:$r $p "$r" ' # Do "/~" -> "~" conversion test_expect_success "fetch-pack --diag-url $h:/~$r" ' - check_prot_host_path $h:/~$r $p "$hh" "~$r" + check_prot_host_path $h:/~$r $p "$h" "~$r" ' done done diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 4db0c0b01e..53a1de9efd 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -364,15 +364,7 @@ do done # Corner cases -# failing -for url in [foo]bar/baz:qux [foo/bar]:baz -do - test_expect_failure "clone $url is not ssh" ' - test_clone_url $url none - ' -done - -for url in foo/bar:baz +for url in foo/bar:baz [foo]bar/baz:qux [foo/bar]:baz do test_expect_success "clone $url is not ssh" ' test_clone_url $url none From c59ab2e52a64abd7fded97e0983a9b7f3d0508a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torsten=20B=C3=B6gershausen?= Date: Thu, 28 Nov 2013 20:50:03 +0100 Subject: [PATCH 09/10] connect.c: refactor url parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make the function is_local() in transport.c public, rename it into url_is_local_not_ssh() and use it in both transport.c and connect.c Use a protocol "local" for URLs for the local file system. One note about using file:// under Windows: The (absolute) path on Unix like system typically starts with "/". When the host is empty, it can be omitted, so that a shell scriptlet url=file://$pwd will give a URL like "file:///home/user/repo". Windows does not have the same concept of a root directory located in "/". When parsing the URL allow "file://C:/user/repo" (even if RFC1738 indicates that "file:///C:/user/repo" should be used). Signed-off-by: Torsten Bögershausen Signed-off-by: Junio C Hamano --- connect.c | 57 +++++++++++++++++++++++-------------------- connect.h | 1 + t/t5500-fetch-pack.sh | 7 ++++++ t/t5601-clone.sh | 8 ++++++ transport.c | 12 ++------- 5 files changed, 48 insertions(+), 37 deletions(-) diff --git a/connect.c b/connect.c index 78745306cb..04093c4180 100644 --- a/connect.c +++ b/connect.c @@ -232,14 +232,24 @@ int server_supports(const char *feature) enum protocol { PROTO_LOCAL = 1, + PROTO_FILE, PROTO_SSH, PROTO_GIT }; +int url_is_local_not_ssh(const char *url) +{ + const char *colon = strchr(url, ':'); + const char *slash = strchr(url, '/'); + return !colon || (slash && slash < colon) || + has_dos_drive_prefix(url); +} + static const char *prot_name(enum protocol protocol) { switch (protocol) { case PROTO_LOCAL: + case PROTO_FILE: return "file"; case PROTO_SSH: return "ssh"; @@ -261,7 +271,7 @@ static enum protocol get_protocol(const char *name) if (!strcmp(name, "ssh+git")) return PROTO_SSH; if (!strcmp(name, "file")) - return PROTO_LOCAL; + return PROTO_FILE; die("I don't handle protocol '%s'", name); } @@ -564,9 +574,8 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host, char *url; char *host, *path; char *end; - int separator; + int separator = '/'; enum protocol protocol = PROTO_LOCAL; - int free_path = 0; if (is_url(url_orig)) url = url_decode(url_orig); @@ -578,10 +587,12 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host, *host = '\0'; protocol = get_protocol(url); host += 3; - separator = '/'; } else { host = url; - separator = ':'; + if (!url_is_local_not_ssh(url)) { + protocol = PROTO_SSH; + separator = ':'; + } } /* @@ -597,17 +608,12 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host, } else end = host; - path = strchr(end, separator); - if (path && !has_dos_drive_prefix(end)) { - if (separator == ':') { - if (host != url || path < strchrnul(host, '/')) { - protocol = PROTO_SSH; - *path++ = '\0'; - } else /* '/' in the host part, assume local path */ - path = end; - } - } else + if (protocol == PROTO_LOCAL) path = end; + else if (protocol == PROTO_FILE && has_dos_drive_prefix(end)) + path = end; /* "file://$(pwd)" may be "file://C:/projects/repo" */ + else + path = strchr(end, separator); if (!path || !*path) die("No path specified. See 'man git-pull' for valid url syntax"); @@ -616,23 +622,20 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host, * null-terminate hostname and point path to ~ for URL's like this: * ssh://host.xz/~user/repo */ - if (protocol != PROTO_LOCAL) { - char *ptr = path; + + end = path; /* Need to \0 terminate host here */ + if (separator == ':') + path++; /* path starts after ':' */ + if (protocol == PROTO_GIT || protocol == PROTO_SSH) { if (path[1] == '~') path++; - else { - path = xstrdup(ptr); - free_path = 1; - } - - *ptr = '\0'; } + path = xstrdup(path); + *end = '\0'; + *ret_host = xstrdup(host); - if (free_path) - *ret_path = path; - else - *ret_path = xstrdup(path); + *ret_path = path; free(url); return protocol; } diff --git a/connect.h b/connect.h index 527d58a3b3..c41a6850f1 100644 --- a/connect.h +++ b/connect.h @@ -9,5 +9,6 @@ extern int git_connection_is_socket(struct child_process *conn); extern int server_supports(const char *feature); extern int parse_feature_request(const char *features, const char *feature); extern const char *server_feature_value(const char *feature, int *len_ret); +extern int url_is_local_not_ssh(const char *url); #endif diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index b4866ea3a3..5b2b1c2c13 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -612,4 +612,11 @@ do done done +test_expect_success MINGW 'fetch-pack --diag-url file://c:/repo' ' + check_prot_path file://c:/repo file c:/repo +' +test_expect_success MINGW 'fetch-pack --diag-url c:repo' ' + check_prot_path c:repo file c:repo +' + test_done diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 53a1de9efd..62fbd7e664 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -362,6 +362,14 @@ do test_clone_url [::1]:$repo ::1 $repo ' done +#home directory +test_expect_success "clone host:/~repo" ' + test_clone_url host:/~repo host "~repo" +' + +test_expect_success "clone [::1]:/~repo" ' + test_clone_url [::1]:/~repo ::1 "~repo" +' # Corner cases for url in foo/bar:baz [foo]bar/baz:qux [foo/bar]:baz diff --git a/transport.c b/transport.c index 7202b7777d..5485e2ab1e 100644 --- a/transport.c +++ b/transport.c @@ -885,14 +885,6 @@ void transport_take_over(struct transport *transport, transport->cannot_reuse = 1; } -static 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); -} - static int is_file(const char *url) { struct stat buf; @@ -941,7 +933,7 @@ struct transport *transport_get(struct remote *remote, const char *url) ret->fetch = fetch_objs_via_rsync; ret->push = rsync_transport_push; ret->smart_options = NULL; - } else if (is_local(url) && is_file(url) && is_bundle(url, 1)) { + } else if (url_is_local_not_ssh(url) && is_file(url) && is_bundle(url, 1)) { struct bundle_transport_data *data = xcalloc(1, sizeof(*data)); ret->data = data; ret->get_refs_list = get_refs_from_bundle; @@ -1297,7 +1289,7 @@ char *transport_anonymize_url(const char *url) size_t anon_len, prefix_len = 0; anon_part = strchr(url, '@'); - if (is_local(url) || !anon_part) + if (url_is_local_not_ssh(url) || !anon_part) goto literal_copy; anon_len = strlen(++anon_part); From a2036d7e00ad8aa16ba010a80078e10f0e4568a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torsten=20B=C3=B6gershausen?= Date: Thu, 28 Nov 2013 20:50:15 +0100 Subject: [PATCH 10/10] git_connect(): use common return point MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use only one return point from git_connect(), doing the free(); return conn; only at one place in the code. There may be a little confusion what the variable "host" is for. At some places it is only the host part, at other places it may include the port number, so change host into hostandport here. Signed-off-by: Torsten Bögershausen Signed-off-by: Junio C Hamano --- connect.c | 100 +++++++++++++++++++++++++----------------------------- 1 file changed, 46 insertions(+), 54 deletions(-) diff --git a/connect.c b/connect.c index 04093c4180..8a013a723b 100644 --- a/connect.c +++ b/connect.c @@ -656,7 +656,7 @@ static struct child_process no_fork; struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags) { - char *host, *path; + char *hostandport, *path; struct child_process *conn = &no_fork; enum protocol protocol; const char **arg; @@ -667,26 +667,22 @@ struct child_process *git_connect(int fd[2], const char *url, */ signal(SIGCHLD, SIG_DFL); - protocol = parse_connect_url(url, &host, &path); + protocol = parse_connect_url(url, &hostandport, &path); if (flags & CONNECT_DIAG_URL) { printf("Diag: url=%s\n", url ? url : "NULL"); printf("Diag: protocol=%s\n", prot_name(protocol)); - printf("Diag: hostandport=%s\n", host ? host : "NULL"); + printf("Diag: hostandport=%s\n", hostandport ? hostandport : "NULL"); printf("Diag: path=%s\n", path ? path : "NULL"); - free(host); - free(path); - return NULL; - } - - if (protocol == PROTO_GIT) { + conn = NULL; + } else if (protocol == PROTO_GIT) { /* These underlying connection commands die() if they * cannot connect. */ - char *target_host = xstrdup(host); - if (git_use_proxy(host)) - conn = git_proxy_connect(fd, host); + char *target_host = xstrdup(hostandport); + if (git_use_proxy(hostandport)) + conn = git_proxy_connect(fd, hostandport); else - git_tcp_connect(fd, host, flags); + git_tcp_connect(fd, hostandport, flags); /* * Separate original protocol components prog and path * from extended host header with a NUL byte. @@ -699,54 +695,50 @@ struct child_process *git_connect(int fd[2], const char *url, prog, path, 0, target_host, 0); free(target_host); - free(host); - free(path); - return conn; - } + } else { + conn = xcalloc(1, sizeof(*conn)); - conn = xcalloc(1, sizeof(*conn)); + strbuf_addstr(&cmd, prog); + strbuf_addch(&cmd, ' '); + sq_quote_buf(&cmd, path); - strbuf_addstr(&cmd, prog); - strbuf_addch(&cmd, ' '); - sq_quote_buf(&cmd, path); + conn->in = conn->out = -1; + conn->argv = arg = xcalloc(7, sizeof(*arg)); + if (protocol == PROTO_SSH) { + const char *ssh = getenv("GIT_SSH"); + int putty = ssh && strcasestr(ssh, "plink"); + char *ssh_host = hostandport; + const char *port = NULL; + get_host_and_port(&ssh_host, &port); + port = get_port_numeric(port); - conn->in = conn->out = -1; - conn->argv = arg = xcalloc(7, sizeof(*arg)); - if (protocol == PROTO_SSH) { - const char *ssh = getenv("GIT_SSH"); - int putty = ssh && strcasestr(ssh, "plink"); - char *ssh_host = host; /* keep host for the free() below */ - const char *port = NULL; - get_host_and_port(&ssh_host, &port); - port = get_port_numeric(port); + if (!ssh) ssh = "ssh"; - if (!ssh) ssh = "ssh"; - - *arg++ = ssh; - if (putty && !strcasestr(ssh, "tortoiseplink")) - *arg++ = "-batch"; - if (port) { - /* P is for PuTTY, p is for OpenSSH */ - *arg++ = putty ? "-P" : "-p"; - *arg++ = port; + *arg++ = ssh; + if (putty && !strcasestr(ssh, "tortoiseplink")) + *arg++ = "-batch"; + if (port) { + /* P is for PuTTY, p is for OpenSSH */ + *arg++ = putty ? "-P" : "-p"; + *arg++ = port; + } + *arg++ = ssh_host; + } else { + /* remove repo-local variables from the environment */ + conn->env = local_repo_env; + conn->use_shell = 1; } - *arg++ = ssh_host; - } - else { - /* remove repo-local variables from the environment */ - conn->env = local_repo_env; - conn->use_shell = 1; - } - *arg++ = cmd.buf; - *arg = NULL; + *arg++ = cmd.buf; + *arg = NULL; - if (start_command(conn)) - die("unable to fork"); + if (start_command(conn)) + die("unable to fork"); - fd[0] = conn->out; /* read from child's stdout */ - fd[1] = conn->in; /* write to child's stdin */ - strbuf_release(&cmd); - free(host); + fd[0] = conn->out; /* read from child's stdout */ + fd[1] = conn->in; /* write to child's stdin */ + strbuf_release(&cmd); + } + free(hostandport); free(path); return conn; }