[PATCHv3 0/8] gitweb: side-by-side diff

[PATCHv3 0/8] gitweb: side-by-side diff

[Jakub Narebski]

NOTE: As it is feature-freeze period, this patch series is for review.

This is refinement and extension of Kato Kazuyoshi patch series, sent
originally as

  [PATCH/RFC] gitweb: add the ability to show side-by-side diff on commitdiff.
  http://thread.gmane.org/gmane.comp.version-control.git/183744

and then refined and split into two-patch series

  [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class
  http://thread.gmane.org/gmane.comp.version-control.git/183770

  [PATCH 2/2] gitweb: add a feature to show side-by-side diff
  http://thread.gmane.org/gmane.comp.version-control.git/183769

This patch series originally started as rebasing second patch in above
two part series on top of diff line classification refactoring
suggested by me and proposed by Junio.  Then I thought about putting
all code printing side-by-side diff in print_sidebyside_diff_chunk()
subroutine, then...


Main changes from v2 version from Kato Kazuyoshi:

* Built on top of refactoring of code related to diff
  output formatting (patches 1 and 2)

* Code reworked so that is in my opinion easier to follow; gitweb now
  handles merges and diffs with incomplete lines correctly (patch 3)
  - well, it handles merges by turning off side-by-side diff for them.

* Adding background color to distinguish empty context lines from
  vertical align, similarly to e.g.
    http://community.activestate.com/files/images/sbsdiffs.png
  but without refinement (word diff of changes).

* Adds some very basic test for side-by-side diff (patch 6 (and 5))

* Split adding navigation into a separate commit, and uses [nav] links
  rather than HTML form for selecting between inline and side-by-side
  diff style (diff 8).  Thanks to more thorough use of href(-replay=>1,..)
  (patch 7) style of diff should be preserved with this series.


Please excuse me for essentially hijacking this patch series.


P.S. I really, really need to finish work on splitting gitweb into
smaller pieces.  With around 8,000 lines it becomes quite unwieldy.
But this would probably need total rework of error handling (the
die_error subroutine), and that would need another changes, etc....


Pull request:
~~~~~~~~~~~~~
These changes are available in the git repository(-y/+ies) at:
  git://repo.or.cz/git/jnareb-git.git gitweb/side-by-side-diff-v4
  git://github.com/jnareb/git         gitweb/side-by-side-diff-v4

Table of contents:
~~~~~~~~~~~~~~~~~~
  [PATCHv3 1/8] gitweb: Refactor diff body line classification
  [PATCHv3 2/8] gitweb: Extract formatting of diff chunk header
  [PATCHv3 3/8] gitweb: Add a feature to show side-by-side diff
  [PATCHv3 4/8] gitweb: Give side-by-side diff extra CSS styling
  [PATCHv3 5/8] t9500: Add test for handling incomplete lines in diff
   by gitweb
  [PATCHv3 6/8] t9500: Add basic sanity tests for side-by-side diff in
   gitweb
  [PATCHv3 7/8] gitweb: Use href(-replay=>1,...) for formats links in
   "commitdiff"
  [PATCHv3 8/8] gitweb: Add navigation to select side-by-side diff

Shortlog:
~~~~~~~~~
Jakub Narebski (6):
  gitweb: Refactor diff body line classification
  gitweb: Extract formatting of diff chunk header
  gitweb: Give side-by-side diff extra CSS styling
  t9500: Add test for handling incomplete lines in diff by gitweb
  t9500: Add basic sanity tests for side-by-side diff in gitweb
  gitweb: Use href(-replay=>1,...) for formats links in "commitdiff"

Kato Kazuyoshi (2):
  gitweb: Add a feature to show side-by-side diff
  gitweb: Add navigation to select side-by-side diff

Diffstat:
~~~~~~~~~
 gitweb/gitweb.perl                     |  339 +++++++++++++++++++++++--------
 gitweb/static/gitweb.css               |   30 +++
 t/t9500-gitweb-standalone-no-errors.sh |   73 +++++++-
 3 files changed, 353 insertions(+), 89 deletions(-)

-- 
1.7.6

[PATCHv3 1/8] gitweb: Refactor diff body line classification

[Jakub Narebski]

Simplify classification of diff line body in format_diff_line(),
replacing two long if-elsif chains (one for ordinary diff and one for
combined diff of a merge commit) with a single 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>
---
This patch is new in this version of side-by-side diff series.

Junio, as your proposal was of "what if" variety, and not as a proper
patch, I have taken authorship (after reqorking and testing it).
Should I revert authorship to you?

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 4f0c3bd..914fd4c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2225,40 +2225,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 = "";
+
+	my $diff_class = diff_line_class($line, $from, $to);
+	my $diff_classes = "diff";
+	$diff_classes .= " $diff_class" if ($diff_class);
 
 	chomp $line;
-
-	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";
-		}
-	}
 	$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}(.*)$/;
@@ -2276,7 +2283,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);
@@ -2309,9 +2316,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

[PATCHv3 2/8] gitweb: Extract formatting of diff chunk header

[Jakub Narebski]

Refactor main parts of HTML-formatting for diff chunk headers
(formatting means here adding links and syntax hightlighting) into
separate subroutines:

 * format_unidiff_chunk_header for ordinary diff,
 * format_cc_diff_chunk_header for combined diff
   (more than one parent)

This makes format_diff_line() subroutine easier to follow.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This patch is new in this version of side-by-side diff series.

After those changes format_diff_line() fits in one page (has less than
25 lines).  Nice, isn't it?

 gitweb/gitweb.perl |  114 ++++++++++++++++++++++++++++++---------------------
 1 files changed, 67 insertions(+), 47 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 914fd4c..95d278a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2254,6 +2254,69 @@ sub diff_line_class {
 	return "";
 }
 
+# assumes that $from and $to are defined and correctly filled,
+# and that $line holds a line of chunk header for unified diff
+sub format_unidiff_chunk_header {
+	my ($line, $from, $to) = @_;
+
+	my ($from_text, $from_start, $from_lines, $to_text, $to_start, $to_lines, $section) =
+		$line =~ m/^\@{2} (-(\d+)(?:,(\d+))?) (\+(\d+)(?:,(\d+))?) \@{2}(.*)$/;
+
+	$from_lines = 0 unless defined $from_lines;
+	$to_lines   = 0 unless defined $to_lines;
+
+	if ($from->{'href'}) {
+		$from_text = $cgi->a({-href=>"$from->{'href'}#l$from_start",
+		                     -class=>"list"}, $from_text);
+	}
+	if ($to->{'href'}) {
+		$to_text   = $cgi->a({-href=>"$to->{'href'}#l$to_start",
+		                     -class=>"list"}, $to_text);
+	}
+	$line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
+	        "<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>";
+	return $line;
+}
+
+# assumes that $from and $to are defined and correctly filled,
+# and that $line holds a line of chunk header for combined diff
+sub format_cc_diff_chunk_header {
+	my ($line, $from, $to) = @_;
+
+	my ($prefix, $ranges, $section) = $line =~ m/^(\@+) (.*?) \@+(.*)$/;
+	my (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);
+
+	@from_text = split(' ', $ranges);
+	for (my $i = 0; $i < @from_text; ++$i) {
+		($from_start[$i], $from_nlines[$i]) =
+			(split(',', substr($from_text[$i], 1)), 0);
+	}
+
+	$to_text   = pop @from_text;
+	$to_start  = pop @from_start;
+	$to_nlines = pop @from_nlines;
+
+	$line = "<span class=\"chunk_info\">$prefix ";
+	for (my $i = 0; $i < @from_text; ++$i) {
+		if ($from->{'href'}[$i]) {
+			$line .= $cgi->a({-href=>"$from->{'href'}[$i]#l$from_start[$i]",
+			                  -class=>"list"}, $from_text[$i]);
+		} else {
+			$line .= $from_text[$i];
+		}
+		$line .= " ";
+	}
+	if ($to->{'href'}) {
+		$line .= $cgi->a({-href=>"$to->{'href'}#l$to_start",
+		                  -class=>"list"}, $to_text);
+	} else {
+		$line .= $to_text;
+	}
+	$line .= " $prefix</span>" .
+	         "<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>";
+	return $line;
+}
+
 # format patch (diff) line (not to be used for diff headers)
 sub format_diff_line {
 	my $line = shift;
@@ -2267,56 +2330,13 @@ sub format_diff_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}(.*)$/;
-
-		$from_lines = 0 unless defined $from_lines;
-		$to_lines   = 0 unless defined $to_lines;
-
-		if ($from->{'href'}) {
-			$from_text = $cgi->a({-href=>"$from->{'href'}#l$from_start",
-			                     -class=>"list"}, $from_text);
-		}
-		if ($to->{'href'}) {
-			$to_text   = $cgi->a({-href=>"$to->{'href'}#l$to_start",
-			                     -class=>"list"}, $to_text);
-		}
-		$line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
-		        "<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>";
+		$line = format_unidiff_chunk_header($line, $from, $to);
 		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);
-
-		@from_text = split(' ', $ranges);
-		for (my $i = 0; $i < @from_text; ++$i) {
-			($from_start[$i], $from_nlines[$i]) =
-				(split(',', substr($from_text[$i], 1)), 0);
-		}
-
-		$to_text   = pop @from_text;
-		$to_start  = pop @from_start;
-		$to_nlines = pop @from_nlines;
-
-		$line = "<span class=\"chunk_info\">$prefix ";
-		for (my $i = 0; $i < @from_text; ++$i) {
-			if ($from->{'href'}[$i]) {
-				$line .= $cgi->a({-href=>"$from->{'href'}[$i]#l$from_start[$i]",
-				                  -class=>"list"}, $from_text[$i]);
-			} else {
-				$line .= $from_text[$i];
-			}
-			$line .= " ";
-		}
-		if ($to->{'href'}) {
-			$line .= $cgi->a({-href=>"$to->{'href'}#l$to_start",
-			                  -class=>"list"}, $to_text);
-		} else {
-			$line .= $to_text;
-		}
-		$line .= " $prefix</span>" .
-		         "<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>";
+		$line = format_cc_diff_chunk_header($line, $from, $to);
 		return "<div class=\"$diff_classes\">$line</div>\n";
+
 	}
 	return "<div class=\"$diff_classes\">" . esc_html($line, -nbsp=>1) . "</div>\n";
 }
-- 
1.7.6

[PATCHv3 3/8] gitweb: Add a feature to show side-by-side diff

[Jakub Narebski]

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

This commits adds to support for showing "side-by-side" style diff.
Currently you have to hand-craft the URL; navigation for selecting
diff style is to be added in the next commit.


The diff output in unified format from "git diff-tree" is reorganized to
side-by-side style chunk by chunk with format_sidebyside_diff_chunk().
This reorganization requires knowledge about diff line classification,
so format_diff_line() was renamed to process_diff_line(), and changed to
return tuple (list) consisting of class of diff line and of
HTML-formatted (but not wrapped in <div class="diff ...">...</div>) diff
line.  Wrapping is now done by caller, i.e. git_patchset_body().

Gitweb uses float+margin CSS-based layout for "side by side" diff.


You can specify style of diff with "ds" ('diff_style') query
parameter.  Currently supported values are 'inline' and 'sidebyside';
the default is 'inline'.

Another solution would be to use "opt" ('extra_options') for that...
though current use of it in gitweb seems to suggest that "opt" is more
about passing extra options to underlying git commands, and "git diff"
doesn't support '--side-by-side' like GNU diff does, (yet?).

Signed-off-by: Kato Kazuyoshi <kato.kazuyoshi@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Heavily changed from original submission by Kato Kazuyoshi, but the
main idea, code structure (somewhat) and CSS-base layout remains.
That's why authorship remains with him; Kato, please speak up if you
would like to change this.

The decision to move some of formatting outside process_diff_line()
(formerly format_diff_line()) was not really necessary, in hindsight...

 gitweb/gitweb.perl       |  116 +++++++++++++++++++++++++++++++++++++++++----
 gitweb/static/gitweb.css |   17 +++++++
 2 files changed, 122 insertions(+), 11 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 95d278a..68629f6 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -759,6 +759,7 @@ our @cgi_param_mapping = (
 	extra_options => "opt",
 	search_use_regexp => "sr",
 	ctag => "by_tag",
+	diff_style => "ds",
 	# this must be last entry (for manipulation from JavaScript)
 	javascript => "js"
 );
@@ -2317,28 +2318,27 @@ sub format_cc_diff_chunk_header {
 	return $line;
 }
 
-# format patch (diff) line (not to be used for diff headers)
-sub format_diff_line {
+# 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);
-	my $diff_classes = "diff";
-	$diff_classes .= " $diff_class" if ($diff_class);
 
 	chomp $line;
 	$line = untabify($line);
 
 	if ($from && $to && $line =~ m/^\@{2} /) {
 		$line = format_unidiff_chunk_header($line, $from, $to);
-		return "<div class=\"$diff_classes\">$line</div>\n";
+		return $diff_class, $line;
 
 	} elsif ($from && $to && $line =~ m/^\@{3}/) {
 		$line = format_cc_diff_chunk_header($line, $from, $to);
-		return "<div class=\"$diff_classes\">$line</div>\n";
+		return $diff_class, $line;
 
 	}
-	return "<div class=\"$diff_classes\">" . esc_html($line, -nbsp=>1) . "</div>\n";
+	return $diff_class, esc_html($line, -nbsp=>1);
 }
 
 # Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
@@ -4860,8 +4860,78 @@ sub git_difftree_body {
 	print "</table>\n";
 }
 
+sub print_sidebyside_diff_chunk {
+	my @chunk = @_;
+	my (@ctx, @rem, @add);
+
+	return unless @chunk;
+
+	# incomplete last line might be among removed or added lines,
+	# or both, or among context lines: find which
+	for (my $i = 1; $i < @chunk; $i++) {
+		if ($chunk[$i][0] eq 'incomplete') {
+			$chunk[$i][0] = $chunk[$i-1][0];
+		}
+	}
+
+	# guardian
+	push @chunk, ["", ""];
+
+	foreach my $line_info (@chunk) {
+		my ($class, $line) = @$line_info;
+
+		# print chunk headers
+		if ($class && $class eq 'chunk_header') {
+			print $line;
+			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">',
+					'<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')) {
+			print join '',
+				'<div class="chunk_block">',
+					'<div class="old">',
+					@rem,
+					'</div>',
+					'<div class="new">',
+					@add,
+					'</div>',
+				'</div>';
+			@rem = @add = ();
+		}
+
+		## adding lines to accumulator
+		# guardian value
+		last unless $line;
+		# rem, add or change
+		if ($class eq 'rem') {
+			push @rem, $line;
+		} elsif ($class eq 'add') {
+			push @add, $line;
+		}
+		# context line
+		if ($class eq 'ctx') {
+			push @ctx, $line;
+		}
+	}
+}
+
 sub git_patchset_body {
-	my ($fd, $difftree, $hash, @hash_parents) = @_;
+	my ($fd, $diff_style, $difftree, $hash, @hash_parents) = @_;
 	my ($hash_parent) = $hash_parents[0];
 
 	my $is_combined = (@hash_parents > 1);
@@ -4871,6 +4941,7 @@ sub git_patchset_body {
 	my $diffinfo;
 	my $to_name;
 	my (%from, %to);
+	my @chunk; # for side-by-side diff
 
 	print "<div class=\"patchset\">\n";
 
@@ -4977,10 +5048,29 @@ sub git_patchset_body {
 
 			next PATCH if ($patch_line =~ m/^diff /);
 
-			print format_diff_line($patch_line, \%from, \%to);
+			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";
+
+			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;
+			}
 		}
 
 	} continue {
+		if (@chunk) {
+			print_sidebyside_diff_chunk(@chunk);
+			@chunk = ();
+		}
 		print "</div>\n"; # class="patch"
 	}
 
@@ -6976,6 +7066,7 @@ sub git_object {
 
 sub git_blobdiff {
 	my $format = shift || 'html';
+	my $diff_style = $input_params{'diff_style'} || 'inline';
 
 	my $fd;
 	my @difftree;
@@ -7085,7 +7176,8 @@ sub git_blobdiff {
 	if ($format eq 'html') {
 		print "<div class=\"page_body\">\n";
 
-		git_patchset_body($fd, [ \%diffinfo ], $hash_base, $hash_parent_base);
+		git_patchset_body($fd, $diff_style,
+		                  [ \%diffinfo ], $hash_base, $hash_parent_base);
 		close $fd;
 
 		print "</div>\n"; # class="page_body"
@@ -7113,6 +7205,7 @@ sub git_blobdiff_plain {
 sub git_commitdiff {
 	my %params = @_;
 	my $format = $params{-format} || 'html';
+	my $diff_style = $input_params{'diff_style'} || 'inline';
 
 	my ($patch_max) = gitweb_get_feature('patches');
 	if ($format eq 'patch') {
@@ -7316,7 +7409,8 @@ sub git_commitdiff {
 		                  $use_parents ? @{$co{'parents'}} : $hash_parent);
 		print "<br/>\n";
 
-		git_patchset_body($fd, \@difftree, $hash,
+		git_patchset_body($fd, $diff_style,
+		                  \@difftree, $hash,
 		                  $use_parents ? @{$co{'parents'}} : $hash_parent);
 		close $fd;
 		print "</div>\n"; # class="page_body"
diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
index 7d88509..21842a6 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -475,6 +475,23 @@ div.diff.nodifferences {
 	color: #600000;
 }
 
+/* side-by-side diff */
+div.chunk_block {
+	overflow: hidden;
+}
+
+div.chunk_block div.old {
+	float: left;
+	width: 50%;
+	overflow: hidden;
+}
+
+div.chunk_block div.new {
+	margin-left: 50%;
+	width: 50%;
+}
+
+
 div.index_include {
 	border: solid #d9d8d1;
 	border-width: 0px 0px 1px;
-- 
1.7.6

[PATCHv3 4/8] gitweb: Give side-by-side diff extra CSS styling

[Jakub Narebski]

Use separate background colors for pure removal, pure addition and
change for side-by-side diff.  This makes reading such diff easier,
allowing to easily distinguish empty lines in diff from vertical
whitespace used to align chunk blocks.

Note that if lines in diff were numbered, the absence of line numbers
[for one side] would help in distinguishing empty lines from vertical
align.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This patch is new in this version of series.

Note that the code could probably be written with less duplication, at
the cost of being more complicated.  I think it is worth tradeoff as
written here.

For how output looks like, compare for example:

  http://confluence.atlassian.com/display/FISHEYE/Using+Side+by+Side+Diff+View

 gitweb/gitweb.perl       |   39 +++++++++++++++++++++++++++++----------
 gitweb/static/gitweb.css |   13 +++++++++++++
 2 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 68629f6..f69ed08 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4890,7 +4890,7 @@ sub print_sidebyside_diff_chunk {
 		# 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">',
+				'<div class="chunk_block ctx">',
 					'<div class="old">',
 					@ctx,
 					'</div>',
@@ -4902,15 +4902,34 @@ sub print_sidebyside_diff_chunk {
 		}
 		# empty add/rem block on start context block, or end of chunk
 		if ((@rem || @add) && (!$class || $class eq 'ctx')) {
-			print join '',
-				'<div class="chunk_block">',
-					'<div class="old">',
-					@rem,
-					'</div>',
-					'<div class="new">',
-					@add,
-					'</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>';
+			}
 			@rem = @add = ();
 		}
 
diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
index 21842a6..c7827e8 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -491,6 +491,19 @@ div.chunk_block div.new {
 	width: 50%;
 }
 
+div.chunk_block.rem div.old div.diff.rem {
+	background-color: #fff5f5;
+}
+div.chunk_block.add div.new div.diff.add {
+	background-color: #f8fff8;
+}
+div.chunk_block.chg div     div.diff {
+	background-color: #fffff0;
+}
+div.chunk_block.ctx div     div.diff.ctx {
+	color: #404040;
+}
+
 
 div.index_include {
 	border: solid #d9d8d1;
-- 
1.7.6

[PATCHv3 5/8] t9500: Add test for handling incomplete lines in diff by gitweb

[Jakub Narebski]

Check that "commitdiff" action in gitweb can handle (without errors)
incomplete lines as added and removed lines, and as context lines.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
New in this series, and independent on side-by-side stuff.

 t/t9500-gitweb-standalone-no-errors.sh |   47 ++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index 5329715..c731507 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -274,6 +274,53 @@ test_expect_success \
 	'gitweb_run "p=.git;a=commitdiff;hp=foo-becomes-a-directory;h=foo-symlinked-to-bar"'
 
 # ----------------------------------------------------------------------
+# commitdiff testing (incomplete lines)
+
+test_expect_success 'setup incomplete lines' '
+	cat >file<<-\EOF &&
+	Dominus regit me,
+	et nihil mihi deerit.
+	In loco pascuae ibi me collocavit,
+	super aquam refectionis educavit me;
+	animam meam convertit,
+	deduxit me super semitas jusitiae,
+	propter nomen suum.
+	CHANGE_ME
+	EOF
+	git commit -a -m "Preparing for incomplete lines" &&
+	echo "incomplete" | tr -d "\\012" >>file &&
+	git commit -a -m "Add incomplete line" &&
+	git tag incomplete_lines_add &&
+	sed -e s/CHANGE_ME/change_me/ <file >file+ &&
+	mv -f file+ file &&
+	git commit -a -m "Incomplete context line" &&
+	git tag incomplete_lines_ctx &&
+	echo "Dominus regit me," >file &&
+	echo "incomplete line" | tr -d "\\012" >>file &&
+	git commit -a -m "Change incomplete line" &&
+	git tag incomplete_lines_chg
+	echo "Dominus regit me," >file &&
+	git commit -a -m "Remove incomplete line" &&
+	git tag incomplete_lines_rem
+'
+
+test_expect_success 'commitdiff(1): addition of incomplete line' '
+	gitweb_run "p=.git;a=commitdiff;h=incomplete_lines_add"
+'
+
+test_expect_success 'commitdiff(1): incomplete line as context line' '
+	gitweb_run "p=.git;a=commitdiff;h=incomplete_lines_ctx"
+'
+
+test_expect_success 'commitdiff(1): change incomplete line' '
+	gitweb_run "p=.git;a=commitdiff;h=incomplete_lines_chg"
+'
+
+test_expect_success 'commitdiff(1): removal of incomplete line' '
+	gitweb_run "p=.git;a=commitdiff;h=incomplete_lines_rem"
+'
+
+# ----------------------------------------------------------------------
 # commit, commitdiff: merge, large
 test_expect_success \
 	'Create a merge' \
-- 
1.7.6

[PATCHv3 6/8] t9500: Add basic sanity tests for side-by-side diff in gitweb

[Jakub Narebski]

Test that side-by-side diff can deal with incomplete lines (and while
at it with pure addition, pure removal, and change), and with merge
commits, producing no errors or warnings.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This patch is new in this version.

Note this while it does not check if the output is sane, it can help
with manual check; just run test with `--debug' option, and point
gitweb to "t/trash\ directory.t9500-gitweb-standalone-no-errors/.git/"
repository.

 t/t9500-gitweb-standalone-no-errors.sh |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index c731507..ab24917 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -329,7 +329,8 @@ test_expect_success \
 	 git add b &&
 	 git commit -a -m "On branch" &&
 	 git checkout master &&
-	 git pull . b'
+	 git pull . b &&
+	 git tag merge_commit'
 
 test_expect_success \
 	'commit(0): merge commit' \
@@ -379,6 +380,29 @@ test_expect_success \
 	'gitweb_run "p=.git;a=commitdiff;h=b"'
 
 # ----------------------------------------------------------------------
+# side-by-side diff
+
+test_expect_success 'side-by-side: addition of incomplete line' '
+	gitweb_run "p=.git;a=commitdiff;h=incomplete_lines_add;ds=sidebyside"
+'
+
+test_expect_success 'side-by-side: incomplete line as context line' '
+	gitweb_run "p=.git;a=commitdiff;h=incomplete_lines_ctx;ds=sidebyside"
+'
+
+test_expect_success 'side-by-side: changed incomplete line' '
+	gitweb_run "p=.git;a=commitdiff;h=incomplete_lines_chg;ds=sidebyside"
+'
+
+test_expect_success 'side-by-side: removal of incomplete line' '
+	gitweb_run "p=.git;a=commitdiff;h=incomplete_lines_rem;ds=sidebyside"
+'
+
+test_expect_success 'side-by-side: merge commit' '
+	gitweb_run "p=.git;a=commitdiff;h=merge_commit;ds=sidebyside"
+'
+
+# ----------------------------------------------------------------------
 # tags testing
 
 test_expect_success \
-- 
1.7.6

[PATCHv3 7/8] gitweb: Use href(-replay=>1,...) for formats links in "commitdiff"

[Jakub Narebski]

Use href(-replay->1,...) in (sub)navigation links (like changing style
of view, or going to parent commit) so that extra options are
preserved.

This is needed so clicking on such (sub)navigation link would preserve
style of diff; for example when using "side-by-side" diff style then
going to parent commit would now also use this style.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This patch is new in this series, and independent on side-by-side stuff.
It can (and perhaps should) be moved earlier in this series.

 gitweb/gitweb.perl |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f69ed08..ffaea45 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -7267,8 +7267,8 @@ sub git_commitdiff {
 				}
 			}
 			$formats_nav .= ': ' .
-				$cgi->a({-href => href(action=>"commitdiff",
-				                       hash=>$hash_parent)},
+				$cgi->a({-href => href(-replay=>1,
+				                       hash=>$hash_parent, hash_base=>undef)},
 				        esc_html($hash_parent_short)) .
 				')';
 		} elsif (!$co{'parent'}) {
@@ -7278,28 +7278,28 @@ sub git_commitdiff {
 			# single parent commit
 			$formats_nav .=
 				' (parent: ' .
-				$cgi->a({-href => href(action=>"commitdiff",
-				                       hash=>$co{'parent'})},
+				$cgi->a({-href => href(-replay=>1,
+				                       hash=>$co{'parent'}, hash_base=>undef)},
 				        esc_html(substr($co{'parent'}, 0, 7))) .
 				')';
 		} else {
 			# merge commit
 			if ($hash_parent eq '--cc') {
 				$formats_nav .= ' | ' .
-					$cgi->a({-href => href(action=>"commitdiff",
+					$cgi->a({-href => href(-replay=>1,
 					                       hash=>$hash, hash_parent=>'-c')},
 					        'combined');
 			} else { # $hash_parent eq '-c'
 				$formats_nav .= ' | ' .
-					$cgi->a({-href => href(action=>"commitdiff",
+					$cgi->a({-href => href(-replay=>1,
 					                       hash=>$hash, hash_parent=>'--cc')},
 					        'compact');
 			}
 			$formats_nav .=
 				' (merge: ' .
 				join(' ', map {
-					$cgi->a({-href => href(action=>"commitdiff",
-					                       hash=>$_)},
+					$cgi->a({-href => href(-replay=>1,
+					                       hash=>$_, hash_base=>undef)},
 					        esc_html(substr($_, 0, 7)));
 				} @{$co{'parents'}} ) .
 				')';
-- 
1.7.6

[PATCHv3 8/8] gitweb: Add navigation to select side-by-side diff

[Jakub Narebski]

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

Add to the lower part of navigation bar (the action specific part)
links allowing to switch between 'inline' (ordinary) diff and
'side by side' style diff.

It is not shown for combined / compact combined diff.

Signed-off-by: Kato Kazuyoshi <kato.kazuyoshi@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
This patch was originally part of other commit, but was split into a
separate patch.

Note that "blobdiff" should probably also get this navigation.

 gitweb/gitweb.perl |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index ffaea45..f80f259 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -7164,6 +7164,7 @@ sub git_blobdiff {
 		my $formats_nav =
 			$cgi->a({-href => href(action=>"blobdiff_plain", -replay=>1)},
 			        "raw");
+		$formats_nav .= diff_style_nav($diff_style);
 		git_header_html(undef, $expires);
 		if (defined $hash_base && (my %co = parse_commit($hash_base))) {
 			git_print_page_nav('','', $hash_base,$co{'tree'},$hash_base, $formats_nav);
@@ -7221,6 +7222,27 @@ sub git_blobdiff_plain {
 	git_blobdiff('plain');
 }
 
+# assumes that it is added as later part of already existing navigation,
+# so it returns "| foo | bar" rather than just "foo | bar"
+sub diff_style_nav {
+	my ($diff_style, $is_combined) = @_;
+	$diff_style ||= 'inline';
+
+	return "" if ($is_combined);
+
+	my @styles = (inline => 'inline', 'sidebyside' => 'side by side');
+	my %styles = @styles;
+	@styles =
+		@styles[ map { $_ * 2 } 0..$#styles/2 ];
+
+	return join '',
+		map { " | ".$_ }
+		map {
+			$_ eq $diff_style ? $styles{$_} :
+			$cgi->a({-href => href(-replay=>1, diff_style => $_)}, $styles{$_})
+		} @styles;
+}
+
 sub git_commitdiff {
 	my %params = @_;
 	my $format = $params{-format} || 'html';
@@ -7250,6 +7272,7 @@ sub git_commitdiff {
 				$cgi->a({-href => href(action=>"patch", -replay=>1)},
 					"patch");
 		}
+		$formats_nav .= diff_style_nav($diff_style, @{$co{'parents'}} > 1);
 
 		if (defined $hash_parent &&
 		    $hash_parent ne '-c' && $hash_parent ne '--cc') {
-- 
1.7.6