From: Patrick Palka <patrick@parcs.ath.cx>
To: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>,
Patrick Palka <patrick@parcs.ath.cx>,
git@vger.kernel.org
Subject: Re: [PATCH] Improve contrib/diff-highlight to highlight unevenly-sized hunks
Date: Thu, 18 Jun 2015 16:14:19 -0400 (EDT) [thread overview]
Message-ID: <alpine.DEB.2.20.8.1506181536070.4322@idea> (raw)
In-Reply-To: <20150618190417.GA12769@peff.net>
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
next prev parent reply other threads:[~2015-06-18 20:14 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-15 17:20 [PATCH] Improve contrib/diff-highlight to highlight unevenly-sized hunks Patrick Palka
2015-06-18 15:50 ` Junio C Hamano
2015-06-18 16:28 ` Patrick Palka
2015-06-18 18:08 ` Junio C Hamano
2015-06-18 19:04 ` Jeff King
2015-06-18 20:14 ` Patrick Palka [this message]
2015-06-18 20:45 ` Jeff King
2015-06-18 21:23 ` Jeff King
2015-06-18 21:39 ` Junio C Hamano
2015-06-18 22:25 ` Patrick Palka
2015-06-19 3:54 ` Jeff King
2015-06-19 4:49 ` Junio C Hamano
2015-06-19 5:32 ` Jeff King
2015-06-19 7:34 ` Jeff King
2015-06-19 11:38 ` Jeff King
2015-06-19 17:20 ` Junio C Hamano
2015-06-18 23:06 ` Patrick Palka
2015-06-18 20:23 ` Patrick Palka
2015-06-18 19:08 ` Jeff King
2015-06-18 20:27 ` Patrick Palka
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.20.8.1506181536070.4322@idea \
--to=patrick@parcs.ath.cx \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.