From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Fri, 23 Jul 2010 22:20:25 GMT
Subject: Re: [PATCH] Do not unquote + into ' ' in URLs
Message-ID: <AANLkTil6sD71n1aDrUQf4ATnlE_L7ltJa500cI6bH6X8@mail.gmail.com>
In-Reply-To: <db9c97908966fa332be07b2a9f5215679e35b9e0.1279920066.git.trast@student.ethz.ch>

On Fri, Jul 23, 2010 at 21:23, Thomas Rast <trast@student.ethz.ch> wrote:
> Since 9d2e942 (decode file:// and ssh:// URLs, 2010-05-23) the URL
> logic unquotes escaped URLs.  For the %2B type of escape, this is
> conformant with RFC 2396.  However, it also unquotes + into a space
> character, which is only appropriate for the query strings in HTTP.
> This notably broke fetching from the gtk+ repository.
>
> Remove the corresponding bit of code.
>
> Reported-by: Jasper St. Pierre <jstpierre@mecheye.net>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>
> Jasper St. Pierre wrote:
>> Yep. http://www.ietf.org/rfc/rfc2396.txt defines '+' as a reserved character,
>> but doesn't give a purpose for it. www-form-encoded replaces space with '+'
>> but in a URL it can mean anything it wants.
>
> So let's do this then, instead?
>
> Based on the discussion, I would consider this a bugfix that should go
> in 1.7.2.1.
>
>
>  t/t5601-clone.sh |   10 ++++++++--
>  url.c            |    5 +----
>  2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 8abb71a..4431dfd 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -178,8 +178,14 @@ test_expect_success 'clone respects global branch.autosetuprebase' '
>
>  test_expect_success 'respect url-encoding of file://' '
>        git init x+y &&
> -       test_must_fail git clone "file://$PWD/x+y" xy-url &&
> -       git clone "file://$PWD/x%2By" xy-url
> +       git clone "file://$PWD/x+y" xy-url-1 &&
> +       git clone "file://$PWD/x%2By" xy-url-2
> +'
> +
> +test_expect_success 'do not query-string-decode + in URLs' '
> +       rm -rf x+y &&
> +       git init "x y" &&
> +       test_must_fail git clone "file://$PWD/x+y" xy-no-plus
>  '
>
>  test_expect_success 'do not respect url-encoding of non-url path' '
> diff --git a/url.c b/url.c
> index 2306236..fa4b8d4 100644
> --- a/url.c
> +++ b/url.c
> @@ -90,10 +90,7 @@ static char *url_decode_internal(const char **query, const char *stop_at, struct
>                        }
>                }
>
> -               if (c == '+')
> -                       strbuf_addch(out, ' ');
> -               else
> -                       strbuf_addch(out, c);
> +               strbuf_addch(out, c);
>                q++;
>        } while (1);
>        *query = q;
> --
> 1.7.2.rc3.335.g26d7d
>

This looks good, Ack.

But as icing, it'd be nice to extend these tests to create files /
clone repositories with the rest of the reserved characters:
http://en.wikipedia.org/wiki/Percent-encoding#Percent-encoding_reserved_characters

Of course that would have to be wrapped in something that skips the
tests if those paths can't be created. E.g. "/" is a no-no on Unix,
and some of the others will probably cause troubles on other systems.