Patchwork [1,of,9] changegroup: use `iter(callable, sentinel)` instead of while True

login
register
mail settings
Submitter Augie Fackler
Date Aug. 6, 2016, 3:02 p.m.
Message ID <84bc06986855488a7237.1470495751@imladris.local>
Download mbox | patch
Permalink /patch/16156/
State Accepted
Headers show

Comments

Augie Fackler - Aug. 6, 2016, 3:02 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1470419998 14400
#      Fri Aug 05 13:59:58 2016 -0400
# Node ID 84bc06986855488a7237adc057655baa1e3c3e09
# Parent  91b2f21763955063b994b07a1060305e0fe93031
changegroup: use `iter(callable, sentinel)` instead of while True

This is functionally equivalent, but is a little more concise.
Gregory Szorc - Aug. 6, 2016, 6:10 p.m.
On Sat, Aug 6, 2016 at 8:02 AM, Augie Fackler <raf@durin42.com> wrote:

> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1470419998 14400
> #      Fri Aug 05 13:59:58 2016 -0400
> # Node ID 84bc06986855488a7237adc057655baa1e3c3e09
> # Parent  91b2f21763955063b994b07a1060305e0fe93031
> changegroup: use `iter(callable, sentinel)` instead of while True
>
>
TIL iter(callable, sentinel) is a thing. That's really cool!

This series is a nice cleanup and LGTM. My only concern would be
performance. But it looks like checking of the sentinel is always done in
the C layer in calliter_iternext(). And we were already effectively doing
the same thing in Python. So if nothing else this should be a little faster
due to avoiding extra Python interpreter instructions.
Pierre-Yves David - Aug. 7, 2016, 2:07 p.m.
On 08/06/2016 08:10 PM, Gregory Szorc wrote:
> On Sat, Aug 6, 2016 at 8:02 AM, Augie Fackler <raf@durin42.com
> <mailto:raf@durin42.com>> wrote:
>
>     # HG changeset patch
>     # User Augie Fackler <augie@google.com <mailto:augie@google.com>>
>     # Date 1470419998 14400
>     #      Fri Aug 05 13:59:58 2016 -0400
>     # Node ID 84bc06986855488a7237adc057655baa1e3c3e09
>     # Parent  91b2f21763955063b994b07a1060305e0fe93031
>     changegroup: use `iter(callable, sentinel)` instead of while True
>
>
> TIL iter(callable, sentinel) is a thing. That's really cool!
>
> This series is a nice cleanup and LGTM. My only concern would be
> performance. But it looks like checking of the sentinel is always done
> in the C layer in calliter_iternext(). And we were already effectively
> doing the same thing in Python. So if nothing else this should be a
> little faster due to avoiding extra Python interpreter instructions.

Nice code improvement all over the place.
Pushed, thanks for the review.

Patch

diff --git a/mercurial/changegroup.py b/mercurial/changegroup.py
--- a/mercurial/changegroup.py
+++ b/mercurial/changegroup.py
@@ -475,10 +475,7 @@  class cg3unpacker(cg2unpacker):
     def _unpackmanifests(self, repo, revmap, trp, prog, numchanges):
         super(cg3unpacker, self)._unpackmanifests(repo, revmap, trp, prog,
                                                   numchanges)
-        while True:
-            chunkdata = self.filelogheader()
-            if not chunkdata:
-                break
+        for chunkdata in iter(self.filelogheader, {}):
             # If we get here, there are directory manifests in the changegroup
             d = chunkdata["filename"]
             repo.ui.debug("adding %s revisions\n" % d)
@@ -1012,10 +1009,7 @@  def changegroup(repo, basenodes, source)
 def _addchangegroupfiles(repo, source, revmap, trp, expectedfiles, needfiles):
     revisions = 0
     files = 0
-    while True:
-        chunkdata = source.filelogheader()
-        if not chunkdata:
-            break
+    for chunkdata in iter(source.filelogheader, {}):
         files += 1
         f = chunkdata["filename"]
         repo.ui.debug("adding %s revisions\n" % f)