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

From: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
Date: Feb 11, 2012, 23:03 UTC
Message-ID: <20120212000325.2f6bc9f0@gmail.com>
In-Reply-To: <m3obt5tn0g.fsf@localhost.localdomain>

Jakub Narebski <jnareb@gmail.com> wrote:

Show 53 quoted lines

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

You are right. I should have described it in the commit message.

Show 5 quoted lines

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

This is also true. I'll describe it in the commit message. I could also create two functions: one that prints @ctx and second one that prints @rem and @add but then I would have to create two other functions for inline diff.