Re: [PATCH v2 4/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Mar 28, 2012, 15:56 UTC
- Message-ID
- <201203281756.42653.jnareb@gmail.com>
- In-Reply-To
- <1332543417-19664-5-git-send-email-michal.kiedrowicz@gmail.com>
On Fri, 23 Mar 2012, Michał Kiedrowicz wrote:
Show 7 quoted lines
> 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 ..."
Show 21 quoted lines
> 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> > ---
[...]
Show 7 quoted lines
> +# 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).
Show 13 quoted lines
> +}
> +
> +# 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!
Show 25 quoted lines
> +
> +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);
Show 11 quoted lines
> @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.
Show 13 quoted lines
> @@ -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.
Show 15 quoted lines
> } 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