threads / patch / 24165

patchDo not decode url protocol.

Subject: [PATCH] Do not decode url protocol.

## tl;dr

6 messages between Jun 22, 2010 and Jun 29, 2010. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Pascal Obry· Jun 22, 2010, 09:25 UTC · lore
Same patch with fixed formatting. Sorry.

When using the protocol git+ssh:// for example we do not want to decode the '+' as a space.

This fixes a regression introduced in 9d2e942.
---
 url.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
Show changes to url.c +9 −0
diff --git a/url.c b/url.c
index cd32b92..94a42a5 100644
--- a/url.c
+++ b/url.c
@@ -70,9 +70,18 @@ static int url_decode_char(const char *q)
 static char *url_decode_internal(const char **query, const char *stop_at)
 {
 	const char *q = *query;
+	const char *first_slash;
 	struct strbuf out;

 	strbuf_init(&out, 16);
+
+	/* Skip protocol. */
+	first_slash = strchr(*query, '/');
+
+	while (q < first_slash) {
+		strbuf_addch(&out, *q++);
+	}
+
 	do {
 		unsigned char c = *q;

-- 
1.7.1.524.g6df2f

-- 

--|------------------------------------------------------
--| Pascal Obry                           Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--|              http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595
Matthieu Moy· Jun 22, 2010, 11:22 UTC · re: Pascal Obry · lore

Re: [PATCH] Do not decode url protocol.

Pascal Obry <pascal@obry.net> writes:
> Same patch with fixed formatting. Sorry.

This part is for the commit message, and will appear in "git log" if Junio applies it as-is ...

> This fixes a regression introduced in 9d2e942.
> ---

... while this place (after ---, before diffstat) is meant for comments addressed to the list, and ignored while applying the patch.

(also, [PATCH v2] in the subject would have made it clear that this is the second version)

Otherwise, the patch sounds good.
>  url.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Matthieu Moy· Jun 22, 2010, 11:34 UTC · re: Pascal Obry · lore

Re: [PATCH] Do not decode url protocol.

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Otherwise, the patch sounds good.
Humm, looking a bit closer ...
Pascal Obry <pascal@obry.net> writes:
> @@ -70,9 +70,18 @@ static int url_decode_char(const char *q)
>  static char *url_decode_internal(const char **query, const char *stop_at)
>  {
This function is called from multiple places :
char *url_decode(const char *url)
{
	return url_decode_internal(&url, NULL);
}
char *url_decode_parameter_name(const char **query)
{
	return url_decode_internal(query, "&=");
}
char *url_decode_parameter_value(const char **query)
{
	return url_decode_internal(query, "&");
}

I don't think you want to avoid escaping until the first slash in url_decode_parameter_name and url_decode_parameter_value. I think you want to patch url_decode, not url_decode_internal.

> +	/* Skip protocol. */
> +	first_slash = strchr(*query, '/');

Are you sure the URL contains a / at this point? That would be a user error if it doesn't, but has this been validated (with a clean error message if needed) earlier in the code?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Pascal Obry· Jun 22, 2010, 11:46 UTC · re: Matthieu Moy · lore

Re: [PATCH] Do not decode url protocol.

Matthieu,
> Are you sure the URL contains a / at this point? That would be a user
> error if it doesn't, but has this been validated (with a clean error
> message if needed) earlier in the code?

Yes, all calls are either from http-backend.c (here we have a protocol specified, http://) or in connect.c where a test is done to make sure we have an url (and not a simple path):

	if (is_url(url_orig))
		url = url_decode(url_orig);
	else
		url = xstrdup(url_orig);
Pascal.
-- 
--|------------------------------------------------------
--| Pascal Obry                           Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--|              http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595
René Scharfe· Jun 23, 2010, 23:54 UTC · re: Pascal Obry · lore

Re: [PATCH] Do not decode url protocol.

Am 22.06.2010 11:25, schrieb Pascal Obry:
Show 33 quoted lines
> Same patch with fixed formatting. Sorry.
> 
> When using the protocol git+ssh:// for example we do not want to
> decode the '+' as a space.
> 
> This fixes a regression introduced in 9d2e942.
> ---
>  url.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/url.c b/url.c
> index cd32b92..94a42a5 100644
> --- a/url.c
> +++ b/url.c
> @@ -70,9 +70,18 @@ static int url_decode_char(const char *q)
>  static char *url_decode_internal(const char **query, const char *stop_at)
>  {
>  	const char *q = *query;
> +	const char *first_slash;
>  	struct strbuf out;
> 
>  	strbuf_init(&out, 16);
> +
> +	/* Skip protocol. */
> +	first_slash = strchr(*query, '/');
> +
> +	while (q < first_slash) {
> +		strbuf_addch(&out, *q++);
> +	}
> +
>  	do {
>  		unsigned char c = *q;
>

The protocol in an URL is followed by a colon and technically a slash doesn't have to be part of it at all. While in practise all URL schemes used by git have two slashes following the colon, it's just as easy to be more correct and search for ':' instead of '/' here.

And you can use strbuf_add() instead of the while loop because you know how many characters to copy.

René
Junio C Hamano· Jun 29, 2010, 16:50 UTC · re: René Scharfe · lore

Re: [PATCH] Do not decode url protocol.

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> The protocol in an URL is followed by a colon and technically a slash
> doesn't have to be part of it at all.  While in practise all URL schemes
> used by git have two slashes following the colon, it's just as easy to
> be more correct and search for ':' instead of '/' here.
Very true.  Thanks.

← back to recent threads