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

Jakub Narebski [off-list ref] wrote:

On Fri, 23 Mar 2012, Michał Kiedrowicz wrote:

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, which is
needed for diff refinement highlightning (implemented in incoming
patches).

While I don't like that we use accumulator for something that was 
straightforwardly printed, I understand that it is necessary preprocessing
required for diff refinement highlighting.  Having refactoring upfront
is a good idea, making for easier review.

Yeah, this change doesn't make sense on its own but is required by later
patch that needs whole chunk at once.

quoted

If left as is, the new function print_inline_diff_lines() could reorder

I think the previous paragraph is long enough that it would be better
to repeat subject of this sentence, i.e. start with

 "If print_diff_chunk() was left as is, the new function ..."

OK.

quoted

diff lines.  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 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

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.

O.K.

quoted

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

[...]

quoted

+# 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);

Why not simply

  +	print @$ctx, @$rem, @$add;

It is "print LIST" for a reason (and $\ is empty here in gitweb, as it
is empty by default).

OK, that's much better.  I really didn't like those 3 prints.

quoted

+}
+
+# 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);
+	}
+}

Very nice subroutine!

quoted

+
+sub print_diff_chunk {
+	my ($diff_style, $is_combined, @chunk) = @_;
 	my (@ctx, @rem, @add);
 
+	# The class of the previous line. 
+	my $prev_class = '';
+
 	return unless @chunk;
 
 	# incomplete last line might be among removed or added lines,

@@ -5072,9 +5096,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)) {
+			print_diff_lines(\@ctx, \@rem, \@add, $diff_style,
+					$is_combined);

Nitpick: the following _might_ be a tiny little bit more readable:

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

OK.

quoted

 			@ctx = @rem = @add = ();
 		}
 

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

Anyway nice change.
  

quoted

@@ -5217,22 +5247,17 @@ sub git_patchset_body {
 			$diff_classes .= " $class" if ($class);
 			$line = "<div class=\"$diff_classes\">$line</div>\n";
 
-			if ($diff_style eq 'sidebyside' && !$is_combined) {
-				if ($class eq 'chunk_header') {
-					print_sidebyside_diff_chunk(@chunk);
-					@chunk = ( [ $class, $line ] );
-				} else {
-					push @chunk, [ $class, $line ];
-				}
+			if ($class eq 'chunk_header') {
+				print_diff_chunk($diff_style, $is_combined, @chunk);

Nice, pushing acting on $diff_style down to print_diff_chunk(), which
simplifies code a bit.

quoted

+				@chunk = ( [ $class, $line ] );

BTW. this could be simplified to

  +				@chunk = ();
  +				push @chunk, [ $class, $line ];

Well, simplified after noticing the common part of those two branches
of a conditional.

But it really doesn't matter.

Sounds sensible.

quoted

 			} else {
-				# default 'inline' style and unknown styles
-				print $line;
+				push @chunk, [ $class, $line ];
 			}
 		}
 
 	} continue {
 		if (@chunk) {
-			print_sidebyside_diff_chunk(@chunk);
+			print_diff_chunk($diff_style, $is_combined, @chunk);
 			@chunk = ();
 		}
 		print "</div>\n"; # class="patch"
-- 

BTW. I was wondering about binary files, but this commit wouldn't make
it worse anyway as we parse diff output assuming unified-like diff for
diff syntax highlighting... and binary diffs are shown as

  "Binary files a/foo.png and b/foo.png differ"

in extended diff header.

Yeah, this is what I wrote in the cover letter:

	* Ensured that binary patches are not supported in HTML format,
	  thus this series canot break it :) (answering Jakub's questions)

but maybe I wasn't clear enough.