Patchwork D2692: merge: return an attrs class from update() and applyupdates()

login
register
mail settings
Submitter phabricator
Date March 5, 2018, 6:17 a.m.
Message ID <differential-rev-PHID-DREV-a2kavgaphyargxftg34h-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/29044/
State Superseded
Headers show

Comments

phabricator - March 5, 2018, 6:17 a.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Previously, we returned a tuple containing counts. The result of an
  update is kind of complex and the use of tuples with nameless fields
  made the code a bit harder to read and constrained future expansion
  of the return value.
  
  Let's invent an attrs-defined class for representing the result of
  an update operation.
  
  We provide __getitem__ and __len__ implementations for backwards
  compatibility as a container type to minimize code churn.
  
  In (at least) Python 2, the % operator seems to insist on using
  tuples. So we had to update a consumer using the % operator.
  
  .. api::
  
    merge.update() and merge.applyupdates() now return a class
    with named attributes instead of a tuple. Switch consumers
    to access elements by name instead of by offset.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/hg.py
  mercurial/merge.py

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - March 22, 2018, 12:16 a.m.
martinvonz added inline comments.

INLINE COMMENTS

> merge.py:1403-1404
>  
> +@attr.s(frozen=True)
> +class updateresult(object):
> +    updatedcount = attr.ib()

mpm made me use a tuple with __slots__ for scmutil.status. Any idea when that's better? I think it was something about performance, but we don't care about that here.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - March 22, 2018, 12:20 a.m.
indygreg added a comment.


  `__slots__` allocates enough space for the exact set of attributes specified rather than backing object instances by `__dict__`. If you create thousands of small objects that all have the same fields, `__slots__` can yield some performance wins.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers, martinvonz
Cc: martinvonz, mercurial-devel

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -22,6 +22,9 @@ 
     nullid,
     nullrev,
 )
+from .thirdparty import (
+    attr,
+)
 from . import (
     copies,
     error,
@@ -1397,6 +1400,30 @@ 
     prefetch = scmutil.fileprefetchhooks
     prefetch(repo, ctx, [f for sublist in oplist for f, args, msg in sublist])
 
+@attr.s(frozen=True)
+class updateresult(object):
+    updatedcount = attr.ib()
+    mergedcount = attr.ib()
+    removedcount = attr.ib()
+    unresolvedcount = attr.ib()
+
+    # TODO remove container emulation once consumers switch to new API.
+
+    def __getitem__(self, x):
+        if x == 0:
+            return self.updatedcount
+        elif x == 1:
+            return self.mergedcount
+        elif x == 2:
+            return self.removedcount
+        elif x == 3:
+            return self.unresolvedcount
+        else:
+            raise IndexError('can only access items 0-3')
+
+    def __len__(self):
+        return 4
+
 def applyupdates(repo, actions, wctx, mctx, overwrite, labels=None):
     """apply the merge action list to the working directory
 
@@ -1580,7 +1607,8 @@ 
         if not proceed:
             # XXX setting unresolved to at least 1 is a hack to make sure we
             # error out
-            return updated, merged, removed, max(len(unresolvedf), 1)
+            return updateresult(updated, merged, removed,
+                                max(len(unresolvedf), 1))
         newactions = []
         for f, args, msg in mergeactions:
             if f in unresolvedf:
@@ -1655,8 +1683,7 @@ 
         actions['m'] = [a for a in actions['m'] if a[0] in mfiles]
 
     progress(_updating, None, total=numupdates, unit=_files)
-
-    return updated, merged, removed, unresolved
+    return updateresult(updated, merged, removed, unresolved)
 
 def recordupdates(repo, actions, branchmerge):
     "record merge actions to the dirstate"
@@ -1877,7 +1904,7 @@ 
                 # call the hooks and exit early
                 repo.hook('preupdate', throw=True, parent1=xp2, parent2='')
                 repo.hook('update', parent1=xp2, parent2='', error=0)
-                return 0, 0, 0, 0
+                return updateresult(0, 0, 0, 0)
 
             if (updatecheck == 'linear' and
                     pas not in ([p1], [p2])):  # nonlinear
diff --git a/mercurial/hg.py b/mercurial/hg.py
--- a/mercurial/hg.py
+++ b/mercurial/hg.py
@@ -747,7 +747,9 @@ 
     if quietempty and not any(stats):
         return
     repo.ui.status(_("%d files updated, %d files merged, "
-                     "%d files removed, %d files unresolved\n") % stats)
+                     "%d files removed, %d files unresolved\n") % (
+                   stats.updatedcount, stats.mergedcount,
+                   stats.removedcount, stats.unresolvedcount))
 
 def updaterepo(repo, node, overwrite, updatecheck=None):
     """Update the working directory to node.