Re: [PATCH 1/8] gitweb: Extract print_sidebyside_diff_lines()

[Jakub Narebski <jnareb@gmail.com>]

Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:

> 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.
> 
> The outcome of this patch is that print_sidebyside_diff_chunk() is now
> much shorter and easier to read.
> 
> This change is meant to be a simple code movement. No behavior change is
> intended.
> 
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>

As it is pure refactoring, and improves readibility of code quite a
bit, I am all for it, but...

You replace the following set of conditions

> -		## 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')) {

and

> -		# empty add/rem block on start context block, or end of chunk
> -		if ((@rem || @add) && (!$class || $class eq 'ctx')) {
> -			if (!@add) {
[...]
> -			} elsif (!@rem) {
[...]
> -			} else {


with these (I have reindented the code to match)

> +		## print from accumulator when have some add/rem lines or end
> +		# of chunk (flush context lines)
> +		if (((@rem || @add) && $class eq 'ctx') || !$class) {

> +                     if (@$ctx) {
[...]
> +                     }
> +                     if (!@$add) {
[...]
> +                     } elsif (!@$rem) {
[...]
> +                     } else {

It is not obvious that the final result is the same.

BTW doesn't new code print context when printing added and removed
lines, and not as soon as class changes?  This doesn't change the
output, but it slightly changes behavior.

-- 
Jakub Narębski