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

Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff

From
Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
Date
Feb 14, 2012, 18:23 UTC
Message-ID
<20120214192340.2d473866@gmail.com>
In-Reply-To
<201202141831.59699.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> wrote:
Show 8 quoted lines
> A few issues I have just noticed about this series.
> 
> 
> First, about naming.  "Highlighting interesting parts of diff" is
> acceptable name, but "syntax highlighting for diff" is not: gitweb
> already use syntax highlighting in diff views.  Call it "diff refinement
> highlighting", "highlighting changes / changed sections", "intraline
> highlighting".
OK, I'll make sure the naming is correct and consistent.
Show 29 quoted lines
> 
> 
> Second, I think (but I am not sure) that there is a bug in code that
> finds common suffix and prefix.
> 
> If I understand correctly the idea is to highlight changed part if
> there is at least one of common non-whitespace suffix or prefix.
> So syntax highlighting should look like this:
> 
> 1. Both prefix and suffix are non empty and non whitespace only
> 
>   -foo -{bar} baz
>   +foo +{quux} baz
> 
> 2. Non empty and non whitespace only prefix
> 
>   -foo -{bar}
>   +foo +{quux}
> 
> 2. Non empty and non whitespace only suffix
> 
>   --{bar} baz
>   ++{quux} baz
> 
> But in your code $prefix is not the length of common prefix, but
> the position of end of prefix in the original line of diff.  So
> you start with $prefix = 1... even though the prefix is empty.
> 
> How is that supposed to work?
But see the check:
+	# Mark lines that are different from each other, but have some common
+	# part that isn't whitespace.  If lines are completely different, don't
+	# mark them because that would make output unreadable, especially if
+	# diff consists of multiple lines.
+	if (($prefix == 1 && $suffix_rem == $#r && $suffix_add == $#a)
                        ^
                        |
                        This part.

+ || ($prefix_is_space && $suffix_is_space)) { + $esc_rem = esc_html($rem); + $esc_add = esc_html($add); + } else { + $esc_rem = esc_html_mark_range(\@r, $prefix, $suffix_rem); + $esc_add = esc_html_mark_range(\@a, $prefix, $suffix_add); + }

I guess it's still not correct because it should be equal to number of parents or $prefix_length should start from 0 like you wrote later.

Show 31 quoted lines
> 
> 
> On Mon, 13 Feb 2012, Michał Kiedrowicz wrote:
> > Jakub Narebski <jnareb@gmail.com> wrote:
> > > On Mon, 13 Feb 2012, Michal Kiedrowicz wrote:
> 
> > > > I haven't found *examples* on GitHub and Trac sites, but what about
> > > > these ones:
> > > > 
> > > > https://github.com/gitster/git/commit/8cad4744ee37ebec1d9491a1381ec1771a1ba795
> > > > http://trac.edgewall.org/changeset/10973
> [...]
> > > BTW GitHub is closed source, but we can check what algorithm does Trac
> > > use for diff refinement highlighting (highlighting changed portions of
> > > diff).
> > > 
> > 
> > I think it's
> > http://trac.edgewall.org/browser/trunk/trac/versioncontrol/diff.py
> > (see markup intraline_changes()).
>  
> It is get_change_extent() that finds extent of changes, as a pair
> containing the offset at which the changes start, and the negative offset
> at which the changes end.  So it is the same solution you use, only
> without ignoring whitespace-only prefixes and suffixes...  This code can
> be easily ported to Perl, BTW.
> 
> The markup_intraline_changes() function compares lines from preimage and
> from postimage pairwise, requiring that number of lines matches, the same
> like in your algorithm.
>

So using Jeff's diff-highlight we remain quite consistent with Trac output. There's nothing we can "steal" from it.

Show 15 quoted lines
> > > > I would also consider ignoring prefixes/suffixes with punctuation, like:
> > > > 
> > > > 	- * I like you.
> > > > 	+ * Alice had a little lamb.
> > > 
> > > But this patch doesn't implement this feature yet, isn't it?
> > 
> > No, but is a matter of adding
> > 
> > 	-$prefix_is_space = 0 if ($r[$prefix] !~ /\s/);
> > 	+$prefix_is_space = 0 if ($r[$prefix] !~ /\s|[[:punct:]]/);
> > 
> > (and the same for suffix)
> 
> All right.  But it is better added as separate patch.
Sure
Show 20 quoted lines
> Perhaps even
> requiring that not only there is at least one of common prefix or common
> suffix, but at least one of them is not whitespace only could be put
> in a separate commit...
> 
> > > > > > +sub format_rem_add_line {
> > > > > > +	my ($rem, $add) = @_;
> > > > > > +	my @r = split(//, $rem);
> > > > > > +	my @a = split(//, $add);
> > > 
> > > BTW the name of variable can be just @add and @rem.
> > >
> > 
> > I know they are different scopes but I don't like it. It makes the code
> > more confusing IMO. But I won't insist.
> 
> In my opinion if the variable refers to the same entity in different
> forms, using @foo and %foo (used in gitweb), or $foo and @foo (could be
> used here) is all right, and even better than trying to come up with
> different name for the same thing because of sigil.
OK. I'll follow that convention then.
Show 20 quoted lines
>  
> > > > > Shouldn't
> > > > > $prefix / $prefix_len start from 0, not from 1?
> > > > 
> > > > It starts from 1 because it skips first +/-. It should become obvious
> > > > after reading the comment from last patch :).
> 
> This means that $prefix is true even if prefix is empty ($prefix == 1).
> Wouldn't it be better for $prefix_len to count length of true prefix,
> without diff adornment?  Or make @r / @rem skip initial characters...
> 
> > > > +	# In combined diff we must ignore two +/- characters.
> > > > +	$prefix = 2 if ($is_combined);
> > > 
> > > Anyway comment about that fact would be nice.
> > 
> > Will do.
> 
> BTW. it is not "2" but "scalar @{$co{'parents'}}".
> 
OK.
Show 18 quoted lines
> 
> > > > The splitting and comparing by characters is taken from diff-highlight.
> > > > I don't think it's worth changing here.
> > > 
> > > You are right.
> > > 
> > > I'll try to come with hacky algorithm using string bitwise xor and regexp,
> > > and benchmark it comparing to your C-like solution, but it can be left for
> > > later (simple is better than clever, usually).
> > 
> > If you have time :).
> 
> Anyway it would be separate commit.  Better to just copy tested code
> from contrib/diff-highlight
> 
> BTW. would "git blame -C -C -C -w" detect this correctly as code
> movement^W copying?
>

Cannot say. Have you considered what I wrote in a separate e-mail, about using diff-highlight output directly / as a library?

Previous: Jakub NarebskiNext: Jeff King
Message 56 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.