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

[Jakub Narebski <jnareb@gmail.com>]

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