[PATCH 5 of 9] opener: add read & write utility methods

[Dan Villiom Podlaski Christiansen]

On 27 Dec 2010, at 15:05, Adrian Buehlmann wrote:

> On 2010-12-26 12:51, Dan Villiom Podlaski Christiansen wrote:
>> # HG changeset patch
>> # User Dan Villiom Podlaski Christiansen <danchr at gmail.com>
>> # Date 1293364212 -3600
>> # Node ID 3b29e00516d7b79cbac6fdd5a9c0e71746bf6df6
>> # Parent  43f605f94f8570b15d768a58179dd43620520a4c
>> opener: add read & write utility methods
>
> Again, that's still a bad idea (as I already expressed). See my  
> attempt
> at a reasoning at the end of this posting.
>
>> 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.
>>
>> A similar method for reading is added to the opener used by static
>> HTTP repositories. It doesn't do any actual closing, as the opener
>> doesn't have access to any underlying files.
>>
>> diff --git a/mercurial/statichttprepo.py b/mercurial/ 
>> statichttprepo.py
>> --- a/mercurial/statichttprepo.py
>> +++ b/mercurial/statichttprepo.py
>> @@ -75,6 +75,8 @@ def build_opener(ui, authinfo):
>>                 raise IOError('Permission denied')
>>             f = "/".join((p, urllib.quote(path)))
>>             return httprangereader(f, urlopener)
>> +        # for compatibility with regular file openers
>> +        o.read = lambda *args, **kwargs: o(*args, **kwargs).read()
>>         return o
>>
>>     opener.options = {'nonlazy': 1}
>> diff --git a/mercurial/util.py b/mercurial/util.py
>> --- a/mercurial/util.py
>> +++ b/mercurial/util.py
>> @@ -879,6 +879,20 @@ class opener(object):
>>             return
>>         os.chmod(name, self.createmode & 0666)
>>
>> +    def read(self, *args, **kwargs):
>> +        fp = self(*args, **kwargs)
>> +        try:
>> +            return fp.read()
>> +        finally:
>> +            fp.close()
>> +
>> +    def write(self, data, *args, **kwargs):
>> +        fp = self(*args, **kwargs)
>> +        try:
>> +            return fp.write(data)
>> +        finally:
>> +            fp.close()
>> +
>>     def __call__(self, path, mode="r", text=False, atomictemp=False):
>>         self.auditor(path)
>>         f = os.path.join(self.base, path)
>> @@ -953,6 +967,12 @@ class filteropener(object):
>>     def __call__(self, path, *args, **kwargs):
>>         return self.orig(self._filter(path), *args, **kwargs)
>>
>> +    def read(self, path, *args, **kwargs):
>> +        return self._orig.read(self._filter(path), *args, **kwargs)
>> +
>> +    def write(self, data, path, *args, **kwargs):
>> +        return self._orig.write(self._filter(path), *args, **kwargs)
>> +
>> class chunkbuffer(object):
>>     """Allow arbitrary sized chunks of data to be efficiently read  
>> from an
>>     iterator over chunks of arbitrary size."""
>
> There are two major abstraction errors in this proposal:
>
> (1) The job of openers is to *open* files, *not* reading and writing
>    them.
>
>    Excerpt from util.py:
>
>      class opener(object):
>          """Open files relative to a base directory
>
>          This class is used to hide the details of COW semantics and
>          remote file access from higher level code.
>          """
>
>    Adding read and write member functions to opener weakens that  
> concept.

See, this is where we disagree. You consider this a fatal error. I  
consider it an acceptable extension of the responsibilities of the  
opener. As you say, the current responsibilities of the opener class  
can be summarized as “opening files”. What my patch does is extend  
this to make them useful for “accessing files”. Personally, I find  
this extension, simple and logical, but I suspect you disagree. The  
code has quite a lot of places that do opener(…).read() and opener 
(…).write() — in my mind, this suggests that the two aren't all that  
unrelated.

There were *quite* a lot of places that needed fixing in this series.  
Even during the time this series has been awaiting review, two new  
places cropped up. Most developers aren't going to run Mercurial on  
PyPy, and many developers probably don't even run the check-code tests  
that frequently. This suggests to me that it's important that the way  
to make the code work on PyPy is intuitive and easy to remember.

In my humble opinion, the proposed patch achieves all this. You  
consider the changes worse, but although you accurately describe the  
changes, you haven't provided a single reason why they're making  
things worse. Please explain why »weakening concepts« by adding  
methods to openers is worse. What concrete risks do you foresee in  
this change? Do you fear that it will make the code less maintainable?  
Something else?

> (2) The proposed new 'filteropener' class builds upon the error (1)  
> and
>    adds yet another abstraction error by misleadingly using the term
>    "filter" in the name.
>
>    For me, a filter keeps some things from passing it. That's not what
>    filteropener seems to do.

I didn't think of that. Do you have any suggestions for a better name  
for this concept? I can't think of any right now…

> My proposal to use free read and write functions instead to fix  
> error (1)
> seems to have been overlooked so far by some commentators in this  
> muddled
> huge thread.

I assure you; I for one did not overlook it. I just don't see how it  
solves anything, and I doubt that a complete implementation is as  
simple as what you proposed. I'd also hate to edit every single place  
that currently does opener.(read|write) in the current patch, without  
a more weighty reason.

> BTW1, it might help to start a new thread for each version of this  
> series.
> Using V1, V2, V3 for --flag of hg email might be useful to separate  
> the
> different versions of the series and so that reviewers can tell on  
> each
> individual patch to which version of the series it belongs.

I'll do that in the future.

> BTW2, I'm not against supporting PyPy and I don't think my concerns  
> are
> limited to the Windows platform in particular. I would just  
> appreciate if
> the current opener/store concepts wouldn't have to be needlessly  
> deteriorated
> in the process.

Again, you don't explain *how* or *why* anything is deteriorated. You  
merely assert that it is!

This is essentially stylistic bikeshedding. You like your bikeshed  
colored ‘util.read(repo.opener(…))’ — I like it colored  
‘repo.opener.read(…)’. I have a complete, functional implementation of  
my proposal. You're welcome to write your own proposal. I'd rather not  
rewrite this patch unless I *really* have to…

--

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/20101227/74491f85/attachment.bin>