Re: [PATCH v2 1/8] gitweb: esc_html_hl_regions(): Don't create empty <span> elements

Jakub Narebski [off-list ref] wrote:

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

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

quoted

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

  ^^
s/if/If/

quoted

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.

quoted

It also minimally increases generated page size for no special reason.

Signed-off-by: Michał Kiedrowicz <redacted>
---

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.

quoted

 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/

quoted

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