From: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
Date: Sat, 24 Mar 2012 23:38:54 GMT
Subject: Re: [PATCH v2 1/8] gitweb: esc_html_hl_regions(): Don't create empty <span> elements
Message-ID: <20120325003854.7a4e309c@gmail.com>
In-Reply-To: <201203241958.23835.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> wrote:

> 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/

Thanks.  I'm terribly sorry for these typos.  I really must tripple
check everything I write.

> 
> > 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.
> 

Yeah, the caller is just

+$esc_rem = esc_html_hl_regions($rem, 'marked', [$prefix_len, @rem - $suffix_len], -nbsp=>1);
+$esc_add = esc_html_hl_regions($add, 'marked', [$prefix_len, @add - $suffix_len], -nbsp=>1);

Without this patch I would have to do (untested)

+# Create <span> only if there is anything to mark.
+if($prefix_len < @rem - $suffix_len) {
+	$esc_rem = esc_html_hl_regions($rem, 'marked', [$prefix_len, @rem - $suffix_len], -nbsp=>1);
+} else {
+	$esc_rem = esc_html($rem, -nbsp=1);
+}
+if($prefix_len < @add - $suffix_len) {
+	$esc_add = esc_html_hl_regions($add, 'marked', [$prefix_len, @add - $suffix_len], -nbsp=>1);
+} else {
+	$esc_add = esc_html($add, -nbsp=1);
+}

or something like that.

> 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.
>