Patchwork [v2] shelve: add a shelve extension to save/restore working changes

login
register
mail settings
Submitter Pierre-Yves David
Date Sept. 9, 2013, 10:13 p.m.
Message ID <8D028B08-CCFF-4D34-9BA1-65FBF9AE874B@ens-lyon.org>
Download mbox | patch
Permalink /patch/2416/
State Superseded
Commit 49d4919d21c2fe79957d160d64acd048bf6a2e7b
Headers show

Comments

Pierre-Yves David - Sept. 9, 2013, 10:13 p.m.
On 9 sept. 2013, at 21:25, dsp@experimentalworks.net wrote:

>> Replacing the ``setattr(obj, self.name, value)`` call with ``obj.__dict__[self.name] = value`` would bypass repoview magic.
>> 
>> I'm actually a bit surprise that propertycache on repoview are so broken for two versions. As far as I understand the current code:
>> (1) All propertycache computed repoview side are actually store on the unfiltered repo. That would expect the unfiltered repo be confused.
>> (2) All repoview seems to not use the cached value anymore. I would have expected some performance impact there.
> 
> I looked into this today and want to clarify what's going on:
> 
> Due to multiple inheritance in the proxycls class we inherit all
> attributes and propertycache descriptors from localrepo/mqrepo as well
> as the __getattr__/__setattr__ from repoview.
> 
> Attribute lookups in Python prefer descriptors (e.g. the propertycache
> decorator) over instance values, over __getattr__/__setattr_
> implementations [1].

I'm pretty sure this is:

(1) Instance values
(2) class values (in mro order) descriptor are stored in class
(3) getattr

> When repo.mq is accessed, Python will choose the __get__ implementation
> from propertycache. When propertycache calls setattr it will choose (due
> to the nonexisting instance value) the __setattr__ from repoview,
> causing an instance variable creation in the unfiltered repo object (as
> repoview redirects to the unfiltered object)
> 
> The next time repo.mq is accessed, the code will still call __get__ as
> the propertycache never set a value on the proxycls object. This is
> causing a new mq object to be created.

You description is correct for a propertycache. The patch below should fix the bug by ensuring the cache is stored on the proxy instance. So the next access on the proxy will get the cache instead of calling the property.


Both patch pass the core-test suite. I've not setup to check it this fix the shelve error.

I'll send proper patch to the list tomorrow.

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -277,11 +277,11 @@  class propertycache(object):
         result = self.func(obj)
         self.cachevalue(obj, result)
         return result
 
     def cachevalue(self, obj, value):
-        setattr(obj, self.name, value)
+        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)

However the repo.mq in a special `unfilteredpropertycache` were a second layer of bug happen. The access to `unfilteredpropertycache` are always forwarded to the unfiltered repo. But it's done wrongly. The `unfilteredpropertycache` explicitly call propertycache.__get__ on each access and therefore never use the cached value. It should just access the attribute on the unfiltered repo and leave the underlying propertycache do the work. The patch below does that.

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -37,11 +37,14 @@  class storecache(repofilecache):
 
 class unfilteredpropertycache(propertycache):
     """propertycache that apply to unfiltered repo only"""
 
     def __get__(self, repo, type=None):
-        return super(unfilteredpropertycache, self).__get__(repo.unfiltered())
+        unfi = repo.unfiltered()
+        if unfi is repo:
+            return super(unfilteredpropertycache, self).__get__(unfi)
+        return getattr(unfi, self.name)
 
 class filteredpropertycache(propertycache):
     """propertycache that must take filtering in account"""
 
     def cachevalue(self, obj, value):