From: Jakub Narebski <jnareb@gmail.com>
Date: Wed, 28 Mar 2012 14:33:53 GMT
Subject: Re: [PATCH v2 3/8] gitweb: Extract print_sidebyside_diff_lines()
Message-ID: <201203281633.53916.jnareb@gmail.com>
In-Reply-To: <1332543417-19664-4-git-send-email-michal.kiedrowicz@gmail.com>

On Fri, 23 Mar 2012, Michał Kiedrowicz wrote:

> Currently, print_sidebyside_diff_chunk() does two things: it
> accumulates diff lines and prints them.  Accumulation may be used to
> perform additional operations on diff lines,  so it makes sense to split
> these two things.  Thus, the code that prints diff lines in a side-by-side
> manner is moved out of print_sidebyside_diff_chunk() to a separate
> subroutine.
>
Right, that is quite sensible.

> The outcome of this patch is that print_sidebyside_diff_chunk() is now
> much shorter and easier to read.
> 
Nice effect.

> This is a preparation patch for diff refinement highlightning.  It should
> not change the gitweb output, but it slightly changes its behavior.
> Before this commit, context is printed on the class change. Now, it'it
> printed just before printing added and removed lines.

                                                      , and at the end
  of chunk.

IMVHO such change is irrelevant.

Acked-by: Jakub Narębski <jnareb@gmail.com>

> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> ---
>  gitweb/gitweb.perl |   97 ++++++++++++++++++++++++++++------------------------
>  1 files changed, 52 insertions(+), 45 deletions(-)

Nice code movement.
 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 1744c40..86d5a02 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4998,6 +4998,53 @@ sub git_difftree_body {
>  	print "</table>\n";
>  }
>  
> +# Print context lines and then rem/add lines in a side-by-side manner.
> +sub print_sidebyside_diff_lines {
> +	my ($ctx, $rem, $add) = @_;
> +
> +	# print context block before add/rem block
> +	if (@$ctx) {
> +		print join '',
> +			'<div class="chunk_block ctx">',
> +				'<div class="old">',
> +				@$ctx,
> +				'</div>',
> +				'<div class="new">',
> +				@$ctx,
> +				'</div>',
> +			'</div>';
> +	}
> +
> +	if (!@$add) {
> +		# pure removal
> +		print join '',
> +			'<div class="chunk_block rem">',
> +				'<div class="old">',
> +				@$rem,
> +				'</div>',
> +			'</div>';
> +	} elsif (!@$rem) {
> +		# pure addition
> +		print join '',
> +			'<div class="chunk_block add">',
> +				'<div class="new">',
> +				@$add,
> +				'</div>',
> +			'</div>';
> +	} else {
> +		# assume that it is change
> +		print join '',
> +			'<div class="chunk_block chg">',
> +				'<div class="old">',
> +				@$rem,
> +				'</div>',
> +				'<div class="new">',
> +				@$add,
> +				'</div>',
> +			'</div>';
> +	}
> +}
> +
>  sub print_sidebyside_diff_chunk {
>  	my @chunk = @_;
>  	my (@ctx, @rem, @add);
> @@ -5024,51 +5071,11 @@ sub print_sidebyside_diff_chunk {
>  			next;
>  		}
>  
> -		## print from accumulator when type of class of lines change
> -		# empty contents block on start rem/add block, or end of chunk
> -		if (@ctx && (!$class || $class eq 'rem' || $class eq 'add')) {
> -			print join '',
> -				'<div class="chunk_block ctx">',
> -					'<div class="old">',
> -					@ctx,
> -					'</div>',
> -					'<div class="new">',
> -					@ctx,
> -					'</div>',
> -				'</div>';
> -			@ctx = ();
> -		}
> -		# empty add/rem block on start context block, or end of chunk
> -		if ((@rem || @add) && (!$class || $class eq 'ctx')) {
> -			if (!@add) {
> -				# pure removal
> -				print join '',
> -					'<div class="chunk_block rem">',
> -						'<div class="old">',
> -						@rem,
> -						'</div>',
> -					'</div>';
> -			} elsif (!@rem) {
> -				# pure addition
> -				print join '',
> -					'<div class="chunk_block add">',
> -						'<div class="new">',
> -						@add,
> -						'</div>',
> -					'</div>';
> -			} else {
> -				# assume that it is change
> -				print join '',
> -					'<div class="chunk_block chg">',
> -						'<div class="old">',
> -						@rem,
> -						'</div>',
> -						'<div class="new">',
> -						@add,
> -						'</div>',
> -					'</div>';
> -			}
> -			@rem = @add = ();
> +		## print from accumulator when have some add/rem lines or end
> +		# of chunk (flush context lines)
> +		if (((@rem || @add) && $class eq 'ctx') || !$class) {
> +			print_sidebyside_diff_lines(\@ctx, \@rem, \@add);
> +			@ctx = @rem = @add = ();
>  		}
>  
>  		## adding lines to accumulator

-- 
Jakub Narebski
Poland