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@xxxxxxxxx>
> ---
[...]
> +# 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html