Patchwork [V3] bundle2: move 'seek' and 'tell' methods off the unpackermixin class

login
register
mail settings
Submitter Pierre-Yves David
Date April 11, 2017, 7:48 p.m.
Message ID <655fb7e6bb117d896ab4.1491940105@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/20111/
State Accepted
Headers show

Comments

Pierre-Yves David - April 11, 2017, 7:48 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1491757747 -7200
#      Sun Apr 09 19:09:07 2017 +0200
# Node ID 655fb7e6bb117d896ab43adbf8d581e3780d82cf
# Parent  cd7aaf344d83988f77d0bcb983b9ee1cf01b704d
# EXP-Topic bundle2.doc
# Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
#              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r 655fb7e6bb11
bundle2: move 'seek' and 'tell' methods off the unpackermixin class

These methods are unrelated to unpacking. They are used internally by the
'unbundlepart' class only. So me move them there as private methods.

In the same go, we clarify their internal role in the their docstring.
Ryan McElroy - April 12, 2017, 11:59 a.m.
On 4/11/17 8:48 PM, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1491757747 -7200
> #      Sun Apr 09 19:09:07 2017 +0200
> # Node ID 655fb7e6bb117d896ab43adbf8d581e3780d82cf
> # Parent  cd7aaf344d83988f77d0bcb983b9ee1cf01b704d
> # EXP-Topic bundle2.doc
> bundle2: move 'seek' and 'tell' methods off the unpackermixin class
>
> These methods are unrelated to unpacking. They are used internally by the
> 'unbundlepart' class only. So me move them there as private methods.
>
> In the same go, we clarify their internal role in the their docstring.
>
>

This version has all of the improvements I hoped for. I've marked as 
pre-reviewed in patchwork.
Augie Fackler - April 12, 2017, 8:12 p.m.
On Wed, Apr 12, 2017 at 12:59:29PM +0100, Ryan McElroy wrote:
> On 4/11/17 8:48 PM, Pierre-Yves David wrote:
> > # HG changeset patch
> > # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> > # Date 1491757747 -7200
> > #      Sun Apr 09 19:09:07 2017 +0200
> > # Node ID 655fb7e6bb117d896ab43adbf8d581e3780d82cf
> > # Parent  cd7aaf344d83988f77d0bcb983b9ee1cf01b704d
> > # EXP-Topic bundle2.doc

queued, thanks

Patch

diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -617,8 +617,6 @@  class unpackermixin(object):
 
     def __init__(self, fp):
         self._fp = fp
-        self._seekable = (util.safehasattr(fp, 'seek') and
-                          util.safehasattr(fp, 'tell'))
 
     def _unpack(self, format):
         """unpack this struct format from the stream
@@ -641,25 +639,6 @@  class unpackermixin(object):
         Do not use it to implement higher-level logic or methods."""
         return changegroup.readexactly(self._fp, size)
 
-    def seek(self, offset, whence=0):
-        """move the underlying file pointer"""
-        if self._seekable:
-            return self._fp.seek(offset, whence)
-        else:
-            raise NotImplementedError(_('File pointer is not seekable'))
-
-    def tell(self):
-        """return the file offset, or None if file is not seekable"""
-        if self._seekable:
-            try:
-                return self._fp.tell()
-            except IOError as e:
-                if e.errno == errno.ESPIPE:
-                    self._seekable = False
-                else:
-                    raise
-        return None
-
 def getunbundler(ui, fp, magicstring=None):
     """return a valid unbundler object for a given magicstring"""
     if magicstring is None:
@@ -1111,6 +1090,8 @@  class unbundlepart(unpackermixin):
 
     def __init__(self, ui, header, fp):
         super(unbundlepart, self).__init__(fp)
+        self._seekable = (util.safehasattr(fp, 'seek') and
+                          util.safehasattr(fp, 'tell'))
         self.ui = ui
         # unbundle state attr
         self._headerdata = header
@@ -1158,11 +1139,11 @@  class unbundlepart(unpackermixin):
         '''seek to specified chunk and start yielding data'''
         if len(self._chunkindex) == 0:
             assert chunknum == 0, 'Must start with chunk 0'
-            self._chunkindex.append((0, super(unbundlepart, self).tell()))
+            self._chunkindex.append((0, self._tellfp()))
         else:
             assert chunknum < len(self._chunkindex), \
                    'Unknown chunk %d' % chunknum
-            super(unbundlepart, self).seek(self._chunkindex[chunknum][1])
+            self._seekfp(self._chunkindex[chunknum][1])
 
         pos = self._chunkindex[chunknum][0]
         payloadsize = self._unpack(_fpayloadsize)[0]
@@ -1180,8 +1161,7 @@  class unbundlepart(unpackermixin):
                 chunknum += 1
                 pos += payloadsize
                 if chunknum == len(self._chunkindex):
-                    self._chunkindex.append((pos,
-                                             super(unbundlepart, self).tell()))
+                    self._chunkindex.append((pos, self._tellfp()))
                 yield result
             payloadsize = self._unpack(_fpayloadsize)[0]
             indebug(self.ui, 'payload chunk size: %i' % payloadsize)
@@ -1274,6 +1254,37 @@  class unbundlepart(unpackermixin):
                 raise error.Abort(_('Seek failed\n'))
             self._pos = newpos
 
+    def _seekfp(self, offset, whence=0):
+        """move the underlying file pointer
+
+        This method is meant for internal usage by the bundle2 protocol only.
+        They directly manipulate the low level stream including bundle2 level
+        instruction.
+
+        Do not use it to implement higher-level logic or methods."""
+        if self._seekable:
+            return self._fp.seek(offset, whence)
+        else:
+            raise NotImplementedError(_('File pointer is not seekable'))
+
+    def _tellfp(self):
+        """return the file offset, or None if file is not seekable
+
+        This method is meant for internal usage by the bundle2 protocol only.
+        They directly manipulate the low level stream including bundle2 level
+        instruction.
+
+        Do not use it to implement higher-level logic or methods."""
+        if self._seekable:
+            try:
+                return self._fp.tell()
+            except IOError as e:
+                if e.errno == errno.ESPIPE:
+                    self._seekable = False
+                else:
+                    raise
+        return None
+
 # These are only the static capabilities.
 # Check the 'getrepocaps' function for the rest.
 capabilities = {'HG20': (),