Re: [PATCH v2 8/8] gitweb: Refinement highlightning in combined diffs

[Michal Kiedrowicz <michal.kiedrowicz@gmail.com>]

Jakub Narebski <jnareb@gmail.com> 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@gmail.com>
> 
>   Acked-by: Jakub Narębski <jnareb@gmail.com>
> 
> > ---
> 
> 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.
>