Re: [PATCH 8/8] gitweb: Highlight combined diffs

[Jakub Narebski <jnareb@gmail.com>]

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.

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

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

> +				}
> +			}
> +		}
> +	} else {
> +		$can_highlight = 0;
> +	}

This 'else' would be not necessary if $can_highlight was initialized
to 0.

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

-- 
Jakub Narębski