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

Pierre-Yves David pierre-yves.david at ens-lyon.org
Mon Apr 10 14:40:56 EDT 2017



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.david at 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


More information about the Mercurial-devel mailing list