threads / patch / 28700

patch, 2 partsgitweb: add a feature to show side-by-side diff

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

## tl;dr

5 messages between Oct 17, 2011 and Oct 30, 2011. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Kato Kazuyoshi· Oct 17, 2011, 07:00 UTC · lore

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(-)
Show changes to 2 files +88 −8

gitweb/gitweb.perl, gitweb/static/gitweb.css

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
Junio C Hamano· Oct 17, 2011, 19:37 UTC · re: Kato Kazuyoshi · lore

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

Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
Show 5 quoted lines
> @@ -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.
Jakub Narebski· Oct 17, 2011, 21:24 UTC · re: Kato Kazuyoshi · lore

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

Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
Show 6 quoted lines
> 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.

Show 17 quoted lines
> ---
>  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.
Show 7 quoted lines
> @@ -1072,6 +1073,8 @@ sub evaluate_and_validate_params {
>  		}
>  		$search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
>  	}
> +
> +	$input_params{diff_style} ||= 'inline';
>  }
O.K.
 
Show 11 quoted lines
>  # 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.

Show 27 quoted lines
> +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?

Show 33 quoted lines
>  	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".

Show 32 quoted lines
>  		}
> 
>  	} 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?

Show 24 quoted lines
> +
>  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?
Show 11 quoted lines
> 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;
???
Show 12 quoted lines
> +}
> +
> +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
Jakub Narebski· Oct 18, 2011, 10:36 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:
Show 17 quoted lines
> 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.

Show 23 quoted lines
> > +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".

Show 30 quoted lines
> > @@ -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
Jakub Narebski· Oct 30, 2011, 18:56 UTC · re: Kato Kazuyoshi · lore

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

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.
 
Show 12 quoted lines
> 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.
Show 7 quoted lines
> @@ -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.

Show 10 quoted lines
> @@ -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?
Show 5 quoted lines
> @@ -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) {
Show 16 quoted lines
> +		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)

Show 12 quoted lines
>  	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.

Show 15 quoted lines
> +				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.]
Show 15 quoted lines
> +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?
Show 23 quoted lines
> 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

← back to recent threads