[PATCH 02 of 10] opener: add read & write utility methods

Dan Villiom Podlaski Christiansen danchr at gmail.com
Fri Dec 3 09:06:34 CST 2010


On 2 Dec 2010, at 00:42, Adrian Buehlmann wrote:

> On 2010-12-01 22:34, Dan Villiom Podlaski Christiansen wrote:
>> # HG changeset patch
>> # User Dan Villiom Podlaski Christiansen <danchr at gmail.com>
>> # Date 1291227679 -3600
>> # Node ID 8c47f61bd78b2bdcbd088e0b2fed3bedc4e995eb
>> # Parent  446c177fbf75f7134eec7671998fd112fbe90bed
>> opener: add read & write utility methods
>>
>> The two new methods are useful for quickly opening a file for reading
>> or writing. Unlike 'opener(...).read()', they ensure they the file is
>                                                       ----
>
>> immediately closed without relying on CPython reference counting.
>>
>> Similar methods are added to custom openers.
>>
>> diff --git a/mercurial/statichttprepo.py b/mercurial/ 
>> statichttprepo.py
>> --- a/mercurial/statichttprepo.py
>> +++ b/mercurial/statichttprepo.py
>> @@ -75,6 +75,7 @@ def build_opener(ui, authinfo):
>>                 raise IOError('Permission denied')
>>             f = "/".join((p, urllib.quote(path)))
>>             return httprangereader(f, urlopener)
>> +        o.read = lambda *args, **kwargs: o(*args, **kwargs).read()
>>         return o
>>
>>     opener.options = {'nonlazy': 1}
>> diff --git a/mercurial/store.py b/mercurial/store.py
>> --- a/mercurial/store.py
>> +++ b/mercurial/store.py
>> @@ -176,6 +176,8 @@ class basicstore(object):
>>         op = opener(self.path)
>>         op.createmode = self.createmode
>>         self.opener = lambda f, *args, **kw: op(encodedir(f),  
>> *args, **kw)
>> +        self.opener.read = (lambda f, *args, **kw:
>> +                            op(encodedir(f), *args, **kw).read())
>>
>>     def join(self, f):
>>         return self.pathjoiner(self.path, encodedir(f))
>> @@ -221,6 +223,8 @@ class encodedstore(basicstore):
>>         op = opener(self.path)
>>         op.createmode = self.createmode
>>         self.opener = lambda f, *args, **kw: op(encodefilename(f),  
>> *args, **kw)
>> +        self.opener.read = (lambda f, *args, **kw:
>> +                            op(encodefilename(f), *args, **kw).read 
>> ())
>
> IMHO, this is too intrusive. You shouldn't have to add a read member  
> function to
> every opener.
>
> Consider creating free functions util.read and write instead, taking  
> an opener
> as a parameter.

My main concern was writing the code in a maintainable fashion.  
Putting read() and write() methods on the opener seemed the simplest  
to me. As the store doesn't use the shared opener class, I had to fake  
it. An alternate approach would be to create a ‘filteropener’ class  
for the stores.

> in util.py:
>
> def read(op, path, text=False):
>    fp = op(path, 'r', text)
>    try:
>        return fp.read()
>    finally:
>        fp.close()
>
> def write(data, op, path, mode='w', text=False, atomictemp=False):
>    fp = op(path, mode, text, atomictemp)
>    try:
>        fp.write(data)
>        if atomictemp:
>            fp.rename()
>    finally:
>        fp.close()
…
> and then use it as (example):
>
>> diff --git a/hgext/mq.py b/hgext/mq.py
>> --- a/hgext/mq.py
>> +++ b/hgext/mq.py
>> @@ -272,14 +272,14 @@ class queue(object):
>>             def parse(l):
>>                 n, name = l.split(':', 1)
>>                 return statusentry(bin(n), name)
>> -            lines = self.opener(self.status_path).read().splitlines 
>> ()
>> +            lines = self.opener.read(self.status_path).splitlines()
>
>               lines = util.read(self.opener,  
> self.status_path).splitlines()
>
>>             return [parse(l) for l in lines]
>>         return []

I don't really see the point to this. I find it logical that  
operations on openers should be part of the opener class. A util.read 
() function might be useful for cases where an opener isn't used; I'll  
look into adding that.

> and you probably need:
>
>
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -822,7 +822,7 @@ class atomictempfile(object):
>             self._fp.close()
>             rename(self.temp, localpath(self.__name))
>
> -    def __del__(self):
> +    def close(self):
>         if not self._fp:
>             return
>         if not self._fp.closed:
> @@ -831,6 +831,9 @@ class atomictempfile(object):
>             except: pass
>             self._fp.close()
>
> +    def __del__(self):
> +        self.close()
> +
> def makedirs(name, mode=None):
>     """recursive directory creation with parent mode inheritance"""
>     try:

I actually had code similar to this in my patch queue, but deleted it  
as it didn't seem to be used. I've restored it for consistency.

--

Dan Villiom Podlaski Christiansen
danchr at gmail.com

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 1943 bytes
Desc: not available
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20101203/69c047db/attachment.bin>


More information about the Mercurial-devel mailing list