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

Matt Harbison mharbison72 at gmail.com
Tue Sep 4 21:52:45 EDT 2018


On Tue, 04 Sep 2018 17:34:11 -0400, Martin von Zweigbergk  
<martinvonz at google.com> wrote:

> On Sat, Sep 1, 2018 at 9:31 PM Matt Harbison <mharbison72 at gmail.com>  
> wrote:
>
>> On Sat, 01 Sep 2018 02:05:32 -0400, Matt Harbison  
>> <mharbison72 at gmail.com>
>>
>> wrote:
>>
>> > # HG changeset patch
>> > # User Matt Harbison <matt_harbison at 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 :)


More information about the Mercurial-devel mailing list