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

[Jakub Narebski <jnareb@gmail.com>]

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