Patchwork D3809: synthrepo: use progress helper

login
register
mail settings
Submitter phabricator
Date June 19, 2018, 12:45 a.m.
Message ID <differential-rev-PHID-DREV-rvskzvzqz5cymw6szgn3-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/32313/
State Superseded
Headers show

Comments

phabricator - June 19, 2018, 12:45 a.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  contrib/synthrepo.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - June 19, 2018, 11:10 a.m.
pulkit accepted this revision.
pulkit added inline comments.

INLINE COMMENTS

> synthrepo.py:178
>          for i, rev in enumerate(revs):
> -            progress(_analyzing, i, unit=_changesets, total=_total)
> +            progress.update(i)
>              ctx = repo[rev]

progress.increment()?

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers, pulkit
Cc: pulkit, mercurial-devel
phabricator - June 19, 2018, 4:14 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> pulkit wrote in synthrepo.py:178
> progress.increment()?

That would mean that it would now start at 1 instead of 0. I made that change in one or two other patches and then I felt unsure if that's what we want, so I left the current behavior in this and later patches.

I meant to have a discussion about which behavior we want. For example, when the progress bar says 1/3, do you interpret that as processing the first item or that 1 item is already complete?

A related issue is that it seems like either 0% or 100% wouldn't happen (for more than a split second, regardless of how slow each item is). Let's say we have 3 items. We can then do either of these:

  progress.update(0)
  for i [1,2,3]:
    progress.update(i)
    process(i)
  progress.complete()



  progress.update(0)
  for i [1,2,3]:
    process(i)
    progress.update(i)
  progress.complete()

As you can see, either the 0/3 or the 3/3 step will never really be displayed.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers, pulkit
Cc: pulkit, mercurial-devel
phabricator - July 1, 2018, 9:24 p.m.
pulkit added inline comments.

INLINE COMMENTS

> martinvonz wrote in synthrepo.py:178
> That would mean that it would now start at 1 instead of 0. I made that change in one or two other patches and then I felt unsure if that's what we want, so I left the current behavior in this and later patches.
> 
> I meant to have a discussion about which behavior we want. For example, when the progress bar says 1/3, do you interpret that as processing the first item or that 1 item is already complete?
> 
> A related issue is that it seems like either 0% or 100% wouldn't happen (for more than a split second, regardless of how slow each item is). Let's say we have 3 items. We can then do either of these:
> 
>   progress.update(0)
>   for i [1,2,3]:
>     progress.update(i)
>     process(i)
>   progress.complete()
> 
> 
> 
>   progress.update(0)
>   for i [1,2,3]:
>     process(i)
>     progress.update(i)
>   progress.complete()
> 
> As you can see, either the 0/3 or the 3/3 step will never really be displayed.

(sorry I missed this. I have my phabricator emails turned off)

> I meant to have a discussion about which behavior we want. For example, when the progress
>  bar says 1/3, do you interpret that as processing the first item or that 1 item is already
>  complete?

I think both :(. In network related bars where number of kbs or mbs are involved I think, 1/3 means 1 is already done. Sometimes in other processing, I think okay this one is processing.

However, I think if have verbs before the progress bars saying `processing`, `sending`, `recieving`, in those cases `processing 1/3` will mean processing the 1st item.

I still don't feel strong either way but I will like to have the same meaning throughout core.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers, pulkit
Cc: pulkit, mercurial-devel
phabricator - July 2, 2018, 5:52 a.m.
martinvonz added inline comments.

INLINE COMMENTS

> pulkit wrote in synthrepo.py:178
> (sorry I missed this. I have my phabricator emails turned off)
> 
> > I meant to have a discussion about which behavior we want. For example, when the progress
> >  bar says 1/3, do you interpret that as processing the first item or that 1 item is already
> >  complete?
> 
> I think both :(. In network related bars where number of kbs or mbs are involved I think, 1/3 means 1 is already done. Sometimes in other processing, I think okay this one is processing.
> 
> However, I think if have verbs before the progress bars saying `processing`, `sending`, `recieving`, in those cases `processing 1/3` will mean processing the 1st item.
> 
> I still don't feel strong either way but I will like to have the same meaning throughout core.

Another point is that when we pass an `item` argument (often a filename), then it's helpful to show it before starting processing of the filename, so one can tell which one is slow. Also, if we updated the progress after processing, the last item would only be printed for a spit second (no matter how long time it took to process it). For those reasons, I now lean towards updating progress before processing. For consistency, I think it makes more sense to update progress before processing also when there's no `item` argument. I agree about doing it after processing when it's about processed bytes, though.

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/contrib/synthrepo.py b/contrib/synthrepo.py
--- a/contrib/synthrepo.py
+++ b/contrib/synthrepo.py
@@ -172,13 +172,10 @@ 
         revs = scmutil.revrange(repo, revs)
         revs.sort()
 
-        progress = ui.progress
-        _analyzing = _('analyzing')
-        _changesets = _('changesets')
-        _total = len(revs)
-
+        progress = ui.makeprogress(_('analyzing'), unit=_('changesets'),
+                                   total=len(revs))
         for i, rev in enumerate(revs):
-            progress(_analyzing, i, unit=_changesets, total=_total)
+            progress.update(i)
             ctx = repo[rev]
             pl = ctx.parents()
             pctx = pl[0]
@@ -338,7 +335,6 @@ 
 
     nevertouch = {'.hgsub', '.hgignore', '.hgtags'}
 
-    progress = ui.progress
     _synthesizing = _('synthesizing')
     _files = _('initial files')
     _changesets = _('changesets')
@@ -362,8 +358,9 @@ 
                 path = os.path.dirname(path)
             return True
 
+        progress = ui.makeprogress(_synthesizing, unit=_files, total=initcount)
         for i in xrange(0, initcount):
-            ui.progress(_synthesizing, i, unit=_files, total=initcount)
+            progress.update(i)
 
             path = pickpath()
             while not validpath(path):
@@ -378,7 +375,7 @@ 
         def filectxfn(repo, memctx, path):
             return context.memfilectx(repo, memctx, path, files[path])
 
-        ui.progress(_synthesizing, None)
+        progress.complete()
         message = 'synthesized wide repo with %d files' % (len(files),)
         mc = context.memctx(repo, [pctx.node(), nullid], message,
                             files, filectxfn, ui.username(),
@@ -394,8 +391,9 @@ 
     # Synthesize incremental revisions to the repository, adding repo depth.
     count = int(opts['count'])
     heads = set(map(repo.changelog.rev, repo.heads()))
+    progress = ui.makeprogress(_synthesizing, unit=_changesets, total=count)
     for i in xrange(count):
-        progress(_synthesizing, i, unit=_changesets, total=count)
+        progress.update(i)
 
         node = repo.changelog.node
         revs = len(repo)