Patchwork D3210: patch: move yielding "\n" to the end of loop

login
register
mail settings
Submitter phabricator
Date April 9, 2018, 10:59 p.m.
Message ID <differential-rev-PHID-DREV-txfrrbsqexbdsle7y3ej-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/30619/
State Superseded
Headers show

Comments

phabricator - April 9, 2018, 10:59 p.m.
quark created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The original logic makes it harder to reason about - it yields the "\n"
  character belonging to the last line in the next loop iteration.
  
  The new code is in theory a little bit slower. But is more readable. It
  makes the following changes easier to read.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3210

AFFECTED FILES
  mercurial/patch.py

CHANGE DETAILS




To: quark, #hg-reviewers
Cc: mercurial-devel
phabricator - April 10, 2018, 2:38 p.m.
yuja added a comment.


  Perhaps, it's simpler to use `mdiff.splitnewlines(chunk)` instead of `chunk.split('\n')`
  because otherwise the next patch has to reconstruct `line + '\n'`.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3210

To: quark, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - April 10, 2018, 7:09 p.m.
quark added a comment.


  That's ideal. But a lot of code in this area expects "line" to not contain "\n". So the change won't be as easy as it looks.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3210

To: quark, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - April 11, 2018, 2:34 p.m.
yuja added a comment.


  It's just a few-line patch after removal of the current worddiff, and can be
  simplified further.
  
  Anyway, I can clean up later.
  
    diff --git a/mercurial/patch.py b/mercurial/patch.py
    --- a/mercurial/patch.py
    +++ b/mercurial/patch.py
    @@ -2490,10 +2490,8 @@ def difflabel(func, *args, **kw):
                         ('+', 'diff.inserted')]
         head = False
         for chunk in func(*args, **kw):
    -        lines = chunk.split('\n')
    -        for i, line in enumerate(lines):
    -            if i != 0:
    -                yield ('\n', '')
    +        for rawline in mdiff.splitnewlines(chunk):
    +            line = rawline.rstrip('\n')
                 if head:
                     if line.startswith('@'):
                         head = False
    @@ -2526,6 +2524,8 @@ def difflabel(func, *args, **kw):
                     yield (line, '')
                 if line != stripline:
                     yield (line[len(stripline):], 'diff.trailingwhitespace')
    +            if rawline != line:
    +                yield (rawline[len(line):], '')
     
     def diffui(*args, **kw):
         '''like diff(), but yields 2-tuples of (output, label) for ui.write()'''

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3210

To: quark, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - April 11, 2018, 10:08 p.m.
quark added a comment.


  I thought it was `for line in mdiff.splitnewlines(...)`. If we have both `rawline` and `line` variables, then it is easier.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3210

To: quark, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - April 12, 2018, 11:56 a.m.
yuja added a comment.


  `rawline` can be removed after the rewrite of +/- lines handling.
  
    -        for rawline in mdiff.splitnewlines(chunk):
    -            line = rawline.rstrip('\n')
    +        for line in mdiff.splitnewlines(chunk):
                 if head:
                     if line.startswith('@'):
                         head = False
                 else:
    -                if line and not line.startswith((' ', '+', '-', '@', '\\')):
    +                if not line.startswith((' ', '+', '-', '@', '\\', '\n')):
                         head = True
     
    @@ -2525,13 +2523,15 @@ def difflabel(func, *args, **kw):
                 if head:
                     prefixes = headprefixes
                 for prefix, label in prefixes:
    -                if stripline.startswith(prefix):
    -                    yield (stripline, label)
    +                if line.startswith(prefix):
    +                    if line.endswith('\n'):
    +                        yield (line[:-1], label)
    +                        yield ('\n', '')
    +                    else:
    +                        yield (line, label)
                         break
                 else:
                     yield (line, '')
    -            if rawline != line:
    -                yield (rawline[len(line):], '')

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3210

To: quark, #hg-reviewers
Cc: yuja, mercurial-devel

Patch

diff --git a/mercurial/patch.py b/mercurial/patch.py
--- a/mercurial/patch.py
+++ b/mercurial/patch.py
@@ -2499,9 +2499,8 @@ 
         matches = {}
         if inlinecolor:
             matches = _findmatches(lines)
+        linecount = len(lines)
         for i, line in enumerate(lines):
-            if i != 0:
-                yield ('\n', '')
             if head:
                 if line.startswith('@'):
                     head = False
@@ -2540,6 +2539,8 @@ 
                 yield (line, '')
             if line != stripline:
                 yield (line[len(stripline):], 'diff.trailingwhitespace')
+            if i + 1 < linecount:
+                yield ('\n', '')
 
 def _findmatches(slist):
     '''Look for insertion matches to deletion and returns a dict of