Patchwork [5,of,7] fsmonitor: avoid instantiation of dirstate in reposetup

login
register
mail settings
Submitter Katsunori FUJIWARA
Date July 2, 2017, 5:53 p.m.
Message ID <9fd405b51136176f2498.1499017999@speaknoevil>
Download mbox | patch
Permalink /patch/21922/
State Accepted
Headers show

Comments

Katsunori FUJIWARA - July 2, 2017, 5:53 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1499017960 -32400
#      Mon Jul 03 02:52:40 2017 +0900
# Node ID 9fd405b51136176f2498561bbd0a0c485a1f2eac
# Parent  ce3d2f9b19ec075671ba185bb77cde064c049b27
fsmonitor: avoid instantiation of dirstate in reposetup

Before this patch, reposetup() of fsmonitor instantiates
repo.dirstate, and this might prevent other extensions from working as
expected.

For example, if journal extensions is enabled after fsmonitor:

  1. fsmonitor wraps repo.dirstate in extsetup()
  2. journal wraps repo.dirstate in extsetup()
  3. fsmonitor does below in reposetup()
     3.1. instantiate repo.dirstate
          (even though repo.dirstate is wrapped by both fsmonitor and
          journal, this dirstate is normal one, because repo is marked
          as neither fsmonitor-ed nor journal-ed)
     3.2. mark repo as fsmonitor-ed
          (after this, newly instantiated dirstate is marked as
          fsmonitor-ed)
     3.3. mark already instantiated dirstate as fsmonitor-ed
  4. journal marks repo as journal-ed
     (after this, newly instantiated dirstate will be marked as
     journal-ed)

At this point, already instantiated (3.1) dirstate isn't marked as
journal-ed, because repo.dirstate wrapper of journal (2) isn't used to
instantiate dirstate after (4). Therefore, journal extension doesn't
work as expected.

This issue can be reproduced by running test-journal.t or
test-journal-share.t with fsmonitor-run-tests.py.

Root cause of this issue is instantiation of dirstate in reposetup()
of fsmonitor. To avoid it, this patch does:

  - use repo.local() instead of util.safehasattr(repo, 'dirstate')

    This also avoid setup procedures for statichttprepo.  This is
    reason why this patch removes subsequent comment.

  - omit procedures for already instantiated dirstate

In addition to these, this patch also completely discards already
(unintentionally) instantiated dirstate, in order to ensure that
dirstate is always instantiated via own wrapdirstate() after
reposetup(). This is required if another extension does as same as
previous fsmonitor.
Yuya Nishihara - July 4, 2017, 2:09 p.m.
On Mon, 03 Jul 2017 02:53:19 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1499017960 -32400
> #      Mon Jul 03 02:52:40 2017 +0900
> # Node ID 9fd405b51136176f2498561bbd0a0c485a1f2eac
> # Parent  ce3d2f9b19ec075671ba185bb77cde064c049b27
> fsmonitor: avoid instantiation of dirstate in reposetup

> diff --git a/hgext/fsmonitor/__init__.py b/hgext/fsmonitor/__init__.py
> --- a/hgext/fsmonitor/__init__.py
> +++ b/hgext/fsmonitor/__init__.py
> @@ -677,10 +677,8 @@ def reposetup(ui, repo):
>                        'extension and has been disabled.\n') % ext)
>              return
>  
> -    if util.safehasattr(repo, 'dirstate'):
> -        # We don't work with subrepos either. Note that we can get passed in
> -        # e.g. a statichttprepo, which throws on trying to access the substate.
> -        # XXX This sucks.
> +    if repo.local():
> +        # We don't work with subrepos either.
>          try:
>              # if repo[None].substate can cause a dirstate parse, which is too
>              # slow. Instead, look for a file called hgsubstate,

So this no longer "sucks", maybe there's no need to catch AttributeError?

> @@ -702,15 +700,17 @@ def reposetup(ui, repo):
>          repo._fsmonitorstate = fsmonitorstate
>          repo._watchmanclient = client
>  
> -        # at this point since fsmonitorstate wasn't present, repo.dirstate is
> -        # not a fsmonitordirstate
> -        dirstate = repo.dirstate
> -        dirstate.__class__ = makedirstate(dirstate.__class__)
> -        dirstate._fsmonitorinit(fsmonitorstate, client)
> -        # invalidate property cache, but keep filecache which contains the
> -        # wrapped dirstate object
> -        del repo.unfiltered().__dict__['dirstate']
> -        assert dirstate is repo._filecache['dirstate'].obj
> +        # Completely discard dirstate, in order to ensure that
> +        # dirstate is instantiated via wrapdirstate() after this
> +        # reposetup(). Otherwise, walk(), rebuild() and invalidate()
> +        # of repo.dirstate already instantiated at this point don't
> +        # work as fsmonitordirstate, because it isn't makedirstate()-ed.
> +        if 'dirstate' in repo._filecache:
> +            del repo._filecache['dirstate']
> +            try:
> +                delattr(repo.unfiltered(), 'dirstate')
> +            except AttributeError:
> +                pass

Perhaps this disagree with bf3af0eced44. CC-ed Jun.
Katsunori FUJIWARA - July 4, 2017, 3:21 p.m.
At Tue, 4 Jul 2017 23:09:24 +0900,
Yuya Nishihara wrote:
> 
> On Mon, 03 Jul 2017 02:53:19 +0900, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1499017960 -32400
> > #      Mon Jul 03 02:52:40 2017 +0900
> > # Node ID 9fd405b51136176f2498561bbd0a0c485a1f2eac
> > # Parent  ce3d2f9b19ec075671ba185bb77cde064c049b27
> > fsmonitor: avoid instantiation of dirstate in reposetup
> 
> > diff --git a/hgext/fsmonitor/__init__.py b/hgext/fsmonitor/__init__.py
> > --- a/hgext/fsmonitor/__init__.py
> > +++ b/hgext/fsmonitor/__init__.py
> > @@ -677,10 +677,8 @@ def reposetup(ui, repo):
> >                        'extension and has been disabled.\n') % ext)
> >              return
> >  
> > -    if util.safehasattr(repo, 'dirstate'):
> > -        # We don't work with subrepos either. Note that we can get passed in
> > -        # e.g. a statichttprepo, which throws on trying to access the substate.
> > -        # XXX This sucks.
> > +    if repo.local():
> > +        # We don't work with subrepos either.
> >          try:
> >              # if repo[None].substate can cause a dirstate parse, which is too
> >              # slow. Instead, look for a file called hgsubstate,
> 
> So this no longer "sucks", maybe there's no need to catch AttributeError?

Oops, you are right. I'll revise it in V2.

> > @@ -702,15 +700,17 @@ def reposetup(ui, repo):
> >          repo._fsmonitorstate = fsmonitorstate
> >          repo._watchmanclient = client
> >  
> > -        # at this point since fsmonitorstate wasn't present, repo.dirstate is
> > -        # not a fsmonitordirstate
> > -        dirstate = repo.dirstate
> > -        dirstate.__class__ = makedirstate(dirstate.__class__)
> > -        dirstate._fsmonitorinit(fsmonitorstate, client)
> > -        # invalidate property cache, but keep filecache which contains the
> > -        # wrapped dirstate object
> > -        del repo.unfiltered().__dict__['dirstate']
> > -        assert dirstate is repo._filecache['dirstate'].obj
> > +        # Completely discard dirstate, in order to ensure that
> > +        # dirstate is instantiated via wrapdirstate() after this
> > +        # reposetup(). Otherwise, walk(), rebuild() and invalidate()
> > +        # of repo.dirstate already instantiated at this point don't
> > +        # work as fsmonitordirstate, because it isn't makedirstate()-ed.
> > +        if 'dirstate' in repo._filecache:
> > +            del repo._filecache['dirstate']
> > +            try:
> > +                delattr(repo.unfiltered(), 'dirstate')
> > +            except AttributeError:
> > +                pass
> 
> Perhaps this disagree with bf3af0eced44. CC-ed Jun.

Oh, I overlooked that, sorry.

If chg prefills repo's dirstate filecache before reposetup(), it
causes issue with an extension, of which wrapper for repo.dirstate
works as expected only after own reposetup() (e.g. journal).

Should I fix journal extension as similarly as bf3af0eced44 ?
Jun Wu - July 5, 2017, 12:49 a.m.
Excerpts from FUJIWARA Katsunori's message of 2017-07-05 00:21:25 +0900:
> > Perhaps this disagree with bf3af0eced44. CC-ed Jun.
> 
> Oh, I overlooked that, sorry.
> 
> If chg prefills repo's dirstate filecache before reposetup(), it
> causes issue with an extension, of which wrapper for repo.dirstate
> works as expected only after own reposetup() (e.g. journal).
> 
> Should I fix journal extension as similarly as bf3af0eced44 ?
 
I think journal should be fixed. My previous attempt was [1]. There is a
conclusion at that time, which is we have 2 choices:

  a) Refactor journal so it's transaction (or command, even better?) aware,
     i.e. it checks changes before and after dispatch.runcommand, instead of
     wrapping everything.
  b) Use "wrapfilecache" like fsmonitor to replace the function inside
     filecache entry.

"b)" is easier but "a)" will make "b)" unnecessary. If we want to fix it now
it could be applying [1] plus a "wrapfilecache" call. At that time
"wrapfilecache" is still in "fsmonitor/", not in extensions.py so I didn't
go ahead.

Since journal is experimental, and eventually "a)" looks nicer. I think it's
fine to leave journal broken for now (until I have more time to do "a)"
refactoring).

[1]: https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-May/098595.html

Patch

diff --git a/hgext/fsmonitor/__init__.py b/hgext/fsmonitor/__init__.py
--- a/hgext/fsmonitor/__init__.py
+++ b/hgext/fsmonitor/__init__.py
@@ -677,10 +677,8 @@  def reposetup(ui, repo):
                       'extension and has been disabled.\n') % ext)
             return
 
-    if util.safehasattr(repo, 'dirstate'):
-        # We don't work with subrepos either. Note that we can get passed in
-        # e.g. a statichttprepo, which throws on trying to access the substate.
-        # XXX This sucks.
+    if repo.local():
+        # We don't work with subrepos either.
         try:
             # if repo[None].substate can cause a dirstate parse, which is too
             # slow. Instead, look for a file called hgsubstate,
@@ -702,15 +700,17 @@  def reposetup(ui, repo):
         repo._fsmonitorstate = fsmonitorstate
         repo._watchmanclient = client
 
-        # at this point since fsmonitorstate wasn't present, repo.dirstate is
-        # not a fsmonitordirstate
-        dirstate = repo.dirstate
-        dirstate.__class__ = makedirstate(dirstate.__class__)
-        dirstate._fsmonitorinit(fsmonitorstate, client)
-        # invalidate property cache, but keep filecache which contains the
-        # wrapped dirstate object
-        del repo.unfiltered().__dict__['dirstate']
-        assert dirstate is repo._filecache['dirstate'].obj
+        # Completely discard dirstate, in order to ensure that
+        # dirstate is instantiated via wrapdirstate() after this
+        # reposetup(). Otherwise, walk(), rebuild() and invalidate()
+        # of repo.dirstate already instantiated at this point don't
+        # work as fsmonitordirstate, because it isn't makedirstate()-ed.
+        if 'dirstate' in repo._filecache:
+            del repo._filecache['dirstate']
+            try:
+                delattr(repo.unfiltered(), 'dirstate')
+            except AttributeError:
+                pass
 
         class fsmonitorrepo(repo.__class__):
             def status(self, *args, **kwargs):