From: Jakub Narebski <jnareb@gmail.com>
Date: Sat, 24 Mar 2012 18:58:23 GMT
Subject: Re: [PATCH v2 1/8] gitweb: esc_html_hl_regions(): Don't create empty <span> elements
Message-ID: <201203241958.23835.jnareb@gmail.com>
In-Reply-To: <1332543417-19664-2-git-send-email-michal.kiedrowicz@gmail.com>

Note: all those minor issues can be fixed while applying, I think.

On Fri, 23 Mar 2012, Michał Kiedrowicz wrote:

> if $s->[1] is equal to or less than $s->[0], esc_html_hl_regions()
  ^^
s/if/If/

> generates an empty <span> element.  It normally shouldn't be visible in
> the web broweser, but it doesn't look good when looking at page source.
          ^^^^^^^^ 
s/broweser/browser/

> It also minimally increases generated page size for no special reason.
> 
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> ---
> 
> I don't know if any code currently can produce empty <span> elements,
> but diff refinement highlighning does it.

I didn't want to make esc_html_hl_regions() paranoid and boggle it down
with checking that gitweb called it with sane values of parameters,
but this one is cheap and simple.  

It might be better to make esc_html_hl_regions() more robust instead
of modifying future caller to skip empty regions.

I have not read the rest of this series; for the time being conditional
ACK from me.

>  gitweb/gitweb.perl |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index a8b5fad..af645e5 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1738,6 +1738,9 @@ sub esc_html_hl_regions {
>  	my $pos = 0;
>  
>  	for my $s (@sel) {
> +		# Don't craete empty <span> elements.
                        ^^^^^^
s/craete/create/

> +		next if $s->[1] <= $s->[0];
> +
>  		$out .= esc_html(substr($str, $pos, $s->[0] - $pos))
>  			if ($s->[0] - $pos > 0);
>  		$out .= $cgi->span({-class => $css_class},
> -- 
> 1.7.8.4
 
P.S. I wonder if it wouldn't be better if we created and used loop-local
variables with descriptive names, e.g.

  my ($beg, $end) = @$s;

and use $beg in place of $s->[0] and $end in place of $s->[1], which are
a bit cryptic.

This of course doesn't affect this patch.

-- 
Jakub Narebski
Poland