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

[Jakub Narebski <jnareb@gmail.com>]

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.

> 
> 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.

>  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.

-- 
Jakub Narebski
Poland