Re: [PATCHv2 GSOC 06/11] gitweb: Create Gitweb::Escape module

[Jakub Narebski <jnareb@gmail.com>]

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

> Create a Gitweb::Escape module in 'gitweb/lib/Gitweb/Escape.pm'
> to store all the quoting/unquoting and escaping subroutines
> regarding the gitweb.perl script.
> 
> This module imports $fallback_encoding variable from
> Gitweb::Config module to use it in sub 'to_utf8'
> 
> Subroutines moved:
> 	to_utf8
> 	esc_param
> 	esc_url
> 	esc_html
> 	esc_path
> 	quot_cec
> 	quot_upr
> 	untabify
> 
> Update gitweb/Makefile to install Gitweb::Escape module alongside gitweb

Nice and straightforward refactoring.

For what it is worth, ACK from me.

[...]
> +# quote unsafe chars, but keep the slash, even when it's not
> +# correct, but quoted slashes look too horrible in bookmarks
> +sub esc_param {
> +	my $str = shift;
> +	return undef unless defined $str;
> +	$str =~ s/([^A-Za-z0-9\-_.~()\/:@ ]+)/CGI::escape($1)/eg;
> +	$str =~ s/ /\+/g;
> +	return $str;
> +}
> +
> +# quote unsafe chars in whole URL, so some charactrs cannot be quoted
> +sub esc_url {
> +	my $str = shift;
> +	return undef unless defined $str;
> +	$str =~ s/([^A-Za-z0-9\-_.~();\/;?:@&= ]+)/CGI::escape($1)/eg;
> +	$str =~ s/ /\+/g;
> +	return $str;
> +}

I see that here (or rather in corresponding preimage) you have dependency
on first patch in series, i.e. "gitweb: fix esc_url".

If you had send first patch as a separate email, not as part of series,
you would have need to mention in cover letter that the series (without
first patch) is based on / requires "gitweb: fix esc_url".

-- 
Jakub Narebski
Poland