Jakub Narebski <jnareb@xxxxxxxxx> 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@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.
>
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.
>
--
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