Patchwork D289: bundle2: seek part back during iteration

login
register
mail settings
Submitter phabricator
Date Aug. 9, 2017, 12:05 a.m.
Message ID <differential-rev-PHID-DREV-6owqhv742enek3u3ktnv-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/22775/
State Superseded
Headers show

Comments

phabricator - Aug. 9, 2017, 12:05 a.m.
durham created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Previously, iterparts would yield the part to users, then consume the part. This
  changed the part after the user was given it and left it at the end, both of
  which seem unexpected.  Let's seek back to the beginning after we've consumed
  it. I tried not seeking to the end at all, but that seems important for the
  overall bundle2 consumption.
  
  This is used in a future patch to let us move the bundlerepo
  bundle2-changegroup-part to be handled entirely within the for loop, instead of
  having to do a seek back to 0 after the entire loop finishes.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/bundle2.py
  mercurial/bundlerepo.py

CHANGE DETAILS




To: durham, #hg-reviewers
Cc: mercurial-devel
phabricator - Aug. 15, 2017, 1:46 a.m.
indygreg accepted this revision.
indygreg added a comment.
This revision is now accepted and ready to land.


  This change feels a bit brittle to me, as it is relying on low-level implementation details of how bundle2 part handling and underlying stream consumption works. But I know from trying to touch this code that we have test coverage of it courtesy of bundlerepo. So I think we'll be fine.

REPOSITORY
  rHG Mercurial

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

To: durham, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - Aug. 23, 2017, 5:56 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> bundle2.py:811
> +            # code consuming this generator has a part that starts at 0.
>              part.seek(0, 2)
> +            part.seek(0)

Unrelated to this patch, but seems like we should define constants for "whence". I don't think seek(0, 2) is clear at all.

REPOSITORY
  rHG Mercurial

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

To: durham, #hg-reviewers, indygreg
Cc: martinvonz, indygreg, mercurial-devel
phabricator - Aug. 23, 2017, 7:08 p.m.
durham added a comment.


  I agree it's brittle, but no more brittle than before, since we still had to seek back.

REPOSITORY
  rHG Mercurial

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

To: durham, #hg-reviewers, indygreg
Cc: martinvonz, indygreg, mercurial-devel

Patch

diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
--- a/mercurial/bundlerepo.py
+++ b/mercurial/bundlerepo.py
@@ -301,7 +301,6 @@ 
 
             if cgstream is None:
                 raise error.Abort(_('No changegroups found'))
-            cgstream.seek(0)
 
             self.bundle = changegroup.getunbundler(version, cgstream, 'UN')
 
diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -805,7 +805,11 @@ 
         while headerblock is not None:
             part = unbundlepart(self.ui, headerblock, self._fp)
             yield part
+            # Seek to the end of the part to force it's consumption so the next
+            # part can be read. But then seek back to the beginning so the
+            # code consuming this generator has a part that starts at 0.
             part.seek(0, 2)
+            part.seek(0)
             headerblock = self._readpartheader()
         indebug(self.ui, 'end of bundle2 stream')