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

Durham Goode durham at fb.com
Mon Sep 12 13:36:16 EDT 2016



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.


More information about the Mercurial-devel mailing list