Merge branch 'jn/gitweb-side-by-side-diff'

* jn/gitweb-side-by-side-diff:
  gitweb: Add navigation to select side-by-side diff
  gitweb: Use href(-replay=>1,...) for formats links in "commitdiff"
  t9500: Add basic sanity tests for side-by-side diff in gitweb
  t9500: Add test for handling incomplete lines in diff by gitweb
  gitweb: Give side-by-side diff extra CSS styling
  gitweb: Add a feature to show side-by-side diff
  gitweb: Extract formatting of diff chunk header
  gitweb: Refactor diff body line classification

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"
 );
@@ -2225,41 +2226,40 @@ sub format_diff_cc_simplified {
 	return $result;
 }
 
-# format patch (diff) line (not to be used for diff headers)
+sub diff_line_class {
-sub format_diff_line {
+	my ($line, $from, $to) = @_;
-	my $line = shift;
-	my ($from, $to) = @_;
-	my $diff_class = "";
 
-	chomp $line;
+	# ordinary diff
-
+	my $num_sign = 1;
-	if ($from && $to && ref($from->{'href'}) eq "ARRAY") {
 	# combined diff
-		my $prefix = substr($line, 0, scalar @{$from->{'href'}});
+	if ($from && $to && ref($from->{'href'}) eq "ARRAY") {
-		if ($line =~ m/^\@{3}/) {
+		$num_sign = scalar @{$from->{'href'}};
-			$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 @diff_line_classifier = (
-		my $char = substr($line, 0, 1);
+		{ regexp => qr/^\@\@{$num_sign} /, class => "chunk_header"},
-		if ($char eq '+') {
+		{ regexp => qr/^\\/,               class => "incomplete"  },
-			$diff_class = " add";
+		{ regexp => qr/^ {$num_sign}/,     class => "ctx" },
-		} elsif ($char eq '-') {
+		# classifier for context must come before classifier add/rem,
-			$diff_class = " rem";
+		# or we would have to use more complicated regexp, for example
-		} elsif ($char eq '@') {
+		# qr/(?= {0,$m}\+)[+ ]{$num_sign}/, where $m = $num_sign - 1;
-			$diff_class = " chunk_header";
+		{ regexp => qr/^[+ ]{$num_sign}/,   class => "add" },
-		} elsif ($char eq "\\") {
+		{ regexp => qr/^[- ]{$num_sign}/,   class => "rem" },
-			$diff_class = " incomplete";
+	);
+	for my $clsfy (@diff_line_classifier) {
+		return $clsfy->{'class'}
+			if ($line =~ $clsfy->{'regexp'});
 	}
-	}
+
-	$line = untabify($line);
+	# fallback
-	if ($from && $to && $line =~ m/^\@{2} /) {
+	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}(.*)$/;
 
@@ -2276,8 +2276,14 @@ 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 $line;
-	} elsif ($from && $to && $line =~ m/^\@{3}/) {
+}
+
+# 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);
 
@@ -2309,9 +2315,30 @@ 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 $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);
+
+	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;
+
 	}
-	return "<div class=\"diff$diff_class\">" . 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_)",
@@ -4833,8 +4860,97 @@ 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 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 = ();
+		}
+
+		## 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);
@@ -4844,6 +4960,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";
 
@@ -4950,10 +5067,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"
 	}
 
@@ -6949,6 +7085,7 @@ sub git_object {
 
 sub git_blobdiff {
 	my $format = shift || 'html';
+	my $diff_style = $input_params{'diff_style'} || 'inline';
 
 	my $fd;
 	my @difftree;
@@ -7027,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);
@@ -7058,7 +7196,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"
@@ -7083,9 +7222,31 @@ 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';
+	my $diff_style = $input_params{'diff_style'} || 'inline';
 
 	my ($patch_max) = gitweb_get_feature('patches');
 	if ($format eq 'patch') {
@@ -7111,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') {
@@ -7128,8 +7290,8 @@ sub git_commitdiff {
 				}
 			}
 			$formats_nav .= ': ' .
-				$cgi->a({-href => href(action=>"commitdiff",
+				$cgi->a({-href => href(-replay=>1,
-				                       hash=>$hash_parent)},
+				                       hash=>$hash_parent, hash_base=>undef)},
 				        esc_html($hash_parent_short)) .
 				')';
 		} elsif (!$co{'parent'}) {
@@ -7139,28 +7301,28 @@ sub git_commitdiff {
 			# single parent commit
 			$formats_nav .=
 				' (parent: ' .
-				$cgi->a({-href => href(action=>"commitdiff",
+				$cgi->a({-href => href(-replay=>1,
-				                       hash=>$co{'parent'})},
+				                       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",
+					$cgi->a({-href => href(-replay=>1,
-					                       hash=>$_)},
+					                       hash=>$_, hash_base=>undef)},
 					        esc_html(substr($_, 0, 7)));
 				} @{$co{'parents'}} ) .
 				')';
@@ -7289,7 +7451,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"

gitweb/static/gitweb.css

@@ -475,6 +475,36 @@ 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.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;
 	border-width: 0px 0px 1px;

t/t9500-gitweb-standalone-no-errors.sh

@@ -273,6 +273,53 @@ test_expect_success \
 	'commitdiff(2): directory becomes symlink' \
 	'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 \
@@ -282,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' \
@@ -331,6 +379,29 @@ test_expect_success \
 	'commitdiff(1): large commit' \
 	'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