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

Jakub Narebski [off-list ref] wrote:

Junio C Hamano wrote:

quoted

Michał Kiedrowicz [off-list ref] writes:

quoted

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

quoted

quoted

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

quoted

                            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.