Re: [PATCH v2 1/8] gitweb: esc_html_hl_regions(): Don't create empty <span> elements
From: Jakub Narebski <hidden>
Date: 2016-06-15 22:53:22
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 <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. I have not read the rest of this series; for the time being conditional ACK from me.
quoted hunk ↗ jump to hunk
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.4P.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