{"thread":{"id":"35387","subject":"[PATCH v6 09/10] connect.c: Refactor url parsing","startedAt":"2013-11-21T20:41:56Z","lastAt":"2013-11-21T23:38:07Z","messageCount":2,"participants":["Torsten Bögershausen","Junio C Hamano"],"isPatch":true,"patchVersion":6,"patchTotal":10},"messages":[{"id":"230912","messageId":"201311212141.57411.tboegi@web.de","threadId":"35387","inReplyTo":null,"subject":"[PATCH v6 09/10] connect.c: Refactor url parsing","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-11-21T20:41:56Z","receivedAt":"2013-11-21T20:41:56Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"Make the function is_local() from tramsport.c public and use it\nin both transport.c and connect.c\nUse a protocol \"local\" for URLs for the local file system.\n---\n connect.c        | 58 ++++++++++++++++++++++++++++++--------------------------\n connect.h        |  1 +\n t/t5601-clone.sh | 10 +---------\n transport.c      |  8 --------\n 4 files changed, 33 insertions(+), 44 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 3d174c8..95568ac 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -232,13 +232,23 @@ int server_supports(const char *feature)\n \n enum protocol {\n \tPROTO_LOCAL = 1,\n+\tPROTO_FILE,\n \tPROTO_SSH,\n \tPROTO_GIT\n };\n \n+int is_local(const char *url)\n+{\n+\tconst char *colon = strchr(url, ':');\n+\tconst char *slash = strchr(url, '/');\n+\treturn !colon || (slash && slash < colon) ||\n+\t\thas_dos_drive_prefix(url);\n+}\n+\n static const char *prot_name(enum protocol protocol) {\n \tswitch (protocol) {\n \t\tcase PROTO_LOCAL:\n+\t\tcase PROTO_FILE:\n \t\t\treturn \"file\";\n \t\tcase PROTO_SSH:\n \t\t\treturn \"ssh\";\n@@ -260,7 +270,7 @@ static enum protocol get_protocol(const char *name)\n \tif (!strcmp(name, \"ssh+git\"))\n \t\treturn PROTO_SSH;\n \tif (!strcmp(name, \"file\"))\n-\t\treturn PROTO_LOCAL;\n+\t\treturn PROTO_FILE;\n \tdie(\"I don't handle protocol '%s'\", name);\n }\n \n@@ -563,9 +573,8 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n \tchar *url;\n \tchar *host, *path;\n \tchar *end;\n-\tint separator;\n+\tint separator = '/';\n \tenum protocol protocol = PROTO_LOCAL;\n-\tint free_path = 0;\n \n \tif (is_url(url_orig))\n \t\turl = url_decode(url_orig);\n@@ -577,10 +586,12 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n \t\t*host = '\\0';\n \t\tprotocol = get_protocol(url);\n \t\thost += 3;\n-\t\tseparator = '/';\n \t} else {\n \t\thost = url;\n-\t\tseparator = ':';\n+\t\tif (!is_local(url)) {\n+\t\t\tprotocol = PROTO_SSH;\n+\t\t\tseparator = ':';\n+\t\t}\n \t}\n \n \t/*\n@@ -596,17 +607,12 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n \t} else\n \t\tend = host;\n \n-\tpath = strchr(end, separator);\n-\tif (path && !has_dos_drive_prefix(end)) {\n-\t\tif (separator == ':') {\n-\t\t\tif (host != url || path < strchrnul(host, '/')) {\n-\t\t\t\tprotocol = PROTO_SSH;\n-\t\t\t\t*path++ = '\\0';\n-\t\t\t} else /* '/' in the host part, assume local path */\n-\t\t\t\tpath = end;\n-\t\t}\n-\t} else\n+\tif (protocol == PROTO_LOCAL)\n+\t\tpath = end;\n+\telse if (protocol == PROTO_FILE && has_dos_drive_prefix(end))\n \t\tpath = end;\n+\telse\n+\t\tpath = strchr(end, separator);\n \n \tif (!path || !*path)\n \t\tdie(\"No path specified. See 'man git-pull' for valid url syntax\");\n@@ -615,23 +621,21 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n \t * null-terminate hostname and point path to ~ for URL's like this:\n \t *    ssh://host.xz/~user/repo\n \t */\n-\tif (protocol != PROTO_LOCAL && separator == '/') {\n-\t\tchar *ptr = path;\n+\n+\tend = path; /* Need to \\0 terminate host here */\n+\tif (separator == ':')\n+\t\tpath++; /* path starts after ':' */\n+\tif ((protocol == PROTO_GIT) ||\n+\t\t\t(protocol == PROTO_SSH && separator == '/')) {\n \t\tif (path[1] == '~')\n \t\t\tpath++;\n-\t\telse {\n-\t\t\tpath = xstrdup(ptr);\n-\t\t\tfree_path = 1;\n-\t\t}\n-\n-\t\t*ptr = '\\0';\n \t}\n \n+\tpath = xstrdup(path);\n+\t*end = '\\0';\n+\n \t*ret_host = xstrdup(host);\n-\tif (free_path)\n-\t\t*ret_path = path;\n-\telse\n-\t\t*ret_path = xstrdup(path);\n+\t*ret_path = path;\n \tfree(url);\n \treturn protocol;\n }\ndiff --git a/connect.h b/connect.h\nindex 527d58a..ce657b3 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -9,5 +9,6 @@ extern int git_connection_is_socket(struct child_process *conn);\n extern int server_supports(const char *feature);\n extern int parse_feature_request(const char *features, const char *feature);\n extern const char *server_feature_value(const char *feature, int *len_ret);\n+int is_local(const char *url);\n \n #endif\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 57234c0..bd1bfd3 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -364,15 +364,7 @@ do\n done\n \n # Corner cases\n-# failing\n-for url in [foo]bar/baz:qux [foo/bar]:baz\n-do\n-\ttest_expect_failure \"clone $url is not ssh\" '\n-\t\ttest_clone_url $url none\n-\t'\n-done\n-\n-for url in foo/bar:baz\n+for url in foo/bar:baz [foo]bar/baz:qux [foo/bar]:baz\n do\n \ttest_expect_success \"clone $url is not ssh\" '\n \t\ttest_clone_url $url none\ndiff --git a/transport.c b/transport.c\nindex 7202b77..a09ba95 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -885,14 +885,6 @@ void transport_take_over(struct transport *transport,\n \ttransport->cannot_reuse = 1;\n }\n \n-static int is_local(const char *url)\n-{\n-\tconst char *colon = strchr(url, ':');\n-\tconst char *slash = strchr(url, '/');\n-\treturn !colon || (slash && slash < colon) ||\n-\t\thas_dos_drive_prefix(url);\n-}\n-\n static int is_file(const char *url)\n {\n \tstruct stat buf;\n-- \n1.8.4.457.g424cb08\n"},{"id":"230925","messageId":"xmqq7gc12un4.fsf@gitster.dls.corp.google.com","threadId":"35387","inReplyTo":"201311212141.57411.tboegi@web.de","subject":"Re: [PATCH v6 09/10] connect.c: Refactor url parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-21T23:38:07Z","receivedAt":"2013-11-21T23:38:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> Make the function is_local() from tramsport.c public and use it\n\ns/ams/ans/, perhaps?\n\n> in both transport.c and connect.c\n> Use a protocol \"local\" for URLs for the local file system.\n> ---\n>  connect.c        | 58 ++++++++++++++++++++++++++++++--------------------------\n>  connect.h        |  1 +\n>  t/t5601-clone.sh | 10 +---------\n>  transport.c      |  8 --------\n>  4 files changed, 33 insertions(+), 44 deletions(-)\n>\n> diff --git a/connect.c b/connect.c\n> index 3d174c8..95568ac 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -232,13 +232,23 @@ int server_supports(const char *feature)\n>  \n>  enum protocol {\n>  \tPROTO_LOCAL = 1,\n> +\tPROTO_FILE,\n>  \tPROTO_SSH,\n>  \tPROTO_GIT\n>  };\n>  \n> +int is_local(const char *url)\n> +{\n> +\tconst char *colon = strchr(url, ':');\n> +\tconst char *slash = strchr(url, '/');\n> +\treturn !colon || (slash && slash < colon) ||\n> +\t\thas_dos_drive_prefix(url);\n> +}\n> +\n>  static const char *prot_name(enum protocol protocol) {\n>  \tswitch (protocol) {\n>  \t\tcase PROTO_LOCAL:\n> +\t\tcase PROTO_FILE:\n>  \t\t\treturn \"file\";\n>  \t\tcase PROTO_SSH:\n>  \t\t\treturn \"ssh\";\n> @@ -260,7 +270,7 @@ static enum protocol get_protocol(const char *name)\n>  \tif (!strcmp(name, \"ssh+git\"))\n>  \t\treturn PROTO_SSH;\n>  \tif (!strcmp(name, \"file\"))\n> -\t\treturn PROTO_LOCAL;\n> +\t\treturn PROTO_FILE;\n>  \tdie(\"I don't handle protocol '%s'\", name);\n>  }\n>  \n> @@ -563,9 +573,8 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n>  \tchar *url;\n>  \tchar *host, *path;\n>  \tchar *end;\n> -\tint separator;\n> +\tint separator = '/';\n\nHmph.  Doesn't this belong to the earlier patch that did s/c/separator/?\n\n>  \tenum protocol protocol = PROTO_LOCAL;\n> -\tint free_path = 0;\n>  \n>  \tif (is_url(url_orig))\n>  \t\turl = url_decode(url_orig);\n> @@ -577,10 +586,12 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n>  \t\t*host = '\\0';\n>  \t\tprotocol = get_protocol(url);\n>  \t\thost += 3;\n> -\t\tseparator = '/';\n>  \t} else {\n>  \t\thost = url;\n> -\t\tseparator = ':';\n> +\t\tif (!is_local(url)) {\n> +\t\t\tprotocol = PROTO_SSH;\n> +\t\t\tseparator = ':';\n> +\t\t}\n>  \t}\n>  \n>  \t/*\n> @@ -596,17 +607,12 @@ static enum protocol parse_connect_url(const char *url_orig, char **ret_host,\n>  \t} else\n>  \t\tend = host;\n>  \n> -\tpath = strchr(end, separator);\n> -\tif (path && !has_dos_drive_prefix(end)) {\n> -\t\tif (separator == ':') {\n> -\t\t\tif (host != url || path < strchrnul(host, '/')) {\n> -\t\t\t\tprotocol = PROTO_SSH;\n> -\t\t\t\t*path++ = '\\0';\n> -\t\t\t} else /* '/' in the host part, assume local path */\n> -\t\t\t\tpath = end;\n> -\t\t}\n> -\t} else\n> +\tif (protocol == PROTO_LOCAL)\n> +\t\tpath = end;\n> +\telse if (protocol == PROTO_FILE && has_dos_drive_prefix(end))\n\nHmm, this is for things like...  \"file:///Z:/Documents/repo.git\"???\n\n> diff --git a/connect.h b/connect.h\n> index 527d58a..ce657b3 100644\n> --- a/connect.h\n> +++ b/connect.h\n> @@ -9,5 +9,6 @@ extern int git_connection_is_socket(struct child_process *conn);\n>  extern int server_supports(const char *feature);\n>  extern int parse_feature_request(const char *features, const char *feature);\n>  extern const char *server_feature_value(const char *feature, int *len_ret);\n> +int is_local(const char *url);\n\nIn .h files, we very strongly prefer \"extern\" in front.\n\nI also have to wonder if \"is_local()\" too generic a name for a\nglobal API function.  It was a perfectly fine name for a static\nfunction within the context of transport.c, though.\n\n>  #endif\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> index 57234c0..bd1bfd3 100755\n> --- a/t/t5601-clone.sh\n> +++ b/t/t5601-clone.sh\n> @@ -364,15 +364,7 @@ do\n>  done\n>  \n>  # Corner cases\n> -# failing\n> -for url in [foo]bar/baz:qux [foo/bar]:baz\n> -do\n> -\ttest_expect_failure \"clone $url is not ssh\" '\n> -\t\ttest_clone_url $url none\n> -\t'\n> -done\n> -\n> -for url in foo/bar:baz\n> +for url in foo/bar:baz [foo]bar/baz:qux [foo/bar]:baz\n>  do\n>  \ttest_expect_success \"clone $url is not ssh\" '\n>  \t\ttest_clone_url $url none\n> diff --git a/transport.c b/transport.c\n> index 7202b77..a09ba95 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -885,14 +885,6 @@ void transport_take_over(struct transport *transport,\n>  \ttransport->cannot_reuse = 1;\n>  }\n>  \n> -static int is_local(const char *url)\n> -{\n> -\tconst char *colon = strchr(url, ':');\n> -\tconst char *slash = strchr(url, '/');\n> -\treturn !colon || (slash && slash < colon) ||\n> -\t\thas_dos_drive_prefix(url);\n> -}\n> -\n>  static int is_file(const char *url)\n>  {\n>  \tstruct stat buf;\n"}]}