[PATCH 3 of 4 STABLE] manifest: make manifestctx store the repo

[Pierre-Yves David]

On 10/27/2016 07:55 PM, Durham Goode wrote:
>
>
> On 10/27/16 5:22 AM, Yuya Nishihara wrote:
>> On Wed, 26 Oct 2016 09:42:09 -0700, Durham Goode wrote:
>>> On 10/26/16 5:54 AM, Yuya Nishihara wrote:
>>>> On Tue, 25 Oct 2016 12:46:58 -0700, Durham Goode wrote:
>>>>> On 10/22/16 1:59 AM, Yuya Nishihara wrote:
>>>>>> On Tue, 18 Oct 2016 17:50:16 -0700, Durham Goode wrote:
>>>>>>> # HG changeset patch
>>>>>>> # User Durham Goode <durham at fb.com>
>>>>>>> # Date 1476837866 25200
>>>>>>> #      Tue Oct 18 17:44:26 2016 -0700
>>>>>>> # Branch stable
>>>>>>> # Node ID 3efece5c59853908d65de53635488361dbf20c49
>>>>>>> # Parent  ed607426a3ff4deda8c7f2de8b86d5b6ca976d67
>>>>>>> manifest: make manifestctx store the repo
>>>>>>>
>>>>>>> The old manifestctx stored a reference to the revlog. If the
>>>>>>> inmemory revlog
>>>>>>> became invalid, the ctx now held an old copy and would be
>>>>>>> incorrect. To fix
>>>>>>> this, we need the ctx to go through the manifestlog for each access.
>>>>>>>
>>>>>>> This is the same pattern that changectx already uses (it stores
>>>>>>> the repo, and
>>>>>>> accesses commit data through self._repo.changelog).
>>>>>>>
>>>>>>> diff --git a/mercurial/manifest.py b/mercurial/manifest.py
>>>>>>> --- a/mercurial/manifest.py
>>>>>>> +++ b/mercurial/manifest.py
>>>>>>> @@ -1276,7 +1276,7 @@ class manifestlog(object):
>>>>>>>             if self._treeinmem:
>>>>>>>                 m = treemanifestctx(self._revlog, '', node)
>>>>>>>             else:
>>>>>>> -            m = manifestctx(self._revlog, node)
>>>>>>> +            m = manifestctx(self._repo, node)
>>>>>>>             if node != revlog.nullid:
>>>>>>>                 self._mancache[node] = m
>>>>>> This will create a reference cycle, but I don't know if the
>>>>>> situation gets
>>>>>> better or worse after this patch.
>>>>>>
>>>>>>      repo -> manifestlog -> _mancache -> manifestctx -> repo
>>>>>>
>>>>>> Is _mancache valid after the manifestlog is recreated?
>>>>> _mancache is not really valid after manifestlog is recreated, since it
>>>>> may contain manifest entries that no longer exist in the file. Even if
>>>>> it didn't contain bad entries, the manifestctx itself needs up-to-date
>>>>> access to the manifest revlog, which is only available through the
>>>>> repo
>>>>> object (since the repo object's property is what handles manifest
>>>>> revlog
>>>>> cache checking).
>>>> Perhaps I miss the point. Do we have stale copies of manifestlog and
>>>> manifestctx
>>>> somewhere? My understanding is we've moved the @storecache to
>>>> manifestlog() at
>>>> 3c8811efdddc, so manifestlog, _revlog (and _mancache) should be
>>>> accessible only
>>>> when they are valid, and manifestlog doesn't live longer than
>>>> repo.manifest().
>>> Ah, yes manifestctx's may be held for longer than the manifestlog lives,
>>> by code that is out of our control.  I'm not aware of specific cases
>>> where this is happening, but it could.
>> Got it, thanks.
>>
>> It seems manifestctx has slightly different guarantee about its
>> lifetime from
>> changectx. changectx is effectively a snapshot when repo[changeid] is
>> called
>> (more precisely, when propertycached function is called.)
> changectx seems the same to me.  From looking at the changectx code, it
> looks like it doesn't actually load any of the commit data until the
> first property is accessed (i.e. changectx._changectx looks in the
> changelog for data).  So if someone held on to the ctx for a bit, and
> accessed ctx._changectx after the changelog had been mutated, it's
> important that it goes through repo.changelog so it gets the most
> up-to-date version.

The changectx seems to be using the revision index to access data. So 
I'm not sure in what kind of scenario would make a reloading of the 
mutated changelog useful. Am I missing something ? Can you elaborate on 
these case?


-- 
Pierre-Yves David