Re: [PATCH 8/8] gitweb: Highlight combined diffs

Jakub Narebski [off-list ref] wrote:

Michał Kiedrowicz [off-list ref] writes:

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.

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.

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

That is a very reasonable approach.

Thanks.

quoted

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

Signed-off-by: Michał Kiedrowicz <redacted>
---
 gitweb/gitweb.perl |   40 +++++++++++++++++++++++++++++++++++++---
 1 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1a5b454..2b6cb9e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -4944,13 +4944,16 @@ sub esc_html_mark_range {
 
 # Format removed and added line, mark changed part and HTML-format

them. sub format_rem_add_line {
-	my ($rem, $add) = @_;
+	my ($rem, $add, $is_combined) = @_;
 	my @r = split(//, $rem);
 	my @a = split(//, $add);
 	my ($esc_rem, $esc_add);
 	my ($prefix, $suffix_rem, $suffix_add) = (1, $#r, $#a);
 	my ($prefix_is_space, $suffix_is_space) = (1, 1);
 
+	# In combined diff we must ignore two +/- characters.
+	$prefix = 2 if ($is_combined);
+

Errr... actually number of prefix is equalt to number of parents, so
it might be in case of octopus merge more than 2.

OK

quoted

 	while ($prefix < @r && $prefix < @a) {
 		last if ($r[$prefix] ne $a[$prefix]);
 

@@ -4988,11 +4991,42 @@ sub format_ctx_rem_add_lines {
 	my ($ctx, $rem, $add, $is_combined) = @_;
 	my (@new_ctx, @new_rem, @new_add);
 	my $num_add_lines = @$add;
+	my $can_highlight;
+
+	# Highlight if every removed line has a corresponding

added line.
+	if ($num_add_lines > 0 && $num_add_lines == @$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 < $num_add_lines; $i++)
{
+				my $prefix_rem =
substr($rem->[$i], 0, 2);
+				my $prefix_add =
substr($add->[$i], 0, 2); +
+				$prefix_rem =~ s/-/+/g;
+
+				if ($prefix_rem ne $prefix_add) {
+					$can_highlight = 0;
+					last;

Nb. this assumes that we cannot refine and highlight something like
this:

   		#    - a
   		#   -  b
   		#    + c
   		#   ++ d

But perhaps this is better left for future improvemnt.

I can add a patch for that at the end of this series.

quoted

+				}
+			}
+		}
+	} else {
+		$can_highlight = 0;
+	}

This 'else' would be not necessary if $can_highlight was initialized
to 0.

OK.

quoted

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

O.K.

Thanks for your thorough review.