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
Lint Skipped
Unit
Unit Tests Skipped

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 7922

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):