Re: [PATCH 2/8] gitweb: Use print_diff_chunk() for both side-by-side and inline diffs

Jakub Narebski [off-list ref] wrote:

Michał Kiedrowicz [off-list ref] writes:

quoted

This renames print_sidebyside_diff_chunk() to print_diff_chunk() and
makes use of it for both side-by-side and inline diffs. Now diff lines
are always accumulated before they are printed. This opens the
possibility to preprocess diff output before it's printed.

The new function print_inline_diff_lines() could reorder diff lines.

I think you meant here "if left as is", isn't it?

Yes.

quoted

It first prints all context lines, then all removed lines and
finally all added lines. If the diff output consisted of mixed added
and removed lines, gitweb would reorder these lines. This is
especially true for combined diff output, for example:

	 - removed line for first parent
	 + added line for first parent
	  -removed line for second parent
	 ++added line for both parents

would be rendered as:

	- removed line for first parent
	 -removed line for second parent
	+ added line for first parent
	++added line for both parents

This is not "is especially true"; it can happen (though: can it
really?) 

Yeah. See
http://git.kernel.org/?p=git/git.git;a=commitdiff;h=5de89d3abfca98b0dfd0280d28576940c913d60d 

in combined diff output; in ordinary diff you would always
have context or end/beginning of chunk between added and removed
lines.

OK, I'll reword this part of the commit message.

quoted

To prevent gitweb from reordering lines, print_diff_chunk() calls
print_diff_lines() as soon as it detects that both added and removed
lines are present and there was a class change.

You really should mention, in my opinion, that this change is
preparation for diff refinement highlighting (higlighting the
differing segments).

Otherwise I don't see the reason for it: ordinary diff didn't need the
fancy stuff with accumulating then printing, and could be send
straightly to output.  This adds complexity for non-sidebyside diff,
unnecessary if not for diff refinement highlighting.

I completely agree with that. This patch doesn't makes sense without
later patches.

quoted

Signed-off-by: Michał Kiedrowicz <redacted>
---
 gitweb/gitweb.perl |   53 +++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1247607..ed63261 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -4908,9 +4908,31 @@ sub print_sidebyside_diff_lines {
 	}
 }
 
-sub print_sidebyside_diff_chunk {
-	my @chunk = @_;
+# Print context lines and then rem/add lines in inline manner.
+sub print_inline_diff_lines {
+	my ($ctx, $rem, $add) = @_;
+
+	print foreach (@$ctx);
+	print foreach (@$rem);
+	print foreach (@$add);
+}
+
+# Print context lines and then rem/add lines.
+sub print_diff_lines {
+	my ($ctx, $rem, $add, $diff_style, $is_combined) = @_;
+
+	if ($diff_style eq 'sidebyside' && !$is_combined) {
+		print_sidebyside_diff_lines($ctx, $rem, $add);
+	} else {
+		# default 'inline' style and unknown styles
+		print_inline_diff_lines($ctx, $rem, $add);
+	}
+}

That looks nice.

BTW. I didn't examine the final code, but what happens for binary
diffs that git supports?  Is it handled outside print_diff_chunk()?

Good question. Will check.

quoted

+
+sub print_diff_chunk {
+	my ($diff_style, $is_combined, @chunk) = @_;
 	my (@ctx, @rem, @add);
+	my $prev_class = '';

Is $prev_class here class of previous chunk fragment or _previous class_
(i.e. always different from $class), or is it class of previous line?
It is not obvious here, from variable name.  Better name or at least
a comment would be appreciated.

It's a class of previous line.

quoted

 
 	return unless @chunk;
 

@@ -4935,9 +4957,13 @@ sub print_sidebyside_diff_chunk {
 		}
 
 		## print from accumulator when have some add/rem lines or end
-		# of chunk (flush context lines)
-		if (((@rem || @add) && $class eq 'ctx') || !$class) {
-			print_sidebyside_diff_lines(\@ctx, \@rem, \@add);
+		# of chunk (flush context lines), or when have add and rem
+		# lines and new block is reached (otherwise add/rem lines could
+		# be reordered)
+		if (((@rem || @add) && $class eq 'ctx') || !$class ||
+				(@rem && @add && $class ne $prev_class)) {

We usually use tabs to align, but spaces to indent in gitweb, so it
would be

quoted

+		if (((@rem || @add) && $class eq 'ctx') || !$class ||
+		    (@rem && @add && $class ne $prev_class)) {

OK.

Anyway, this conditional gets ridicously complicated.  I wonder if we
could simplify it.

If we allow printing context together widh added/removed lines, and
not as soon as possible, perhaps good conditional would be: class
changed, and accumulator for current class is full:

quoted

+		if ($class ne $prev_class && @{ $acc{$class} }) {

Or something like that, where

        my %acc = (ctx => \@ctx, add => \@add, rem => \@rem);

If we want to print context as soon as possible, like it was before
1st patch in this series, we could uuse the following conditional

quoted

+		if ($class ne $prev_class && 
+                 ($prev_class eq 'ctx' || @{ $acc{$class} })) {

Perhaps with "scalar @{ $acc{$class} }" if it would make things more
clear.

Though probably to avoid "Use of uninitialized value in string ne"
we would probably have to use empty string instead of undefined
value for "no class" case, i.e. beginning or end of chunk.

OK, I'll try to work out something.
 

quoted

+			print_diff_lines(\@ctx, \@rem, \@add, $diff_style,
+					$is_combined);
 			@ctx = @rem = @add = ();
 		}
 

@@ -4954,6 +4980,8 @@ sub print_sidebyside_diff_chunk {
 		if ($class eq 'ctx') {
 			push @ctx, $line;
 		}
+
+		$prev_class = $class;
 	}
 }

[...]