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

[Jakub Narebski <jnareb@gmail.com>]

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

-- 
Jakub Narebski
Poland