Patchwork [1,of,2,STABLE] repoview: make propertycache.setcache compatible with repoview

login
register
mail settings
Submitter Pierre-Yves David
Date Sept. 30, 2013, 12:53 p.m.
Message ID <365d4888627295bb0368.1380545612@vulgaris>
Download mbox | patch
Permalink /patch/2666/
State Accepted
Commit a1237a4b437d6404d035c49573955cedaa57236b
Headers show

Comments

Pierre-Yves David - Sept. 30, 2013, 12:53 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1380544571 -7200
#      Mon Sep 30 14:36:11 2013 +0200
# Branch stable
# Node ID 365d4888627295bb0368d658224cb9900fdcb6f7
# Parent  4d2bea6604d3bfb34860590fbb6e302cbf273f98
repoview: make propertycache.setcache compatible with repoview

Propertycache used standard attribute assignment. In the repoview case, this
assignment was forwarded to the unfiltered repo. This result in:
(1) unfiltered repo got a potentially wrong cache value,
(2) repoview never reused the cached value.

This patch replaces the standard attribute assignment by an assignment to
`objc.__dict__` which will bypass the `repoview.__setattr__`. This will not
affects other `propertycache` users and it is actually closer to the semantic we
need.

The interaction of `propertycache` and `repoview` are now tested in a python
test file.
Idan Kamara - Oct. 1, 2013, 11:04 a.m.
On Mon, Sep 30, 2013 at 3:53 PM, <pierre-yves.david@ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1380544571 -7200
> #      Mon Sep 30 14:36:11 2013 +0200
> # Branch stable
> # Node ID 365d4888627295bb0368d658224cb9900fdcb6f7
> # Parent  4d2bea6604d3bfb34860590fbb6e302cbf273f98
> repoview: make propertycache.setcache compatible with repoview
>
> Propertycache used standard attribute assignment. In the repoview case,
> this
> assignment was forwarded to the unfiltered repo. This result in:
> (1) unfiltered repo got a potentially wrong cache value,
> (2) repoview never reused the cached value.
>
> This patch replaces the standard attribute assignment by an assignment to
> `objc.__dict__` which will bypass the `repoview.__setattr__`. This will not
> affects other `propertycache` users and it is actually closer to the
> semantic we
> need.
>
> The interaction of `propertycache` and `repoview` are now tested in a
> python
> test file.
>

Wouldn't this test be a lot more readable as a standard Python unit test?
Pierre-Yves David - Oct. 1, 2013, 2:08 p.m.
On 10/01/2013 01:04 PM, Idan Kamara wrote:
>
> On Mon, Sep 30, 2013 at 3:53 PM, <pierre-yves.david@ens-lyon.org 
> <mailto:pierre-yves.david@ens-lyon.org>> wrote:
>
>     # HG changeset patch
>     # User Pierre-Yves David <pierre-yves.david@ens-lyon.org
>     <mailto:pierre-yves.david@ens-lyon.org>>
>     # Date 1380544571 -7200
>     #      Mon Sep 30 14:36:11 2013 +0200
>     # Branch stable
>     # Node ID 365d4888627295bb0368d658224cb9900fdcb6f7
>     # Parent  4d2bea6604d3bfb34860590fbb6e302cbf273f98
>     repoview: make propertycache.setcache compatible with repoview
>
>     Propertycache used standard attribute assignment. In the repoview
>     case, this
>     assignment was forwarded to the unfiltered repo. This result in:
>     (1) unfiltered repo got a potentially wrong cache value,
>     (2) repoview never reused the cached value.
>
>     This patch replaces the standard attribute assignment by an
>     assignment to
>     `objc.__dict__` which will bypass the `repoview.__setattr__`. This
>     will not
>     affects other `propertycache` users and it is actually closer to
>     the semantic we
>     need.
>
>     The interaction of `propertycache` and `repoview` are now tested
>     in a python
>     test file.
>
>
> Wouldn't this test be a lot more readable as a standard Python unit test?

That's not how other pure python test are written.
Idan Kamara - Oct. 1, 2013, 3:11 p.m.
On Oct 1, 2013 5:09 PM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
wrote:
>
> On 10/01/2013 01:04 PM, Idan Kamara wrote:
>>
>>
>> On Mon, Sep 30, 2013 at 3:53 PM, <pierre-yves.david@ens-lyon.org> wrote:
>>>
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>>> # Date 1380544571 -7200
>>> #      Mon Sep 30 14:36:11 2013 +0200
>>> # Branch stable
>>> # Node ID 365d4888627295bb0368d658224cb9900fdcb6f7
>>> # Parent  4d2bea6604d3bfb34860590fbb6e302cbf273f98
>>> repoview: make propertycache.setcache compatible with repoview
>>>
>>> Propertycache used standard attribute assignment. In the repoview case,
this
>>> assignment was forwarded to the unfiltered repo. This result in:
>>> (1) unfiltered repo got a potentially wrong cache value,
>>> (2) repoview never reused the cached value.
>>>
>>> This patch replaces the standard attribute assignment by an assignment
to
>>> `objc.__dict__` which will bypass the `repoview.__setattr__`. This will
not
>>> affects other `propertycache` users and it is actually closer to the
semantic we
>>> need.
>>>
>>> The interaction of `propertycache` and `repoview` are now tested in a
python
>>> test file.
>>
>>
>> Wouldn't this test be a lot more readable as a standard Python unit test?
>
>
> That's not how other pure python test are written.

Because most of them predate the time the testing framework supported unit
tests.

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -277,11 +277,12 @@  class propertycache(object):
         result = self.func(obj)
         self.cachevalue(obj, result)
         return result
 
     def cachevalue(self, obj, value):
-        setattr(obj, self.name, value)
+        # __dict__ assigment required to bypass __setattr__ (eg: repoview)
+        obj.__dict__[self.name] = value
 
 def pipefilter(s, cmd):
     '''filter string S through command CMD, returning its output'''
     p = subprocess.Popen(cmd, shell=True, close_fds=closefds,
                          stdin=subprocess.PIPE, stdout=subprocess.PIPE)
diff --git a/tests/test-propertycache.py b/tests/test-propertycache.py
new file mode 100644
--- /dev/null
+++ b/tests/test-propertycache.py
@@ -0,0 +1,94 @@ 
+"""test behavior of propertycache and unfiltered propertycache
+
+The repoview overlay is quite complexe. We test the behavior of
+property cache of both localrepo and repoview to prevent
+regression."""
+
+import os, subprocess
+import mercurial.localrepo
+import mercurial.repoview
+import mercurial.util
+import mercurial.hg
+import mercurial.ui as uimod
+
+
+# create some special property cache that trace they call
+
+calllog = []
+@mercurial.util.propertycache
+def testcachedfoobar(repo):
+    name = repo.filtername
+    if name is None:
+        name = ''
+    val = len(name)
+    calllog.append(val)
+    return val
+
+#plug them on repo
+mercurial.localrepo.localrepository.testcachedfoobar = testcachedfoobar
+
+
+# create an empty repo. and instanciate it. It is important to run
+# those test on the real object to detect regression.
+repopath = os.path.join(os.environ['TESTTMP'], 'repo')
+subprocess.check_call(['hg', 'init', repopath])
+ui = uimod.ui()
+repo = mercurial.hg.repository(ui, path=repopath).unfiltered()
+
+
+print ''
+print '=== property cache ==='
+print ''
+print 'calllog:', calllog
+print 'cached value (unfiltered):',
+print vars(repo).get('testcachedfoobar', 'NOCACHE')
+
+print ''
+print '= first access on unfiltered, should do a call'
+print 'access:', repo.testcachedfoobar
+print 'calllog:', calllog
+print 'cached value (unfiltered):',
+print vars(repo).get('testcachedfoobar', 'NOCACHE')
+
+print ''
+print '= second access on unfiltered, should not do call'
+print 'access', repo.testcachedfoobar
+print 'calllog:', calllog
+print 'cached value (unfiltered):',
+print vars(repo).get('testcachedfoobar', 'NOCACHE')
+
+print ''
+print '= first access on "visible" view, should do a call'
+visibleview = repo.filtered('visible')
+print 'cached value ("visible" view):',
+print vars(visibleview).get('testcachedfoobar', 'NOCACHE')
+print 'access:', visibleview.testcachedfoobar
+print 'calllog:', calllog
+print 'cached value (unfiltered):',
+print vars(repo).get('testcachedfoobar', 'NOCACHE')
+print 'cached value ("visible" view):',
+print vars(visibleview).get('testcachedfoobar', 'NOCACHE')
+
+print ''
+print '= second access on "visible view", should not do call'
+print 'access:', visibleview.testcachedfoobar
+print 'calllog:', calllog
+print 'cached value (unfiltered):',
+print vars(repo).get('testcachedfoobar', 'NOCACHE')
+print 'cached value ("visible" view):',
+print vars(visibleview).get('testcachedfoobar', 'NOCACHE')
+
+print ''
+print '= no effect on other view'
+immutableview = repo.filtered('immutable')
+print 'cached value ("immutable" view):',
+print vars(immutableview).get('testcachedfoobar', 'NOCACHE')
+print 'access:', immutableview.testcachedfoobar
+print 'calllog:', calllog
+print 'cached value (unfiltered):',
+print vars(repo).get('testcachedfoobar', 'NOCACHE')
+print 'cached value ("visible" view):',
+print vars(visibleview).get('testcachedfoobar', 'NOCACHE')
+print 'cached value ("immutable" view):',
+print vars(immutableview).get('testcachedfoobar', 'NOCACHE')
+
diff --git a/tests/test-propertycache.py.out b/tests/test-propertycache.py.out
new file mode 100644
--- /dev/null
+++ b/tests/test-propertycache.py.out
@@ -0,0 +1,36 @@ 
+
+=== property cache ===
+
+calllog: []
+cached value (unfiltered): NOCACHE
+
+= first access on unfiltered, should do a call
+access: 0
+calllog: [0]
+cached value (unfiltered): 0
+
+= second access on unfiltered, should not do call
+access 0
+calllog: [0]
+cached value (unfiltered): 0
+
+= first access on "visible" view, should do a call
+cached value ("visible" view): NOCACHE
+access: 7
+calllog: [0, 7]
+cached value (unfiltered): 0
+cached value ("visible" view): 7
+
+= second access on "visible view", should not do call
+access: 7
+calllog: [0, 7]
+cached value (unfiltered): 0
+cached value ("visible" view): 7
+
+= no effect on other view
+cached value ("immutable" view): NOCACHE
+access: 9
+calllog: [0, 7, 9]
+cached value (unfiltered): 0
+cached value ("visible" view): 7
+cached value ("immutable" view): 9