Patchwork [4,of,4] bundle2: move 'seek' and 'tell' method off the unpackermixin class

login
register
mail settings
Submitter Pierre-Yves David
Date April 9, 2017, 5:41 p.m.
Message ID <b9a2f30bf1c3fb1809ac.1491759716@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/20041/
State Changes Requested
Headers show

Comments

Pierre-Yves David - April 9, 2017, 5:41 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 b9a2f30bf1c3fb1809acda2036c7614972449276
# Parent  9df8644ff8483025564479285c0e652677bae0a0
# 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 b9a2f30bf1c3
bundle2: move 'seek' and 'tell' method 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 method.

In the same go, we clarify their internal role in the their docstring.
Ryan McElroy - April 10, 2017, 9:17 a.m.
On 4/9/17 6:41 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 b9a2f30bf1c3fb1809acda2036c7614972449276
> # Parent  9df8644ff8483025564479285c0e652677bae0a0
> # EXP-Topic bundle2.doc
> bundle2: move 'seek' and 'tell' method 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 method.
s/method/methods

>
> In the same go, we clarify their internal role in the their docstring.
>
> 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
> @@ -635,24 +633,6 @@ class unpackermixin(object):
>           Do not use it to implement higher level"""
>           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:
> @@ -1104,6 +1084,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
> @@ -1151,11 +1133,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]
> @@ -1173,8 +1155,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)
> @@ -1267,6 +1248,31 @@ 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 of bundle2 protocol.
> +        Do not use it to implement higher level"""
Some comments as in previous patches on these comments.

> +        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 of bundle2 protocol.
> +        Do not use it to implement higher level"""
Some comments as in previous patches on these comments.

> +        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': (),
>

Overall, this series feels like moving in the right direction. It's a 
good code cleanup and make the relationships between the parts of the 
code clearer to me, so I'm a big +1.

However, since it's heavy on documentation and I have some suggestions 
to clean up the language inline, I have marked this series as "changes 
requested" in patchwork.

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
@@ -635,24 +633,6 @@  class unpackermixin(object):
         Do not use it to implement higher level"""
         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:
@@ -1104,6 +1084,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
@@ -1151,11 +1133,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]
@@ -1173,8 +1155,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)
@@ -1267,6 +1248,31 @@  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 of bundle2 protocol.
+        Do not use it to implement higher level"""
+        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 of bundle2 protocol.
+        Do not use it to implement higher level"""
+        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': (),