Re: [PATCH 2/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs

["Michał Kiedrowicz" <michal.kiedrowicz@gmail.com>]

Jakub Narebski <jnareb@gmail.com> wrote:

> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> 
> > This renames print_sidebyside_diff_chunk() to print_diff_chunk() and
> > makes use of it for both side-by-side and inline diffs. Now diff lines
> > are always accumulated before they are printed. This opens the
> > possibility to preprocess diff output before it's printed.
> > 
> > The new function print_inline_diff_lines() could reorder diff lines.
> 
> I think you meant here "if left as is", isn't it?

Yes.

> > It first prints all context lines, then all removed lines and
> > finally all added lines. If the diff output consisted of mixed added
> > and removed lines, gitweb would reorder these lines. This is
> > especially true for combined diff output, for example:
> > 
> > 	 - removed line for first parent
> > 	 + added line for first parent
> > 	  -removed line for second parent
> > 	 ++added line for both parents
> > 
> > would be rendered as:
> > 
> > 	- removed line for first parent
> > 	 -removed line for second parent
> > 	+ added line for first parent
> > 	++added line for both parents
> 
> This is not "is especially true"; it can happen (though: can it
> really?) 

Yeah. See
http://git.kernel.org/?p=git/git.git;a=commitdiff;h=5de89d3abfca98b0dfd0280d28576940c913d60d 


> in combined diff output; in ordinary diff you would always
> have context or end/beginning of chunk between added and removed
> lines.

OK, I'll reword this part of the commit message.
> 
> > 
> > To prevent gitweb from reordering lines, print_diff_chunk() calls
> > print_diff_lines() as soon as it detects that both added and removed
> > lines are present and there was a class change.
> 
> You really should mention, in my opinion, that this change is
> preparation for diff refinement highlighting (higlighting the
> differing segments).
> 
> Otherwise I don't see the reason for it: ordinary diff didn't need the
> fancy stuff with accumulating then printing, and could be send
> straightly to output.  This adds complexity for non-sidebyside diff,
> unnecessary if not for diff refinement highlighting.

I completely agree with that. This patch doesn't makes sense without
later patches.

>  
> > Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> > ---
> >  gitweb/gitweb.perl |   53 +++++++++++++++++++++++++++++++++++++--------------
> >  1 files changed, 38 insertions(+), 15 deletions(-)
> > 
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 1247607..ed63261 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -4908,9 +4908,31 @@ sub print_sidebyside_diff_lines {
> >  	}
> >  }
> >  
> > -sub print_sidebyside_diff_chunk {
> > -	my @chunk = @_;
> > +# Print context lines and then rem/add lines in inline manner.
> > +sub print_inline_diff_lines {
> > +	my ($ctx, $rem, $add) = @_;
> > +
> > +	print foreach (@$ctx);
> > +	print foreach (@$rem);
> > +	print foreach (@$add);
> > +}
> > +
> > +# Print context lines and then rem/add lines.
> > +sub print_diff_lines {
> > +	my ($ctx, $rem, $add, $diff_style, $is_combined) = @_;
> > +
> > +	if ($diff_style eq 'sidebyside' && !$is_combined) {
> > +		print_sidebyside_diff_lines($ctx, $rem, $add);
> > +	} else {
> > +		# default 'inline' style and unknown styles
> > +		print_inline_diff_lines($ctx, $rem, $add);
> > +	}
> > +}
> 
> That looks nice.
> 
> BTW. I didn't examine the final code, but what happens for binary
> diffs that git supports?  Is it handled outside print_diff_chunk()?

Good question. Will check.

> 
> > +
> > +sub print_diff_chunk {
> > +	my ($diff_style, $is_combined, @chunk) = @_;
> >  	my (@ctx, @rem, @add);
> > +	my $prev_class = '';
> 
> Is $prev_class here class of previous chunk fragment or _previous class_
> (i.e. always different from $class), or is it class of previous line?
> It is not obvious here, from variable name.  Better name or at least
> a comment would be appreciated.
> 

It's a class of previous line.

> >  
> >  	return unless @chunk;
> >  
> > @@ -4935,9 +4957,13 @@ sub print_sidebyside_diff_chunk {
> >  		}
> >  
> >  		## print from accumulator when have some add/rem lines or end
> > -		# of chunk (flush context lines)
> > -		if (((@rem || @add) && $class eq 'ctx') || !$class) {
> > -			print_sidebyside_diff_lines(\@ctx, \@rem, \@add);
> > +		# of chunk (flush context lines), or when have add and rem
> > +		# lines and new block is reached (otherwise add/rem lines could
> > +		# be reordered)
> > +		if (((@rem || @add) && $class eq 'ctx') || !$class ||
> > +				(@rem && @add && $class ne $prev_class)) {
> 
> We usually use tabs to align, but spaces to indent in gitweb, so it
> would be
> 
> > +		if (((@rem || @add) && $class eq 'ctx') || !$class ||
> > +		    (@rem && @add && $class ne $prev_class)) {
> 

OK.

> Anyway, this conditional gets ridicously complicated.  I wonder if we
> could simplify it.
> 
> If we allow printing context together widh added/removed lines, and
> not as soon as possible, perhaps good conditional would be: class
> changed, and accumulator for current class is full:
> 
> > +		if ($class ne $prev_class && @{ $acc{$class} }) {
> 
> Or something like that, where
> 
>         my %acc = (ctx => \@ctx, add => \@add, rem => \@rem);
> 
> If we want to print context as soon as possible, like it was before
> 1st patch in this series, we could uuse the following conditional
> 
> > +		if ($class ne $prev_class && 
> > +                 ($prev_class eq 'ctx' || @{ $acc{$class} })) {
> 
> Perhaps with "scalar @{ $acc{$class} }" if it would make things more
> clear.
> 
> Though probably to avoid "Use of uninitialized value in string ne"
> we would probably have to use empty string instead of undefined
> value for "no class" case, i.e. beginning or end of chunk.
>

OK, I'll try to work out something.
 
> > +			print_diff_lines(\@ctx, \@rem, \@add, $diff_style,
> > +					$is_combined);
> >  			@ctx = @rem = @add = ();
> >  		}
> >  
> > @@ -4954,6 +4980,8 @@ sub print_sidebyside_diff_chunk {
> >  		if ($class eq 'ctx') {
> >  			push @ctx, $line;
> >  		}
> > +
> > +		$prev_class = $class;
> >  	}
> >  }
> [...]  
>