| 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
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.
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.
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