Re: [PATCH] Improve contrib/diff-highlight to highlight unevenly-sized hunks

From: Patrick Palka <patrick@parcs.ath.cx>
Date: Jun 18, 2015, 20:14 UTC
Message-ID: <alpine.DEB.2.20.8.1506181536070.4322@idea>
In-Reply-To: <20150618190417.GA12769@peff.net>

On Thu, 18 Jun 2015, Jeff King wrote:

Show 29 quoted lines

> 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