[PATCH] gitweb: Refactor diff body line classification
Simplify classification of diff line body in format_diff_line(), replacing two if-elsif chains (one for ordinary diff and one for combined diff of a merge commit) with regexp match. Refactor this code into diff_line_class() function.
While at it:
* Fix an artifact in that $diff_class included leading space to be
able to compose classes like this "class=\"diff$diff_class\"', even
when $diff_class was an empty string. This made code unnecessary
ugly: $diff_class is now just class name or an empty string.
* Introduce "ctx" class for context lines ($diff_class was set to ""
in this case before this commit).
Idea and initial code by Junio C Hamano, polish and testing by Jakub Narebski. Inspired by patch adding side-by-side diff by Kato Kazuyoshi, which required $diff_class to be name of class without extra space.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>> Kato Kazuyoshi wrote:
Show 51 quoted lines
>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>>> index 85d64b2..095adda 100755
>>> --- a/gitweb/gitweb.perl
>>> +++ b/gitweb/gitweb.perl
>>> @@ -2235,28 +2235,30 @@ sub format_diff_line {
>>> # combined diff
>>> my $prefix = substr($line, 0, scalar @{$from->{'href'}});
>>> if ($line =~ m/^\@{3}/) {
>>> - $diff_class = " chunk_header";
>>> + $diff_class = "chunk_header";
>>> } elsif ($line =~ m/^\\/) {
>>> - $diff_class = " incomplete";
>>> + $diff_class = "incomplete";
>>> } elsif ($prefix =~ tr/+/+/) {
>>> - $diff_class = " add";
>>> + $diff_class = "add";
>>> } elsif ($prefix =~ tr/-/-/) {
>>> - $diff_class = " rem";
>>> + $diff_class = "rem";
>>> }
>>
>> Hmmm... that reminds me: this if-elsif chain is a bit ugly, but would
>> be hard to replace without given ... when, I think.
>
> I wasn't reading the existing context line, but now that you mention it,
> they are not just ugly but are borderline of being incorrect (e.g. it does
> not catch a broken hunk-header that begins with "@@@@" for a two-way
> merge).
>
> Assuming that $from->{'href'} has all the parents (i.e. 2 for two-way
> merge), shouldn't the code be more like this?
>
> # combined diff
> my $num_sign = @{$from->{'href'}} + 1;
> my @cc_classifier = (
> ["\@{$num_sign} ", "chunk_header"],
> ['\\', "incomplete"],
> [" {$num_sign}", ""],
> ["[+ ]{$num_sign}", "add"],
> ["[- ]{$num_sign}", "rem"],
> );
> for my $cls (@cc_classifier) {
> if ($line =~ /^$cls->[0]$/) {
> $diff_class = $cls->[1];
> last;
> }
> }
>
> Also don't we want to use "context" or something for the css class for the
> context lines, instead of assuming that we won't want to paint it in any
> special color?How about this?
Kato Kazuyoshi, this should replace your
[PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class
gitweb/gitweb.perl | 67 ++++++++++++++++++++++++++++-----------------------
1 files changed, 37 insertions(+), 30 deletions(-)
Show changes to gitweb/gitweb.perl +37 −30
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b0eaad7..b3284d4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2247,40 +2247,47 @@ sub format_diff_cc_simplified {
return $result;
}
+sub diff_line_class {
+ my ($line, $from, $to) = @_;
+
+ # ordinary diff
+ my $num_sign = 1;
+ # combined diff
+ if ($from && $to && ref($from->{'href'}) eq "ARRAY") {
+ $num_sign = scalar @{$from->{'href'}};
+ }
+
+ my @diff_line_classifier = (
+ { regexp => qr/^\@\@{$num_sign} /, class => "chunk_header"},
+ { regexp => qr/^\\/, class => "incomplete" },
+ { regexp => qr/^ {$num_sign}/, class => "ctx" },
+ # classifier for context must come before classifier add/rem,
+ # or we would have to use more complicated regexp, for example
+ # qr/(?= {0,$m}\+)[+ ]{$num_sign}/, where $m = $num_sign - 1;
+ { regexp => qr/^[+ ]{$num_sign}/, class => "add" },
+ { regexp => qr/^[- ]{$num_sign}/, class => "rem" },
+ );
+ for my $clsfy (@diff_line_classifier) {
+ return $clsfy->{'class'}
+ if ($line =~ $clsfy->{'regexp'});
+ }
+
+ # fallback
+ return "";
+}
+
# format patch (diff) line (not to be used for diff headers)
sub format_diff_line {
my $line = shift;
my ($from, $to) = @_;
- my $diff_class = "";
- chomp $line;
+ my $diff_class = diff_line_class($line, $from, $to);
+ my $diff_classes = "diff";
+ $diff_classes .= " $diff_class" if ($diff_class);
- if ($from && $to && ref($from->{'href'}) eq "ARRAY") {
- # combined diff
- my $prefix = substr($line, 0, scalar @{$from->{'href'}});
- if ($line =~ m/^\@{3}/) {
- $diff_class = " chunk_header";
- } elsif ($line =~ m/^\\/) {
- $diff_class = " incomplete";
- } elsif ($prefix =~ tr/+/+/) {
- $diff_class = " add";
- } elsif ($prefix =~ tr/-/-/) {
- $diff_class = " rem";
- }
- } else {
- # assume ordinary diff
- my $char = substr($line, 0, 1);
- if ($char eq '+') {
- $diff_class = " add";
- } elsif ($char eq '-') {
- $diff_class = " rem";
- } elsif ($char eq '@') {
- $diff_class = " chunk_header";
- } elsif ($char eq "\\") {
- $diff_class = " incomplete";
- }
- }
+ chomp $line;
$line = untabify($line);
+
if ($from && $to && $line =~ m/^\@{2} /) {
my ($from_text, $from_start, $from_lines, $to_text, $to_start, $to_lines, $section) =
$line =~ m/^\@{2} (-(\d+)(?:,(\d+))?) (\+(\d+)(?:,(\d+))?) \@{2}(.*)$/;
@@ -2298,7 +2305,7 @@ sub format_diff_line {
}
$line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
"<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>";
- return "<div class=\"diff$diff_class\">$line</div>\n";
+ return "<div class=\"$diff_classes\">$line</div>\n";
} elsif ($from && $to && $line =~ m/^\@{3}/) {
my ($prefix, $ranges, $section) = $line =~ m/^(\@+) (.*?) \@+(.*)$/;
my (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);
@@ -2331,9 +2338,9 @@ sub format_diff_line {
}
$line .= " $prefix</span>" .
"<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>";
- return "<div class=\"diff$diff_class\">$line</div>\n";
+ return "<div class=\"$diff_classes\">$line</div>\n";
}
- return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1) . "</div>\n";
+ return "<div class=\"$diff_classes\">" . esc_html($line, -nbsp=>1) . "</div>\n";
}
# Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
--
1.7.6
Kato Kazuyoshi
