Re: [PATCHv2 GSOC 01/11] gitweb: fix esc_url

Jakub Narebski [off-list ref] writes:

On Thu, 15 Jul 2010, Pavan Kumar Sunkara wrote:

quoted

The custom CGI escaping done in esc_url failed to escape UTF-8
properly. Fix by using CGI::escape on each sequence of matched
characters instead of sprintf()ing a custom escaping for each byte.

Additionally, the space -> + escape was being escaped due to greedy
matching on the first substitution. Fix by adding space to the
list of characters not handled on the first substitution.

Finally, remove an unnecessary escaping of the + sign.

commit 452e225 has missed fixing esc_url.

Signed-off-by: Pavan Kumar Sunkara <redacted>

First, as this patch is independent and unrelated to either splitting
gitweb, or write support, it would be better if this patch was sent
individually to git mailing list, and not only as a part of a large
patch series.  It would likely to be applied, as it is pure bugfix.

Second, I would probably write commit message differently, to emphasize
that it is just finishing work of commit 452e225 (gitweb: fix esc_param,
2009-10-13) by fixing esc_url like it fixed esc_params.  But it is not
something very important.

I tentatively rewrote the message like so:

    Earlier, 452e225 (gitweb: fix esc_param, 2009-10-13) fixed CGI
    escaping rules used in esc_url.  A very similar logic exists in
    esc_param and needs to be fixed the same way.

It makes one wonder why they have to be separate functions, doesn't it,
though?

For what it is worth, ACK from me for this patch.

Thanks

Dnia czwartek 15. lipca 2010 20:57, Junio C Hamano napisał:

Jakub Narebski [off-list ref] writes:

quoted

On Thu, 15 Jul 2010, Pavan Kumar Sunkara wrote:

quoted

The custom CGI escaping done in esc_url failed to escape UTF-8
properly. Fix by using CGI::escape on each sequence of matched
characters instead of sprintf()ing a custom escaping for each byte.

Additionally, the space -> + escape was being escaped due to greedy
matching on the first substitution. Fix by adding space to the
list of characters not handled on the first substitution.

Finally, remove an unnecessary escaping of the + sign.

commit 452e225 has missed fixing esc_url.

Signed-off-by: Pavan Kumar Sunkara <redacted>

[...]

quoted

Second, I would probably write commit message differently, to emphasize
that it is just finishing work of commit 452e225 (gitweb: fix esc_param,
2009-10-13) by fixing esc_url like it fixed esc_params.  But it is not
something very important.

I tentatively rewrote the message like so:

    Earlier, 452e225 (gitweb: fix esc_param, 2009-10-13) fixed CGI
    escaping rules used in esc_url.  A very similar logic exists in
    esc_param and needs to be fixed the same way.

Thanks.

It makes one wonder why they have to be separate functions, doesn't it,
though?

They need to be separate because you have to escape params-related
special characters ('?', ';', '=') when quoting params, but you
shouldn't when escaping (external) URL as a whole.

-- 
Jakub Narebski
Poland