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

Jakub Narebski [off-list ref] wrote:

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.

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.

On Mon, 13 Feb 2012, Michał Kiedrowicz wrote:

quoted

Jakub Narebski [off-list ref] wrote:

quoted

On Mon, 13 Feb 2012, Michal Kiedrowicz wrote:

quoted

quoted

quoted

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

[...]

quoted

quoted

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. 

quoted

quoted

quoted

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 

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

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

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

quoted

quoted

quoted

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?