[PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class

[PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class

[Kato Kazuyoshi]

The format_diff_line() will return $diff_class and HTML in upcoming changes.
---
 gitweb/gitweb.perl |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

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";
 		}
 	} else {
 		# assume ordinary diff
 		my $char = substr($line, 0, 1);
 		if ($char eq '+') {
-			$diff_class = " add";
+			$diff_class = "add";
 		} elsif ($char eq '-') {
-			$diff_class = " rem";
+			$diff_class = "rem";
 		} elsif ($char eq '@') {
-			$diff_class = " chunk_header";
+			$diff_class = "chunk_header";
 		} elsif ($char eq "\\") {
-			$diff_class = " incomplete";
+			$diff_class = "incomplete";
 		}
 	}
 	$line = untabify($line);
+
+	my $div_open = '<div class="' . (join ' ', ('diff', $diff_class)) . '">';
 	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}(.*)$/;
@@ -2274,7 +2276,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_open$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);
@@ -2307,9 +2309,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_open$line</div>\n";
 	}
-	return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1)
. "</div>\n";
+	return $div_open . esc_html($line, -nbsp=>1) . "</div>\n";
 }

 # Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
-- 
1.7.7.213.g8b0e1.dirty

Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class

[Junio C Hamano]

Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:

> The format_diff_line() will return $diff_class and HTML in upcoming changes.

An auxiliary piece of information like this is fine at the end of the
commit log message, but the patch itself wants to be justified
standalone.  Perhaps this should be sufficient:

	The $diff_class variable to classify the kind of line in the diff
	output was prefixed with a SP, only so that the code to synthesize
	value for "class" attribute can blindly concatenate it with
	another value "diff". This made the code unnecessarily ugly.

	Instead, add SP that separates the value of $diff_class from
	another class value "diff" where <div class="..."> string is
	created and drop the leading SP from the value of $diff_class.
	
Explained this way, it does not even have to mention that the return value
will be changed in a different patch.

We all know that the hidden motivation of this change is that the caller
of this function, after it starts using the returned value of $diff_class,
does not want to worry about the ugly SP prefix in that variable. Saying
only "This function will return this variable in the future" still does
not fully explain that hidden motivation unless you say "and the caller
shouldn't have to worry about the leading SP", so let's not even mention
it in the log message of this change.

> ---

Sign-off?

>  gitweb/gitweb.perl |   24 +++++++++++++-----------
>  1 files changed, 13 insertions(+), 11 deletions(-)
>
> 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 {
> ...
> +
> +	my $div_open = '<div class="' . (join ' ', ('diff', $diff_class)) . '">';

I think using a separate helper variable like this is a good change.  You
do not have to worry about the issue in three different places.

But doesn't join(" ", ("frotz", "")) still give you "frotz "?  It is OK to
punt and say

	my $div_open = '<div class="diff $diff_class">';

which would be far easier to read. It may sacrifice a bit of tidiness in
the resulting HTML but the tidiness of the source outweighs it.

Of course, if you have tons of classes, it may be worth doing something
like

	join(" ", grep { defined $_ && $_ ne ""}  @diff_classes);

but I do not think it is worth it in this particular case.

Thanks.

Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class

[Jakub Narebski]

By the way, it is common to either have following patches to be chain
reply to first patch, or better provide cover letter for patch series
and have all patches be reply to cover letter.


Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:

This commit needs some more explanation in the commit message, like
Junio said.

> The format_diff_line() will return $diff_class and HTML in upcoming changes.

This is not necessary, I think; this change stands alone as a style
(semantic) cleanup.

Signoff?  See Documentation/SubmittingPatches

> ---
>  gitweb/gitweb.perl |   24 +++++++++++++-----------
>  1 files changed, 13 insertions(+), 11 deletions(-)
> 
> 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.

>  	} else {
>  		# assume ordinary diff
>  		my $char = substr($line, 0, 1);
>  		if ($char eq '+') {
> -			$diff_class = " add";
> +			$diff_class = "add";
>  		} elsif ($char eq '-') {
> -			$diff_class = " rem";
> +			$diff_class = "rem";
>  		} elsif ($char eq '@') {
> -			$diff_class = " chunk_header";
> +			$diff_class = "chunk_header";
>  		} elsif ($char eq "\\") {
> -			$diff_class = " incomplete";
> +			$diff_class = "incomplete";
>  		}

This is also ugly, but this one could in theory be replaced by hash
lookup.  But this would remove symmetry...

>  	}
>  	$line = untabify($line);
> +
> +	my $div_open = '<div class="' . (join ' ', ('diff', $diff_class)) . '">';

Another solution would be to use

  +
  +	my $diff_classes = "diff";
  +	$diff_classes .= " $diff_class" if ($diff_class);
  +

>  	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}(.*)$/;
> @@ -2274,7 +2276,7 @@ sub format_diff_line {
>  		}
>  		$line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
>  		        "<span class=\"section\">" . esc_html($section, -nbsp=>1) .
> "</span>";

The above is word-wrapped.  Please turn off word wrapping in the
future to avoid such damage to the patch.

> -		return "<div class=\"diff$diff_class\">$line</div>\n";
> +		return "$div_open$line</div>\n";

This would be instead

  +		return "<div class=\"$diff_classes\">$line</div>\n";

which is a bit more symmetric.

But that is just a matter of taste.

>  	} 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);
> @@ -2307,9 +2309,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_open$line</div>\n";
>  	}
> -	return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1)
> . "</div>\n";
> +	return $div_open . esc_html($line, -nbsp=>1) . "</div>\n";
>  }
> 
>  # Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
> -- 
> 1.7.7.213.g8b0e1.dirty

-- 
Jakub Narębski

Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> By the way, it is common to either have following patches to be chain
> reply to first patch, or better provide cover letter for patch series
> and have all patches be reply to cover letter.

It may be a good discipline for 14 patch series, but it doesn't matter for
something this small.

>>  gitweb/gitweb.perl |   24 +++++++++++++-----------
>>  1 files changed, 13 insertions(+), 11 deletions(-)
>> 
>> 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?

Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class

[Jakub Narebski]

On Mon, 17 Oct 2011, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> By the way, it is common to either have following patches to be chain
>> reply to first patch, or better provide cover letter for patch series
>> and have all patches be reply to cover letter.
> 
> It may be a good discipline for 14 patch series, but it doesn't matter for
> something this small.

Well, cover letter for 2-patch series might be overkill, but having
patches in series connected e.g. chain-replied-to, or all replies to
first patch in series, is IMHO a good idea.

>>>  gitweb/gitweb.perl |   24 +++++++++++++-----------
>>>  1 files changed, 13 insertions(+), 11 deletions(-)
>>> 
>>> 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

Wouldn't that cover not only combined diff, but an "ordinary" diff as
well then (i.e. comment should change)?  I think that was the intent,
isn't it?

>       my $num_sign = @{$from->{'href'}} + 1;

$from->{'href'} is array reference only for combined diff (>= 2 parents).
For ordinary diff it is a scalar.

> 	my @cc_classifier = (
> 		  ["\@{$num_sign} ", "chunk_header"],

O.K.

Nb., it is "chunk_header", or "hunk_header"?

>                 ['\\', "incomplete"],

O.K.

>                 [" {$num_sign}", ""],

O.K.

>                 ["[+ ]{$num_sign}", "add"],
>                 ["[- ]{$num_sign}", "rem"],

It would be slightly different to what current code does.

Current code for combined diff uses "add" if there is at least one '+',
"rem" if there are no '+' and at least one '-', and context otherwise.


I wonder if with sufficiently evil merge we can have a line that is
added (changed) in some children, and removed in other, i.e. pluses
and minuses combined.

Nb. we can put regexp here, not only stringification of regexp.
i.e.

                  [qr/[+ ]{$num_sign}/, "add"],
                  [qr/[- ]{$num_sign}/, "rem"],


>         );
>         for my $cls (@cc_classifier) {
>                 if ($line =~ /^$cls->[0]$/) {
>                 	$diff_class = $cls->[1];
>                         last;
> 		}
> 	}

Nice trick.
 
> 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?

Right.  We use "diff" class without anything else for context, but probably
it would be better to state this explicitly.

-- 
Jakub Narebski
Poland

Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> O.K.
>
>>                 [" {$num_sign}", ""],
>
> O.K.
>
>>                 ["[+ ]{$num_sign}", "add"],
>>                 ["[- ]{$num_sign}", "rem"],
>
> It would be slightly different to what current code does.

> Current code for combined diff uses "add" if there is at least one '+',
> "rem" if there are no '+' and at least one '-', and context otherwise.

The only possible difference would be for a line with all blank, but
because there is an additional explicit rule for context, the behaviour is
the same.  In a combined diff, you will never see + and - together on the
same line.

> I wonder if with sufficiently evil merge we can have a line that is
> added (changed) in some children, and removed in other, i.e. pluses
> and minuses combined.

The logic in combine-diff.c::dump_sline() was written in such a way to
avoid such a confusing output.

> Nb. we can put regexp here, not only stringification of regexp.
> i.e.
>
>                   [qr/[+ ]{$num_sign}/, "add"],
>                   [qr/[- ]{$num_sign}/, "rem"],

That would be a good change.

>> 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?
>
> Right.  We use "diff" class without anything else for context, but probably
> it would be better to state this explicitly.

Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class

[Kato Kazuyoshi]

On Tue, Oct 18, 2011 at 4:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
>
>> The format_diff_line() will return $diff_class and HTML in upcoming changes.
>
> An auxiliary piece of information like this is fine at the end of the
> commit log message, but the patch itself wants to be justified
> standalone.  Perhaps this should be sufficient:
>
>        The $diff_class variable to classify the kind of line in the diff
>        output was prefixed with a SP, only so that the code to synthesize
>        value for "class" attribute can blindly concatenate it with
>        another value "diff". This made the code unnecessarily ugly.
>
>        Instead, add SP that separates the value of $diff_class from
>        another class value "diff" where <div class="..."> string is
>        created and drop the leading SP from the value of $diff_class.
>
> Explained this way, it does not even have to mention that the return value
> will be changed in a different patch.
>

Thanks. I couldn't write a good summary for my patch because it was just
an "adjust" for me. However your summary is really clear!

>>  gitweb/gitweb.perl |   24 +++++++++++++-----------
>>  1 files changed, 13 insertions(+), 11 deletions(-)
>>
>> 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 {
>> ...
>> +
>> +     my $div_open = '<div class="' . (join ' ', ('diff', $diff_class)) . '">';
>
> I think using a separate helper variable like this is a good change.  You
> do not have to worry about the issue in three different places.
>
> But doesn't join(" ", ("frotz", "")) still give you "frotz "?  It is OK to
> punt and say
>
>        my $div_open = '<div class="diff $diff_class">';
>
> which would be far easier to read. It may sacrifice a bit of tidiness in
> the resulting HTML but the tidiness of the source outweighs it.
>
> Of course, if you have tons of classes, it may be worth doing something
> like
>
>        join(" ", grep { defined $_ && $_ ne ""}  @diff_classes);
>
> but I do not think it is worth it in this particular case.
>

Yeah, I want to remove unnecessary SP that you mentioned before.
But well, join(" ", ("frotz", "")) give me "frotz ".
I will add some per-function test cases to gitweb before this patch series.

Thanks,

-- 
Kato Kazuyoshi

[PATCH] gitweb: Refactor diff body line classification

[Jakub Narebski]

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