Patchwork [3,of,4] largefiles: use a context manager to control the progress bar lifetime

login
register
mail settings
Submitter Matt Harbison
Date Sept. 1, 2018, 6:05 a.m.
Message ID <9a5c52a1933072c6d88b.1535781932@Envy>
Download mbox | patch
Permalink /patch/34232/
State Accepted
Headers show

Comments

Matt Harbison - Sept. 1, 2018, 6:05 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1535216964 14400
#      Sat Aug 25 13:09:24 2018 -0400
# Node ID 9a5c52a1933072c6d88ba33d64ccc14f24472115
# Parent  f01ec535806db02e65db9845d80914de7663a1bd
largefiles: use a context manager to control the progress bar lifetime
Matt Harbison - Sept. 2, 2018, 4:30 a.m.
On Sat, 01 Sep 2018 02:05:32 -0400, Matt Harbison <mharbison72@gmail.com>  
wrote:

> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1535216964 14400
> #      Sat Aug 25 13:09:24 2018 -0400
> # Node ID 9a5c52a1933072c6d88ba33d64ccc14f24472115
> # Parent  f01ec535806db02e65db9845d80914de7663a1bd
> largefiles: use a context manager to control the progress bar lifetime
>
> diff --git a/hgext/largefiles/basestore.py  
> b/hgext/largefiles/basestore.py
> --- a/hgext/largefiles/basestore.py
> +++ b/hgext/largefiles/basestore.py
> @@ -62,25 +62,24 @@ class basestore(object):
>         at = 0
>          available = self.exists(set(hash for (_filename, hash) in  
> files))
> -        progress = ui.makeprogress(_('getting largefiles'),  
> unit=_('files'),
> -                                   total=len(files))
> -        for filename, hash in files:
> -            progress.update(at)
> -            at += 1
> -            ui.note(_('getting %s:%s\n') % (filename, hash))
> +        with ui.makeprogress(_('getting largefiles'), unit=_('files'),
> +                             total=len(files)) as progress:
> +            for filename, hash in files:
> +                progress.update(at)
> +                at += 1
> +                ui.note(_('getting %s:%s\n') % (filename, hash))

I see this sort of manual tracking of the progress in several places  
(instead of just calling increment()).  The best I could surmise is that  
this is trying to show the progressbar immediately, but only if there's  
something to iterate over (so it doesn't show 0/0).  Does it seem  
reasonable for update(0) to be called when entering the context manager if  
a total is set and it's not 0?  That way, most of these loops could just  
increment at the bottom.

This probably dovetails with Martin's question a few weeks ago about  
starting at 0/X vs 1/X, but I couldn't find that again to see how it was  
decided.
via Mercurial-devel - Sept. 4, 2018, 9:34 p.m.
On Sat, Sep 1, 2018 at 9:31 PM Matt Harbison <mharbison72@gmail.com> wrote:

> On Sat, 01 Sep 2018 02:05:32 -0400, Matt Harbison <mharbison72@gmail.com>
>
> wrote:
>
> > # HG changeset patch
> > # User Matt Harbison <matt_harbison@yahoo.com>
> > # Date 1535216964 14400
> > #      Sat Aug 25 13:09:24 2018 -0400
> > # Node ID 9a5c52a1933072c6d88ba33d64ccc14f24472115
> > # Parent  f01ec535806db02e65db9845d80914de7663a1bd
> > largefiles: use a context manager to control the progress bar lifetime
> >
> > diff --git a/hgext/largefiles/basestore.py
> > b/hgext/largefiles/basestore.py
> > --- a/hgext/largefiles/basestore.py
> > +++ b/hgext/largefiles/basestore.py
> > @@ -62,25 +62,24 @@ class basestore(object):
> >         at = 0
> >          available = self.exists(set(hash for (_filename, hash) in
> > files))
> > -        progress = ui.makeprogress(_('getting largefiles'),
> > unit=_('files'),
> > -                                   total=len(files))
> > -        for filename, hash in files:
> > -            progress.update(at)
> > -            at += 1
> > -            ui.note(_('getting %s:%s\n') % (filename, hash))
> > +        with ui.makeprogress(_('getting largefiles'), unit=_('files'),
> > +                             total=len(files)) as progress:
> > +            for filename, hash in files:
> > +                progress.update(at)
> > +                at += 1
> > +                ui.note(_('getting %s:%s\n') % (filename, hash))
>
> I see this sort of manual tracking of the progress in several places
> (instead of just calling increment()).  The best I could surmise is that
> this is trying to show the progressbar immediately, but only if there's
> something to iterate over (so it doesn't show 0/0).  Does it seem
> reasonable for update(0) to be called when entering the context manager
> if
> a total is set and it's not 0?  That way, most of these loops could just
> increment at the bottom.
>
> This probably dovetails with Martin's question a few weeks ago about
> starting at 0/X vs 1/X, but I couldn't find that again to see how it was
> decided.
>

Yes, I also considered making __enter__ do that, but I didn't know what the
right behavior would be. Perhaps what you suggest makes sense.

Note that the example above isn't trivial to fix because there are two
"continue" statements in the loop body, so doing "progress.update()" at the
end of the loop body won't be correct. I saw that synthrepo.py
uses enumerate() for keeping track of the index, so we could at least reuse
that trick here.
Matt Harbison - Sept. 5, 2018, 1:52 a.m.
On Tue, 04 Sep 2018 17:34:11 -0400, Martin von Zweigbergk  
<martinvonz@google.com> wrote:

> On Sat, Sep 1, 2018 at 9:31 PM Matt Harbison <mharbison72@gmail.com>  
> wrote:
>
>> On Sat, 01 Sep 2018 02:05:32 -0400, Matt Harbison  
>> <mharbison72@gmail.com>
>>
>> wrote:
>>
>> > # HG changeset patch
>> > # User Matt Harbison <matt_harbison@yahoo.com>
>> > # Date 1535216964 14400
>> > #      Sat Aug 25 13:09:24 2018 -0400
>> > # Node ID 9a5c52a1933072c6d88ba33d64ccc14f24472115
>> > # Parent  f01ec535806db02e65db9845d80914de7663a1bd
>> > largefiles: use a context manager to control the progress bar lifetime
>> >
>> > diff --git a/hgext/largefiles/basestore.py
>> > b/hgext/largefiles/basestore.py
>> > --- a/hgext/largefiles/basestore.py
>> > +++ b/hgext/largefiles/basestore.py
>> > @@ -62,25 +62,24 @@ class basestore(object):
>> >         at = 0
>> >          available = self.exists(set(hash for (_filename, hash) in
>> > files))
>> > -        progress = ui.makeprogress(_('getting largefiles'),
>> > unit=_('files'),
>> > -                                   total=len(files))
>> > -        for filename, hash in files:
>> > -            progress.update(at)
>> > -            at += 1
>> > -            ui.note(_('getting %s:%s\n') % (filename, hash))
>> > +        with ui.makeprogress(_('getting largefiles'),  
>> unit=_('files'),
>> > +                             total=len(files)) as progress:
>> > +            for filename, hash in files:
>> > +                progress.update(at)
>> > +                at += 1
>> > +                ui.note(_('getting %s:%s\n') % (filename, hash))
>>
>> I see this sort of manual tracking of the progress in several places
>> (instead of just calling increment()).  The best I could surmise is that
>> this is trying to show the progressbar immediately, but only if there's
>> something to iterate over (so it doesn't show 0/0).  Does it seem
>> reasonable for update(0) to be called when entering the context manager
>> if
>> a total is set and it's not 0?  That way, most of these loops could just
>> increment at the bottom.
>>
>> This probably dovetails with Martin's question a few weeks ago about
>> starting at 0/X vs 1/X, but I couldn't find that again to see how it was
>> decided.
>>
>
> Yes, I also considered making __enter__ do that, but I didn't know what  
> the
> right behavior would be. Perhaps what you suggest makes sense.
>
> Note that the example above isn't trivial to fix because there are two
> "continue" statements in the loop body, so doing "progress.update()" at  
> the
> end of the loop body won't be correct. I saw that synthrepo.py
> uses enumerate() for keeping track of the index, so we could at least  
> reuse
> that trick here.

Yeah, I coded up one that removed enumerate() and already had flow  
control, and then stopped because it seemed like just a different kind of  
ugly to hit the progress bar before each continue.  If it was decided to  
first show 1/X, then it could still be primed to 0, and increment at the  
top.  It would be inconsistent with the raw byte count progress bars  
(since they add bytes that are done), but I doubt anyone would notice :)

Patch

diff --git a/hgext/largefiles/basestore.py b/hgext/largefiles/basestore.py
--- a/hgext/largefiles/basestore.py
+++ b/hgext/largefiles/basestore.py
@@ -62,25 +62,24 @@  class basestore(object):
 
         at = 0
         available = self.exists(set(hash for (_filename, hash) in files))
-        progress = ui.makeprogress(_('getting largefiles'), unit=_('files'),
-                                   total=len(files))
-        for filename, hash in files:
-            progress.update(at)
-            at += 1
-            ui.note(_('getting %s:%s\n') % (filename, hash))
+        with ui.makeprogress(_('getting largefiles'), unit=_('files'),
+                             total=len(files)) as progress:
+            for filename, hash in files:
+                progress.update(at)
+                at += 1
+                ui.note(_('getting %s:%s\n') % (filename, hash))
 
-            if not available.get(hash):
-                ui.warn(_('%s: largefile %s not available from %s\n')
-                        % (filename, hash, util.hidepassword(self.url)))
-                missing.append(filename)
-                continue
+                if not available.get(hash):
+                    ui.warn(_('%s: largefile %s not available from %s\n')
+                            % (filename, hash, util.hidepassword(self.url)))
+                    missing.append(filename)
+                    continue
 
-            if self._gethash(filename, hash):
-                success.append((filename, hash))
-            else:
-                missing.append(filename)
+                if self._gethash(filename, hash):
+                    success.append((filename, hash))
+                else:
+                    missing.append(filename)
 
-        progress.complete()
         return (success, missing)
 
     def _gethash(self, filename, hash):
diff --git a/hgext/largefiles/lfcommands.py b/hgext/largefiles/lfcommands.py
--- a/hgext/largefiles/lfcommands.py
+++ b/hgext/largefiles/lfcommands.py
@@ -118,14 +118,13 @@  def lfconvert(ui, src, dest, *pats, **op
                 matcher = None
 
             lfiletohash = {}
-            progress = ui.makeprogress(_('converting revisions'),
-                                       unit=_('revisions'),
-                                       total=rsrc['tip'].rev())
-            for ctx in ctxs:
-                progress.update(ctx.rev())
-                _lfconvert_addchangeset(rsrc, rdst, ctx, revmap,
-                    lfiles, normalfiles, matcher, size, lfiletohash)
-            progress.complete()
+            with ui.makeprogress(_('converting revisions'),
+                                 unit=_('revisions'),
+                                 total=rsrc['tip'].rev()) as progress:
+                for ctx in ctxs:
+                    progress.update(ctx.rev())
+                    _lfconvert_addchangeset(rsrc, rdst, ctx, revmap,
+                        lfiles, normalfiles, matcher, size, lfiletohash)
 
             if rdst.wvfs.exists(lfutil.shortname):
                 rdst.wvfs.rmtree(lfutil.shortname)
@@ -370,18 +369,17 @@  def uploadlfiles(ui, rsrc, rdst, files):
     files = [h for h in files if not retval[h]]
     ui.debug("%d largefiles need to be uploaded\n" % len(files))
 
-    progress = ui.makeprogress(_('uploading largefiles'), unit=_('files'),
-                               total=len(files))
-    for hash in files:
-        progress.update(at)
-        source = lfutil.findfile(rsrc, hash)
-        if not source:
-            raise error.Abort(_('largefile %s missing from store'
-                               ' (needs to be uploaded)') % hash)
-        # XXX check for errors here
-        store.put(source, hash)
-        at += 1
-    progress.complete()
+    with ui.makeprogress(_('uploading largefiles'), unit=_('files'),
+                         total=len(files)) as progress:
+        for hash in files:
+            progress.update(at)
+            source = lfutil.findfile(rsrc, hash)
+            if not source:
+                raise error.Abort(_('largefile %s missing from store'
+                                   ' (needs to be uploaded)') % hash)
+            # XXX check for errors here
+            store.put(source, hash)
+            at += 1
 
 def verifylfiles(ui, repo, all=False, contents=False):
     '''Verify that every largefile revision in the current changeset
diff --git a/hgext/largefiles/lfutil.py b/hgext/largefiles/lfutil.py
--- a/hgext/largefiles/lfutil.py
+++ b/hgext/largefiles/lfutil.py
@@ -501,37 +501,37 @@  def getlfilestoupdate(oldstandins, newst
     return filelist
 
 def getlfilestoupload(repo, missing, addfunc):
-    progress = repo.ui.makeprogress(_('finding outgoing largefiles'),
-                                    unit=_('revisions'), total=len(missing))
-    for i, n in enumerate(missing):
-        progress.update(i)
-        parents = [p for p in repo[n].parents() if p != node.nullid]
+    makeprogress = repo.ui.makeprogress
+    with makeprogress(_('finding outgoing largefiles'),
+                      unit=_('revisions'), total=len(missing)) as progress:
+        for i, n in enumerate(missing):
+            progress.update(i)
+            parents = [p for p in repo[n].parents() if p != node.nullid]
 
-        oldlfstatus = repo.lfstatus
-        repo.lfstatus = False
-        try:
-            ctx = repo[n]
-        finally:
-            repo.lfstatus = oldlfstatus
+            oldlfstatus = repo.lfstatus
+            repo.lfstatus = False
+            try:
+                ctx = repo[n]
+            finally:
+                repo.lfstatus = oldlfstatus
 
-        files = set(ctx.files())
-        if len(parents) == 2:
-            mc = ctx.manifest()
-            mp1 = ctx.parents()[0].manifest()
-            mp2 = ctx.parents()[1].manifest()
-            for f in mp1:
-                if f not in mc:
-                    files.add(f)
-            for f in mp2:
-                if f not in mc:
-                    files.add(f)
-            for f in mc:
-                if mc[f] != mp1.get(f, None) or mc[f] != mp2.get(f, None):
-                    files.add(f)
-        for fn in files:
-            if isstandin(fn) and fn in ctx:
-                addfunc(fn, readasstandin(ctx[fn]))
-    progress.complete()
+            files = set(ctx.files())
+            if len(parents) == 2:
+                mc = ctx.manifest()
+                mp1 = ctx.parents()[0].manifest()
+                mp2 = ctx.parents()[1].manifest()
+                for f in mp1:
+                    if f not in mc:
+                        files.add(f)
+                for f in mp2:
+                    if f not in mc:
+                        files.add(f)
+                for f in mc:
+                    if mc[f] != mp1.get(f, None) or mc[f] != mp2.get(f, None):
+                        files.add(f)
+            for fn in files:
+                if isstandin(fn) and fn in ctx:
+                    addfunc(fn, readasstandin(ctx[fn]))
 
 def updatestandinsbymatch(repo, match):
     '''Update standins in the working directory according to specified match