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

From: Jakub Narebski <jnareb@gmail.com>
Date: Oct 18, 2011, 10:36 UTC
Message-ID: <m3zkgy6103.fsf@localhost.localdomain>
In-Reply-To: <m34nz771mj.fsf@localhost.localdomain>

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