Re: [PATCH v2 8/8] gitweb: Refinement highlightning in combined diffs

Jakub Narebski [off-list ref] wrote:

On Fri, 23 Mar 2012, Michał Kiedrowicz wrote:

quoted

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

Is -> was?

quoted

Moreover, code that compares added and removed lines doesn't care
about combined diffs. It only skips first +/- character, treating
second +/- as a line content.

Well, we explicitly skip combined diffs.  I think what you want to say
here is that it is not possible to simply use existing algorithm
unchanged for combined diffs.

Yep, your wording is much better.

quoted

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

O.K., that's a nice and sensible first step.

I wonder if it would be worth to specify that we currently require
that pattern of '-'-es in pre-image match pattern of '+'-es in
postimage.

Nb. the prefix of combined diff would either include '+', or '-',
but never mixed (this is documented, but I had trouble with this).

quoted

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

Very sensible approach.

quoted

Signed-off-by: Michał Kiedrowicz <redacted>

  Acked-by: Jakub Narębski [off-list ref]

quoted

---

BTW. I went and checked if this approach helps for non-trivial merges
in git.git history:

* b10656c - helps a bit, though one can see limitation of pre/post-fix
  matching here, but it is present also for non-combined diff.

* 8b132bc - helps a bit, though char-interdiff or word-interdiff might
  be better.  Nb. I think that red background for 'marked' is a bit
  too dark (intensive).

* c58499c - doesn't help too much.

* f629c23, aa145bf - helps.

Thanks for testing.  I'll experiment with less intensive backgrounds.

quoted

 gitweb/gitweb.perl |   57
+++++++++++++++++++++++++++++++++++++++------------ 1 files
changed, 43 insertions(+), 14 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 872ba12..c056e83 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -5057,12 +5057,12 @@ sub print_inline_diff_lines {
 # Format removed and added line, mark changed part and HTML-format

them. # Impementation is based on contrib/diff-highlight

Implementation
   ^---

quoted

 sub format_rem_add_line {
-	my ($rem, $add) = @_;
+	my ($rem, $add, $num_parents) = @_;
 	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);

Nice.

[...]

quoted

@@ -5099,15 +5099,43 @@ sub format_rem_add_line {
 
 # 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 $can_highlight = 0;
+	my $is_combined = ($num_parents > 1);
 
 	# Highlight if every removed line has a corresponding

added line.
-	# Combined diffs are not supported ATM.
-	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;
+				}
+			}
+		}
+	}

Good.

quoted

+
+	if ($can_highlight) {
 		for (my $i = 0; $i < @$add; $i++) {
 			my ($line_rem, $line_add) =
format_rem_add_line(
-			        $rem->[$i], $add->[$i]);
+			        $rem->[$i], $add->[$i],
$num_parents); push @new_rem, $line_rem;
 			push @new_add, $line_add;
 		}

[...]

quoted

@@ -5326,7 +5355,7 @@ sub git_patchset_body {
 
 	} 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"

I was wondering about 'commitdiff' between two commits, which is not
combined even ifany of those commits is a merge commit... but it looks
like it works all right.