Jakub Narebski <jnareb@xxxxxxxxx> 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@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).
>
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.
--
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