Re: [PATCH 8/8] gitweb: Highlight combined diffs
Jakub Narebski <jnareb@gmail.com> wrote:
Show 37 quoted lines
> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
>
> > The highlightning of combined diffs is currently disabled. This is
> > because output from a combined diff is much harder to highlight
> > because it's not obvious which removed and added lines should be
> > compared.
> >
> > Moreover, code that compares added and removed lines doesn't care
> > about combined diffs. It only skips first +/- character, treating
> > second +/- as a line content.
> >
> > Let's start with a simple case: only highlight changes that come
> > from one parent, i.e. when every removed line has a corresponding
> > added line for the same parent. This way the highlightning cannot
> > get wrong. For example, following diffs would be highlighted:
> >
> > - removed line for first parent
> > + added line for first parent
> > context line
> > -removed line for second parent
> > +added line for second parent
> >
> > or
> >
> > - removed line for first parent
> > -removed line for second parent
> > + added line for first parent
> > +added line for second parent
> >
> > but following output will not:
> >
> > - removed line for first parent
> > -removed line for second parent
> > +added line for second parent
> > ++added line for both parents
> >
> That is a very reasonable approach.
Show 31 quoted lines
>
> > Further changes may introduce more intelligent approach that better
> > handles combined diffs.
> >
> > Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> > ---
> > gitweb/gitweb.perl | 40 +++++++++++++++++++++++++++++++++++++---
> > 1 files changed, 37 insertions(+), 3 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 1a5b454..2b6cb9e 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -4944,13 +4944,16 @@ sub esc_html_mark_range {
> >
> > # Format removed and added line, mark changed part and HTML-format
> > them. sub format_rem_add_line {
> > - my ($rem, $add) = @_;
> > + my ($rem, $add, $is_combined) = @_;
> > my @r = split(//, $rem);
> > my @a = split(//, $add);
> > my ($esc_rem, $esc_add);
> > my ($prefix, $suffix_rem, $suffix_add) = (1, $#r, $#a);
> > my ($prefix_is_space, $suffix_is_space) = (1, 1);
> >
> > + # In combined diff we must ignore two +/- characters.
> > + $prefix = 2 if ($is_combined);
> > +
>
> Errr... actually number of prefix is equalt to number of parents, so
> it might be in case of octopus merge more than 2.Show 47 quoted lines
>
> > while ($prefix < @r && $prefix < @a) {
> > last if ($r[$prefix] ne $a[$prefix]);
> >
> > @@ -4988,11 +4991,42 @@ sub format_ctx_rem_add_lines {
> > my ($ctx, $rem, $add, $is_combined) = @_;
> > my (@new_ctx, @new_rem, @new_add);
> > my $num_add_lines = @$add;
> > + my $can_highlight;
> > +
> > + # Highlight if every removed line has a corresponding
> > added line.
> > + if ($num_add_lines > 0 && $num_add_lines == @$rem) {
> > + $can_highlight = 1;
> > +
> > + # Highlight lines in combined diff only if the
> > chunk contains
> > + # diff between the same version, e.g.
> > + #
> > + # - a
> > + # - b
> > + # + c
> > + # + d
> > + #
> > + # Otherwise the highlightling would be confusing.
> > + if ($is_combined) {
> > + for (my $i = 0; $i < $num_add_lines; $i++)
> > {
> > + my $prefix_rem =
> > substr($rem->[$i], 0, 2);
> > + my $prefix_add =
> > substr($add->[$i], 0, 2); +
> > + $prefix_rem =~ s/-/+/g;
> > +
> > + if ($prefix_rem ne $prefix_add) {
> > + $can_highlight = 0;
> > + last;
>
> Nb. this assumes that we cannot refine and highlight something like
> this:
>
> # - a
> # - b
> # + c
> # ++ d
>
> But perhaps this is better left for future improvemnt.I can add a patch for that at the end of this series.
Show 11 quoted lines
>
> > + }
> > + }
> > + }
> > + } else {
> > + $can_highlight = 0;
> > + }
>
> This 'else' would be not necessary if $can_highlight was initialized
> to 0.
> Show 14 quoted lines
> >
> > - if (!$is_combined && $num_add_lines > 0 && $num_add_lines
> > == @$rem) {
> > + if ($can_highlight) {
> > for (my $i = 0; $i < $num_add_lines; $i++) {
> > my ($line_rem, $line_add) =
> > format_rem_add_line(
> > - $rem->[$i], $add->[$i]);
> > + $rem->[$i], $add->[$i],
> > $is_combined); push @new_rem, $line_rem;
> > push @new_add, $line_add;
>
> O.K.
> Thanks for your thorough review.
Michal Kiedrowicz <michal.kiedrowicz@gmail.com>