[PATCH 1 of 6 RFC] manifest: introduce an accessor class for manifests

[Pierre-Yves David]

On 11/05/2016 02:05 PM, Yuya Nishihara wrote:
> On Thu, 3 Nov 2016 15:27:37 -0700, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham at fb.com>
>> # Date 1478208817 25200
>> #      Thu Nov 03 14:33:37 2016 -0700
>> # Branch stable
>> # Node ID 1788ee9e1df92ac94b9be84eac6d16e3bad903a9
>> # Parent  b9f7b0c10027764cee77f9c6d61877fcffea837f
>> manifest: introduce an accessor class for manifests
>>
>> This introduces a revlogaccessor class which can be used to allow multiple
>> objects hold an auto-invalidating reference to a revlog, without having to hold
>> a reference to the actual repo object. Future patches will switch repo.manifest
>> and repo.manifestlog to access the manifest through this accessor. This will fix
>> the circular reference caused by manifestlog and manifestctx holding a reference
>> to the repo
>>
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -514,6 +514,11 @@ class localrepository(object):
>>          # manifest creation.
>>          return manifest.manifest(self.svfs)
>>
>> +    @unfilteredpropertycache
>> +    def manifestaccessor(self):
>> +        return revlogaccessor('00manifest.i', self.svfs,
>> +                              self._constructmanifest)
>
> Honestly I don't get why manifestlog and manifestctxs have to live longer
> than the other repo properties.

I'm also a bit curious about that.

> But suppose that is necessary, I agree we'll
> need this kind of a wrapper.

Any reason why we are using the wrapper approach over using a weak 
reference ? Weakref are not great but we use them in multiple spot when 
needed. this seems it would be simpler than the current approach, but I 
might be missing something.

> Maybe we can move the accessor to the manifestlog,
> but still the accessor will have to be shared (and updated transparently) by
> the manifestlog and its cachable manifestctxs.
>
>> +def revlogaccessor(filename, opener, constructor):
>> +    """Creates an accessor that provides cached and invalidated access to a
>> +    revlog, via instance.revlog. This is useful for letting multiple objects
>> +    hold a reference to the revlog, without having to hold a possibly-circular
>> +    reference to the actual repository.  """
>> +
>> +    # We have to use a runtime type here, because the only way to create a
>> +    # property is to put it on a class itself, and the property is dynamically
>> +    # defined by the filename parameter.
>> +    class accessor(object):
>
> Perhaps we could refactor filecache to avoid dynamically creating a class, but
> that would be a minor issue of this RFC series.

-- 
Pierre-Yves David