From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: 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 21:49:19 -0700 [thread overview]
Message-ID: <xmqqmvzwb8vk.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <20150619035408.GA23679@peff.net> (Jeff King's message of "Thu, 18 Jun 2015 23:54:09 -0400")
Jeff King <peff@peff.net> writes:
> ... I think you could also argue that
> because of whitespace-highlighting, colorized diffs are fundamentally
> going to have colors intermingled with the content and should not be
> parsed this way.
Painting of whitespace breakages is asymmetric [*1*]. If you change
something on a badly indented line without fixing the indentation,
e.g.
-<SP><TAB>hello-world
+<SP><TAB>hello-people
only the space-before-tab on the latter line is painted.
For the purpose of your diff highlighting, however, you would want
to treat the leading "<SP><TAB>hello-" on preimage and postimage
lines as unchanged.
Which means that you shouldn't be reading a colored output without
ignoring the color-control sequences.
So I think you arrived at the correct conclusion.
> All the more reason to try to move this inside diff.c, I guess.
That too, probably.
If we were to do this, I think it may make sense to separate the
logic to compute which span of the string need to be painted in what
color and the routine to actually emit the colored output. That is,
instead of letting ws-check-emit to decide which part should be in
what color _and_ emitting the result, we should have a helper that
reads <line, len>, and give us an array of spans to flag as
whitespace violation. Then an optional diff-highlight code would
scan the same <line, len> (or a collection of <line, len>) without
getting confused by the whitespace errors and annotate the spans to
be highlighted. After all that happens, the output routine would
coalesce the spans and produce colored output.
Or something like that ;-)
[Footnote]
*1* We recently added a knob to allow us paint them on preimage and
common lines, too, but that is not the default.
next prev parent reply other threads:[~2015-06-19 4:49 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
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 [this message]
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=xmqqmvzwb8vk.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=patrick@parcs.ath.cx \
--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.