[PATCH v3 0/8] Highlight interesting parts of diff

[PATCH v3 0/8] Highlight interesting parts of diff

[Michał Kiedrowicz]

Reading diff output is sometimes very hard, even if it's colored, especially if
lines differ only in few characters. This is often true when a commit fixes a
typo or renames some variables or functions.

This patch series teaches gitweb to highlight fragments that are different
between old and new line. This should mimic the same feature in Trac or GitHub.

Changes since v2:

* Added first patch (gitweb: Use descriptive names in esc_html_hl_regions())
  suggested by Jakub.

* Squashed patches (gitweb: Move HTML-formatting diff line back to
  process_diff_line()) and (gitweb: Push formatting diff lines to
  print_diff_chunk())

* Reauthored patch (gitweb: Pass esc_html_hl_regions() options to esc_html())
  to Jakub.

* Fixed typos in commit messages and added comments based on Jakub's review.

* Added Acked-by from Jakub.

* Fixed minor issues in code pointed by Jakub (hopefully all; I had to drop
  few due to overlong lines etc).

* Renamed format_rem_add_line() to format_rem_add_lines_pair().

* Renamed $prefix_is_space and $suffix_is_space to $prefix_has_nonspace and
  $suffix_has_nonspace and simplified a condition

* Moved untabify() and chomp() back to format_diff_line() and introduced in
  format_rem_add_lines_pair().

* Made red and green background a bit brighter.

Jakub Narębski (1):
  gitweb: Pass esc_html_hl_regions() options to esc_html()

Michał Kiedrowicz (7):
  gitweb: Use descriptive names in esc_html_hl_regions()
  gitweb: esc_html_hl_regions(): Don't create empty <span> elements
  gitweb: Extract print_sidebyside_diff_lines()
  gitweb: Use print_diff_chunk() for both side-by-side and inline diffs
  gitweb: Push formatting diff lines to print_diff_chunk()
  gitweb: Highlight interesting parts of diff
  gitweb: Refinement highlightning in combined diffs

 gitweb/gitweb.perl       |  319 +++++++++++++++++++++++++++++++++-------------
 gitweb/static/gitweb.css |    8 +
 2 files changed, 241 insertions(+), 86 deletions(-)

-- 
1.7.8.4

[PATCH v3 1/8] gitweb: Use descriptive names in esc_html_hl_regions()

[Michał Kiedrowicz]

The $s->[0] and $s->[1] variables look a bit cryptic.  Let's rename them
to $beg and $end so that it's clear what they do.

Suggested-by: Jakub Narębski <jnareb@gmail.com>
Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
---
 gitweb/gitweb.perl |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a8b5fad..a3754ff 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1738,12 +1738,14 @@ sub esc_html_hl_regions {
 	my $pos = 0;
 
 	for my $s (@sel) {
-		$out .= esc_html(substr($str, $pos, $s->[0] - $pos))
-			if ($s->[0] - $pos > 0);
+		my ($beg, $end) = @$s;
+
+		$out .= esc_html(substr($str, $pos, $beg - $pos))
+			if ($beg - $pos > 0);
 		$out .= $cgi->span({-class => $css_class},
-		                   esc_html(substr($str, $s->[0], $s->[1] - $s->[0])));
+		                   esc_html(substr($str, $beg, $end - $beg)));
 
-		$pos = $s->[1];
+		$pos = $end;
 	}
 	$out .= esc_html(substr($str, $pos))
 		if ($pos < length($str));
-- 
1.7.8.4

[PATCH v3 2/8] gitweb: esc_html_hl_regions(): Don't create empty <span> elements

[Michał Kiedrowicz]

If $end is equal to or less than $beg, esc_html_hl_regions()
generates an empty <span> element.  It normally shouldn't be visible in
the web browser, but it doesn't look good when looking at page source.
It also minimally increases generated page size for no special reason.

Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
Acked-by: Jakub Narębski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a3754ff..ca3058c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1740,6 +1740,9 @@ sub esc_html_hl_regions {
 	for my $s (@sel) {
 		my ($beg, $end) = @$s;
 
+		# Don't create empty <span> elements.
+		next if $end <= $beg; 
+
 		$out .= esc_html(substr($str, $pos, $beg - $pos))
 			if ($beg - $pos > 0);
 		$out .= $cgi->span({-class => $css_class},
-- 
1.7.8.4

[PATCH v3 3/8] gitweb: Pass esc_html_hl_regions() options to esc_html()

[Michał Kiedrowicz]

From: Jakub Narębski <jnareb@gmail.com>

With this change, esc_html_hl_regions() accepts options and passes them
down to esc_html().  This may be needed if a caller wants to pass
-nbsp=>1 to esc_html().

The idea and implementation example of this change was described in
337da8d2 (gitweb: Introduce esc_html_match_hl and esc_html_hl_regions,
2012-02-27).  While other suggestions may be more useful in some cases,
there is no need to implement them at the moment.  The
esc_html_hl_regions() interface may be changed later if it's needed.

[mk: extracted from larger patch and wrote commit message]

Signed-off-by: Jakub Narębski <jnareb@gmail.com>
Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
---
 gitweb/gitweb.perl |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index ca3058c..d5f802f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1732,7 +1732,9 @@ sub chop_and_escape_str {
 # '<span class="mark">foo</span>bar'
 sub esc_html_hl_regions {
 	my ($str, $css_class, @sel) = @_;
-	return esc_html($str) unless @sel;
+	my %opts = grep { ref($_) ne 'ARRAY' } @sel;
+	@sel     = grep { ref($_) eq 'ARRAY' } @sel;
+	return esc_html($str, %opts) unless @sel;
 
 	my $out = '';
 	my $pos = 0;
@@ -1743,14 +1745,14 @@ sub esc_html_hl_regions {
 		# Don't create empty <span> elements.
 		next if $end <= $beg; 
 
-		$out .= esc_html(substr($str, $pos, $beg - $pos))
+		$out .= esc_html(substr($str, $pos, $beg - $pos), %opts)
 			if ($beg - $pos > 0);
 		$out .= $cgi->span({-class => $css_class},
-		                   esc_html(substr($str, $beg, $end - $beg)));
+		                   esc_html(substr($str, $beg, $end - $beg), %opts));
 
 		$pos = $end;
 	}
-	$out .= esc_html(substr($str, $pos))
+	$out .= esc_html(substr($str, $pos), %opts)
 		if ($pos < length($str));
 
 	return $out;
-- 
1.7.8.4

[PATCH v3 4/8] gitweb: Extract print_sidebyside_diff_lines()

[Michał Kiedrowicz]

Currently, print_sidebyside_diff_chunk() does two things: it
accumulates diff lines and prints them.  Accumulation may be used to
perform additional operations on diff lines,  so it makes sense to split
these two things.  Thus, the code that prints diff lines in a side-by-side
manner is moved out of print_sidebyside_diff_chunk() to a separate
subroutine.

The outcome of this patch is that print_sidebyside_diff_chunk() is now
much shorter and easier to read.

This is a preparation patch for diff refinement highlightning.  It should
not change the gitweb output, but it slightly changes its behavior.
Before this commit, context is printed on the class change. Now,  it's
printed just before printing added and removed lines, and at the end of
chunk.

Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
Acked-by: Jakub Narębski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   97 ++++++++++++++++++++++++++++------------------------
 1 files changed, 52 insertions(+), 45 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index d5f802f..56721a3 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5000,6 +5000,53 @@ sub git_difftree_body {
 	print "</table>\n";
 }
 
+# Print context lines and then rem/add lines in a side-by-side manner.
+sub print_sidebyside_diff_lines {
+	my ($ctx, $rem, $add) = @_;
+
+	# print context block before add/rem block
+	if (@$ctx) {
+		print join '',
+			'<div class="chunk_block ctx">',
+				'<div class="old">',
+				@$ctx,
+				'</div>',
+				'<div class="new">',
+				@$ctx,
+				'</div>',
+			'</div>';
+	}
+
+	if (!@$add) {
+		# pure removal
+		print join '',
+			'<div class="chunk_block rem">',
+				'<div class="old">',
+				@$rem,
+				'</div>',
+			'</div>';
+	} elsif (!@$rem) {
+		# pure addition
+		print join '',
+			'<div class="chunk_block add">',
+				'<div class="new">',
+				@$add,
+				'</div>',
+			'</div>';
+	} else {
+		# assume that it is change
+		print join '',
+			'<div class="chunk_block chg">',
+				'<div class="old">',
+				@$rem,
+				'</div>',
+				'<div class="new">',
+				@$add,
+				'</div>',
+			'</div>';
+	}
+}
+
 sub print_sidebyside_diff_chunk {
 	my @chunk = @_;
 	my (@ctx, @rem, @add);
@@ -5026,51 +5073,11 @@ sub print_sidebyside_diff_chunk {
 			next;
 		}
 
-		## print from accumulator when type of class of lines change
-		# empty contents block on start rem/add block, or end of chunk
-		if (@ctx && (!$class || $class eq 'rem' || $class eq 'add')) {
-			print join '',
-				'<div class="chunk_block ctx">',
-					'<div class="old">',
-					@ctx,
-					'</div>',
-					'<div class="new">',
-					@ctx,
-					'</div>',
-				'</div>';
-			@ctx = ();
-		}
-		# empty add/rem block on start context block, or end of chunk
-		if ((@rem || @add) && (!$class || $class eq 'ctx')) {
-			if (!@add) {
-				# pure removal
-				print join '',
-					'<div class="chunk_block rem">',
-						'<div class="old">',
-						@rem,
-						'</div>',
-					'</div>';
-			} elsif (!@rem) {
-				# pure addition
-				print join '',
-					'<div class="chunk_block add">',
-						'<div class="new">',
-						@add,
-						'</div>',
-					'</div>';
-			} else {
-				# assume that it is change
-				print join '',
-					'<div class="chunk_block chg">',
-						'<div class="old">',
-						@rem,
-						'</div>',
-						'<div class="new">',
-						@add,
-						'</div>',
-					'</div>';
-			}
-			@rem = @add = ();
+		## print from accumulator when have some add/rem lines or end
+		# of chunk (flush context lines)
+		if (((@rem || @add) && $class eq 'ctx') || !$class) {
+			print_sidebyside_diff_lines(\@ctx, \@rem, \@add);
+			@ctx = @rem = @add = ();
 		}
 
 		## adding lines to accumulator
-- 
1.7.8.4

[PATCH v3 5/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs

[Michał Kiedrowicz]

This renames print_sidebyside_diff_chunk() to print_diff_chunk() and
makes use of it for both side-by-side and inline diffs.  Now diff lines
are always accumulated before they are printed.  This opens the
possibility to preprocess diff output before it's printed, which is
needed for diff refinement highlightning (implemented in incoming
patches).

If print_diff_chunk() was left as is, the new function
print_inline_diff_lines() could reorder diff lines.  It first prints all
context lines, then all removed lines and finally all added lines.  If
the diff output consisted of mixed added and removed lines, gitweb would
reorder these lines.  This is true for combined diff output, for
example:

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

would be rendered as:

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

To prevent gitweb from reordering lines, print_diff_chunk() calls
print_diff_lines() as soon as it detects that both added and removed
lines are present and there was a class change.

Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
---
 gitweb/gitweb.perl |   55 ++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 56721a3..4f6b043 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5047,10 +5047,32 @@ sub print_sidebyside_diff_lines {
 	}
 }
 
-sub print_sidebyside_diff_chunk {
-	my @chunk = @_;
+# Print context lines and then rem/add lines in inline manner.
+sub print_inline_diff_lines {
+	my ($ctx, $rem, $add) = @_;
+
+	print @$ctx, @$rem, @$add;
+}
+
+# Print context lines and then rem/add lines.
+sub print_diff_lines {
+	my ($ctx, $rem, $add, $diff_style, $is_combined) = @_;
+
+	if ($diff_style eq 'sidebyside' && !$is_combined) {
+		print_sidebyside_diff_lines($ctx, $rem, $add);
+	} else {
+		# default 'inline' style and unknown styles
+		print_inline_diff_lines($ctx, $rem, $add);
+	}
+}
+
+sub print_diff_chunk {
+	my ($diff_style, $is_combined, @chunk) = @_;
 	my (@ctx, @rem, @add);
 
+	# The class of the previous line. 
+	my $prev_class = '';
+
 	return unless @chunk;
 
 	# incomplete last line might be among removed or added lines,
@@ -5074,9 +5096,13 @@ sub print_sidebyside_diff_chunk {
 		}
 
 		## print from accumulator when have some add/rem lines or end
-		# of chunk (flush context lines)
-		if (((@rem || @add) && $class eq 'ctx') || !$class) {
-			print_sidebyside_diff_lines(\@ctx, \@rem, \@add);
+		# of chunk (flush context lines), or when have add and rem
+		# lines and new block is reached (otherwise add/rem lines could
+		# be reordered)
+		if (((@rem || @add) && $class eq 'ctx') || !$class ||
+		    (@rem && @add && $class ne $prev_class)) {
+			print_diff_lines(\@ctx, \@rem, \@add,
+		                         $diff_style, $is_combined);
 			@ctx = @rem = @add = ();
 		}
 
@@ -5093,6 +5119,8 @@ sub print_sidebyside_diff_chunk {
 		if ($class eq 'ctx') {
 			push @ctx, $line;
 		}
+
+		$prev_class = $class;
 	}
 }
 
@@ -5219,22 +5247,17 @@ sub git_patchset_body {
 			$diff_classes .= " $class" if ($class);
 			$line = "<div class=\"$diff_classes\">$line</div>\n";
 
-			if ($diff_style eq 'sidebyside' && !$is_combined) {
-				if ($class eq 'chunk_header') {
-					print_sidebyside_diff_chunk(@chunk);
-					@chunk = ( [ $class, $line ] );
-				} else {
-					push @chunk, [ $class, $line ];
-				}
-			} else {
-				# default 'inline' style and unknown styles
-				print $line;
+			if ($class eq 'chunk_header') {
+				print_diff_chunk($diff_style, $is_combined, @chunk);
+				@chunk = ();
 			}
+
+			push @chunk, [ $class, $line ];
 		}
 
 	} continue {
 		if (@chunk) {
-			print_sidebyside_diff_chunk(@chunk);
+			print_diff_chunk($diff_style, $is_combined, @chunk);
 			@chunk = ();
 		}
 		print "</div>\n"; # class="patch"
-- 
1.7.8.4

[PATCH v3 6/8] gitweb: Push formatting diff lines to print_diff_chunk()

[Michał Kiedrowicz]

Now lines are formatted closer to place where we actually use HTML
formatted output.

This means that we put raw lines in the @chunk accumulator, rather than
formatted lines.  Because we still need to know class (type) of line
when accumulating data to post-process and print, process_diff_line()
subroutine was retired and replaced by diff_line_class() used in
git_patchset_body() and new restructured format_diff_line() used in
print_diff_chunk().

As a side effect, we have to pass \%from and \%to down to callstack.

This is a preparation patch for diff refinement highlightning. It's not
meant to change gitweb output.

[jn: wrote commit message]

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

Jakub, I took your comment as-is as the message for this patch.  It seems to
describe the change much better than my description.

 gitweb/gitweb.perl |   37 ++++++++++++++++++-------------------
 1 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4f6b043..926e83c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2428,26 +2428,26 @@ sub format_cc_diff_chunk_header {
 }
 
 # process patch (diff) line (not to be used for diff headers),
-# returning class and HTML-formatted (but not wrapped) line
-sub process_diff_line {
-	my $line = shift;
-	my ($from, $to) = @_;
-
-	my $diff_class = diff_line_class($line, $from, $to);
+# returning HTML-formatted (but not wrapped) line
+sub format_diff_line {
+	my ($line, $diff_class, $from, $to) = @_;
 
 	chomp $line;
 	$line = untabify($line);
 
 	if ($from && $to && $line =~ m/^\@{2} /) {
 		$line = format_unidiff_chunk_header($line, $from, $to);
-		return $diff_class, $line;
-
 	} elsif ($from && $to && $line =~ m/^\@{3}/) {
 		$line = format_cc_diff_chunk_header($line, $from, $to);
-		return $diff_class, $line;
-
+	} else {
+		$line = esc_html($line, -nbsp=>1);
 	}
-	return $diff_class, esc_html($line, -nbsp=>1);
+
+	my $diff_classes = "diff";
+	$diff_classes .= " $diff_class" if ($diff_class);
+	$line = "<div class=\"$diff_classes\">$line</div>\n";
+
+	return $line;
 }
 
 # Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
@@ -5067,7 +5067,7 @@ sub print_diff_lines {
 }
 
 sub print_diff_chunk {
-	my ($diff_style, $is_combined, @chunk) = @_;
+	my ($diff_style, $is_combined, $from, $to, @chunk) = @_;
 	my (@ctx, @rem, @add);
 
 	# The class of the previous line. 
@@ -5089,6 +5089,8 @@ sub print_diff_chunk {
 	foreach my $line_info (@chunk) {
 		my ($class, $line) = @$line_info;
 
+		$line = format_diff_line($line, $class, $from, $to);
+
 		# print chunk headers
 		if ($class && $class eq 'chunk_header') {
 			print $line;
@@ -5242,22 +5244,19 @@ sub git_patchset_body {
 
 			next PATCH if ($patch_line =~ m/^diff /);
 
-			my ($class, $line) = process_diff_line($patch_line, \%from, \%to);
-			my $diff_classes = "diff";
-			$diff_classes .= " $class" if ($class);
-			$line = "<div class=\"$diff_classes\">$line</div>\n";
+			my $class = diff_line_class($patch_line, \%from, \%to);
 
 			if ($class eq 'chunk_header') {
-				print_diff_chunk($diff_style, $is_combined, @chunk);
+				print_diff_chunk($diff_style, $is_combined, \%from, \%to, @chunk);
 				@chunk = ();
 			}
 
-			push @chunk, [ $class, $line ];
+			push @chunk, [ $class, $patch_line ];
 		}
 
 	} continue {
 		if (@chunk) {
-			print_diff_chunk($diff_style, $is_combined, @chunk);
+			print_diff_chunk($diff_style, $is_combined, \%from, \%to, @chunk);
 			@chunk = ();
 		}
 		print "</div>\n"; # class="patch"
-- 
1.7.8.4

[PATCH v3 7/8] gitweb: Highlight interesting parts of diff

[Michał Kiedrowicz]

Reading diff output is sometimes very hard, even if it's colored,
especially if lines differ only in few characters.  This is often true
when a commit fixes a typo or renames some variables or functions.

This commit teaches gitweb to highlight characters that are different
between old and new line with a light green/red background.  This should
work in the similar manner as in Trac or GitHub.

The algorithm that compares lines is based on contrib/diff-highlight.
Basically, it works by determining common prefix/suffix of corresponding
lines and highlightning only the middle part of lines.  For more
information, see contrib/diff-highlight/README.

Combined diffs are not supported but a following commit will change it.

Since we need to pass esc_html()'ed or esc_html_hl_regions()'ed lines to
format_diff_lines(), so it was taught to accept preformatted lines
passed as a reference.

Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
Acked-by: Jakub Narębski <jnareb@gmail.com>
---
 gitweb/gitweb.perl       |  106 ++++++++++++++++++++++++++++++++++++++++-----
 gitweb/static/gitweb.css |    8 ++++
 2 files changed, 102 insertions(+), 12 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 926e83c..579592a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2428,19 +2428,25 @@ sub format_cc_diff_chunk_header {
 }
 
 # process patch (diff) line (not to be used for diff headers),
-# returning HTML-formatted (but not wrapped) line
+# returning HTML-formatted (but not wrapped) line.
+# If the line is passed as a reference, it is treated as HTML and not
+# esc_html()'ed.
 sub format_diff_line {
 	my ($line, $diff_class, $from, $to) = @_;
 
-	chomp $line;
-	$line = untabify($line);
-
-	if ($from && $to && $line =~ m/^\@{2} /) {
-		$line = format_unidiff_chunk_header($line, $from, $to);
-	} elsif ($from && $to && $line =~ m/^\@{3}/) {
-		$line = format_cc_diff_chunk_header($line, $from, $to);
+	if (ref($line)) {
+		$line = $$line;
 	} else {
-		$line = esc_html($line, -nbsp=>1);
+		chomp $line;
+		$line = untabify($line);
+
+		if ($from && $to && $line =~ m/^\@{2} /) {
+			$line = format_unidiff_chunk_header($line, $from, $to);
+		} elsif ($from && $to && $line =~ m/^\@{3}/) {
+			$line = format_cc_diff_chunk_header($line, $from, $to);
+		} else {
+			$line = esc_html($line, -nbsp=>1);
+		}
 	}
 
 	my $diff_classes = "diff";
@@ -5054,10 +5060,88 @@ sub print_inline_diff_lines {
 	print @$ctx, @$rem, @$add;
 }
 
+# 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) = @_;
+
+	# We need to untabify lines before split()'ing them;
+	# otherwise offsets would be invalid.
+	chomp $rem, $add;
+	$rem = untabify($rem);
+	$add = untabify($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);
+	my ($prefix_has_nonspace, $suffix_has_nonspace);
+
+	my $shorter = (@rem < @add) ? @rem : @add;
+	while ($prefix_len < $shorter) {
+		last if ($rem[$prefix_len] ne $add[$prefix_len]);
+
+		$prefix_has_nonspace = 1 if ($rem[$prefix_len] !~ /\s/);
+		$prefix_len++;
+	}
+
+	while ($prefix_len + $suffix_len < $shorter) {
+		last if ($rem[-1 - $suffix_len] ne $add[-1 - $suffix_len]);
+
+		$suffix_has_nonspace = 1 if ($rem[-1 - $suffix_len] !~ /\s/);
+		$suffix_len++;
+	}
+
+	# Mark lines that are different from each other, but have some common
+	# part that isn't whitespace.  If lines are completely different, don't
+	# mark them because that would make output unreadable, especially if
+	# diff consists of multiple lines.
+	if ($prefix_has_nonspace || $suffix_has_nonspace) {
+		$esc_rem = esc_html_hl_regions($rem, 'marked',
+		        [$prefix_len, @rem - $suffix_len], -nbsp=>1);
+		$esc_add = esc_html_hl_regions($add, 'marked',
+		        [$prefix_len, @add - $suffix_len], -nbsp=>1);
+	} else {
+		$esc_rem = esc_html($rem, -nbsp=>1);
+		$esc_add = esc_html($add, -nbsp=>1);
+	}
+
+	return format_diff_line(\$esc_rem, 'rem'),
+	       format_diff_line(\$esc_add, 'add');
+}
+
+# HTML-format diff context, removed and added lines.
+sub format_ctx_rem_add_lines {
+	my ($ctx, $rem, $add, $is_combined) = @_;
+	my (@new_ctx, @new_rem, @new_add);
+
+	# 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) {
+		for (my $i = 0; $i < @$add; $i++) {
+			my ($line_rem, $line_add) = format_rem_add_lines_pair(
+			        $rem->[$i], $add->[$i]);
+			push @new_rem, $line_rem;
+			push @new_add, $line_add;
+		}
+	} else {
+		@new_rem = map { format_diff_line($_, 'rem') } @$rem;
+		@new_add = map { format_diff_line($_, 'add') } @$add;
+	}
+
+	@new_ctx = map { format_diff_line($_, 'ctx') } @$ctx;
+
+	return (\@new_ctx, \@new_rem, \@new_add);
+}
+
 # Print context lines and then rem/add lines.
 sub print_diff_lines {
 	my ($ctx, $rem, $add, $diff_style, $is_combined) = @_;
 
+	($ctx, $rem, $add) = format_ctx_rem_add_lines($ctx, $rem, $add,
+	        $is_combined);
+
 	if ($diff_style eq 'sidebyside' && !$is_combined) {
 		print_sidebyside_diff_lines($ctx, $rem, $add);
 	} else {
@@ -5089,11 +5173,9 @@ sub print_diff_chunk {
 	foreach my $line_info (@chunk) {
 		my ($class, $line) = @$line_info;
 
-		$line = format_diff_line($line, $class, $from, $to);
-
 		# print chunk headers
 		if ($class && $class eq 'chunk_header') {
-			print $line;
+			print format_diff_line($line, $class, $from, $to);
 			next;
 		}
 
diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
index c530355..cb86d2d 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -438,6 +438,10 @@ div.diff.add {
 	color: #008800;
 }
 
+div.diff.add span.marked {
+	background-color: #aaffaa;
+}
+
 div.diff.from_file a.path,
 div.diff.from_file {
 	color: #aa0000;
@@ -447,6 +451,10 @@ div.diff.rem {
 	color: #cc0000;
 }
 
+div.diff.rem span.marked {
+	background-color: #ffaaaa;
+}
+
 div.diff.chunk_header a,
 div.diff.chunk_header {
 	color: #990099;
-- 
1.7.8.4

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

[Michał Kiedrowicz]

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 579592a..e66aa84 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5063,7 +5063,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 {
-	my ($rem, $add) = @_;
+	my ($rem, $add, $num_parents) = @_;
 
 	# We need to untabify lines before split()'ing them;
 	# otherwise offsets would be invalid.
@@ -5074,8 +5074,8 @@ sub format_rem_add_lines_pair {
 	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;
@@ -5113,15 +5113,43 @@ sub format_rem_add_lines_pair {
 
 # 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 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(
-			        $rem->[$i], $add->[$i]);
+			        $rem->[$i], $add->[$i], $num_parents);
 			push @new_rem, $line_rem;
 			push @new_add, $line_add;
 		}
@@ -5137,10 +5165,11 @@ sub format_ctx_rem_add_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,
-	        $is_combined);
+	        $num_parents);
 
 	if ($diff_style eq 'sidebyside' && !$is_combined) {
 		print_sidebyside_diff_lines($ctx, $rem, $add);
@@ -5151,7 +5180,7 @@ sub print_diff_lines {
 }
 
 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. 
@@ -5186,7 +5215,7 @@ sub print_diff_chunk {
 		if (((@rem || @add) && $class eq 'ctx') || !$class ||
 		    (@rem && @add && $class ne $prev_class)) {
 			print_diff_lines(\@ctx, \@rem, \@add,
-		                         $diff_style, $is_combined);
+		                         $diff_style, $num_parents);
 			@ctx = @rem = @add = ();
 		}
 
@@ -5329,7 +5358,7 @@ sub git_patchset_body {
 			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 = ();
 			}
 
@@ -5338,7 +5367,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"
-- 
1.7.8.4

Re: [PATCH v3 1/8] gitweb: Use descriptive names in esc_html_hl_regions()

[Junio C Hamano]

Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:

> The $s->[0] and $s->[1] variables look a bit cryptic.  Let's rename them
> to $beg and $end so that it's clear what they do.

Why not $begin and $end?

>
> Suggested-by: Jakub Narębski <jnareb@gmail.com>
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> ---
>  gitweb/gitweb.perl |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index a8b5fad..a3754ff 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -1738,12 +1738,14 @@ sub esc_html_hl_regions {
>  	my $pos = 0;
>  
>  	for my $s (@sel) {
> -		$out .= esc_html(substr($str, $pos, $s->[0] - $pos))
> -			if ($s->[0] - $pos > 0);
> +		my ($beg, $end) = @$s;
> +
> +		$out .= esc_html(substr($str, $pos, $beg - $pos))
> +			if ($beg - $pos > 0);
>  		$out .= $cgi->span({-class => $css_class},
> -		                   esc_html(substr($str, $s->[0], $s->[1] - $s->[0])));
> +		                   esc_html(substr($str, $beg, $end - $beg)));
>  
> -		$pos = $s->[1];
> +		$pos = $end;
>  	}
>  	$out .= esc_html(substr($str, $pos))
>  		if ($pos < length($str));

Re: [PATCH v3 4/8] gitweb: Extract print_sidebyside_diff_lines()

[Junio C Hamano]

Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:

> +	if (!@$add) {
> +		# pure removal
> +...
> +	} elsif (!@$rem) {
> +		# pure addition
> +...
> +	} else {
> +		# assume that it is change
> +		print join '',

I know this is not a new problem, but if your patch hunk has both '-' and
'+' lines, what's there to "assume" that it is a change?  Isn't it always?


> -		# empty add/rem block on start context block, or end of chunk
> -		if ((@rem || @add) && (!$class || $class eq 'ctx')) {
> -...
> +		## print from accumulator when have some add/rem lines or end
> +		# of chunk (flush context lines)
> +		if (((@rem || @add) && $class eq 'ctx') || !$class) {

This seems to change the condition.  Earlier, it held true if (there is
anything to show), and (class is unset or equal to ctx).  The new code
says something different.  Also can $class be undef, and if so, doesn't
it trigger comparison between undef and 'ctx' by having !$class check at
the end of || chain?

Re: [PATCH v3 4/8] gitweb: Extract print_sidebyside_diff_lines()

[Jakub Narebski]

Junio C Hamano wrote:
> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> 
> > +	if (!@$add) {
> > +		# pure removal
> > +...
> > +	} elsif (!@$rem) {
> > +		# pure addition
> > +...
> > +	} else {
> > +		# assume that it is change
> > +		print join '',
> 
> I know this is not a new problem, but if your patch hunk has both '-' and
> '+' lines, what's there to "assume" that it is a change?  Isn't it always?

What I meant here when I was writing it that they are lines that changed
between two versions, like '!' in original (not unified) context format.

We can omit this comment.

> > -		# empty add/rem block on start context block, or end of chunk
> > -		if ((@rem || @add) && (!$class || $class eq 'ctx')) {
> > -...
> > +		## print from accumulator when have some add/rem lines or end
> > +		# of chunk (flush context lines)
> > +		if (((@rem || @add) && $class eq 'ctx') || !$class) {
> 
> This seems to change the condition.  Earlier, it held true if (there is
> anything to show), and (class is unset or equal to ctx).  The new code
> says something different.

Yes it does, as described in the commit message:

                                                    [...] It should
  not change the gitweb output, but it **slightly changes its behavior**.
  Before this commit, context is printed on the class change. Now,  it's
  printed just before printing added and removed lines, and at the end of
  chunk.

The difference is that context lines are also printed accumulated now.
Though why this change is required for refactoring could have been
described in more detail...

>                             Also can $class be undef, and if so, doesn't 
> it trigger comparison between undef and 'ctx' by having !$class check at
> the end of || chain?

Thanks for noticing this (I wonder why testsuite didn't caught it).
It should be

 +		## print from accumulator when have some add/rem lines or end
 +		# of chunk (flush context lines)
 +		if (!$class || ((@rem || @add) && $class eq 'ctx')) {

-- 
Jakub Narebski
Poland

Re: [PATCH v3 1/8] gitweb: Use descriptive names in esc_html_hl_regions()

[Michal Kiedrowicz]

Junio C Hamano <gitster@pobox.com> wrote:

> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> 
> > The $s->[0] and $s->[1] variables look a bit cryptic.  Let's rename
> > them to $beg and $end so that it's clear what they do.
> 
> Why not $begin and $end?

For no special reason.  I just took the names that Jakub proposed in

	http://article.gmane.org/gmane.comp.version-control.git/193839/
> 
> >
> > Suggested-by: Jakub Narębski <jnareb@gmail.com>
> > Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> > ---
> >  gitweb/gitweb.perl |   10 ++++++----
> >  1 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index a8b5fad..a3754ff 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -1738,12 +1738,14 @@ sub esc_html_hl_regions {
> >  	my $pos = 0;
> >  
> >  	for my $s (@sel) {
> > -		$out .= esc_html(substr($str, $pos, $s->[0] -
> > $pos))
> > -			if ($s->[0] - $pos > 0);
> > +		my ($beg, $end) = @$s;
> > +
> > +		$out .= esc_html(substr($str, $pos, $beg - $pos))
> > +			if ($beg - $pos > 0);
> >  		$out .= $cgi->span({-class => $css_class},
> > -		                   esc_html(substr($str, $s->[0],
> > $s->[1] - $s->[0])));
> > +		                   esc_html(substr($str, $beg,
> > $end - $beg))); 
> > -		$pos = $s->[1];
> > +		$pos = $end;
> >  	}
> >  	$out .= esc_html(substr($str, $pos))
> >  		if ($pos < length($str));

Re: [PATCH v3 4/8] gitweb: Extract print_sidebyside_diff_lines()

[Michal Kiedrowicz]

Jakub Narebski <jnareb@gmail.com> wrote:

> Junio C Hamano wrote:
> > Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> > 
> > > +	if (!@$add) {
> > > +		# pure removal
> > > +...
> > > +	} elsif (!@$rem) {
> > > +		# pure addition
> > > +...
> > > +	} else {
> > > +		# assume that it is change
> > > +		print join '',
> > 
> > I know this is not a new problem, but if your patch hunk has both
> > '-' and '+' lines, what's there to "assume" that it is a change?
> > Isn't it always?
> 
> What I meant here when I was writing it that they are lines that
> changed between two versions, like '!' in original (not unified)
> context format.
> 
> We can omit this comment.

OK.

> 
> > > -		# empty add/rem block on start context block, or
> > > end of chunk
> > > -		if ((@rem || @add) && (!$class || $class eq
> > > 'ctx')) { -...
> > > +		## print from accumulator when have some add/rem
> > > lines or end
> > > +		# of chunk (flush context lines)
> > > +		if (((@rem || @add) && $class eq 'ctx')
> > > || !$class) {
> > 
> > This seems to change the condition.  Earlier, it held true if
> > (there is anything to show), and (class is unset or equal to ctx).
> > The new code says something different.
> 
> Yes it does, as described in the commit message:
> 
>                                                     [...] It should
>   not change the gitweb output, but it **slightly changes its
> behavior**. Before this commit, context is printed on the class
> change. Now,  it's printed just before printing added and removed
> lines, and at the end of chunk.
> 
> The difference is that context lines are also printed accumulated now.
> Though why this change is required for refactoring could have been
> described in more detail...

I changed that because I wanted to squash both conditions (the one that
checks if @ctx should be printed and the one that prints @add/@rem
lines) and have just one call to print_sidebyside_diff_lines().  Later,
this function is changed to print_diff_lines() and controls whether
'inline' or 'side-by-side' diff should be printed.  Having two
conditions and two calls/functions would make the code redundant.  Then
I thought that instead of calling twice print_sidebyside_diff_lines()
(for @ctx and @add/@rem lines, like the code from pre-image prints
these lines separatedly), I can just call it once.

I can revert this change to previous behavior but I think that would
make the condition more complicated.

> 
> >                             Also can $class be undef, and if so,
> > doesn't it trigger comparison between undef and 'ctx' by
> > having !$class check at the end of || chain?
> 
> Thanks for noticing this (I wonder why testsuite didn't caught it).
> It should be
> 
>  +		## print from accumulator when have some add/rem
> lines or end
>  +		# of chunk (flush context lines)
>  +		if (!$class || ((@rem || @add) && $class eq 'ctx'))
> {
> 

OK, I'll fix that.

Re: [PATCH v3 7/8] gitweb: Highlight interesting parts of diff

[Michal Kiedrowicz]

Michał Kiedrowicz <michal.kiedrowicz@gmail.com> wrote:

> Reading diff output is sometimes very hard, even if it's colored,
> especially if lines differ only in few characters.  This is often true
> when a commit fixes a typo or renames some variables or functions.
> 
> This commit teaches gitweb to highlight characters that are different
> between old and new line with a light green/red background.  This
> should work in the similar manner as in Trac or GitHub.
> 
> The algorithm that compares lines is based on contrib/diff-highlight.
> Basically, it works by determining common prefix/suffix of
> corresponding lines and highlightning only the middle part of lines.
> For more information, see contrib/diff-highlight/README.
> 
> Combined diffs are not supported but a following commit will change
> it.
> 
> Since we need to pass esc_html()'ed or esc_html_hl_regions()'ed lines
> to format_diff_lines(), so it was taught to accept preformatted lines
> passed as a reference.
> 
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> Acked-by: Jakub Narębski <jnareb@gmail.com>

Junio,

can you please fixup this patch?  I just noticed "chomp $rem, $add" breaks
the testsuite.

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index e4351fe..961fbdc 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5067,7 +5067,8 @@ sub format_rem_add_lines_pair {

        # We need to untabify lines before split()'ing them;
        # otherwise offsets would be invalid.
-       chomp $rem, $add;
+       chomp $rem;
+       chomp $add;
        $rem = untabify($rem);
        $add = untabify($add);

Re: [PATCH v3 4/8] gitweb: Extract print_sidebyside_diff_lines()

[Jakub Narebski]

Michal Kiedrowicz wrote:
> Jakub Narebski <jnareb@gmail.com> wrote:
>> Junio C Hamano wrote:
>>> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:

>>>> -		# empty add/rem block on start context block, or
>>>> end of chunk
>>>> -		if ((@rem || @add) && (!$class || $class eq
>>>> 'ctx')) { -...
>>>> +		## print from accumulator when have some add/rem
>>>> lines or end
>>>> +		# of chunk (flush context lines)
>>>> +		if (((@rem || @add) && $class eq 'ctx')
>>>> || !$class) {
>>> 
>>> This seems to change the condition.  Earlier, it held true if
>>> (there is anything to show), and (class is unset or equal to ctx).
>>> The new code says something different.
>> 
>> Yes it does, as described in the commit message:
>> 
>>                                                     [...] It should
>>   not change the gitweb output, but it **slightly changes its
>> behavior**. Before this commit, context is printed on the class
>> change. Now,  it's printed just before printing added and removed
>> lines, and at the end of chunk.
>> 
>> The difference is that context lines are also printed accumulated now.
>> Though why this change is required for refactoring could have been
>> described in more detail...
> 
> I changed that because I wanted to squash both conditions (the one that
> checks if @ctx should be printed and the one that prints @add/@rem
> lines) and have just one call to print_sidebyside_diff_lines().  Later,
> this function is changed to print_diff_lines() and controls whether
> 'inline' or 'side-by-side' diff should be printed.  Having two
> conditions and two calls/functions would make the code redundant.  Then
> I thought that instead of calling twice print_sidebyside_diff_lines()
> (for @ctx and @add/@rem lines, like the code from pre-image prints
> these lines separatedly), I can just call it once.
> 
> I can revert this change to previous behavior but I think that would
> make the condition more complicated.

No, I think that this change is good idea if it simplifies code flow.
But it really should be described in commit message, not only "what"
(which you did describe), but also "whys".

-- 
Jakub Narebski
Poland

Re: [PATCH v3 5/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs

[Jakub Narebski]

Michał Kiedrowicz wrote:

> This renames print_sidebyside_diff_chunk() to print_diff_chunk() and
> makes use of it for both side-by-side and inline diffs.  Now diff lines
> are always accumulated before they are printed.  This opens the
> possibility to preprocess diff output before it's printed, which is
> needed for diff refinement highlightning (implemented in incoming
> patches).
> 
> If print_diff_chunk() was left as is, the new function
> print_inline_diff_lines() could reorder diff lines.  It first prints all
> context lines, then all removed lines and finally all added lines.  If
> the diff output consisted of mixed added and removed lines, gitweb would
> reorder these lines.  This is true for combined diff output, for
> example:
> 
> 	 - removed line for first parent
> 	 + added line for first parent
> 	  -removed line for second parent
> 	 ++added line for both parents
> 
> would be rendered as:
> 
> 	- removed line for first parent
> 	 -removed line for second parent
> 	+ added line for first parent
> 	++added line for both parents
> 
> To prevent gitweb from reordering lines, print_diff_chunk() calls
> print_diff_lines() as soon as it detects that both added and removed
> lines are present and there was a class change.
                                                 , and at the end of hunk.


I think it is worth adding the above to the commit message.

-- 
Jakub Narebski
Poland

Re: [PATCH v3 5/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs

[Michal Kiedrowicz]

Jakub Narebski <jnareb@gmail.com> wrote:

> Michał Kiedrowicz wrote:
> > 
> > To prevent gitweb from reordering lines, print_diff_chunk() calls
> > print_diff_lines() as soon as it detects that both added and removed
> > lines are present and there was a class change.
>                                                  , and at the end of
> hunk.
> 
> 
> I think it is worth adding the above to the commit message.
> 

OK, will do.

Re: [PATCH v3 4/8] gitweb: Extract print_sidebyside_diff_lines()

[Michal Kiedrowicz]

Jakub Narebski <jnareb@gmail.com> wrote:

> Michal Kiedrowicz wrote:
> > Jakub Narebski <jnareb@gmail.com> wrote:
> >> Junio C Hamano wrote:
> >>> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> 
> >>>> -		# empty add/rem block on start context block, or
> >>>> end of chunk
> >>>> -		if ((@rem || @add) && (!$class || $class eq
> >>>> 'ctx')) { -...
> >>>> +		## print from accumulator when have some add/rem
> >>>> lines or end
> >>>> +		# of chunk (flush context lines)
> >>>> +		if (((@rem || @add) && $class eq 'ctx')
> >>>> || !$class) {
> >>> 
> >>> This seems to change the condition.  Earlier, it held true if
> >>> (there is anything to show), and (class is unset or equal to ctx).
> >>> The new code says something different.
> >> 
> >> Yes it does, as described in the commit message:
> >> 
> >>                                                     [...] It should
> >>   not change the gitweb output, but it **slightly changes its
> >> behavior**. Before this commit, context is printed on the class
> >> change. Now,  it's printed just before printing added and removed
> >> lines, and at the end of chunk.
> >> 
> >> The difference is that context lines are also printed accumulated
> >> now. Though why this change is required for refactoring could have
> >> been described in more detail...
> > 
> > I changed that because I wanted to squash both conditions (the one
> > that checks if @ctx should be printed and the one that prints
> > @add/@rem lines) and have just one call to
> > print_sidebyside_diff_lines().  Later, this function is changed to
> > print_diff_lines() and controls whether 'inline' or 'side-by-side'
> > diff should be printed.  Having two conditions and two
> > calls/functions would make the code redundant.  Then I thought that
> > instead of calling twice print_sidebyside_diff_lines() (for @ctx
> > and @add/@rem lines, like the code from pre-image prints these
> > lines separatedly), I can just call it once.
> > 
> > I can revert this change to previous behavior but I think that would
> > make the condition more complicated.
> 
> No, I think that this change is good idea if it simplifies code flow.
> But it really should be described in commit message, not only "what"
> (which you did describe), but also "whys".
> 

Sure, I'll try to put my explanation to the commit message.