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

Ryan McElroy rm at fb.com
Mon Apr 10 05:17:24 EDT 2017


On 4/9/17 6:41 PM, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david at 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.


More information about the Mercurial-devel mailing list