Re: [PATCH] gitweb: Strip non-printable characters from syntax highlighter output

[Jakub Narebski <jnareb@gmail.com>]

On Fri, 16 Sep 2011, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:

> Micronit:
> 
> > +# Sanitize for use in XHTML + application/xml+xhtm (valid XML 1.0)
> > +sub sanitize {
> > +	my $str = shift;
> > +
> > +	return undef unless defined $str;
> 
> Given that the _whole_ point of this subroutine is to make $str safe for
> printing, wouldn't you want to either (1) die, declaring that feeding an
> undef to this subroutine is a programming error, or (2) return an empty
> string?

Well, that

	return undef unless defined $str;

line is copy'n'paste (as is most of sanitize() body) from esc_html().
This line was added in 1df4876 (gitweb: Protect escaping functions against
calling on undef, 2010-02-07) with the following explanation

    This is a bit of future-proofing esc_html and friends: when called
    with undefined value they would now would return undef... which would
    probably mean that error would still occur, but closer to the source
    of problem.
    
    This means that we can safely use
      esc_html(shift) || "Internal Server Error"
    in die_error() instead of
      esc_html(shift || "Internal Server Error")

So actually now I see that while this line is good to have in esc_html(),
it is not really necessary in sanitize().

But anyway we don't want to replace undef with an empty string; undef is
(usually) an error, and we want to catch it, not to hide it.
 
> Given that the input to this function is from the result of feeding $line
> to untabify, which relies on $line being defined, and that $line comes
> from "while (my $line = <$fd>)" (and then chomp $line), it may be Ok for
> this subroutine to make the same assumption as untabify makes.

Right.

Passing undef to sanitize() is usually an error, and we don't want to hide
it.  We want for gitweb test to detect it.

-- 
Jakub Narebski
Poland