Jakub Narebski <jnareb@xxxxxxxxx> wrote: > Michał Kiedrowicz <michal.kiedrowicz@xxxxxxxxx> writes: > > > 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. > > > > The new function print_inline_diff_lines() could reorder diff lines. > > I think you meant here "if left as is", isn't it? Yes. > > 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 > > especially 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 > > This is not "is especially true"; it can happen (though: can it > really?) Yeah. See http://git.kernel.org/?p=git/git.git;a=commitdiff;h=5de89d3abfca98b0dfd0280d28576940c913d60d > in combined diff output; in ordinary diff you would always > have context or end/beginning of chunk between added and removed > lines. OK, I'll reword this part of the commit message. > > > > > 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. > > You really should mention, in my opinion, that this change is > preparation for diff refinement highlighting (higlighting the > differing segments). > > Otherwise I don't see the reason for it: ordinary diff didn't need the > fancy stuff with accumulating then printing, and could be send > straightly to output. This adds complexity for non-sidebyside diff, > unnecessary if not for diff refinement highlighting. I completely agree with that. This patch doesn't makes sense without later patches. > > > Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@xxxxxxxxx> > > --- > > gitweb/gitweb.perl | 53 +++++++++++++++++++++++++++++++++++++-------------- > > 1 files changed, 38 insertions(+), 15 deletions(-) > > > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > > index 1247607..ed63261 100755 > > --- a/gitweb/gitweb.perl > > +++ b/gitweb/gitweb.perl > > @@ -4908,9 +4908,31 @@ sub print_sidebyside_diff_lines { > > } > > } > > > > -sub print_sidebyside_diff_chunk { > > - my @chunk = @_; > > +# 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); > > +} > > + > > +# 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); > > + } > > +} > > That looks nice. > > BTW. I didn't examine the final code, but what happens for binary > diffs that git supports? Is it handled outside print_diff_chunk()? Good question. Will check. > > > + > > +sub print_diff_chunk { > > + my ($diff_style, $is_combined, @chunk) = @_; > > my (@ctx, @rem, @add); > > + my $prev_class = ''; > > Is $prev_class here class of previous chunk fragment or _previous class_ > (i.e. always different from $class), or is it class of previous line? > It is not obvious here, from variable name. Better name or at least > a comment would be appreciated. > It's a class of previous line. > > > > return unless @chunk; > > > > @@ -4935,9 +4957,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)) { > > We usually use tabs to align, but spaces to indent in gitweb, so it > would be > > > + if (((@rem || @add) && $class eq 'ctx') || !$class || > > + (@rem && @add && $class ne $prev_class)) { > OK. > Anyway, this conditional gets ridicously complicated. I wonder if we > could simplify it. > > If we allow printing context together widh added/removed lines, and > not as soon as possible, perhaps good conditional would be: class > changed, and accumulator for current class is full: > > > + if ($class ne $prev_class && @{ $acc{$class} }) { > > Or something like that, where > > my %acc = (ctx => \@ctx, add => \@add, rem => \@rem); > > If we want to print context as soon as possible, like it was before > 1st patch in this series, we could uuse the following conditional > > > + if ($class ne $prev_class && > > + ($prev_class eq 'ctx' || @{ $acc{$class} })) { > > Perhaps with "scalar @{ $acc{$class} }" if it would make things more > clear. > > Though probably to avoid "Use of uninitialized value in string ne" > we would probably have to use empty string instead of undefined > value for "no class" case, i.e. beginning or end of chunk. > OK, I'll try to work out something. > > + print_diff_lines(\@ctx, \@rem, \@add, $diff_style, > > + $is_combined); > > @ctx = @rem = @add = (); > > } > > > > @@ -4954,6 +4980,8 @@ sub print_sidebyside_diff_chunk { > > if ($class eq 'ctx') { > > push @ctx, $line; > > } > > + > > + $prev_class = $class; > > } > > } > [...] > -- 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