Patchwork [V2] patch: don't separate \r and \n when colorizing diff output

login
register
mail settings
Submitter via Mercurial-devel
Date July 11, 2018, 1:03 p.m.
Message ID <dc12175e9e5ba1bcb8ed.1531314222@firefly.edlund.dk>
Download mbox | patch
Permalink /patch/32771/
State New
Headers show

Comments

via Mercurial-devel - July 11, 2018, 1:03 p.m.
# HG changeset patch
# User Sune Foldager <cryo@cyanite.org>
# Date 1531314149 -7200
#      Wed Jul 11 15:02:29 2018 +0200
# Node ID dc12175e9e5ba1bcb8ed7658a9218211727a9c49
# Parent  4d5fb4062f0bb159230062701461fa6cab9b539b
patch: don't separate \r and \n when colorizing diff output

When displaying diffs, \r at the end of a line is treated as trailing
whitespace. This causes an ANSI escape code to be inserted between \r and \n.
Some programs, such as less since version 530 (maybe earlier, but at least not
version 487) displays ^M when it encounters a lone \r. This causes a lot of
noise in diff output on Windows, where \r\n is used to terminate lines.

We avoid that by treating both \n and \r\n as end of line when considering
trailing whitespace.

A test case for trailing whitespace has also been added.
Yuya Nishihara - July 11, 2018, 1:59 p.m.
On Wed, 11 Jul 2018 15:03:42 +0200, Sune Foldager via Mercurial-devel wrote:
> # HG changeset patch
> # User Sune Foldager <cryo@cyanite.org>
> # Date 1531314149 -7200
> #      Wed Jul 11 15:02:29 2018 +0200
> # Node ID dc12175e9e5ba1bcb8ed7658a9218211727a9c49
> # Parent  4d5fb4062f0bb159230062701461fa6cab9b539b
> patch: don't separate \r and \n when colorizing diff output

V1 is already in. Can you send only the test?

> +trailing whitespace
> +
> +  $ sed -i 's/^dd$/dd \r/' a

+  tests/test-diff-color.t:56:
+   >   $ sed -i 's/^dd$/dd \r/' a
+   don't use 'sed -i', use a temporary file
+  tests/test-diff-color.t:71:
+   >   $ sed -i 's/\s*\r$//' a
+   don't use 'sed -i', use a temporary file
+  [1]
via Mercurial-devel - July 11, 2018, 2:10 p.m.
It’s not in the repo as far as I can see, but ok then...

> On 11 Jul 2018, at 15.59, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> On Wed, 11 Jul 2018 15:03:42 +0200, Sune Foldager via Mercurial-devel wrote:
>> # HG changeset patch
>> # User Sune Foldager <cryo@cyanite.org>
>> # Date 1531314149 -7200
>> #      Wed Jul 11 15:02:29 2018 +0200
>> # Node ID dc12175e9e5ba1bcb8ed7658a9218211727a9c49
>> # Parent  4d5fb4062f0bb159230062701461fa6cab9b539b
>> patch: don't separate \r and \n when colorizing diff output
> 
> V1 is already in. Can you send only the test?
> 
>> +trailing whitespace
>> +
>> +  $ sed -i 's/^dd$/dd \r/' a
> 
> +  tests/test-diff-color.t:56:
> +   >   $ sed -i 's/^dd$/dd \r/' a
> +   don't use 'sed -i', use a temporary file
> +  tests/test-diff-color.t:71:
> +   >   $ sed -i 's/\s*\r$//' a
> +   don't use 'sed -i', use a temporary file
> +  [1]
Yuya Nishihara - July 11, 2018, 2:22 p.m.
On Wed, 11 Jul 2018 16:10:32 +0200, Sune Foldager wrote:
> It’s not in the repo as far as I can see, but ok then...

Maybe you aren't based on the hg-committed repo?

https://www.mercurial-scm.org/repo/hg-committed/

> >> +trailing whitespace
> >> +
> >> +  $ sed -i 's/^dd$/dd \r/' a
> > 
> > +  tests/test-diff-color.t:56:
> > +   >   $ sed -i 's/^dd$/dd \r/' a
> > +   don't use 'sed -i', use a temporary file
> > +  tests/test-diff-color.t:71:
> > +   >   $ sed -i 's/\s*\r$//' a
> > +   don't use 'sed -i', use a temporary file
> > +  [1]

Also please fix the check-code failure.
via Mercurial-devel - July 11, 2018, 2:26 p.m.
> On 11 Jul 2018, at 16.22, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> On Wed, 11 Jul 2018 16:10:32 +0200, Sune Foldager wrote:
>> It’s not in the repo as far as I can see, but ok then...
> 
> Maybe you aren't based on the hg-committed repo?
> 
> https://www.mercurial-scm.org/repo/hg-committed/

Never heard of it, so no :p. 

> 
>>>> +trailing whitespace
>>>> +
>>>> +  $ sed -i 's/^dd$/dd \r/' a
>>> 
>>> +  tests/test-diff-color.t:56:
>>> +   >   $ sed -i 's/^dd$/dd \r/' a
>>> +   don't use 'sed -i', use a temporary file
>>> +  tests/test-diff-color.t:71:
>>> +   >   $ sed -i 's/\s*\r$//' a
>>> +   don't use 'sed -i', use a temporary file
>>> +  [1]
> 
> Also please fix the check-code failure.

Hm what’s wrong with -i? At any rate, I don’t have time to do this today. 

/Sune
Yuya Nishihara - July 11, 2018, 2:29 p.m.
On Wed, 11 Jul 2018 16:26:25 +0200, Sune Foldager wrote:
> Hm what’s wrong with -i? At any rate, I don’t have time to do this today.

Perhaps, it's GNU extension.
Augie Fackler - July 11, 2018, 3:21 p.m.
> On Jul 11, 2018, at 10:26, Sune Foldager via Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
> 
>> Also please fix the check-code failure.
> 
> Hm what’s wrong with -i? At any rate, I don’t have time to do this today. 

-i was added with different behaviors on BSD and GNU sed. It's incompatible between those seds.
via Mercurial-devel - July 11, 2018, 9:33 p.m.
> On 11 Jul 2018, at 17.21, Augie Fackler <raf@durin42.com> wrote:
> 
> 
> 
>> On Jul 11, 2018, at 10:26, Sune Foldager via Mercurial-devel <mercurial-devel@mercurial-scm.org> wrote:
>> 
>>> Also please fix the check-code failure.
>> 
>> Hm what’s wrong with -i? At any rate, I don’t have time to do this today. 
> 
> -i was added with different behaviors on BSD and GNU sed. It's incompatible between those seds.

Right, although it seems plain -i would work identically on both BSD and GNU, at least from reading the man pages on macOS and Linux. They both edit in-place, but the GNU version can take an optional argument after -i, for backup. Anyway, I changed the patch.

/Sun
Yuya Nishihara - July 12, 2018, 12:12 p.m.
On Wed, 11 Jul 2018 23:33:16 +0200, Sune Foldager wrote:
> >> Hm what’s wrong with -i? At any rate, I don’t have time to do this today. 
> > 
> > -i was added with different behaviors on BSD and GNU sed. It's incompatible between those seds.
> 
> Right, although it seems plain -i would work identically on both BSD and GNU, at least from reading the man pages on macOS and Linux. They both edit in-place, but the GNU version can take an optional argument after -i, for backup. Anyway, I changed the patch.

'sed -i' didn't work on FreeBSD 10. macOS userland isn't pretty much a BSD.

Patch

diff --git a/mercurial/patch.py b/mercurial/patch.py
--- a/mercurial/patch.py
+++ b/mercurial/patch.py
@@ -2405,7 +2405,7 @@  def diffsinglehunk(hunklines):
     """yield tokens for a list of lines in a single hunk"""
     for line in hunklines:
         # chomp
-        chompline = line.rstrip('\n')
+        chompline = line.rstrip('\r\n')
         # highlight tabs and trailing whitespace
         stripline = chompline.rstrip()
         if line.startswith('-'):
@@ -2473,6 +2473,9 @@  def diffsinglehunkinline(hunklines):
             isendofline = token.endswith('\n')
             if isendofline:
                 chomp = token[:-1] # chomp
+                if chomp.endswith('\r'):
+                    chomp = chomp[:-1]
+                endofline = token[len(chomp):]
                 token = chomp.rstrip() # detect spaces at the end
                 endspaces = chomp[len(token):]
             # scan tabs
@@ -2488,7 +2491,7 @@  def diffsinglehunkinline(hunklines):
             if isendofline:
                 if endspaces:
                     yield (endspaces, 'diff.trailingwhitespace')
-                yield ('\n', '')
+                yield (endofline, '')
                 nextisnewline = True
 
 def difflabel(func, *args, **kw):
diff --git a/tests/test-diff-color.t b/tests/test-diff-color.t
--- a/tests/test-diff-color.t
+++ b/tests/test-diff-color.t
@@ -51,6 +51,25 @@  default context
    a
    c
 
+trailing whitespace
+
+  $ sed -i 's/^dd$/dd \r/' a
+  $ hg diff --nodates
+  \x1b[0;1mdiff -r cf9f4ba66af2 a\x1b[0m (esc)
+  \x1b[0;31;1m--- a/a\x1b[0m (esc)
+  \x1b[0;32;1m+++ b/a\x1b[0m (esc)
+  \x1b[0;35m@@ -2,7 +2,7 @@\x1b[0m (esc)
+   c
+   a
+   a
+  \x1b[0;31m-b\x1b[0m (esc)
+  \x1b[0;32m+dd\x1b[0m\x1b[0;1;41m \x1b[0m\r (esc)
+   a
+   a
+   c
+
+  $ sed -i 's/\s*\r$//' a
+
 (check that 'ui.color=yes' match '--color=auto')
 
   $ hg diff --nodates --config ui.formatted=no