Re: [PATCH v2 4/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:

> On Fri, 23 Mar 2012, Michał Kiedrowicz wrote:
> 
> > 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, which is
> > needed for diff refinement highlightning (implemented in incoming
> > patches).
> > 
> While I don't like that we use accumulator for something that was 
> straightforwardly printed, I understand that it is necessary preprocessing
> required for diff refinement highlighting.  Having refactoring upfront
> is a good idea, making for easier review.

Yeah, this change doesn't make sense on its own but is required by later
patch that needs whole chunk at once.

> 
> > If left as is, the new function print_inline_diff_lines() could reorder
> 
> I think the previous paragraph is long enough that it would be better
> to repeat subject of this sentence, i.e. start with
> 
>  "If print_diff_chunk() was left as is, the new function ..."

OK.

> 
> > diff lines.  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 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
> > 
> > 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.
> > 
> O.K.
> 
> > Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> > ---
> [...]
> 
> > +# 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);
> 
> Why not simply
> 
>   +	print @$ctx, @$rem, @$add;
> 
> It is "print LIST" for a reason (and $\ is empty here in gitweb, as it
> is empty by default).
> 

OK, that's much better.  I really didn't like those 3 prints.

> > +}
> > +
> > +# 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);
> > +	}
> > +}
> 
> Very nice subroutine!
> 
> > +
> > +sub print_diff_chunk {
> > +	my ($diff_style, $is_combined, @chunk) = @_;
> >  	my (@ctx, @rem, @add);
> >  
> > +	# The class of the previous line. 
> > +	my $prev_class = '';
> > +
> >  	return unless @chunk;
> >  
> >  	# incomplete last line might be among removed or added lines,
> > @@ -5072,9 +5096,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)) {
> > +			print_diff_lines(\@ctx, \@rem, \@add, $diff_style,
> > +					$is_combined);
> 
> Nitpick: the following _might_ be a tiny little bit more readable:
> 
>   +			print_diff_lines(\@ctx, \@rem, \@add,
>   +			                 $diff_style, $is_combined);
> 

OK.

> >  			@ctx = @rem = @add = ();
> >  		}
> >  
> > @@ -5091,6 +5119,8 @@ sub print_sidebyside_diff_chunk {
> >  		if ($class eq 'ctx') {
> >  			push @ctx, $line;
> >  		}
> > +
> > +		$prev_class = $class;
> >  	}
> >  }
> 
> Anyway nice change.
>   
> > @@ -5217,22 +5247,17 @@ sub git_patchset_body {
> >  			$diff_classes .= " $class" if ($class);
> >  			$line = "<div class=\"$diff_classes\">$line</div>\n";
> >  
> > -			if ($diff_style eq 'sidebyside' && !$is_combined) {
> > -				if ($class eq 'chunk_header') {
> > -					print_sidebyside_diff_chunk(@chunk);
> > -					@chunk = ( [ $class, $line ] );
> > -				} else {
> > -					push @chunk, [ $class, $line ];
> > -				}
> > +			if ($class eq 'chunk_header') {
> > +				print_diff_chunk($diff_style, $is_combined, @chunk);
> 
> Nice, pushing acting on $diff_style down to print_diff_chunk(), which
> simplifies code a bit.
> 
> > +				@chunk = ( [ $class, $line ] );
> 
> BTW. this could be simplified to
> 
>   +				@chunk = ();
>   +				push @chunk, [ $class, $line ];
> 
> Well, simplified after noticing the common part of those two branches
> of a conditional.
> 
> But it really doesn't matter.

Sounds sensible.

> 
> >  			} else {
> > -				# default 'inline' style and unknown styles
> > -				print $line;
> > +				push @chunk, [ $class, $line ];
> >  			}
> >  		}
> >  
> >  	} continue {
> >  		if (@chunk) {
> > -			print_sidebyside_diff_chunk(@chunk);
> > +			print_diff_chunk($diff_style, $is_combined, @chunk);
> >  			@chunk = ();
> >  		}
> >  		print "</div>\n"; # class="patch"
> > -- 
> 
> BTW. I was wondering about binary files, but this commit wouldn't make
> it worse anyway as we parse diff output assuming unified-like diff for
> diff syntax highlighting... and binary diffs are shown as
> 
>   "Binary files a/foo.png and b/foo.png differ"
> 
> in extended diff header.

Yeah, this is what I wrote in the cover letter:

	* Ensured that binary patches are not supported in HTML format,
	  thus this series canot break it :) (answering Jakub's questions)

but maybe I wasn't clear enough.