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

From: Jakub Narebski <jnareb@gmail.com>
Date: Oct 30, 2011, 18:56 UTC
Message-ID: <m3sjma48ce.fsf@localhost.localdomain>
In-Reply-To: <CAFo4x0L4BAKnCDa1uEK0Rskd9kTsR-94D4mkYKnLGqVDnuyuBA@mail.gmail.com>

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