From: Michal Kiedrowicz <michal.kiedrowicz@gmail.com>
Date: Fri, 06 Apr 2012 08:36:03 GMT
Subject: Re: [PATCH v3 4/8] gitweb: Extract print_sidebyside_diff_lines()
Message-ID: <20120406103603.1f1ee90d@mkiedrowicz.ivo.pl>
In-Reply-To: <201204060057.34138.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> wrote:

> Michal Kiedrowicz wrote:
> > Jakub Narebski <jnareb@gmail.com> wrote:
> >> Junio C Hamano wrote:
> >>> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> 
> >>>> -		# 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.
> 
> No, I think that this change is good idea if it simplifies code flow.
> But it really should be described in commit message, not only "what"
> (which you did describe), but also "whys".
> 

Sure, I'll try to put my explanation to the commit message.