Jakub Narebski <jnareb@xxxxxxxxx> wrote:
> On Fri, 23 Mar 2012, Michał Kiedrowicz wrote:
>
> > 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.
> >
> Is -> was?
>
> > 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.
>
> Well, we explicitly skip combined diffs. I think what you want to say
> here is that it is not possible to simply use existing algorithm
> unchanged for combined diffs.
Yep, your wording is much better.
>
> >
> > 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
>
> O.K., that's a nice and sensible first step.
>
> I wonder if it would be worth to specify that we currently require
> that pattern of '-'-es in pre-image match pattern of '+'-es in
> postimage.
>
> Nb. the prefix of combined diff would either include '+', or '-',
> but never mixed (this is documented, but I had trouble with this).
>
> >
> > Further changes may introduce more intelligent approach that better
> > handles combined diffs.
>
> Very sensible approach.
> >
> > Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@xxxxxxxxx>
>
> Acked-by: Jakub Narębski <jnareb@xxxxxxxxx>
>
> > ---
>
> BTW. I went and checked if this approach helps for non-trivial merges
> in git.git history:
>
> * b10656c - helps a bit, though one can see limitation of pre/post-fix
> matching here, but it is present also for non-combined diff.
>
> * 8b132bc - helps a bit, though char-interdiff or word-interdiff might
> be better. Nb. I think that red background for 'marked' is a bit
> too dark (intensive).
>
> * c58499c - doesn't help too much.
>
> * f629c23, aa145bf - helps.
>
Thanks for testing. I'll experiment with less intensive backgrounds.
> > gitweb/gitweb.perl | 57
> > +++++++++++++++++++++++++++++++++++++++------------ 1 files
> > changed, 43 insertions(+), 14 deletions(-)
> >
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 872ba12..c056e83 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -5057,12 +5057,12 @@ sub print_inline_diff_lines {
> > # Format removed and added line, mark changed part and HTML-format
> > them. # Impementation is based on contrib/diff-highlight
>
> Implementation
> ^---
>
> > sub format_rem_add_line {
> > - my ($rem, $add) = @_;
> > + my ($rem, $add, $num_parents) = @_;
> > my @rem = split(//, $rem);
> > my @add = split(//, $add);
> > my ($esc_rem, $esc_add);
> > - # Ignore +/- character, thus $prefix_len is set to 1.
> > - my ($prefix_len, $suffix_len) = (1, 0);
> > + # Ignore leading +/- characters for each parent.
> > + my ($prefix_len, $suffix_len) = ($num_parents, 0);
>
> Nice.
>
> [...]
> > @@ -5099,15 +5099,43 @@ sub format_rem_add_line {
> >
> > # HTML-format diff context, removed and added lines.
> > sub format_ctx_rem_add_lines {
> > - my ($ctx, $rem, $add, $is_combined) = @_;
> > + my ($ctx, $rem, $add, $num_parents) = @_;
> > my (@new_ctx, @new_rem, @new_add);
> > + my $can_highlight = 0;
> > + my $is_combined = ($num_parents > 1);
> >
> > # Highlight if every removed line has a corresponding
> > added line.
> > - # Combined diffs are not supported ATM.
> > - if (!$is_combined && @$add > 0 && @$add == @$rem) {
> > + if (@$add > 0 && @$add == @$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 < @$add; $i++) {
> > + my $prefix_rem =
> > substr($rem->[$i], 0, $num_parents);
> > + my $prefix_add =
> > substr($add->[$i], 0, $num_parents); +
> > + $prefix_rem =~ s/-/+/g;
> > +
> > + if ($prefix_rem ne $prefix_add) {
> > + $can_highlight = 0;
> > + last;
> > + }
> > + }
> > + }
> > + }
>
> Good.
>
> > +
> > + if ($can_highlight) {
> > for (my $i = 0; $i < @$add; $i++) {
> > my ($line_rem, $line_add) =
> > format_rem_add_line(
> > - $rem->[$i], $add->[$i]);
> > + $rem->[$i], $add->[$i],
> > $num_parents); push @new_rem, $line_rem;
> > push @new_add, $line_add;
> > }
>
> [...]
> > @@ -5326,7 +5355,7 @@ sub git_patchset_body {
> >
> > } continue {
> > if (@chunk) {
> > - print_diff_chunk($diff_style,
> > $is_combined, \%from, \%to, @chunk);
> > + print_diff_chunk($diff_style, scalar
> > @hash_parents, \%from, \%to, @chunk); @chunk = ();
> > }
> > print "</div>\n"; # class="patch"
>
> I was wondering about 'commitdiff' between two commits, which is not
> combined even ifany of those commits is a merge commit... but it looks
> like it works all right.
>
--
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