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

Re: [PATCH 8/8] gitweb: Highlight combined diffs

From
Jakub Narebski <jnareb@gmail.com>
Date
Feb 12, 2012, 00:03 UTC
Message-ID
<m3ty2wsyt6.fsf@localhost.localdomain>
In-Reply-To
<1328865494-24415-9-git-send-email-michal.kiedrowicz@gmail.com>
Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
Show 33 quoted lines
> The highlightning of combined diffs is currently disabled.  This is
> because output from a combined diff is much harder to highlight because
> it's not obvious which removed and added lines should be compared.
> 
> Moreover, code that compares added and removed lines doesn't care about
> combined diffs. It only skips first +/- character, treating second +/-
> as a line content.
> 
> Let's start with a simple case: only highlight changes that come from
> one parent, i.e. when every removed line has a corresponding added line
> for the same parent.  This way the highlightning cannot get wrong. For
> example, following diffs would be highlighted:
> 
> 	- removed line for first parent
> 	+ added line for first parent
> 	  context line
> 	 -removed line for second parent
> 	 +added line for second parent
> 
> or
> 
> 	- removed line for first parent
> 	 -removed line for second parent
> 	+ added line for first parent
> 	 +added line for second parent
> 
> but following output will not:
> 
> 	- removed line for first parent
> 	 -removed line for second parent
> 	 +added line for second parent
> 	++added line for both parents
> 
That is a very reasonable approach.
Show 27 quoted lines
> Further changes may introduce more intelligent approach that better
> handles combined diffs.
> 
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> ---
>  gitweb/gitweb.perl |   40 +++++++++++++++++++++++++++++++++++++---
>  1 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 1a5b454..2b6cb9e 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -4944,13 +4944,16 @@ sub esc_html_mark_range {
>  
>  # Format removed and added line, mark changed part and HTML-format them.
>  sub format_rem_add_line {
> -	my ($rem, $add) = @_;
> +	my ($rem, $add, $is_combined) = @_;
>  	my @r = split(//, $rem);
>  	my @a = split(//, $add);
>  	my ($esc_rem, $esc_add);
>  	my ($prefix, $suffix_rem, $suffix_add) = (1, $#r, $#a);
>  	my ($prefix_is_space, $suffix_is_space) = (1, 1);
>  
> +	# In combined diff we must ignore two +/- characters.
> +	$prefix = 2 if ($is_combined);
> +

Errr... actually number of prefix is equalt to number of parents, so it might be in case of octopus merge more than 2.

Show 32 quoted lines
>  	while ($prefix < @r && $prefix < @a) {
>  		last if ($r[$prefix] ne $a[$prefix]);
>  
> @@ -4988,11 +4991,42 @@ sub format_ctx_rem_add_lines {
>  	my ($ctx, $rem, $add, $is_combined) = @_;
>  	my (@new_ctx, @new_rem, @new_add);
>  	my $num_add_lines = @$add;
> +	my $can_highlight;
> +
> +	# Highlight if every removed line has a corresponding added line.
> +	if ($num_add_lines > 0 && $num_add_lines == @$rem) {
> +		$can_highlight = 1;
> +
> +		# Highlight lines in combined diff only if the chunk contains
> +		# diff between the same version, e.g.
> +		#
> +		#    - a
> +		#   -  b
> +		#    + c
> +		#   +  d
> +		#
> +		# Otherwise the highlightling would be confusing.
> +		if ($is_combined) {
> +			for (my $i = 0; $i < $num_add_lines; $i++) {
> +				my $prefix_rem = substr($rem->[$i], 0, 2);
> +				my $prefix_add = substr($add->[$i], 0, 2);
> +
> +				$prefix_rem =~ s/-/+/g;
> +
> +				if ($prefix_rem ne $prefix_add) {
> +					$can_highlight = 0;
> +					last;

Nb. this assumes that we cannot refine and highlight something like this:

   		#    - a
   		#   -  b
   		#    + c
   		#   ++ d
But perhaps this is better left for future improvemnt.
Show 6 quoted lines
> +				}
> +			}
> +		}
> +	} else {
> +		$can_highlight = 0;
> +	}

This 'else' would be not necessary if $can_highlight was initialized to 0.

Show 9 quoted lines
>  
> -	if (!$is_combined && $num_add_lines > 0 && $num_add_lines == @$rem) {
> +	if ($can_highlight) {
>  		for (my $i = 0; $i < $num_add_lines; $i++) {
>  			my ($line_rem, $line_add) = format_rem_add_line(
> -				$rem->[$i], $add->[$i]);
> +				$rem->[$i], $add->[$i], $is_combined);
>  			push @new_rem, $line_rem;
>  			push @new_add, $line_add;
O.K.
-- 
Jakub Narębski
Previous: Michał KiedrowiczNext: Michal Kiedrowicz
Message 64 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.