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

From: Michal Kiedrowicz <michal.kiedrowicz@gmail.com>
Date: Apr 5, 2012, 06:06 UTC
Message-ID: <20120405080633.3836f49b@mkiedrowicz.ivo.pl>
In-Reply-To: <201204050047.10357.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> wrote:

Show 22 quoted lines

> Junio C Hamano wrote:
> > Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> > 
> > > +	if (!@$add) {
> > > +		# pure removal
> > > +...
> > > +	} elsif (!@$rem) {
> > > +		# pure addition
> > > +...
> > > +	} else {
> > > +		# assume that it is change
> > > +		print join '',
> > 
> > I know this is not a new problem, but if your patch hunk has both
> > '-' and '+' lines, what's there to "assume" that it is a change?
> > Isn't it always?
> 
> What I meant here when I was writing it that they are lines that
> changed between two versions, like '!' in original (not unified)
> context format.
> 
> We can omit this comment.

OK.

Show 26 quoted lines

> 
> > > -		# empty add/rem block on start context block, or
> > > end of chunk
> > > -		if ((@rem || @add) && (!$class || $class eq
> > > 'ctx')) { -...
> > > +		## print from accumulator when have some add/rem
> > > lines or end
> > > +		# of chunk (flush context lines)
> > > +		if (((@rem || @add) && $class eq 'ctx')
> > > || !$class) {
> > 
> > This seems to change the condition.  Earlier, it held true if
> > (there is anything to show), and (class is unset or equal to ctx).
> > The new code says something different.
> 
> Yes it does, as described in the commit message:
> 
>                                                     [...] 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's printed just before printing added and removed
> lines, and at the end of chunk.
> 
> The difference is that context lines are also printed accumulated now.
> Though why this change is required for refactoring could have been
> described in more detail...

I changed that because I wanted to squash both conditions (the one that checks if @ctx should be printed and the one that prints @add/@rem lines) and have just one call to print_sidebyside_diff_lines(). Later, this function is changed to print_diff_lines() and controls whether 'inline' or 'side-by-side' diff should be printed. Having two conditions and two calls/functions would make the code redundant. Then I thought that instead of calling twice print_sidebyside_diff_lines() (for @ctx and @add/@rem lines, like the code from pre-image prints these lines separatedly), I can just call it once.

I can revert this change to previous behavior but I think that would make the condition more complicated.

Show 14 quoted lines

> 
> >                             Also can $class be undef, and if so,
> > doesn't it trigger comparison between undef and 'ctx' by
> > having !$class check at the end of || chain?
> 
> Thanks for noticing this (I wonder why testsuite didn't caught it).
> It should be
> 
>  +		## print from accumulator when have some add/rem
> lines or end
>  +		# of chunk (flush context lines)
>  +		if (!$class || ((@rem || @add) && $class eq 'ctx'))
> {
>

OK, I'll fix that.