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

Jakub Narebski [off-list ref] wrote:

Michał Kiedrowicz [off-list ref] writes:

quoted

Currently, print_sidebyside_diff_chunk() does two things: it
accumulates diff lines and prints them. Accumulation may be used to
perform additional operations on diff lines, so it makes sense to split
these two things. Thus, the code that prints diff lines in a side-by-side
manner is moved out of print_sidebyside_diff_chunk() to a separate
subroutine.

The outcome of this patch is that print_sidebyside_diff_chunk() is now
much shorter and easier to read.

This change is meant to be a simple code movement. No behavior change is
intended.

Signed-off-by: Michał Kiedrowicz <redacted>

As it is pure refactoring, and improves readibility of code quite a
bit, I am all for it, but...

You replace the following set of conditions

quoted

-		## print from accumulator when type of class of lines change
-		# empty contents block on start rem/add block, or end of chunk
-		if (@ctx && (!$class || $class eq 'rem' || $class eq 'add')) {

and

quoted

-		# empty add/rem block on start context block, or end of chunk
-		if ((@rem || @add) && (!$class || $class eq 'ctx')) {
-			if (!@add) {

[...]

quoted

-			} elsif (!@rem) {

[...]

quoted

-			} else {

with these (I have reindented the code to match)

quoted

+		## print from accumulator when have some add/rem lines or end
+		# of chunk (flush context lines)
+		if (((@rem || @add) && $class eq 'ctx') || !$class) {

quoted

+                     if (@$ctx) {

[...]

quoted

+                     }
+                     if (!@$add) {

[...]

quoted

+                     } elsif (!@$rem) {

[...]

quoted

+                     } else {

It is not obvious that the final result is the same.

You are right. I should have described it in the commit message.

BTW doesn't new code print context when printing added and removed
lines, and not as soon as class changes?  This doesn't change the
output, but it slightly changes behavior.

This is also true. I'll describe it in the commit message. I could also
create two functions: one that prints @ctx and second one that prints
@rem and @add but then I would have to create two other functions for
inline diff.