Second version of my diff.tab color extension patches
Jordi Gutiérrez Hermoso
jordigh at octave.org
Sun Aug 24 22:47:49 UTC 2014
This was originally my [0 of 2] email to the list which explained my
new version of my diff.tab color extension patches , but it's being
rejected on automatic filters.
Matt, c'mon, get rid of that filter. It's a totally reasonable thing
to send 0 of N emails when explaining how a new version of a patch
series differs from the previous one. It goes together with the
patches, and the reference to past versions of a patch should not go
inside the commit messages of the new version.
On Thu, 2014-08-21 at 00:34 -0700, Pierre-Yves David wrote:
> On 08/20/2014 12:24 PM, Jordi Gutiérrez Hermoso wrote:
[snip]
> > @@ -1669,15 +1670,26 @@ def difflabel(func, *args, **kw):
> > if line and line[0] not in ' +-@\\':
> > head = True
> > stripline = line
> > + showtabs = False
> > if not head and line and line[0] in '+-':
> > - # highlight trailing whitespace, but only in changed lines
> > + # highlight tabs and trailing whitespace, but only in
> > + # changed lines
> > stripline = line.rstrip()
> > + showtabs = True
>
> consider something more generic than "showtabs" like "diff line" or
> "touched line"
Okay, I went with "diffline".
> > @@ -18,6 +18,7 @@ from node import hex, short
> > import base85, mdiff, scmutil, util, diffhelpers, copies, encoding, error
> >
> > gitre = re.compile('diff --git a/(.*) b/(.*)')
> > +tabsplitter = re.compile(r'(\t+|[^\t]+)')
[snip]
> > +
> > prefixes = textprefixes
> > if head:
> > prefixes = headprefixes
> > for prefix, label in prefixes:
> > if stripline.startswith(prefix):
> > - yield (stripline, label)
> > + if showtabs:
> > + for token in tabsplitter.findall(stripline):
> > + if token == '\t':
> > + yield (token, 'diff.tab')
>
> you regexp match multiple tab. you conditional does not.
Oops, that was dumb. Thanks for catching that. I've fixed it.
> Not super fan of the regexp approach but I've not good idea for now. I
> wonder what is the performance of it.
I don't know what alternatives there are, nor do I think we can do
much better than a regexp. Fundamentally, I think we have to split up
a line into tab and non-tab blocks. I don't think this can be avoided.
> This patch want a test to convince people it actually works.
Done, this version now includes a test.
On Thu, 2014-08-21 at 00:40 -0700, Pierre-Yves David wrote:
> while I love the idea of having the option to colorize tab, I do not
> believe we can do it by default. There is real people out there that use
> tab for indentation. I do not hink they will be happy to have their diff
> turning pink all around the place.
>
> We can either try to target mixed tab and space. Or just leave the
> default to "not colorise" and suggest this in the example config.
Okay, I went with the more conservative approach in the second patch.
No BC, just some documentation that suggests that colourising tabs is
possible.
More information about the Mercurial-devel
mailing list