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

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 4, 2012, 22:47 UTC
Message-ID: <201204050047.10357.jnareb@gmail.com>
In-Reply-To: <7vsjgj6ufi.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

Show 14 quoted lines

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

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

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

-- 
Jakub Narebski
Poland