Patchwork [1,of,7] vfs: use propertycache for open

login
register
mail settings
Submitter Pierre-Yves David
Date Aug. 5, 2016, 1:59 p.m.
Message ID <9b65c0820c940442454e.1470405554@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/16117/
State Accepted
Headers show

Comments

Pierre-Yves David - Aug. 5, 2016, 1:59 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1470322610 -7200
#      Thu Aug 04 16:56:50 2016 +0200
# Node ID 9b65c0820c940442454e5f92a9475c4ec97f705c
# Parent  73ff159923c1f05899c27238409ca398342d9ae0
# EXP-Topic vfsward
vfs: use propertycache for open

The current open method is currently behaving like a property cache. We use our
utility decorator to make this explicit.
Jun Wu - Aug. 5, 2016, 2:26 p.m.
I don't think this is correct, as the filesystem is mutable. Consider:

  vfs.open(path)
  os.unlink(path)
  vfs.open(path) # <= Wrong result using propertycache

Excerpts from Pierre-Yves David's message of 2016-08-05 15:59:14 +0200:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1470322610 -7200
> #      Thu Aug 04 16:56:50 2016 +0200
> # Node ID 9b65c0820c940442454e5f92a9475c4ec97f705c
> # Parent  73ff159923c1f05899c27238409ca398342d9ae0
> # EXP-Topic vfsward
> vfs: use propertycache for open
> 
> The current open method is currently behaving like a property cache. We use our
> utility decorator to make this explicit.
> 
> diff -r 73ff159923c1 -r 9b65c0820c94 mercurial/scmutil.py
> --- a/mercurial/scmutil.py    Mon Aug 01 13:14:13 2016 -0400
> +++ b/mercurial/scmutil.py    Thu Aug 04 16:56:50 2016 +0200
> @@ -256,17 +256,15 @@ class abstractvfs(object):
>                  raise
>          return []
>  
> -    def open(self, path, mode="r", text=False, atomictemp=False,
> -             notindexed=False, backgroundclose=False):
> +    @util.propertycache
> +    def open(self):
>          '''Open ``path`` file, which is relative to vfs root.
>  
>          Newly created directories are marked as "not to be indexed by
>          the content indexing service", if ``notindexed`` is specified
>          for "write" mode access.
>          '''
> -        self.open = self.__call__
> -        return self.__call__(path, mode, text, atomictemp, notindexed,
> -                             backgroundclose=backgroundclose)
> +        return self.__call__
>  
>      def read(self, path):
>          with self(path, 'rb') as fp:
Yuya Nishihara - Aug. 5, 2016, 3:53 p.m.
On Fri, 5 Aug 2016 15:26:48 +0100, Jun Wu wrote:
> I don't think this is correct, as the filesystem is mutable. Consider:
> 
>   vfs.open(path)
>   os.unlink(path)
>   vfs.open(path) # <= Wrong result using propertycache

It just caches "open" function. Basically it is open = __call__, but __call__
is provided by a sub class. I don't know which is the canonical form.
Jun Wu - Aug. 5, 2016, 4:01 p.m.
Ahh, it's trickier than I thought. Thanks for pointing this out!

Excerpts from Yuya Nishihara's message of 2016-08-06 00:53:19 +0900:
> On Fri, 5 Aug 2016 15:26:48 +0100, Jun Wu wrote:
> > I don't think this is correct, as the filesystem is mutable. Consider:
> > 
> >   vfs.open(path)
> >   os.unlink(path)
> >   vfs.open(path) # <= Wrong result using propertycache
> 
> It just caches "open" function. Basically it is open = __call__, but __call__
> is provided by a sub class. I don't know which is the canonical form.
Yuya Nishihara - Aug. 6, 2016, 3:18 a.m.
On Fri, 05 Aug 2016 15:59:14 +0200, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1470322610 -7200
> #      Thu Aug 04 16:56:50 2016 +0200
> # Node ID 9b65c0820c940442454e5f92a9475c4ec97f705c
> # Parent  73ff159923c1f05899c27238409ca398342d9ae0
> # EXP-Topic vfsward
> vfs: use propertycache for open

Queued this one, thanks.

> --- a/mercurial/scmutil.py	Mon Aug 01 13:14:13 2016 -0400
> +++ b/mercurial/scmutil.py	Thu Aug 04 16:56:50 2016 +0200
> @@ -256,17 +256,15 @@ class abstractvfs(object):
>                  raise
>          return []
>  
> -    def open(self, path, mode="r", text=False, atomictemp=False,
> -             notindexed=False, backgroundclose=False):
> +    @util.propertycache
> +    def open(self):
>          '''Open ``path`` file, which is relative to vfs root.
>  
>          Newly created directories are marked as "not to be indexed by
>          the content indexing service", if ``notindexed`` is specified
>          for "write" mode access.
>          '''
> -        self.open = self.__call__
> -        return self.__call__(path, mode, text, atomictemp, notindexed,
> -                             backgroundclose=backgroundclose)
> +        return self.__call__

But I think we should deprecate either __call__ or open. Keeping bound method
creates reference cycle.
Augie Fackler - Aug. 11, 2016, 6:25 p.m.
On Fri, Aug 05, 2016 at 03:59:14PM +0200, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1470322610 -7200
> #      Thu Aug 04 16:56:50 2016 +0200
> # Node ID 9b65c0820c940442454e5f92a9475c4ec97f705c
> # Parent  73ff159923c1f05899c27238409ca398342d9ae0
> # EXP-Topic vfsward
> vfs: use propertycache for open
>
> The current open method is currently behaving like a property cache. We use our
> utility decorator to make this explicit.
>
> diff -r 73ff159923c1 -r 9b65c0820c94 mercurial/scmutil.py
> --- a/mercurial/scmutil.py	Mon Aug 01 13:14:13 2016 -0400
> +++ b/mercurial/scmutil.py	Thu Aug 04 16:56:50 2016 +0200
> @@ -256,17 +256,15 @@ class abstractvfs(object):
>                  raise
>          return []
>
> -    def open(self, path, mode="r", text=False, atomictemp=False,
> -             notindexed=False, backgroundclose=False):
> +    @util.propertycache
> +    def open(self):
>          '''Open ``path`` file, which is relative to vfs root.
>
>          Newly created directories are marked as "not to be indexed by
>          the content indexing service", if ``notindexed`` is specified
>          for "write" mode access.
>          '''
> -        self.open = self.__call__
> -        return self.__call__(path, mode, text, atomictemp, notindexed,
> -                             backgroundclose=backgroundclose)
> +        return self.__call__

Is there a reason we can't just do `open = __call__` instead of the
@property{,cache} trickery? What does using the property decorator
mechanism really buy us here?

>
>      def read(self, path):
>          with self(path, 'rb') as fp:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - Aug. 11, 2016, 7:24 p.m.
On 08/11/2016 08:25 PM, Augie Fackler wrote:
> On Fri, Aug 05, 2016 at 03:59:14PM +0200, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1470322610 -7200
>> #      Thu Aug 04 16:56:50 2016 +0200
>> # Node ID 9b65c0820c940442454e5f92a9475c4ec97f705c
>> # Parent  73ff159923c1f05899c27238409ca398342d9ae0
>> # EXP-Topic vfsward
>> vfs: use propertycache for open
>>
>> The current open method is currently behaving like a property cache. We use our
>> utility decorator to make this explicit.
>>
>> diff -r 73ff159923c1 -r 9b65c0820c94 mercurial/scmutil.py
>> --- a/mercurial/scmutil.py	Mon Aug 01 13:14:13 2016 -0400
>> +++ b/mercurial/scmutil.py	Thu Aug 04 16:56:50 2016 +0200
>> @@ -256,17 +256,15 @@ class abstractvfs(object):
>>                  raise
>>          return []
>>
>> -    def open(self, path, mode="r", text=False, atomictemp=False,
>> -             notindexed=False, backgroundclose=False):
>> +    @util.propertycache
>> +    def open(self):
>>          '''Open ``path`` file, which is relative to vfs root.
>>
>>          Newly created directories are marked as "not to be indexed by
>>          the content indexing service", if ``notindexed`` is specified
>>          for "write" mode access.
>>          '''
>> -        self.open = self.__call__
>> -        return self.__call__(path, mode, text, atomictemp, notindexed,
>> -                             backgroundclose=backgroundclose)
>> +        return self.__call__
>
> Is there a reason we can't just do `open = __call__` instead of the
> @property{,cache} trickery? What does using the property decorator
> mechanism really buy us here?

yes, pretty much all vfs subclass redefine __call__

Patch

diff -r 73ff159923c1 -r 9b65c0820c94 mercurial/scmutil.py
--- a/mercurial/scmutil.py	Mon Aug 01 13:14:13 2016 -0400
+++ b/mercurial/scmutil.py	Thu Aug 04 16:56:50 2016 +0200
@@ -256,17 +256,15 @@  class abstractvfs(object):
                 raise
         return []
 
-    def open(self, path, mode="r", text=False, atomictemp=False,
-             notindexed=False, backgroundclose=False):
+    @util.propertycache
+    def open(self):
         '''Open ``path`` file, which is relative to vfs root.
 
         Newly created directories are marked as "not to be indexed by
         the content indexing service", if ``notindexed`` is specified
         for "write" mode access.
         '''
-        self.open = self.__call__
-        return self.__call__(path, mode, text, atomictemp, notindexed,
-                             backgroundclose=backgroundclose)
+        return self.__call__
 
     def read(self, path):
         with self(path, 'rb') as fp: