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