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

login
register
mail settings
Submitter Durham Goode
Date Aug. 31, 2016, 8:30 p.m.
Message ID <59bc68e1d78538bb83f6.1472675420@dev8486.prn1.facebook.com>
Download mbox | patch
Permalink /patch/16517/
State Accepted
Headers show

Comments

Durham Goode - Aug. 31, 2016, 8:30 p.m.
# HG changeset patch
# User Durham Goode <durham@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

In a future patch we will change manifestctx and treemanifestctx to no longer
derive from manifestdict and treemanifest, respectively. This means that
consumers of the _mancache will now need to be aware of the different between
the two, until we get rid of the manifest entirely and the _mancache becomes
only filled with ctxs.
Yuya Nishihara - Sept. 3, 2016, 3:51 p.m.
On Wed, 31 Aug 2016 13:30:20 -0700, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@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.
Pierre-Yves David - Sept. 7, 2016, 3:35 p.m.
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@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.
Pierre-Yves David - Sept. 7, 2016, 4:10 p.m.
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@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've pushed a draft for evolve that display the various debug data here: 
https://www.mercurial-scm.org/repo/users/marmoute/evolve/rev/7a76f9a43e89

If runs fine with:

   # Node ID f3157d8a10fb02a1a051ddaaf6fe996f7334f8f1
   manifest: add treemanifestctx class

And fails with:

   # Node ID d130a38ef41f3c9e2d2f26df3535d89a93f87301
   manifest: change manifestctx to not inherit from manifestdict

I can't test the other change because it is foobar:

   # Node ID ff884112d0efa24f60cd8e3334adc8ef36f5da0f
   manifest: make uses of _mancache aware of contexts

The test failure are:

> --- /home/marmoute/src/mutable-history/tests/test-evolve.t
> +++ /home/marmoute/src/mutable-history/tests/test-evolve.t.err
> @@ -401,8 +401,8 @@
>    recreate:[8] another feature that rox
>    atop:[7] another feature (child of ba0ec09b1bab)
>    computing new diff
> -  committed as 2d8c5414e9f0
> -  working directory is now at 2d8c5414e9f0
> +  committed as 6707c5e1c49d
> +  working directory is now at 6707c5e1c49d
>    $ hg status --change .
>    M main-file-1
>    $ hg export .
> @@ -410,7 +410,7 @@
>    # User test
>    # Date 0 0
>    #      Thu Jan 01 00:00:00 1970 +0000
> -  # Node ID 2d8c5414e9f0cabf0286a53ec526ab7cda76468c
> +  # Node ID 6707c5e1c49d54b986803331aaf78eae02764fc9
>    # Parent  99833d22b0c6526806fe26fe79b61ad51a72458d
>    bumped update to 99833d22b0c6:
>
> @@ -423,13 +423,13 @@
>    -Zwei
>    +deux
>    $ hg log --debug --rev . --config ui.logtemplate=
> -  changeset:   9:2d8c5414e9f0cabf0286a53ec526ab7cda76468c
> +  changeset:   9:6707c5e1c49d54b986803331aaf78eae02764fc9
>    bookmark:    feature-B
>    tag:         tip
>    phase:       draft
>    parent:      7:99833d22b0c6526806fe26fe79b61ad51a72458d
>    parent:      -1:0000000000000000000000000000000000000000
> -  manifest:    9:9dee7532189f6d1b2eadb9cad8f3407a3ed02162
> +  manifest:    9:4f07da51791a798dcf81546115081323819b7a83
>    user:        test
>    date:        Thu Jan 01 00:00:00 1970 +0000
>    files:       main-file-1
> @@ -443,7 +443,7 @@
>    $ hg manifest --debug -r .
>    669c54abb32b6f06b395c85feca4511e433ad00b 644   file-from-A
>    94f2f82a0c3f341ead55ea8d2b130f972ed56666 644   file-from-B
> -  006a74077e75026fda3ebe59f330125443f13e8c 644   main-file-1
> +  64ad1b503f917abc9118376ab94f7d762466d9e7 644   main-file-1
>    94f2f82a0c3f341ead55ea8d2b130f972ed56666 644   main-file-2
>    $ hg debugindex -m --debug
>       rev    offset  length  delta linkrev nodeid                                   p1                                       p2
> @@ -456,7 +456,7 @@
>         6       522     101      0       6 e656c939e387427a6b077ef798ccebd237d03ef5 7ec8c19bea7d58a3435e518951d344f145f00f21 0000000000000000000000000000000000000000
>         7       623     103      6       7 bf464c9d2093ac0524289f031f6c2e906041afb5 e656c939e387427a6b077ef798ccebd237d03ef5 0000000000000000000000000000000000000000
>         8       726     102      6       8 1da1c04471dbf03a362b0fdfeacb18989e6ec370 e656c939e387427a6b077ef798ccebd237d03ef5 0000000000000000000000000000000000000000
> -       9       828     129     -1       9 9dee7532189f6d1b2eadb9cad8f3407a3ed02162 bf464c9d2093ac0524289f031f6c2e906041afb5 0000000000000000000000000000000000000000
> +       9       828     131     -1       9 4f07da51791a798dcf81546115081323819b7a83 bf464c9d2093ac0524289f031f6c2e906041afb5 0000000000000000000000000000000000000000
>    $ hg cat -r . main-file-1
>    Un
>
> @@ -474,7 +474,7 @@
>         6        86       0      5       6 d6ada519b989 492807de722e 000000000000
>         7        86      17     -1       7 8d64e54db1c4 d6ada519b989 000000000000
>         8       103      17     -1       8 ba9aab3c7032 d6ada519b989 000000000000
> -       9       120       0      8       9 006a74077e75 000000000000 000000000000
> +       9       120       0      8       9 64ad1b503f91 8d64e54db1c4 000000000000
>    $ hg glog
>    @  9	feature-B: bumped update to 99833d22b0c6: - test
>    |
Durham Goode - Sept. 8, 2016, 6:19 p.m.
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@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'll look into this when I get a moment.  Feel free to drop the series 
until then.
via Mercurial-devel - Sept. 8, 2016, 7:16 p.m.
Dropped.

On Thu, Sep 8, 2016 at 11:19 AM, Durham Goode <durham@fb.com> 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@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'll look into this when I get a moment.  Feel free to drop the series until
> then.
Pierre-Yves David - Sept. 10, 2016, 12:10 a.m.
On 09/08/2016 09:16 PM, Martin von Zweigbergk via Mercurial-devel wrote:
> Dropped.

Okay, apparently they made it back into the repository and got 
published. After chatting with Martin and Durham I've pushed a backout 
of these two. If some other reviewers could give them a second 
acceptance stamp (Martin did one) that would be great.

>
> On Thu, Sep 8, 2016 at 11:19 AM, Durham Goode <durham@fb.com> 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@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'll look into this when I get a moment.  Feel free to drop the series until
>> then.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Durham Goode - Sept. 12, 2016, 5:36 p.m.
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@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.
Pierre-Yves David - Sept. 13, 2016, 12:39 a.m.
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@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,

Patch

diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
--- a/mercurial/bundlerepo.py
+++ b/mercurial/bundlerepo.py
@@ -204,8 +204,8 @@  class bundlemanifest(bundlerevlog, manif
         if isinstance(node, int):
             node = self.node(node)
 
-        if node in self._mancache:
-            result = self._mancache[node].text()
+        if node in self.fulltextcache:
+            result = self.fulltextcache[node].tostring()
         else:
             result = manifest.manifest.revision(self, nodeorrev)
         return result
diff --git a/mercurial/manifest.py b/mercurial/manifest.py
--- 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()
+            return cached
         if self._treeondisk:
             def gettext():
                 return self.revision(node)