On Thu, 18 Jun 2015, Jeff King wrote:
On Thu, Jun 18, 2015 at 11:08:16AM -0700, Junio C Hamano wrote:
So as I said, I do not think it would hurt to have this as an
incremental improvement (albeit going in a possibly wrong
direction).
Of course, it is a separate question if this change makes the output
worse, by comparing unmatched early parts of two hunks and making
nonsense highlight by calling highlight_pair() more often. As long
as that is not an issue, I am not opposed to this change, which was
what I meant to say by "this might not hurt".
Yes, that is my big concern, and why I punted on mismatched-size hunks
in the first place. Now that we have a patch, it is easy enough to "git
log -p | diff-highlight" with the old and new versions to compare the
results.
It certainly does improve some cases. E.g.:
-foo
+foo &&
+bar
in a test script becomes more clear. But some of the output is not so
great. For instance, the very commit under discussion has a
confusing and useless highlight. Or take a documentation patch like
5c31acfb, where I find the highlights actively distracting. We are saved
a little by the "if the whole line is different, do not highlight at
all" behavior of 097128d1bc.
To fix the useless highlights for both evenly and unevenly sized hunks
(like when all but a semicolon on a line changes), one can loosen the
criterion for not highlighting from "do not highlight if 0% of the
before and after lines are common between them" to, say, "do not
highlight if less than 10% of the before and after lines are common
between them". Then most of these useless highlights are gone for both
evenly and unevenly sized hunks.
Here is a patch that changes the criterion as mentioned. Testing this
change on the documentation patch 5c31acfb, only two pairs of lines are
highlighted instead of six. On my original patch, the useless highlight
is gone. The useless semicolon-related highlights on e.g. commit
99a2cfb are gone.
Ten percent is a modest threshold, and perhaps it should be increased
when highlighting unevenly sized hunks and decreased when highlighting
evenly sized hunks.
Of course, these patches are both hacks but they seem to be surprisingly
effective hacks especially when paired together.
So I dunno. IMHO this does more harm than good, and I would not want to
use it myself. But it is somewhat a matter of taste; I am not opposed to
making it a configurable option.
That is something I can do :)
-Peff
-- >8 --
Subject: [PATCH] diff-highlight: don't highlight lines that have little in
common
---
contrib/diff-highlight/diff-highlight | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index 85d2eb0..e4829ec 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -218,8 +218,13 @@ sub is_pair_interesting {
my $suffix_a = join('', @$a[($sa+1)..$#$a]);
my $suffix_b = join('', @$b[($sb+1)..$#$b]);
- return $prefix_a !~ /^$COLOR*-$BORING*$/ ||
- $prefix_b !~ /^$COLOR*\+$BORING*$/ ||
- $suffix_a !~ /^$BORING*$/ ||
- $suffix_b !~ /^$BORING*$/;
+ $prefix_a =~ s/^$COLOR*-$BORING*//;
+ $prefix_b =~ s/^$COLOR*\+$BORING*//;
+ $suffix_a =~ s/$BORING*$//;
+ $suffix_b =~ s/$BORING*$//;
+
+ # Only bother highlighting if at least 10% of each line is common among
+ # the lines.
+ return ((length($prefix_a)+length($suffix_a))*100 >= @$a*10) &&
+ ((length($prefix_b)+length($suffix_b))*100 >= @$b*10);
}
--
2.4.4.410.g43ed522.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html