[PATCH 2/2] gitweb: add a feature to show side-by-side diff

[PATCH 2/2] gitweb: add a feature to show side-by-side diff

[Kato Kazuyoshi]

gitweb currently has a feature to show diff but it doesn't support
"side-by-side" style. This modification introduces:

 * The "ds" query parameter to specify the style of diff.
 * The format_diff_chunk() to reorganize an output of diff.
 * The diff_nav() for form.
---
 gitweb/gitweb.perl       |   81 +++++++++++++++++++++++++++++++++++++++++----
 gitweb/static/gitweb.css |   15 ++++++++
 2 files changed, 88 insertions(+), 8 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 095adda..dca09dc 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -757,6 +757,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"
 );
@@ -1072,6 +1073,8 @@ sub evaluate_and_validate_params {
 		}
 		$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
 	}
+
+	$input_params{diff_style} ||= 'inline';
 }

 # path to the current git repository
@@ -2276,7 +2279,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_open$line</div>\n";
+		return $diff_class, "$div_open$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 +2312,9 @@ sub format_diff_line {
 		}
 		$line .= " $prefix</span>" .
 		         "<span class=\"section\">" . esc_html($section, -nbsp=>1)
. "</span>";
-		return "$div_open$line</div>\n";
+		return $diff_class, "$div_open$line</div>\n";
 	}
-	return $div_open . esc_html($line, -nbsp=>1) . "</div>\n";
+	return $diff_class, $div_open . esc_html($line, -nbsp=>1) . "</div>\n";
 }

 # Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
@@ -4828,8 +4831,32 @@ sub git_difftree_body {
 	print "</table>\n";
 }

+sub format_diff_chunk {
+	my @chunk = @_;
+
+	my $first_class = $chunk[0]->[0];
+	my @partial = map { $_->[1] } grep { $_->[0] eq $first_class } @chunk;
+
+	if (scalar @partial < scalar @chunk) {
+		return join '', ("<div class='chunk'><div class='old'>",
+		             @partial,
+		             "</div>",
+		             "<div class='new'>",
+		             (map {
+		                 $_->[1];
+		             } @chunk[scalar @partial..scalar @chunk-1]),
+		             "</div></div>");
+	} else {
+		return join '', ("<div class='chunk'><div class='",
+		             ($first_class eq 'add' ? 'new' : 'old'),
+		             "'>",
+		             @partial,
+		             "</div></div>");
+	}
+}
+
 sub git_patchset_body {
-	my ($fd, $difftree, $hash, @hash_parents) = @_;
+	my ($fd, $is_inline, $difftree, $hash, @hash_parents) = @_;
 	my ($hash_parent) = $hash_parents[0];

 	my $is_combined = (@hash_parents > 1);
@@ -4940,12 +4967,31 @@ sub git_patchset_body {

 		# the patch itself
 	LINE:
+		my @chunk;
 		while ($patch_line = <$fd>) {
 			chomp $patch_line;

 			next PATCH if ($patch_line =~ m/^diff /);

-			print format_diff_line($patch_line, \%from, \%to);
+			my ($class, $line) = format_diff_line($patch_line, \%from, \%to);
+			if ($is_inline) {
+				print $line;
+			} elsif ($class eq 'add' || $class eq 'rem') {
+				push @chunk, [ $class, $line ];
+			} else {
+				if (@chunk) {
+					print format_diff_chunk(@chunk);
+					@chunk = ();
+				} elsif ($class eq 'chunk_header') {
+					print $line;
+				} else {
+					print '<div class="chunk"><div class="old">',
+					      $line,
+					      '</div><div class="new">',
+					      $line,
+					      '</div></div>';
+				}
+			}
 		}

 	} continue {
@@ -7053,7 +7099,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, $input_params{diff_style} eq 'inline',
+		                  [ \%diffinfo ], $hash_base, $hash_parent_base);
 		close $fd;

 		print "</div>\n"; # class="page_body"
@@ -7078,6 +7125,22 @@ sub git_blobdiff_plain {
 	git_blobdiff('plain');
 }

+sub diff_nav {
+	my ($style) = @_;
+
+	my %pairs = (inline => 'inline', 'sidebyside' => 'side by side');
+	join '', ($cgi->start_form({ method => 'get' }),
+	          $cgi->hidden('p'),
+	          $cgi->hidden('a'),
+	          $cgi->hidden('h'),
+	          $cgi->hidden('hp'),
+	          $cgi->hidden('hb'),
+	          $cgi->hidden('hpb'),
+	          $cgi->popup_menu('ds', [keys %pairs], $style, \%pairs),
+	          $cgi->submit('change'),
+	          $cgi->end_form);
+}
+
 sub git_commitdiff {
 	my %params = @_;
 	my $format = $params{-format} || 'html';
@@ -7230,7 +7293,8 @@ sub git_commitdiff {
 		my $ref = format_ref_marker($refs, $co{'id'});

 		git_header_html(undef, $expires);
-		git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);
+		git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash,
+		                   $formats_nav . diff_nav($input_params{diff_style}));
 		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);
 		print "<div class=\"title_text\">\n" .
 		      "<table class=\"object_header\">\n";
@@ -7284,7 +7348,8 @@ sub git_commitdiff {
 		                  $use_parents ? @{$co{'parents'}} : $hash_parent);
 		print "<br/>\n";

-		git_patchset_body($fd, \@difftree, $hash,
+		git_patchset_body($fd, $input_params{diff_style} eq 'inline',
+		                  \@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..dc84db2 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -618,6 +618,21 @@ div.remote {
 	cursor: pointer;
 }

+/* side-by-side diff */
+div.chunk {
+	overflow: hidden;
+}
+
+div.chunk div.old {
+	float: left;
+	width: 50%;
+	overflow: hidden;
+}
+
+div.chunk div.new {
+	margin-left: 50%;
+	width: 50%;
+}

 /* Style definition generated by highlight 2.4.5, http://www.andre-simon.de/ */

-- 
1.7.7.213.g8b0e1.dirty

Re: [PATCH 2/2] gitweb: add a feature to show side-by-side diff

[Junio C Hamano]

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

> @@ -2276,7 +2279,7 @@ sub format_diff_line {
>  		}
>  		$line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
>  		        "<span class=\"section\">" . esc_html($section, -nbsp=>1) .
> "</span>";

Corrupt patch (wrapped long line); please fix.

I suspect you have similar issues in the [PATCH 1/2] message, too.

Re: [PATCH 2/2] gitweb: add a feature to show side-by-side diff

[Jakub Narebski]

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

> gitweb currently has a feature to show diff but it doesn't support
> "side-by-side" style. This modification introduces:
> 
>  * The "ds" query parameter to specify the style of diff.
>  * The format_diff_chunk() to reorganize an output of diff.
>  * The diff_nav() for form.

I would state it a bit differently.

I would say that this patch introduces support for side-by-side diff
in git_patchset_body, and that style of diff is controlled by newly
introduces 'diff_style' ("ds") parameter.

I would say a bit later that navigation bar got extended to make it
easy to switch between 'inline' and 'sidebyside' diff.

> ---
>  gitweb/gitweb.perl       |   81 +++++++++++++++++++++++++++++++++++++++++----
>  gitweb/static/gitweb.css |   15 ++++++++
>  2 files changed, 88 insertions(+), 8 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 095adda..dca09dc 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -757,6 +757,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"
>  );

O.K.

> @@ -1072,6 +1073,8 @@ sub evaluate_and_validate_params {
>  		}
>  		$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
>  	}
> +
> +	$input_params{diff_style} ||= 'inline';
>  }

O.K.
 
>  # path to the current git repository
> @@ -2276,7 +2279,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_open$line</div>\n";
> +		return $diff_class, "$div_open$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);

If you are changing behavior of format_diff_line, by having it return
<class>, <formatted line> line pair, I think you should document it,
and change the name of function.

All format_* functions in gitweb return a single string, so that

  print format_*(...);

works.

> @@ -4828,8 +4831,32 @@ sub git_difftree_body {
>  	print "</table>\n";
>  }
> 

It would be good idea to add some comment about this subroutine,
e.g. what parameters it accepts.  And perhaps how it works, as it is
not obvious.

> +sub format_diff_chunk {
> +	my @chunk = @_;
> +
> +	my $first_class = $chunk[0]->[0];
> +	my @partial = map { $_->[1] } grep { $_->[0] eq $first_class } @chunk;
> +
> +	if (scalar @partial < scalar @chunk) {
> +		return join '', ("<div class='chunk'><div class='old'>",
> +		             @partial,
> +		             "</div>",
> +		             "<div class='new'>",
> +		             (map {
> +		                 $_->[1];
> +		             } @chunk[scalar @partial..scalar @chunk-1]),
> +		             "</div></div>");
> +	} else {
> +		return join '', ("<div class='chunk'><div class='",
> +		             ($first_class eq 'add' ? 'new' : 'old'),
> +		             "'>",
> +		             @partial,
> +		             "</div></div>");
> +	}
> +}
> +
>  sub git_patchset_body {
> -	my ($fd, $difftree, $hash, @hash_parents) = @_;
> +	my ($fd, $is_inline, $difftree, $hash, @hash_parents) = @_;

Hmmm... shouldn't changing git_patchset_body signature (calling
convention) be mentioned in commit message?

>  	my ($hash_parent) = $hash_parents[0];
> 
>  	my $is_combined = (@hash_parents > 1);
> @@ -4940,12 +4967,31 @@ sub git_patchset_body {
> 
>  		# the patch itself
>  	LINE:
> +		my @chunk;
>  		while ($patch_line = <$fd>) {
>  			chomp $patch_line;
> 
>  			next PATCH if ($patch_line =~ m/^diff /);
> 
> -			print format_diff_line($patch_line, \%from, \%to);
> +			my ($class, $line) = format_diff_line($patch_line, \%from, \%to);
> +			if ($is_inline) {
> +				print $line;
> +			} elsif ($class eq 'add' || $class eq 'rem') {
> +				push @chunk, [ $class, $line ];
> +			} else {
> +				if (@chunk) {
> +					print format_diff_chunk(@chunk);
> +					@chunk = ();
> +				} elsif ($class eq 'chunk_header') {
> +					print $line;
> +				} else {
> +					print '<div class="chunk"><div class="old">',
> +					      $line,
> +					      '</div><div class="new">',
> +					      $line,
> +					      '</div></div>';
> +				}
> +			}

I wonder why you have output of context lines in side-by-side diff in
git_patchset_body, instead of having it all in format_diff_chunk.

Unless by chunk you mean something different than "unified format hunk"
that is defined e.g. in (diff.info)"Detailed Unified".

>  		}
> 
>  	} continue {
> @@ -7053,7 +7099,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, $input_params{diff_style} eq 'inline',
> +		                  [ \%diffinfo ], $hash_base, $hash_parent_base);
>  		close $fd;
> 
>  		print "</div>\n"; # class="page_body"
> @@ -7078,6 +7125,22 @@ sub git_blobdiff_plain {
>  	git_blobdiff('plain');
>  }
> 
> +sub diff_nav {
> +	my ($style) = @_;
> +
> +	my %pairs = (inline => 'inline', 'sidebyside' => 'side by side');
> +	join '', ($cgi->start_form({ method => 'get' }),
> +	          $cgi->hidden('p'),
> +	          $cgi->hidden('a'),
> +	          $cgi->hidden('h'),
> +	          $cgi->hidden('hp'),
> +	          $cgi->hidden('hb'),
> +	          $cgi->hidden('hpb'),
> +	          $cgi->popup_menu('ds', [keys %pairs], $style, \%pairs),
> +	          $cgi->submit('change'),
> +	          $cgi->end_form);
> +}

Why do you feel the need for form to select between diff formats,
instead of links switching between them, like for interactive and
non-interactive blame?

> +
>  sub git_commitdiff {
>  	my %params = @_;
>  	my $format = $params{-format} || 'html';
> @@ -7230,7 +7293,8 @@ sub git_commitdiff {
>  		my $ref = format_ref_marker($refs, $co{'id'});
> 
>  		git_header_html(undef, $expires);
> -		git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);
> +		git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash,
> +		                   $formats_nav . diff_nav($input_params{diff_style}));
>  		git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);
>  		print "<div class=\"title_text\">\n" .
>  		      "<table class=\"object_header\">\n";
> @@ -7284,7 +7348,8 @@ sub git_commitdiff {
>  		                  $use_parents ? @{$co{'parents'}} : $hash_parent);
>  		print "<br/>\n";
> 
> -		git_patchset_body($fd, \@difftree, $hash,
> +		git_patchset_body($fd, $input_params{diff_style} eq 'inline',
> +		                  \@difftree, $hash,
>  		                  $use_parents ? @{$co{'parents'}} : $hash_parent);
>  		close $fd;
>  		print "</div>\n"; # class="page_body"

Only commitdiff?  What about blobdiff?

> diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
> index 7d88509..dc84db2 100644
> --- a/gitweb/static/gitweb.css
> +++ b/gitweb/static/gitweb.css
> @@ -618,6 +618,21 @@ div.remote {
>  	cursor: pointer;
>  }
> 
> +/* side-by-side diff */
> +div.chunk {
> +	overflow: hidden;

???

> +}
> +
> +div.chunk div.old {
> +	float: left;
> +	width: 50%;
> +	overflow: hidden;
> +}
> +
> +div.chunk div.new {
> +	margin-left: 50%;
> +	width: 50%;
> +}

Hmmm... I think the way side-by-side diff is styled should be
explained in commit message, or in comments.

I also wonder if it wouldn't be simpler to use table here, instead of
fiddling with floats, widths and margins.

-- 
Jakub Narębski

Re: [PATCH 2/2] gitweb: add a feature to show side-by-side diff

[Jakub Narebski]

Jakub Narebski <jnareb@gmail.com> writes:

> Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
> 
> > gitweb currently has a feature to show diff but it doesn't support
> > "side-by-side" style. This modification introduces:
> > 
> >  * The "ds" query parameter to specify the style of diff.
> >  * The format_diff_chunk() to reorganize an output of diff.
> >  * The diff_nav() for form.
> 
> I would state it a bit differently.
> 
> I would say that this patch introduces support for side-by-side diff
> in git_patchset_body, and that style of diff is controlled by newly
> introduces 'diff_style' ("ds") parameter.
> 
> I would say a bit later that navigation bar got extended to make it
> easy to switch between 'inline' and 'sidebyside' diff.

I think it would be good idea to explain algorithm here, and perhaps
also layout used.

When I was thinking about implementing side-by-side diff in gitweb, I
was always stopped by the problem of aligning changes.  In your
solution changes are always aligned to top, which is a simple
solution.

> > +sub format_diff_chunk {
> > +	my @chunk = @_;
> > +
> > +	my $first_class = $chunk[0]->[0];
> > +	my @partial = map { $_->[1] } grep { $_->[0] eq $first_class } @chunk;
> > +
> > +	if (scalar @partial < scalar @chunk) {
> > +		return join '', ("<div class='chunk'><div class='old'>",
> > +		             @partial,
> > +		             "</div>",
> > +		             "<div class='new'>",
> > +		             (map {
> > +		                 $_->[1];
> > +		             } @chunk[scalar @partial..scalar @chunk-1]),
> > +		             "</div></div>");
> > +	} else {
> > +		return join '', ("<div class='chunk'><div class='",
> > +		             ($first_class eq 'add' ? 'new' : 'old'),
> > +		             "'>",
> > +		             @partial,
> > +		             "</div></div>");
> > +	}
> > +}

This is I think unnecessary complicated.  What you do here is
separating removed and added lines (either can be empty), and putting
removed on left (as "old"), and added on right (as "new").

It means that the following unified diff:

     --- a/foo
     +++ b/foo
     @@ -1,5 +1,4 @@
     -foo
     -FOO
      bar
     -baz
     +b
     +baZ
      quux

would be turned into following side by side diff:

      foo             | 
      FOO             |
      bar             |  bar
      baz             |  b
                      |  baZ
      quux            |  quux

It's a bit strange that context is put line by line, and changed lines
are put in "blocks".

> > @@ -4940,12 +4967,31 @@ sub git_patchset_body {
> > 
> >  		# the patch itself
> >  	LINE:
> > +		my @chunk;
> >  		while ($patch_line = <$fd>) {
> >  			chomp $patch_line;
> > 
> >  			next PATCH if ($patch_line =~ m/^diff /);
> > 
> > -			print format_diff_line($patch_line, \%from, \%to);
> > +			my ($class, $line) = format_diff_line($patch_line, \%from, \%to);
> > +			if ($is_inline) {
> > +				print $line;
> > +			} elsif ($class eq 'add' || $class eq 'rem') {
> > +				push @chunk, [ $class, $line ];
> > +			} else {
> > +				if (@chunk) {
> > +					print format_diff_chunk(@chunk);
> > +					@chunk = ();
> > +				} elsif ($class eq 'chunk_header') {
> > +					print $line;
> > +				} else {
> > +					print '<div class="chunk"><div class="old">',
> > +					      $line,
> > +					      '</div><div class="new">',
> > +					      $line,
> > +					      '</div></div>';
> > +				}
> > +			}

Note that your side by side diff wouldn't work for combined diff,
which gitweb supports.  You should show 'unified' / 'inline' format
for combined diff (more than one parent / from).

-- 
Jakub Narębski

Re: [PATCH 2/2] gitweb: add a feature to show side-by-side diff

[Jakub Narebski]

Those are additional comments, most of which I have come about when
rewriting this series and testing it.

There are to complement existing comments in other post(s).

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

> ---
>  gitweb/gitweb.perl       |   81 +++++++++++++++++++++++++++++++++++++++++----
>  gitweb/static/gitweb.css |   15 ++++++++
>  2 files changed, 88 insertions(+), 8 deletions(-)

No tests.
 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 095adda..dca09dc 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -757,6 +757,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"
>  );

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

So currently I favor neither.

> @@ -1072,6 +1073,8 @@ sub evaluate_and_validate_params {
>  		}
>  		$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
>  	}
> +
> +	$input_params{diff_style} ||= 'inline';
>  }

Hmmm... similar option 'order' ("o") had default value set in action
subroutine.  I wonder if it wouldn't be better to do the same also in
this situation.

> @@ -2276,7 +2279,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_open$line</div>\n";
> +		return $diff_class, "$div_open$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);

This subroutine no longer *format* line to be printed, isn't it?

> @@ -4828,8 +4831,32 @@ sub git_difftree_body {
>  	print "</table>\n";
>  }
> 
> +sub format_diff_chunk {

Name: it is not about diff chunk (or hunk), but about a block of lines
in a chunk; in this case block of change lines (rem / add).  Also, it
is not about generic diff, only about sidebyside one.

BTW. I think it would be better if this subroutine also managed
context lines.

> +	my @chunk = @_;
> +
> +	my $first_class = $chunk[0]->[0];

Style: You can use simply $chunk[0][0] here.  perlref(1) says:

  "The arrow is optional between brackets subscripts, [...]" 

> +	my @partial = map { $_->[1] } grep { $_->[0] eq $first_class } @chunk;
> +
> +	if (scalar @partial < scalar @chunk) {

Style: you can write simply

  +	if (@partial < @chunk) {

> +		return join '', ("<div class='chunk'><div class='old'>",
> +		             @partial,
> +		             "</div>",
> +		             "<div class='new'>",
> +		             (map {
> +		                 $_->[1];
> +		             } @chunk[scalar @partial..scalar @chunk-1]),
> +		             "</div></div>");
> +	} else {
> +		return join '', ("<div class='chunk'><div class='",
> +		             ($first_class eq 'add' ? 'new' : 'old'),
> +		             "'>",
> +		             @partial,
> +		             "</div></div>");
> +	}
> +}

Anyway this code is not very clear.  You rely on the fact that if
there are two classes, then they are "rem" first, and "add" second.

Also, it is I think overly complicated.

> +
>  sub git_patchset_body {
> -	my ($fd, $difftree, $hash, @hash_parents) = @_;
> +	my ($fd, $is_inline, $difftree, $hash, @hash_parents) = @_;

Rather than passing $is_inline, I think it would be better to pass
$diff_style (with default value filled in)

>  	my ($hash_parent) = $hash_parents[0];
> 
>  	my $is_combined = (@hash_parents > 1);
> @@ -4940,12 +4967,31 @@ sub git_patchset_body {
> 
>  		# the patch itself
>  	LINE:
> +		my @chunk;
>  		while ($patch_line = <$fd>) {
>  			chomp $patch_line;
> 
>  			next PATCH if ($patch_line =~ m/^diff /);

Here is a bug.  If patchset consists of more than one patch, if
not-last patches have change that does not have trailing context lines
(changed, added or removed lines at the end of file), then the last
block will be lost (@chunk can be non-empty here).

> 
> -			print format_diff_line($patch_line, \%from, \%to);
> +			my ($class, $line) = format_diff_line($patch_line, \%from, \%to);
> +			if ($is_inline) {

That is wrong to test for.  You should test if you can use
side-by-side diff, not if you use default output.  

Especially that diff can be combined diff of a merge commit, which
cannot be represented as 2-sided side-by-side diff; for such diff
gitweb needs to use inline diff.

> +				print $line;
> +			} elsif ($class eq 'add' || $class eq 'rem') {
> +				push @chunk, [ $class, $line ];
> +			} else {
> +				if (@chunk) {
> +					print format_diff_chunk(@chunk);
> +					@chunk = ();
> +				} elsif ($class eq 'chunk_header') {
> +					print $line;
> +				} else {
> +					print '<div class="chunk"><div class="old">',
> +					      $line,
> +					      '</div><div class="new">',
> +					      $line,
> +					      '</div></div>';

All of this should in my opinion be done in format_diff_chunk(), not
in caller.  This also introduces a bit of inconsistency in that
added/removed lines are in single block and context lines are each in
its own block.

Additionally you forgot about incomplete lines here, which can apply
either to added lines, removed lines, both of added and removed lines,
and to context lines.  Your code generates incorrect info in the case
if incomplete line is either removed line only, or added line only.

[Nb. I have to check my code yet again.]

> +sub diff_nav {
> +	my ($style) = @_;
> +
> +	my %pairs = (inline => 'inline', 'sidebyside' => 'side by side');
> +	join '', ($cgi->start_form({ method => 'get' }),
> +	          $cgi->hidden('p'),
> +	          $cgi->hidden('a'),
> +	          $cgi->hidden('h'),
> +	          $cgi->hidden('hp'),
> +	          $cgi->hidden('hb'),
> +	          $cgi->hidden('hpb'),
> +	          $cgi->popup_menu('ds', [keys %pairs], $style, \%pairs),
> +	          $cgi->submit('change'),
> +	          $cgi->end_form);
> +}

What about 'f' and 'fp' for "blobdiff" view?

> diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
> index 7d88509..dc84db2 100644
> --- a/gitweb/static/gitweb.css
> +++ b/gitweb/static/gitweb.css
> @@ -618,6 +618,21 @@ div.remote {
>  	cursor: pointer;
>  }
> 
> +/* side-by-side diff */
> +div.chunk {
> +	overflow: hidden;
> +}
> +
> +div.chunk div.old {
> +	float: left;
> +	width: 50%;
> +	overflow: hidden;
> +}
> +
> +div.chunk div.new {
> +	margin-left: 50%;
> +	width: 50%;
> +}

Nice trick of composing CSS layout... though I wonder if there is
perhaps a better solution.

Anyway, I think this addition should be put near style for div.diff
etc.

-- 
Jakub Narębski