Re: [PATCH 4 of 4 V2] bundle2: move 'seek' and 'tell' methods off the unpackermixin class

[Pierre-Yves David]

On 04/10/2017 08:32 PM, Ryan McElroy wrote:


On 4/10/17 4:33 PM, Pierre-Yves David wrote:
# HG changeset patch
# User Pierre-Yves David <pierre-yves.da...@ens-lyon.org>
# Date 1491757747 -7200
#      Sun Apr 09 19:09:07 2017 +0200
# Node ID 7ee5ad1a4ba42801ce0fcd2b5182f0d9ad42a169
# Parent  80068be95fbee53cc30784e1bd61df90c2b31ffe
# 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.

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,24 +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:
@@ -1110,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
@@ -1157,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]
@@ -1179,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)
@@ -1273,6 +1254,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"""

The comments in the earlier patches in this series got major upgrades
which make it really clear why they shouldn't be used in higher-level
functions. These ones are stuck on the old version though, which is much
less clear in my opinion.

I think 1-3 are good, but I think this one deserves a V3 with the better
wording you came up with used here.

I'll mark 1-3 as pre-reviewed, and this one as "changes requested" in
patchwork.

Snap, you are right. I'll send a V3 of the last once on the first three 
are in (or changes are requested on them)


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



--
Pierre-Yves David
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel