From: Junio C Hamano <gitster@pobox•com>
To: "Torsten Bögershausen" <tboegi@web•de>
Cc: git@vger•kernel.org
Subject: Re: [PATCH v6 09/10] connect.c: Refactor url parsing
Date: Thu, 21 Nov 2013 15:38:07 -0800 [thread overview]
Message-ID: <xmqq7gc12un4.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <201311212141.57411.tboegi@web.de> ("Torsten Bögershausen"'s message of "Thu, 21 Nov 2013 21:41:56 +0100")
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;
prev parent reply other threads:[~2013-11-21 23:38 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-11-21 20:41 [PATCH v6 09/10] connect.c: Refactor url parsing Torsten Bögershausen
2013-11-21 23:38 ` Junio C Hamano [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq7gc12un4.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=tboegi@web$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox