[PATCH 2 of 2] context: do not cache manifestctx

[Yuya Nishihara]

On Sun, 28 May 2017 10:46:45 -0700, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2017-05-28 22:27:48 +0900:
> > Although I think it's generally wrong to reuse a ctx object after
> > repo.invalidate(), this change seems fine.
> 
> "repo.invalidate()" is not related to this issue, as demonstrated in the
> test:
>   
>   ctx = context.workingcommitctx(...)
>   ctx.p1().manifest()
>   n = repo.commitctx(ctx) # <- repo.invalidate() called
>   repo.svfs.utime('00manifest.i', (st.st_mtime + 1, st.st_mtime + 1))
>   # ctx is not reused, repo[n] is used to construct a new ctx
>   try:
>       if repo[n][i].data() != i:
>           print('data mismatch')
>   except Exception as ex:
> 
> Looking at repo.commitctx, a common code path to write manifest is:
> 
>   ctx.p1().manifestctx().write(...)
> 
> ctx.p1().manifestctx()._revlog() being different from repo.manifestlog
> triggers this issue. There are no old ctx being reused, as in the test, it's
> using a new ctx: repo[n][path] to access the content.

Maybe that's a kind of an API-level inconsistency. repo.lock() and repo.wlock()
may discard cache and that could make existing ctx objects stale.

> > > diff --git a/mercurial/context.py b/mercurial/context.py
> > > --- a/mercurial/context.py
> > > +++ b/mercurial/context.py
> > > @@ -552,5 +552,5 @@ class changectx(basectx):
> > >          return self._manifestctx.read()
> > >  
> > > -    @propertycache
> > > +    @property
> > >      def _manifestctx(self):
> > >          return self._repo.manifestlog[self._changeset.manifest]
> > 
> > Shouldn't we update metadataonlyctx._manifestctx as well?
> 
> Yes. I didn't notice that.

Updated it in flight and queued, thanks.