Patchwork D3765: progress: create helper class for incrementing progress

login
register
mail settings
Submitter phabricator
Date June 17, 2018, 4:42 p.m.
Message ID <differential-rev-PHID-DREV-wzsbijhrrrtcbfzco4xc-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/32222/
State Superseded
Headers show

Comments

phabricator - June 17, 2018, 4:42 p.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  When using ui.progress(), there's a clear pattern that is followed:
  
  - Pass the same topic and unit
  - Usually pass the same total
  - Call with pos=None to close the progress bar
  - Often keep track of the current position and increment it
  
  This patch creates a simple helper class for this. I'll probably make
  it implement the context manager protocol later (calling update(None)
  on __exit__).
  
  Progress is used in low-level modules like changegroup, so I also
  exposed it via a method on the ui object. Perhaps the class itself
  should also live in ui.py?
  
  This patch also makes merge.oy use it to show that it works.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/merge.py
  mercurial/scmutil.py
  mercurial/ui.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - June 17, 2018, 6:01 p.m.
indygreg accepted this revision.
indygreg added a comment.
This revision is now accepted and ready to land.


  The low-level progress API has always bothered me as well.
  
  Please do follow up and add a context manager to the API!

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - June 17, 2018, 6:12 p.m.
indygreg added a comment.


  Also, it's worth keeping in mind Python function call overhead when working on this code. I'm optimistic that things that need progress bars won't be concerned about this. But it might be worth measuring and keeping in the back of your head.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - June 17, 2018, 6:54 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D3765#59079, @indygreg wrote:
  
  > Also, it's worth keeping in mind Python function call overhead when working on this code. I'm optimistic that things that need progress bars won't be concerned about this. But it might be worth measuring and keeping in the back of your head.
  
  
  Good point. I also think it's unlikely to be noticeable. Also, we can probably inline the ui.progress() logic if it turns out to be an issue.

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -1596,6 +1596,10 @@ 
         else:
             self.debug('%s:%s %d%s\n' % (topic, item, pos, unit))
 
+    def makeprogress(self, topic, unit="", total=None):
+        '''exists only so low-level modules won't need to import scmutil'''
+        return scmutil.progress(self, topic, unit, total)
+
     def log(self, service, *msg, **opts):
         '''hook for logging facility extensions
 
diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -1285,6 +1285,27 @@ 
     return _locksub(repo, repo.currentwlock(), 'HG_WLOCK_LOCKER', cmd, *args,
                     **kwargs)
 
+class progress(object):
+    def __init__(self, ui, topic, unit="", total=None):
+        self.ui = ui
+        self.pos = 0
+        self.topic = topic
+        self.unit = unit
+        self.total = total
+
+    def update(self, pos, item="", total=None):
+        if total:
+            self.total = total
+        self.pos = pos
+        self._print(item)
+
+    def increment(self, step=1, item="", total=None):
+        self.update(self.pos + step, item, total)
+
+    def _print(self, item):
+        self.ui.progress(self.topic, self.pos, item, self.unit,
+                         self.total)
+
 def gdinitconfig(ui):
     """helper function to know if a repo should be created as general delta
     """
diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -1584,10 +1584,6 @@ 
         if f1 != f and move:
             moves.append(f1)
 
-    _updating = _('updating')
-    _files = _('files')
-    progress = repo.ui.progress
-
     # remove renamed files after safely stored
     for f in moves:
         if wctx[f].lexists():
@@ -1597,7 +1593,8 @@ 
 
     numupdates = sum(len(l) for m, l in actions.items()
                      if m != ACTION_KEEP)
-    z = 0
+    progress = repo.ui.makeprogress(_('updating'), unit=_('files'),
+                                    total=numupdates)
 
     if [a for a in actions[ACTION_REMOVE] if a[0] == '.hgsubstate']:
         subrepoutil.submerge(repo, wctx, mctx, wctx, overwrite, labels)
@@ -1614,8 +1611,7 @@ 
             s(_("the remote file has been renamed to %s\n") % f1)
         s(_("resolve manually then use 'hg resolve --mark %s'\n") % f)
         ms.addpath(f, f1, fo)
-        z += 1
-        progress(_updating, z, item=f, total=numupdates, unit=_files)
+        progress.increment(item=f)
 
     # When merging in-memory, we can't support worker processes, so set the
     # per-item cost at 0 in that case.
@@ -1625,8 +1621,7 @@ 
     prog = worker.worker(repo.ui, cost, batchremove, (repo, wctx),
                          actions[ACTION_REMOVE])
     for i, item in prog:
-        z += i
-        progress(_updating, z, item=item, total=numupdates, unit=_files)
+        progress.increment(step=i, item=item)
     removed = len(actions[ACTION_REMOVE])
 
     # resolve path conflicts (must come before getting)
@@ -1638,37 +1633,32 @@ 
             wctx[f].audit()
             wctx[f].write(wctx.filectx(f0).data(), wctx.filectx(f0).flags())
             wctx[f0].remove()
-        z += 1
-        progress(_updating, z, item=f, total=numupdates, unit=_files)
+        progress.increment(item=f)
 
     # get in parallel
     prog = worker.worker(repo.ui, cost, batchget, (repo, mctx, wctx),
                          actions[ACTION_GET])
     for i, item in prog:
-        z += i
-        progress(_updating, z, item=item, total=numupdates, unit=_files)
+        progress.increment(step=i, item=item)
     updated = len(actions[ACTION_GET])
 
     if [a for a in actions[ACTION_GET] if a[0] == '.hgsubstate']:
         subrepoutil.submerge(repo, wctx, mctx, wctx, overwrite, labels)
 
     # forget (manifest only, just log it) (must come first)
     for f, args, msg in actions[ACTION_FORGET]:
         repo.ui.debug(" %s: %s -> f\n" % (f, msg))
-        z += 1
-        progress(_updating, z, item=f, total=numupdates, unit=_files)
+        progress.increment(item=f)
 
     # re-add (manifest only, just log it)
     for f, args, msg in actions[ACTION_ADD]:
         repo.ui.debug(" %s: %s -> a\n" % (f, msg))
-        z += 1
-        progress(_updating, z, item=f, total=numupdates, unit=_files)
+        progress.increment(item=f)
 
     # re-add/mark as modified (manifest only, just log it)
     for f, args, msg in actions[ACTION_ADD_MODIFIED]:
         repo.ui.debug(" %s: %s -> am\n" % (f, msg))
-        z += 1
-        progress(_updating, z, item=f, total=numupdates, unit=_files)
+        progress.increment(item=f)
 
     # keep (noop, just log it)
     for f, args, msg in actions[ACTION_KEEP]:
@@ -1678,8 +1668,7 @@ 
     # directory rename, move local
     for f, args, msg in actions[ACTION_DIR_RENAME_MOVE_LOCAL]:
         repo.ui.debug(" %s: %s -> dm\n" % (f, msg))
-        z += 1
-        progress(_updating, z, item=f, total=numupdates, unit=_files)
+        progress.increment(item=f)
         f0, flags = args
         repo.ui.note(_("moving %s to %s\n") % (f0, f))
         wctx[f].audit()
@@ -1690,18 +1679,16 @@ 
     # local directory rename, get
     for f, args, msg in actions[ACTION_LOCAL_DIR_RENAME_GET]:
         repo.ui.debug(" %s: %s -> dg\n" % (f, msg))
-        z += 1
-        progress(_updating, z, item=f, total=numupdates, unit=_files)
+        progress.increment(item=f)
         f0, flags = args
         repo.ui.note(_("getting %s to %s\n") % (f0, f))
         wctx[f].write(mctx.filectx(f0).data(), flags)
         updated += 1
 
     # exec
     for f, args, msg in actions[ACTION_EXEC]:
         repo.ui.debug(" %s: %s -> e\n" % (f, msg))
-        z += 1
-        progress(_updating, z, item=f, total=numupdates, unit=_files)
+        progress.increment(item=f)
         flags, = args
         wctx[f].audit()
         wctx[f].setflags('l' in flags, 'x' in flags)
@@ -1736,8 +1723,7 @@ 
         tocomplete = []
         for f, args, msg in mergeactions:
             repo.ui.debug(" %s: %s -> m (premerge)\n" % (f, msg))
-            z += 1
-            progress(_updating, z, item=f, total=numupdates, unit=_files)
+            progress.increment(item=f)
             if f == '.hgsubstate': # subrepo states need updating
                 subrepoutil.submerge(repo, wctx, mctx, wctx.ancestor(mctx),
                                      overwrite, labels)
@@ -1751,8 +1737,7 @@ 
         # merge
         for f, args, msg in tocomplete:
             repo.ui.debug(" %s: %s -> m (merge)\n" % (f, msg))
-            z += 1
-            progress(_updating, z, item=f, total=numupdates, unit=_files)
+            progress.increment(item=f, total=numupdates)
             ms.resolve(f, wctx)
 
     finally:
@@ -1800,7 +1785,7 @@ 
         actions[ACTION_MERGE] = [a for a in actions[ACTION_MERGE]
                                  if a[0] in mfiles]
 
-    progress(_updating, None, total=numupdates, unit=_files)
+    progress.update(None)
     return updateresult(updated, merged, removed, unresolved)
 
 def recordupdates(repo, actions, branchmerge):