On Thu, 15 Jul 2010, Pavan Kumar Sunkara wrote:
quoted text
> 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 <pavan.sss1991@gmail.com>

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.

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

quoted text
> ---
>  gitweb/gitweb.perl |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 9446376..518328f 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1322,8 +1322,7 @@ sub esc_param {
>  sub esc_url {
>  	my $str = shift;
>  	return undef unless defined $str;
> -	$str =~ s/([^A-Za-z0-9\-_.~();\/;?:@&=])/sprintf("%%%02X", ord())/eg;
> -	$str =~ s/\+/%2B/g;
> +	$str =~ s/([^A-Za-z0-9\-_.~();\/;?:@&= ]+)/CGI::escape()/eg;
>  	$str =~ s/ /\+/g;
>  	return $str;
>  }
> -- 
> 1.7.1.455.g8f441
> 
> 

-- 
Jakub Narebski
Poland
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html