gitweb: Refinement highlightning in combined diffs

The highlightning of combined diffs is currently disabled.  This is
because output from a combined diff is much harder to highlight because
it is not obvious which removed and added lines should be compared.

Current code requires that the number of added lines is equal to the
number of removed lines and only skips first +/- character, treating
second +/- as a line content, Thus, it is not possible to simply use
existing algorithm unchanged for combined diffs.

Let's start with a simple case: only highlight changes that come from
one parent, i.e. when every removed line has a corresponding added line
for the same parent.  This way the highlightning cannot get wrong. For
example, following diffs would be highlighted:

- removed line for first parent
+ added line for first parent
context line
-removed line for second parent
+added line for second parent

or

- removed line for first parent
-removed line for second parent
+ added line for first parent
+added line for second parent

but following output will not:

- removed line for first parent
-removed line for second parent
+added line for second parent
++added line for both parents

In other words, we require that pattern of '-'-es in pre-image matches
pattern of '+'-es in post-image.

Further changes may introduce more intelligent approach that better
handles combined diffs.

Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
Acked-by: Jakub Narębski <jnareb@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index d5b3f04e118d1b86bf009a163bc9e7ade060b017..f4feaccfdae2972d3564985ed8af829b8388eed4 100755 (executable)
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5064,7 +5064,7 @@ sub print_inline_diff_lines {
 # Format removed and added line, mark changed part and HTML-format them.
 # Implementation is based on contrib/diff-highlight
 sub format_rem_add_lines_pair {
 # Format removed and added line, mark changed part and HTML-format them.
 # Implementation is based on contrib/diff-highlight
 sub format_rem_add_lines_pair {

-       my ($rem, $add) = @_;
+       my ($rem, $add, $num_parents) = @_;

 
        # We need to untabify lines before split()'ing them;
        # otherwise offsets would be invalid.
 
        # We need to untabify lines before split()'ing them;
        # otherwise offsets would be invalid.


@@ -5076,8 +5076,8 @@ sub format_rem_add_lines_pair {
        my @rem = split(//, $rem);
        my @add = split(//, $add);
        my ($esc_rem, $esc_add);
        my @rem = split(//, $rem);
        my @add = split(//, $add);
        my ($esc_rem, $esc_add);

-       # Ignore +/- character, thus $prefix_len is set to 1.
-       my ($prefix_len, $suffix_len) = (1, 0);
+       # Ignore leading +/- characters for each parent.
+       my ($prefix_len, $suffix_len) = ($num_parents, 0);

        my ($prefix_has_nonspace, $suffix_has_nonspace);
 
        my $shorter = (@rem < @add) ? @rem : @add;
        my ($prefix_has_nonspace, $suffix_has_nonspace);
 
        my $shorter = (@rem < @add) ? @rem : @add;


@@ -5115,15 +5115,43 @@ sub format_rem_add_lines_pair {
 
 # HTML-format diff context, removed and added lines.
 sub format_ctx_rem_add_lines {
 
 # HTML-format diff context, removed and added lines.
 sub format_ctx_rem_add_lines {

-       my ($ctx, $rem, $add, $is_combined) = @_;
+       my ($ctx, $rem, $add, $num_parents) = @_;

        my (@new_ctx, @new_rem, @new_add);
        my (@new_ctx, @new_rem, @new_add);

+       my $can_highlight = 0;
+       my $is_combined = ($num_parents > 1);

 
        # Highlight if every removed line has a corresponding added line.
 
        # Highlight if every removed line has a corresponding added line.

-       # Combined diffs are not supported at this moment.
-       if (!$is_combined && @$add > 0 && @$add == @$rem) {
+       if (@$add > 0 && @$add == @$rem) {
+               $can_highlight = 1;
+
+               # Highlight lines in combined diff only if the chunk contains
+               # diff between the same version, e.g.
+               #
+               #    - a
+               #   -  b
+               #    + c
+               #   +  d
+               #
+               # Otherwise the highlightling would be confusing.
+               if ($is_combined) {
+                       for (my $i = 0; $i < @$add; $i++) {
+                               my $prefix_rem = substr($rem->[$i], 0, $num_parents);
+                               my $prefix_add = substr($add->[$i], 0, $num_parents);
+
+                               $prefix_rem =~ s/-/+/g;
+
+                               if ($prefix_rem ne $prefix_add) {
+                                       $can_highlight = 0;
+                                       last;
+                               }
+                       }
+               }
+       }
+
+       if ($can_highlight) {

                for (my $i = 0; $i < @$add; $i++) {
                        my ($line_rem, $line_add) = format_rem_add_lines_pair(
                for (my $i = 0; $i < @$add; $i++) {
                        my ($line_rem, $line_add) = format_rem_add_lines_pair(

-                               $rem->[$i], $add->[$i]);
+                               $rem->[$i], $add->[$i], $num_parents);

                        push @new_rem, $line_rem;
                        push @new_add, $line_add;
                }
                        push @new_rem, $line_rem;
                        push @new_add, $line_add;
                }


@@ -5139,10 +5167,11 @@ sub format_ctx_rem_add_lines {
 
 # Print context lines and then rem/add lines.
 sub print_diff_lines {
 
 # Print context lines and then rem/add lines.
 sub print_diff_lines {

-       my ($ctx, $rem, $add, $diff_style, $is_combined) = @_;
+       my ($ctx, $rem, $add, $diff_style, $num_parents) = @_;
+       my $is_combined = $num_parents > 1;

 
        ($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem, $add,
 
        ($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem, $add,

-               $is_combined);
+               $num_parents);

 
        if ($diff_style eq 'sidebyside' && !$is_combined) {
                print_sidebyside_diff_lines($ctx, $rem, $add);
 
        if ($diff_style eq 'sidebyside' && !$is_combined) {
                print_sidebyside_diff_lines($ctx, $rem, $add);


@@ -5153,7 +5182,7 @@ sub print_diff_lines {
 }
 
 sub print_diff_chunk {
 }
 
 sub print_diff_chunk {

-       my ($diff_style, $is_combined, $from, $to, @chunk) = @_;
+       my ($diff_style, $num_parents, $from, $to, @chunk) = @_;

        my (@ctx, @rem, @add);
 
        # The class of the previous line.
        my (@ctx, @rem, @add);
 
        # The class of the previous line.


@@ -5188,7 +5217,7 @@ sub print_diff_chunk {
                if (!$class || ((@rem || @add) && $class eq 'ctx') ||
                    (@rem && @add && $class ne $prev_class)) {
                        print_diff_lines(\@ctx, \@rem, \@add,
                if (!$class || ((@rem || @add) && $class eq 'ctx') ||
                    (@rem && @add && $class ne $prev_class)) {
                        print_diff_lines(\@ctx, \@rem, \@add,

-                                        $diff_style, $is_combined);
+                                        $diff_style, $num_parents);

                        @ctx = @rem = @add = ();
                }
 
                        @ctx = @rem = @add = ();
                }
 


@@ -5331,7 +5360,7 @@ sub git_patchset_body {
                        my $class = diff_line_class($patch_line, \%from, \%to);
 
                        if ($class eq 'chunk_header') {
                        my $class = diff_line_class($patch_line, \%from, \%to);
 
                        if ($class eq 'chunk_header') {

-                               print_diff_chunk($diff_style, $is_combined, \%from, \%to, @chunk);
+                               print_diff_chunk($diff_style, scalar @hash_parents, \%from, \%to, @chunk);

                                @chunk = ();
                        }
 
                                @chunk = ();
                        }
 


@@ -5340,7 +5369,7 @@ sub git_patchset_body {
 
        } continue {
                if (@chunk) {
 
        } continue {
                if (@chunk) {

-                       print_diff_chunk($diff_style, $is_combined, \%from, \%to, @chunk);
+                       print_diff_chunk($diff_style, scalar @hash_parents, \%from, \%to, @chunk);

                        @chunk = ();
                }
                print "</div>\n"; # class="patch"
                        @chunk = ();
                }
                print "</div>\n"; # class="patch"