[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