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

Adrian Buehlmann adrian at cadifra.com
Mon Dec 27 08:05:30 CST 2010


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.

(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.

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.

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.

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.

Cheers,

Adrian


More information about the Mercurial-devel mailing list