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

[Jakub Narebski <jnareb@gmail.com>]

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.

> 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 ..."

> 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).

> +}
> +
> +# 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);

>  			@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.

>  			} 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.
-- 
Jakub Narebski
Poland