Patchwork [2,of,2] context: do not cache manifestctx

login
register
mail settings
Submitter Jun Wu
Date May 26, 2017, 12:36 a.m.
Message ID <0fc8a815bb45f00b1c7a.1495758962@x1c>
Download mbox | patch
Permalink /patch/20922/
State Accepted
Headers show

Comments

Jun Wu - May 26, 2017, 12:36 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1495758043 25200
#      Thu May 25 17:20:43 2017 -0700
# Node ID 0fc8a815bb45f00b1c7aea04aa722c9f65f28bd9
# Parent  379e5047c6472df9de70a5b990b1330e3857c4db
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 0fc8a815bb45
context: do not cache manifestctx

This will make sure when ctx.repo.manifestlog changes, a correct new
manifestctx is returned. repo.manifestlog takes care of caching so the
manifestctx won't be reconstructed every time.
Yuya Nishihara - May 28, 2017, 1:27 p.m.
On Thu, 25 May 2017 17:36:02 -0700, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1495758043 25200
> #      Thu May 25 17:20:43 2017 -0700
> # Node ID 0fc8a815bb45f00b1c7aea04aa722c9f65f28bd9
> # Parent  379e5047c6472df9de70a5b990b1330e3857c4db
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 0fc8a815bb45
> context: do not cache manifestctx
> 
> This will make sure when ctx.repo.manifestlog changes, a correct new
> manifestctx is returned. repo.manifestlog takes care of caching so the
> manifestctx won't be reconstructed every time.

Although I think it's generally wrong to reuse a ctx object after
repo.invalidate(), this change seems fine.

> 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?
Jun Wu - May 28, 2017, 5:46 p.m.
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 I should update commit message to make it easier to understand.

> > 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.
Yuya Nishihara - May 29, 2017, 12:35 p.m.
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.

Patch

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]
diff --git a/tests/test-context.py.out b/tests/test-context.py.out
--- a/tests/test-context.py.out
+++ b/tests/test-context.py.out
@@ -48,5 +48,3 @@  wcctx._status=<status modified=['bar-m']
 commit 1: 2efe531a913fa648867ab8824360371679d05a65
 commit 2: 2caca91f6362020334384ebe27bae67315298abf
-cannot read data: LookupError('Q\xa3L\xa5Ou\x8f\xce8\xda<Q\x7f\x9f(\xc9;Li/', '00manifest.i', 'no node')
 commit 3: abd6b0f49f338be22b094ef2b7425e8048f8337b
-cannot read data: LookupError("\x82\x15\xb8\xd3\x85\xf6H'\x9cP'D\x97\x1e\xab\x98O\xbb\x05\x9d", '00manifest.i', 'no node')