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@xxxxxxxxx>
> ---
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html