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

[Adrian Buehlmann]

On 2010-12-27 16:52, Dan Villiom Podlaski Christiansen wrote:
> 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…

I don't see much productive content in this reply. I fail to see in what
angle I should elaborate this more.

So, unless someone else has something to add here, I'll wait and see if
Matt is going to take this stuff or not. In the end it's his design
(which I did like as it was before this patch).

I already know that Dan and I disagree on this.