[PATCH] gitweb: Refactor diff body line classification

[Jakub Narebski <jnareb@gmail.com>]

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:

>>> 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(-)

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