git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jakub Narebski <jnareb@gmail.com>
Date
Feb 11, 2012, 15:53 UTC
Message-ID
<m3k43ttlh9.fsf@localhost.localdomain>
In-Reply-To
<1328865494-24415-3-git-send-email-michal.kiedrowicz@gmail.com>
Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
Show 6 quoted lines
> 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?
Show 16 quoted 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
> 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?) in combined diff output; in ordinary diff you would always have context or end/beginning of chunk between added and removed lines.

> 
> 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.
Show 35 quoted lines
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> ---
>  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()?

Show 5 quoted lines
> +
> +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.

Show 15 quoted lines
>  
>  	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

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

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:

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

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

Show 13 quoted lines
> +			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;
>  	}
>  }
[...]  
-- 
Jakub Narębski
Previous: Michał KiedrowiczNext: Michał Kiedrowicz
Message 6 of 67 in “gitweb: Highlight interesting parts of diff”
  1. 0/8 gitweb: Highlight interesting parts of diffMichał Kiedrowicz, Feb 10, 2012
  2. 1/8 gitweb: Extract print_sidebyside_diff_lines()Michał Kiedrowicz, Feb 10, 2012
  3. Jakub NarebskiFeb 11, 2012
  4. Michał KiedrowiczFeb 11, 2012
  5. 2/8 gitweb: Use print_diff_chunk() for both side-by-side and inline diffsMichał Kiedrowicz, Feb 10, 2012
  6. Jakub NarebskiFeb 11, 2012
  7. Michał KiedrowiczFeb 11, 2012
  8. Michał KiedrowiczFeb 25, 2012
  9. 3/8 gitweb: Move HTML-formatting diff line back to process_diff_line()Michał Kiedrowicz, Feb 10, 2012
  10. Jakub NarebskiFeb 11, 2012
  11. 4/8 gitweb: Push formatting diff lines to print_diff_chunk()Michał Kiedrowicz, Feb 10, 2012
  12. Jakub NarebskiFeb 11, 2012
  13. Michał KiedrowiczFeb 11, 2012
  14. Michał KiedrowiczFeb 11, 2012
  15. 5/8 gitweb: Format diff lines just before printingMichał Kiedrowicz, Feb 10, 2012
  16. Jakub NarebskiFeb 11, 2012
  17. Michał KiedrowiczFeb 11, 2012
  18. 6/8 gitweb: Highlight interesting parts of diffMichał Kiedrowicz, Feb 10, 2012
  19. Jakub NarebskiFeb 10, 2012
  20. Michał KiedrowiczFeb 10, 2012
  21. Jakub NarebskiFeb 10, 2012
  22. Michał KiedrowiczFeb 10, 2012
  23. Jakub NarebskiFeb 10, 2012
  24. Jeff KingFeb 10, 2012
  25. Michal KiedrowiczFeb 14, 2012
  26. Junio C HamanoFeb 14, 2012
  27. Jeff KingFeb 14, 2012
  28. Jeff KingFeb 10, 2012
  29. Michał KiedrowiczFeb 10, 2012
  30. Jeff KingFeb 10, 2012
  31. Michał KiedrowiczFeb 10, 2012
  32. diff-highlight: Work for multiline changes tooMichał Kiedrowicz, Feb 10, 2012
  33. Jeff KingFeb 13, 2012
  34. 1/5 diff-highlight: make perl strict and warnings fatalJeff King, Feb 13, 2012
  35. 2/5 diff-highlight: don't highlight whole linesJeff King, Feb 13, 2012
  36. Michal KiedrowiczFeb 14, 2012
  37. 3/5 diff-highlight: refactor to prepare for multi-line hunksJeff King, Feb 13, 2012
  38. 4/5 diff-highlight: match multi-line hunksJeff King, Feb 13, 2012
  39. 5/5 diff-highlight: document some non-optimal casesJeff King, Feb 13, 2012
  40. Michal KiedrowiczFeb 14, 2012
  41. Junio C HamanoFeb 14, 2012
  42. Jeff KingFeb 14, 2012
  43. Junio C HamanoFeb 14, 2012
  44. Jeff KingFeb 14, 2012
  45. Michal KiedrowiczFeb 14, 2012
  46. Jakub NarebskiFeb 10, 2012
  47. Jakub NarebskiFeb 11, 2012
  48. Jakub NarebskiFeb 12, 2012
  49. Michal KiedrowiczFeb 13, 2012
  50. Jakub NarebskiFeb 13, 2012
  51. Michał KiedrowiczFeb 13, 2012
  52. Michal KiedrowiczFeb 13, 2012
  53. Jakub NarebskiFeb 13, 2012
  54. Michał KiedrowiczFeb 13, 2012
  55. Jakub NarebskiFeb 14, 2012
  56. Michał KiedrowiczFeb 14, 2012
  57. Jeff KingFeb 14, 2012
  58. Michał KiedrowiczFeb 14, 2012
  59. Jeff KingFeb 14, 2012
  60. 7/8 gitweb: Use different colors to present marked changesMichał Kiedrowicz, Feb 10, 2012
  61. Jakub NarebskiFeb 12, 2012
  62. Michal KiedrowiczFeb 13, 2012
  63. 8/8 gitweb: Highlight combined diffsMichał Kiedrowicz, Feb 10, 2012
  64. Jakub NarebskiFeb 12, 2012
  65. Michal KiedrowiczFeb 13, 2012
  66. Jakub NarebskiFeb 11, 2012
  67. Michał KiedrowiczFeb 11, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.