Patchwork journal: use a more compatible way to wrap dirstate

login
register
mail settings
Submitter Jun Wu
Date May 26, 2017, 2:56 a.m.
Message ID <27b3fcbc4fd5125d86a4.1495767401@x1c>
Download mbox | patch
Permalink /patch/20923/
State Superseded
Headers show

Comments

Jun Wu - May 26, 2017, 2:56 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1495767385 25200
#      Thu May 25 19:56:25 2017 -0700
# Node ID 27b3fcbc4fd5125d86a4f4f2cb3e9accdae71265
# Parent  2b5953a49f1407f825d65b45986d213cb5c79203
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 27b3fcbc4fd5
journal: use a more compatible way to wrap dirstate

Previously journal uses

  extensions.wrapfunction(localrepo.localrepository.dirstate, 'func', wrap)

in extsetup to change dirstate. That assumes the dirstate filecache is not
touched by others, and is incompatible with bf3af0eced.

This patch moves the dirstate change to reposetup and does not assume
filecache internal implementation. It is more compatible with other code.

This fixes "--extra-config-opt=extensions.fsmonitor= test-journal.t" test.
Jun Wu - May 27, 2017, 2:51 a.m.
This fixes the test with fsmonitor but is not 100% correct. The dirstate
state will be lost when dirstate object is reconstructed.

That said, I think the journal extension is not doing the most correct
things.  I think recording journal should be transaction aware. Ideally
journal runs at the end of transaction, instead of wrapping
non-transactional changes like what we have today.

If journal will eventually be refactored to run at the end of a transaction,
I think this patch as a temporary fix might be fine.

Excerpts from Jun Wu's message of 2017-05-25 19:56:41 -0700:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1495767385 25200
> #      Thu May 25 19:56:25 2017 -0700
> # Node ID 27b3fcbc4fd5125d86a4f4f2cb3e9accdae71265
> # Parent  2b5953a49f1407f825d65b45986d213cb5c79203
> # Available At https://bitbucket.org/quark-zju/hg-draft 
> #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 27b3fcbc4fd5
> journal: use a more compatible way to wrap dirstate
> 
> Previously journal uses
> 
>   extensions.wrapfunction(localrepo.localrepository.dirstate, 'func', wrap)
> 
> in extsetup to change dirstate. That assumes the dirstate filecache is not
> touched by others, and is incompatible with bf3af0eced.
> 
> This patch moves the dirstate change to reposetup and does not assume
> filecache internal implementation. It is more compatible with other code.
> 
> This fixes "--extra-config-opt=extensions.fsmonitor= test-journal.t" test.
> 
> diff --git a/hgext/journal.py b/hgext/journal.py
> --- a/hgext/journal.py
> +++ b/hgext/journal.py
> @@ -62,6 +62,4 @@ def extsetup(ui):
>      extensions.wrapfunction(dispatch, 'runcommand', runcommand)
>      extensions.wrapfunction(bookmarks.bmstore, '_write', recordbookmarks)
> -    extensions.wrapfunction(
> -        localrepo.localrepository.dirstate, 'func', wrapdirstate)
>      extensions.wrapfunction(hg, 'postshare', wrappostshare)
>      extensions.wrapfunction(hg, 'copystore', unsharejournal)
> @@ -70,4 +68,6 @@ def reposetup(ui, repo):
>      if repo.local():
>          repo.journal = journalstorage(repo)
> +        repo.dirstate.journalstorage = repo.journal
> +        repo.dirstate.addparentchangecallback('journal', recorddirstateparents)
>  
>  def runcommand(orig, lui, repo, cmd, fullargs, *args):
> @@ -76,13 +76,4 @@ def runcommand(orig, lui, repo, cmd, ful
>      return orig(lui, repo, cmd, fullargs, *args)
>  
> -# hooks to record dirstate changes
> -def wrapdirstate(orig, repo):
> -    """Make journal storage available to the dirstate object"""
> -    dirstate = orig(repo)
> -    if util.safehasattr(repo, 'journal'):
> -        dirstate.journalstorage = repo.journal
> -        dirstate.addparentchangecallback('journal', recorddirstateparents)
> -    return dirstate
> -
>  def recorddirstateparents(dirstate, old, new):
>      """Records all dirstate parent changes in the journal."""
Yuya Nishihara - May 27, 2017, 1:55 p.m.
On Fri, 26 May 2017 19:51:09 -0700, Jun Wu wrote:
> This fixes the test with fsmonitor but is not 100% correct. The dirstate
> state will be lost when dirstate object is reconstructed.

Yes, and that would be more likely to be happen on Windows where filecache
isn't cacheable.

> That said, I think the journal extension is not doing the most correct
> things.  I think recording journal should be transaction aware. Ideally
> journal runs at the end of transaction, instead of wrapping
> non-transactional changes like what we have today.
> 
> If journal will eventually be refactored to run at the end of a transaction,
> I think this patch as a temporary fix might be fine.

I suppose marmoute's tr.changes will solve the issue.

> Excerpts from Jun Wu's message of 2017-05-25 19:56:41 -0700:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1495767385 25200
> > #      Thu May 25 19:56:25 2017 -0700
> > # Node ID 27b3fcbc4fd5125d86a4f4f2cb3e9accdae71265
> > # Parent  2b5953a49f1407f825d65b45986d213cb5c79203
> > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 27b3fcbc4fd5
> > journal: use a more compatible way to wrap dirstate
> > 
> > Previously journal uses
> > 
> >   extensions.wrapfunction(localrepo.localrepository.dirstate, 'func', wrap)
> > 
> > in extsetup to change dirstate. That assumes the dirstate filecache is not
> > touched by others, and is incompatible with bf3af0eced.

I don't think journal extension is guilty. Both fsmonitor and journal do weird
thing. Can't it be addressed if localrepository had a plain _dirstate() factory?

  @filecache
  def dirstate(self):
      return self._dirstate()  # override this by journalrepo and fsmonitorrepo
Jun Wu - May 27, 2017, 5:27 p.m.
Excerpts from Yuya Nishihara's message of 2017-05-27 22:55:57 +0900:
> On Fri, 26 May 2017 19:51:09 -0700, Jun Wu wrote:
> > This fixes the test with fsmonitor but is not 100% correct. The dirstate
> > state will be lost when dirstate object is reconstructed.
> 
> Yes, and that would be more likely to be happen on Windows where filecache
> isn't cacheable.

Ah, good to know that. That explains the manifestctx caching issue I have
seen.

> [...] 
> I suppose marmoute's tr.changes will solve the issue.

Maybe I should just refactor journal to be transition aware then.

> [...]
> I don't think journal extension is guilty. Both fsmonitor and journal do
> weird thing. Can't it be addressed if localrepository had a plain
> _dirstate() factory?

It's more complex when chg starts to populate filecache state. If journal
only wraps "_dirstate", then filecache could still have the outdated object.

"wrapfilecache" in fsmonitor/__init__.py seems important to make it correct.

>   @filecache
>   def dirstate(self):
>       return self._dirstate()  # override this by journalrepo and fsmonitorrepo

Anyway, I'll drop that patch and explore the transaction approach, or port
wrapfilecache to extensions.py

Patch

diff --git a/hgext/journal.py b/hgext/journal.py
--- a/hgext/journal.py
+++ b/hgext/journal.py
@@ -62,6 +62,4 @@  def extsetup(ui):
     extensions.wrapfunction(dispatch, 'runcommand', runcommand)
     extensions.wrapfunction(bookmarks.bmstore, '_write', recordbookmarks)
-    extensions.wrapfunction(
-        localrepo.localrepository.dirstate, 'func', wrapdirstate)
     extensions.wrapfunction(hg, 'postshare', wrappostshare)
     extensions.wrapfunction(hg, 'copystore', unsharejournal)
@@ -70,4 +68,6 @@  def reposetup(ui, repo):
     if repo.local():
         repo.journal = journalstorage(repo)
+        repo.dirstate.journalstorage = repo.journal
+        repo.dirstate.addparentchangecallback('journal', recorddirstateparents)
 
 def runcommand(orig, lui, repo, cmd, fullargs, *args):
@@ -76,13 +76,4 @@  def runcommand(orig, lui, repo, cmd, ful
     return orig(lui, repo, cmd, fullargs, *args)
 
-# hooks to record dirstate changes
-def wrapdirstate(orig, repo):
-    """Make journal storage available to the dirstate object"""
-    dirstate = orig(repo)
-    if util.safehasattr(repo, 'journal'):
-        dirstate.journalstorage = repo.journal
-        dirstate.addparentchangecallback('journal', recorddirstateparents)
-    return dirstate
-
 def recorddirstateparents(dirstate, old, new):
     """Records all dirstate parent changes in the journal."""