Patchwork [v3] cmdutil: add within-line color diff capacity

login
register
mail settings
Submitter Denis Laxalde
Date Nov. 23, 2017, 8:12 p.m.
Message ID <713fa75c-cac7-a7c6-4052-bb3884c99e51@laxalde.org>
Download mbox | patch
Permalink /patch/25729/
State Accepted, archived
Headers show

Comments

Denis Laxalde - Nov. 23, 2017, 8:12 p.m.
matthieu.laneuville@octobus.net a écrit :
> # HG changeset patch
> # User Matthieu Laneuville <matthieu.laneuville@octobus.net>
> # Date 1508944418 -32400
> #      Thu Oct 26 00:13:38 2017 +0900
> # Node ID e4b4d581af0fc4e39a28150470b5a658d8315a3f
> # Parent  75013952d8d9608f73cd45f68405fbd6ec112bf2
> # EXP-Topic hg248
> cmdutil: add within-line color diff capacity
> 
> The `diff' command usually writes deletion in red and insertions in green. This
> patch adds within-line colors, to highlight which part of the lines differ.
> 
> The patch passes all tests except `test-diff-color.t' that had to be amended
> because of the new default inline coloring.

The paragraph above reads strangely. It's usual that tests are updated,
no need to mention it I think.

> 
> The current implementation is kept behind an experimental flag in order to test
> the effect on performance. In order to activate it, set inline-color-diff to
> true in [experimental].

Some remarks below from a quick read. I did not dive into the algorithm 
details. Nevertheless, I tried the feature on a couple of changesets in 
Mercurial and found that this behaves strangely sometimes. For instance 
on b445fc69b86cea1d0d81590dd6260bd3c513ed23, it gives:

(using *text* for bold colors)

As you can notice, the line with "padding: " is wrong as the bold text 
should be "5px" and "4px", not "padding".

> 
> diff -r 75013952d8d9 -r e4b4d581af0f mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py	Fri Nov 10 19:14:06 2017 +0800
> +++ b/mercurial/cmdutil.py	Thu Oct 26 00:13:38 2017 +0900
> @@ -7,6 +7,7 @@
>   
>   from __future__ import absolute_import
>   
> +import difflib
>   import errno
>   import itertools
>   import os
> @@ -1513,6 +1514,11 @@ def diffordiffstat(ui, repo, diffopts, n
>                   ui.warn(_('warning: %s not inside relative root %s\n') % (
>                       match.uipath(matchroot), uirelroot))
>   
> +    store = {
> +        'diff.inserted': [],
> +        'diff.deleted': []
> +    }
> +    status = False
>       if stat:
>           diffopts = diffopts.copy(context=0)
>           width = 80
> @@ -1529,7 +1535,61 @@ def diffordiffstat(ui, repo, diffopts, n
>                                            changes, diffopts, prefix=prefix,
>                                            relroot=relroot,
>                                            hunksfilterfn=hunksfilterfn):
> -            write(chunk, label=label)
> +
> +            if not ui.configbool("experimental", "inline-color-diff"):
> +                write(chunk, label=label)
> +                continue
> +
> +            # Each deleted/inserted chunk is followed by an EOL chunk with ''
> +            # label. The 'status' flag helps us grab that second line.
> +            if label in ['diff.deleted', 'diff.inserted'] or status:
> +                if status:
> +                    store[status].append(chunk)
> +                    status = False
> +                else:
> +                    store[label].append(chunk)
> +                    status = label
> +                continue
> +
> +            if store['diff.inserted'] or store['diff.deleted']:
> +                # It is possible that the amount of deleted/inserted lines
> +                # differ, therefore we have to pad them before 1-on-1 comparison
> +                while len(store['diff.deleted']) < len(store['diff.inserted']):
> +                    store['diff.deleted'].append(None)
> +                while len(store['diff.deleted']) > len(store['diff.inserted']):
> +                    store['diff.inserted'].append(None)

Instead of the padding above, can itertools.izip_longest() be used below?

> +                # First print all deletions
> +                for insert, delete in zip(store['diff.inserted'],
> +                                          store['diff.deleted']):
> +                    if not delete:
> +                        continue
> +                    if not insert: # no matching insertion, no diff needed
> +                        write(delete, label='diff.deleted')
> +
> +                    else:
> +                        buff = _inlinediff(insert, delete, direction='deleted')
> +                        for line in buff:
> +                            write(line[1], label=line[0])
> +
> +                # Then print all deletions
> +                for insert, delete in zip(store['diff.inserted'],
> +                                          store['diff.deleted']):
> +                    if not insert:
> +                        continue
> +                    if not delete: # no matching insertion, no diff needed
> +                        write(insert, label='diff.inserted')
> +
> +                    else:
> +                        buff = _inlinediff(insert, delete, direction='inserted')
> +                        for line in buff:
> +                            write(line[1], label=line[0])
> +
> +                store['diff.inserted'] = []
> +                store['diff.deleted'] = []
> +
> +            if chunk:
> +                write(chunk, label=label)

Can this code be extracted in a function? At a first glance I'd suggest
a generator function yielding (chunk, label) to call write() on.

>   
>       if listsubrepos:
>           ctx1 = repo[node1]
> @@ -1548,6 +1608,23 @@ def diffordiffstat(ui, repo, diffopts, n
>               sub.diff(ui, diffopts, tempnode2, submatch, changes=changes,
>                        stat=stat, fp=fp, prefix=prefix)
>   
> +def _inlinediff(s1, s2, direction):
> +    '''Perform string diff to highlight specific changes.'''
> +    direction_skip = '+?' if direction == 'deleted' else '-?'
> +    s = difflib.ndiff(s2.split(' '), s1.split(' '))
> +    # buffer required to remove last space, there may be smarter ways to do this
> +    buff = []
> +    for line in s:
> +        if line[0] in direction_skip:
> +            continue
> +        l = 'diff.' + direction + '.highlight'
> +        if line[0] == ' ':
> +            l = 'diff.' + direction
> +        buff.append((l, line[2:] + ' '))
> +
> +    buff[-1] = (buff[-1][0], buff[-1][1].strip(' '))
> +    return buff
> +

I think it'd be nice to have unit tests (or doctests) for the function 
above and the extra logic in diffordiffstat().


>   def _changesetlabels(ctx):
>       labels = ['log.changeset', 'changeset.%s' % ctx.phasestr()]
>       if ctx.obsolete():
> diff -r 75013952d8d9 -r e4b4d581af0f mercurial/color.py
> --- a/mercurial/color.py	Fri Nov 10 19:14:06 2017 +0800
> +++ b/mercurial/color.py	Thu Oct 26 00:13:38 2017 +0900
> @@ -87,12 +87,14 @@ except ImportError:
>       'branches.inactive': 'none',
>       'diff.changed': 'white',
>       'diff.deleted': 'red',
> +    'diff.deleted.highlight': 'red bold',
>       'diff.diffline': 'bold',
>       'diff.extended': 'cyan bold',
>       'diff.file_a': 'red bold',
>       'diff.file_b': 'green bold',
>       'diff.hunk': 'magenta',
>       'diff.inserted': 'green',
> +    'diff.inserted.highlight': 'green bold',
>       'diff.tab': '',
>       'diff.trailingwhitespace': 'bold red_background',
>       'changeset.public': '',
> diff -r 75013952d8d9 -r e4b4d581af0f mercurial/configitems.py
> --- a/mercurial/configitems.py	Fri Nov 10 19:14:06 2017 +0800
> +++ b/mercurial/configitems.py	Thu Oct 26 00:13:38 2017 +0900
> @@ -388,6 +388,9 @@ coreconfigitem('experimental', 'evolutio
>   coreconfigitem('experimental', 'evolution.track-operation',
>       default=True,
>   )
> +coreconfigitem('experimental', 'inline-color-diff',
> +    default=False,
> +)
>   coreconfigitem('experimental', 'maxdeltachainspan',
>       default=-1,
>   )
> diff -r 75013952d8d9 -r e4b4d581af0f tests/test-diff-color.t
> --- a/tests/test-diff-color.t	Fri Nov 10 19:14:06 2017 +0800
> +++ b/tests/test-diff-color.t	Thu Oct 26 00:13:38 2017 +0900

It seems that activation of the option is missing here.

> @@ -45,8 +45,8 @@ default context
>      c
>      a
>      a
> -  \x1b[0;31m-b\x1b[0m (esc)
> -  \x1b[0;32m+dd\x1b[0m (esc)
> +  \x1b[0;31;1m-b\x1b[0m (esc)
> +  \x1b[0;32;1m+dd\x1b[0m (esc)
>      a
>      a
>      c
> @@ -93,8 +93,8 @@ default context
>      c
>      a
>      a
> -  \x1b[0;31m-b\x1b[0m (esc)
> -  \x1b[0;32m+dd\x1b[0m (esc)
> +  \x1b[0;31;1m-b\x1b[0m (esc)
> +  \x1b[0;32;1m+dd\x1b[0m (esc)
>      a
>      a
>      c
> @@ -108,8 +108,8 @@ default context
>     \x1b[0;35m@@ -3,5 +3,5 @@\x1b[0m (esc)
>      a
>      a
> -  \x1b[0;31m-b\x1b[0m (esc)
> -  \x1b[0;32m+dd\x1b[0m (esc)
> +  \x1b[0;31;1m-b\x1b[0m (esc)
> +  \x1b[0;32;1m+dd\x1b[0m (esc)
>      a
>      a
>   
> @@ -236,11 +236,11 @@ test tabs
>      c
>      c
>     \x1b[0;32m+aa\x1b[0m (esc)
> -  \x1b[0;32m+\x1b[0m	\x1b[0;32mone tab\x1b[0m (esc)
> -  \x1b[0;32m+\x1b[0m		\x1b[0;32mtwo tabs\x1b[0m (esc)
> -  \x1b[0;32m+end tab\x1b[0m\x1b[0;1;41m	\x1b[0m (esc)
> -  \x1b[0;32m+mid\x1b[0m	\x1b[0;32mtab\x1b[0m (esc)
> -  \x1b[0;32m+\x1b[0m	\x1b[0;32mall\x1b[0m		\x1b[0;32mtabs\x1b[0m\x1b[0;1;41m	\x1b[0m (esc)
> +  \x1b[0;32m+\x1b[0m\x1b[0;32m	\x1b[0m\x1b[0;32mone tab\x1b[0m (esc)
> +  \x1b[0;32m+\x1b[0m\x1b[0;32m		\x1b[0m\x1b[0;32mtwo tabs\x1b[0m (esc)
> +  \x1b[0;32m+end tab\x1b[0m\x1b[0;32m	\x1b[0m (esc)
> +  \x1b[0;32m+mid\x1b[0m\x1b[0;32m	\x1b[0m\x1b[0;32mtab\x1b[0m (esc)
> +  \x1b[0;32m+\x1b[0m\x1b[0;32m	\x1b[0m\x1b[0;32mall\x1b[0m\x1b[0;32m		\x1b[0m\x1b[0;32mtabs\x1b[0m\x1b[0;32m	\x1b[0m (esc)
>     $ echo "[color]" >> $HGRCPATH
>     $ echo "diff.tab = bold magenta" >> $HGRCPATH
>     $ hg diff --nodates
> @@ -252,10 +252,10 @@ test tabs
>      c
>      c
>     \x1b[0;32m+aa\x1b[0m (esc)
> -  \x1b[0;32m+\x1b[0m\x1b[0;1;35m	\x1b[0m\x1b[0;32mone tab\x1b[0m (esc)
> -  \x1b[0;32m+\x1b[0m\x1b[0;1;35m		\x1b[0m\x1b[0;32mtwo tabs\x1b[0m (esc)
> -  \x1b[0;32m+end tab\x1b[0m\x1b[0;1;41m	\x1b[0m (esc)
> -  \x1b[0;32m+mid\x1b[0m\x1b[0;1;35m	\x1b[0m\x1b[0;32mtab\x1b[0m (esc)
> -  \x1b[0;32m+\x1b[0m\x1b[0;1;35m	\x1b[0m\x1b[0;32mall\x1b[0m\x1b[0;1;35m		\x1b[0m\x1b[0;32mtabs\x1b[0m\x1b[0;1;41m	\x1b[0m (esc)
> +  \x1b[0;32m+\x1b[0m\x1b[0;32m	\x1b[0m\x1b[0;32mone tab\x1b[0m (esc)
> +  \x1b[0;32m+\x1b[0m\x1b[0;32m		\x1b[0m\x1b[0;32mtwo tabs\x1b[0m (esc)
> +  \x1b[0;32m+end tab\x1b[0m\x1b[0;32m	\x1b[0m (esc)
> +  \x1b[0;32m+mid\x1b[0m\x1b[0;32m	\x1b[0m\x1b[0;32mtab\x1b[0m (esc)
> +  \x1b[0;32m+\x1b[0m\x1b[0;32m	\x1b[0m\x1b[0;32mall\x1b[0m\x1b[0;32m		\x1b[0m\x1b[0;32mtabs\x1b[0m\x1b[0;32m	\x1b[0m (esc)
>   
>     $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

Patch

diff --git a/mercurial/templates/static/style-gitweb.css 
b/mercurial/templates/static/style-gitweb.css
--- a/mercurial/templates/static/style-gitweb.css
+++ b/mercurial/templates/static/style-gitweb.css
@@ -191,10 +191,9 @@  pre.sourcelines > span.followlines-selec
  }

  div#followlines {
-  background-color: *#B7B7B7*;
-  border: 1px solid *#CCC*;
-  *border-radius*: 5px;
-  padding: 4px;
+  background-color: *#FFF*;
+  border: 1px solid *#d9d8d1*;
+  *padding*: 5px;
    position: fixed;
  }