Re: [PATCH] gitweb: fix error when highlight is enabled

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 26, 2012, 09:55 UTC
Message-ID: <7vpq1xp271.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1356508456-17454-1-git-send-email-orgads@gmail.com>

Orgad Shaneh <orgads@gmail.com> writes:

> Use of uninitialized value in substitution iterator at gitweb.cgi line 1560

This is not just about squelching an error message, but more importantly, attempting to fix an information lossage, no?

The statement captures each control character in the string to $1, then matches a class of known/safe control chars against that control character we just have seen. If matches, it just wants to use that control character, otherwise it wants to apply quot_cec() on that control character. It forgets that "$1" is reset immediately when =~ matches with the class of known/safe control chars, and your version attempts to fix it by recapturing it.

What if you are looking at a non-safe control, say "\001"? It is matched and is captured by ([[;cntrl:]]), making $1 -eq "\001", and then the replacement side of s///e operator, tries to match and capture it with ([\t\n\r]), but it does *not* match.

What does that "$1" you are feeding quot_cec() contain at that point? I _think_ "$1" is left intact when the inner match fails and you are correctly feeding "\001" to quot_cec(), but it is not immediately obvious. Perl regexp, especially s///e, is a yucky language X-<.

I wonder if there is a better way to express what goes inside the replacement side of this s///e construct in a more obvious way. The updated one may be correct but it looks too subtle to my taste..

Show 17 quoted lines

> Signed-off-by: Orgad Shaneh <orgads@gmail.com>
> ---
>  gitweb/gitweb.perl |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 0f207f2..862b9cd 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1556,7 +1556,7 @@ sub sanitize {
>  	return undef unless defined $str;
>  
>  	$str = to_utf8($str);
> -	$str =~ s|([[:cntrl:]])|($1 =~ /[\t\n\r]/ ? $1 : quot_cec($1))|eg;
> +	$str =~ s|([[:cntrl:]])|($1 =~ /([\t\n\r])/ ? $1 : quot_cec($1))|eg;
>  	return $str;
>  }