[Reviewers] [PATCH 2 of 3 V2] manifest: make uses of _mancache aware of contexts

[Pierre-Yves David]

On 09/12/2016 07:36 PM, Durham Goode wrote:
>
>
> On 9/7/16 9:10 AM, Pierre-Yves David wrote:
>>
>>
>> On 09/07/2016 05:35 PM, Pierre-Yves David wrote:
>>>
>>>
>>> On 09/03/2016 05:51 PM, Yuya Nishihara wrote:
>>>> On Wed, 31 Aug 2016 13:30:20 -0700, Durham Goode wrote:
>>>>> # HG changeset patch
>>>>> # User Durham Goode <durham at fb.com>
>>>>> # Date 1472518929 25200
>>>>> #      Mon Aug 29 18:02:09 2016 -0700
>>>>> # Node ID 59bc68e1d78538bb83f60c0d4f9342ec0a8893bf
>>>>> # Parent  abce9af35512d8589683d94f34f6d8aa21163568
>>>>> manifest: make uses of _mancache aware of contexts
>>>>
>>>>> --- a/mercurial/manifest.py
>>>>> +++ b/mercurial/manifest.py
>>>>> @@ -1128,7 +1128,11 @@ class manifest(manifestrevlog):
>>>>>          if node == revlog.nullid:
>>>>>              return self._newmanifest() # don't upset local cache
>>>>>          if node in self._mancache:
>>>>> -            return self._mancache[node]
>>>>> +            cached = self._mancache[node]
>>>>> +            if (isinstance(cached, manifestctx) or
>>>>> +                isinstance(cached, treemanifestctx)):
>>>>> +                cached = cached.read()
>>>>
>>>> manifestctx.read() is added by the next patch, so tests fail at this
>>>> revision.
>>>> But that wouldn't be a good reason enough to rework this series.
>>>
>>> This changeset or the next also introduce a strange failure in evolve
>>> tests where a manifest hash changes (eg: test-evolve.t). please hold on
>>> during investigation.
>>
>> So, this is strange, this changeset or the next affect various test in
>> evolve. I've digged in details for test-evolve.t where evolve create a
>> changeset with a different hash.
>>
>> I've added various debug data to the test to track how they change and:
>>
>> * the operation is a bump fix
>>
>> * the changeset have a different hash because manifest hash change
>> (kinda expected),
>>
>> * the manifest have a different hash because one of the file hash change,
>>
>> * file content and commit diff are similar
>>
>> * the file hash change because the revlog gains a p1 (from a previous
>> null ID),
>>
>> * The previous value (no parent for the filerev) is most probably
>> wrong as the file appear "Modified" in the commit.
>>
>> So, something in this changes seems to be magically "fix" evolve.
>>
>> That's suspicious and I would be much more comfortable if we
>> understood what is going on here. I'm about to switch out of work mode
>> for the evening, can someone else look into this (ideal Durham as he
>> is familliar with his change.
> I think I've found the issue here.  The bump code accesses the parent
> manifest and modifies the manifest dictionary (precmanifest in
> _solvebumped()). Because that manifestdict is in the mancache, those
> changes are persisted and the next thing to access the manifest (which
> is repo.commit()) gets the modified version which causes the new commit
> to be incorrect since it sees a bad p1.manifest().
>
> With my code, since the manifest cache gets broken into two, the version
> being modified is in a different cache from the version being accessed
> later, so it accidentally "fixes" the problem.
>
> If I add a ".copy()" to the bump code to copy the manifest before
> editing it, I get the exact same failures as if my manifest patches are
> applied.
>
> I'll send an evolve patch with the .copy() and the test updates. This
> manifestdict editing code has been around since 2014 at least. We may
> want to make non-copied manifests be read-only to prevent such errors in
> the future.

Thanks for digging this out. I agree we should make any cached manifest 
read only to prevent to this class of error. Can you handle this as part 
of your current rework ?

Cheers,

-- 
Pierre-Yves David