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