This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3210

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

Authored by quark on Apr 9 2018, 6:59 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHG8d730f96e792: patch: move yielding "\n" to the end of loop
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.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

quark created this revision.Apr 9 2018, 6:59 PM
yuja added a subscriber: yuja.Apr 10 2018, 10:38 AM

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

quark added a comment.Apr 10 2018, 3:09 PM

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.

yuja added a comment.Apr 11 2018, 10:34 AM

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()'''
quark added a comment.Apr 11 2018, 6:08 PM

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

yuja added a comment.Apr 12 2018, 7:56 AM

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):], '')
durin42 accepted this revision.Apr 16 2018, 7:00 PM
This revision is now accepted and ready to land.Apr 16 2018, 7:00 PM
Closed by commit rHG8d730f96e792: patch: move yielding "\n" to the end of loop (authored by quark). · Explain WhyApr 16 2018, 7:11 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/patch.py (5 lines)

Diff 8333

mercurial/patch.py

('-', 'diff.deleted'), ('-', 'diff.deleted'),
('+', 'diff.inserted')] ('+', 'diff.inserted')]
head = False head = False
for chunk in func(*args, **kw): for chunk in func(*args, **kw):
lines = chunk.split('\n') lines = chunk.split('\n')
matches = {} matches = {}
if inlinecolor: if inlinecolor:
matches = _findmatches(lines) matches = _findmatches(lines)
linecount = len(lines)
for i, line in enumerate(lines): for i, line in enumerate(lines):
if i != 0:
yield ('\n', '')
if head: if head:
if line.startswith('@'): if line.startswith('@'):
head = False head = False
else: else:
if line and not line.startswith((' ', '+', '-', '@', '\\')): if line and not line.startswith((' ', '+', '-', '@', '\\')):
head = True head = True
stripline = line stripline = line
diffline = False diffline = False
yield (token, label) yield (token, label)
else: else:
yield (stripline, label) yield (stripline, label)
break break
else: else:
yield (line, '') yield (line, '')
if line != stripline: if line != stripline:
yield (line[len(stripline):], 'diff.trailingwhitespace') yield (line[len(stripline):], 'diff.trailingwhitespace')
if i + 1 < linecount:
yield ('\n', '')
def _findmatches(slist): def _findmatches(slist):
'''Look for insertion matches to deletion and returns a dict of '''Look for insertion matches to deletion and returns a dict of
correspondences. correspondences.
''' '''
lastmatch = 0 lastmatch = 0
matches = {} matches = {}
for i, line in enumerate(slist): for i, line in enumerate(slist):