[PATCH v6 09/10] connect.c: Refactor url parsing

[PATCH v6 09/10] connect.c: Refactor url parsing

[Torsten Bögershausen]

Make the function is_local() from tramsport.c public and use it
in both transport.c and connect.c
Use a protocol "local" for URLs for the local file system.
---
 connect.c        | 58 ++++++++++++++++++++++++++++++--------------------------
 connect.h        |  1 +
 t/t5601-clone.sh | 10 +---------
 transport.c      |  8 --------
 4 files changed, 33 insertions(+), 44 deletions(-)

diff --git a/connect.c b/connect.c
index 3d174c8..95568ac 100644
--- a/connect.c
+++ b/connect.c
@@ -232,13 +232,23 @@ int server_supports(const char *feature)
 
 enum protocol {
 	PROTO_LOCAL = 1,
+	PROTO_FILE,
 	PROTO_SSH,
 	PROTO_GIT
 };
 
+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 const char *prot_name(enum protocol protocol) {
 	switch (protocol) {
 		case PROTO_LOCAL:
+		case PROTO_FILE:
 			return "file";
 		case PROTO_SSH:
 			return "ssh";
@@ -260,7 +270,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);
 }
 
@@ -563,9 +573,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);
@@ -577,10 +586,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 (!is_local(url)) {
+			protocol = PROTO_SSH;
+			separator = ':';
+		}
 	}
 
 	/*
@@ -596,17 +607,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;
+	else
+		path = strchr(end, separator);
 
 	if (!path || !*path)
 		die("No path specified. See 'man git-pull' for valid url syntax");
@@ -615,23 +621,21 @@ 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 && separator == '/') {
-		char *ptr = path;
+
+	end = path; /* Need to \0 terminate host here */
+	if (separator == ':')
+		path++; /* path starts after ':' */
+	if ((protocol == PROTO_GIT) ||
+			(protocol == PROTO_SSH && separator == '/')) {
 		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 527d58a..ce657b3 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);
+int is_local(const char *url);
 
 #endif
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 57234c0..bd1bfd3 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
diff --git a/transport.c b/transport.c
index 7202b77..a09ba95 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;
-- 
1.8.4.457.g424cb08

Re: [PATCH v6 09/10] connect.c: Refactor url parsing

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> Make the function is_local() from tramsport.c public and use it

s/ams/ans/, perhaps?

> in both transport.c and connect.c
> Use a protocol "local" for URLs for the local file system.
> ---
>  connect.c        | 58 ++++++++++++++++++++++++++++++--------------------------
>  connect.h        |  1 +
>  t/t5601-clone.sh | 10 +---------
>  transport.c      |  8 --------
>  4 files changed, 33 insertions(+), 44 deletions(-)
>
> diff --git a/connect.c b/connect.c
> index 3d174c8..95568ac 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -232,13 +232,23 @@ int server_supports(const char *feature)
>  
>  enum protocol {
>  	PROTO_LOCAL = 1,
> +	PROTO_FILE,
>  	PROTO_SSH,
>  	PROTO_GIT
>  };
>  
> +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 const char *prot_name(enum protocol protocol) {
>  	switch (protocol) {
>  		case PROTO_LOCAL:
> +		case PROTO_FILE:
>  			return "file";
>  		case PROTO_SSH:
>  			return "ssh";
> @@ -260,7 +270,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);
>  }
>  
> @@ -563,9 +573,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 = '/';

Hmph.  Doesn't this belong to the earlier patch that did s/c/separator/?

>  	enum protocol protocol = PROTO_LOCAL;
> -	int free_path = 0;
>  
>  	if (is_url(url_orig))
>  		url = url_decode(url_orig);
> @@ -577,10 +586,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 (!is_local(url)) {
> +			protocol = PROTO_SSH;
> +			separator = ':';
> +		}
>  	}
>  
>  	/*
> @@ -596,17 +607,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))

Hmm, this is for things like...  "file:///Z:/Documents/repo.git"???

> diff --git a/connect.h b/connect.h
> index 527d58a..ce657b3 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);
> +int is_local(const char *url);

In .h files, we very strongly prefer "extern" in front.

I also have to wonder if "is_local()" too generic a name for a
global API function.  It was a perfectly fine name for a static
function within the context of transport.c, though.

>  #endif
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 57234c0..bd1bfd3 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
> diff --git a/transport.c b/transport.c
> index 7202b77..a09ba95 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;