Patchwork [1,of,4] bookmarks: use recordchange instead of writing if transaction is active

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Oct. 4, 2015, 12:44 p.m.
Message ID <1655b19c19bdd94da85d.1443962657@feefifofum>
Download mbox | patch
Permalink /patch/10775/
State Superseded
Commit 46dec89fe88821df819ee54a15e7f919095b10fc
Delegated to: Pierre-Yves David
Headers show

Comments

Katsunori FUJIWARA - Oct. 4, 2015, 12:44 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1443962009 -32400
#      Sun Oct 04 21:33:29 2015 +0900
# Node ID 1655b19c19bdd94da85d568c1da65b09de4b402d
# Parent  97dc6ab42aad232c73180dee648685c26662230b
bookmarks: use recordchange instead of writing if transaction is active

Before this patch, 'bmstore.write()' always write in-memory bookmark
changes into '.hg/bookmarks' regardless of transaction activity.

If 'bmstore.write()' is invoked inside a transaction and it writes
changes into '.hg/bookmarks', then:

  - original bookmarks aren't restored at failure of that transaction

    This breaks "all or nothing" policy of the transaction.

    BTW, "hg rollback" can restore bookmarks successfully even before
    this patch, because original bookmarks are saved into
    '.hg/journal.bookmarks' at the beginning of the transaction, and
    it (actually renamed as '.hg/undo.bookmarks') is used by "hg
    rollback".

  - uncommitted bookmark changes are visible to other processes

    This is a kind of "dirty read"

For example, 'rebase.rebase()' implies 'bmstore.write()', and it may
be executed inside the transaction of "hg unshelve". Then, intentional
aborting at the end of "hg unshelve" transaction doesn't restore
original bookmarks (this is obviously a bug).

This patch uses 'bmstore.recordchange()' instead of actual writing by
'bmstore._writerepo()', if any transaction is active

This patch also removes meaningless restoring bmstore explicitly at
the end of "hg shelve".

This patch doesn't choose fixing each 'bmstore.write()' callers as
like below, because writing similar code here and there is very
redundant.

  before:
    bmstore.write()

  after:
    tr = repo.currenttransaction()
    if tr:
        bmstore.recordchange(tr)
    else:
        bmstore.write()
Pierre-Yves David - Oct. 4, 2015, 11:01 p.m.
On 10/04/2015 05:44 AM, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1443962009 -32400
> #      Sun Oct 04 21:33:29 2015 +0900
> # Node ID 1655b19c19bdd94da85d568c1da65b09de4b402d
> # Parent  97dc6ab42aad232c73180dee648685c26662230b
> bookmarks: use recordchange instead of writing if transaction is active

I was initially not very enthousiastic about this patch because I would 
like bookmark.write to just die and all bookmarks move to be done in the 
transaction. And I would like code touching bookmark to have to 
explicitly pass a transaction object so that they have to think about 
the transaction life time when doing so.

However this seems like a step forward in all cases so I'll probably 
take that patch anyway.
Katsunori FUJIWARA - Oct. 5, 2015, 2:42 p.m.
At Sun, 04 Oct 2015 16:01:27 -0700,
Pierre-Yves David wrote:
> 
> 
> On 10/04/2015 05:44 AM, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1443962009 -32400
> > #      Sun Oct 04 21:33:29 2015 +0900
> > # Node ID 1655b19c19bdd94da85d568c1da65b09de4b402d
> > # Parent  97dc6ab42aad232c73180dee648685c26662230b
> > bookmarks: use recordchange instead of writing if transaction is active
> 
> I was initially not very enthousiastic about this patch because I would 
> like bookmark.write to just die and all bookmarks move to be done in the 
> transaction. And I would like code touching bookmark to have to 
> explicitly pass a transaction object so that they have to think about 
> the transaction life time when doing so.

BTW, would you plan to put updating bookmark via "hg update" into a
transaction scope, too ?


FYI, 'bmstore.write()' are invoked also from functions below, and
these may not be inside transaction at runtime, AFAIK:

- histedit.movebookmarks
  at the end of histedit-ing (via _histedit)

- mq.queue.refresh
  at the end of refreshing for qrefresh/qfold

- rebase.rebase
  at the end of rebasing (mainly focused by this patch)

- repair.repair
- strip
  - strip
  - stripcmd

  refactoring bookmark operations between them may be able to put
  bookmark updating into a transaction scope in 'repair.repair'.


> However this seems like a step forward in all cases so I'll probably 
> take that patch anyway.

To implement "transactional dirstate" at first, I'll post this patch
as it is also in V2 series (but with some additional comment), because
discarding 'bmstore.write()' needs some more complicated working as
described above.

> -- 
> Pierre-Yves David
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Pierre-Yves David - Oct. 5, 2015, 5:57 p.m.
On 10/05/2015 07:42 AM, FUJIWARA Katsunori wrote:
>
> At Sun, 04 Oct 2015 16:01:27 -0700,
> Pierre-Yves David wrote:
>>
>>
>> On 10/04/2015 05:44 AM, FUJIWARA Katsunori wrote:
>>> # HG changeset patch
>>> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>>> # Date 1443962009 -32400
>>> #      Sun Oct 04 21:33:29 2015 +0900
>>> # Node ID 1655b19c19bdd94da85d568c1da65b09de4b402d
>>> # Parent  97dc6ab42aad232c73180dee648685c26662230b
>>> bookmarks: use recordchange instead of writing if transaction is active
>>
>> I was initially not very enthousiastic about this patch because I would
>> like bookmark.write to just die and all bookmarks move to be done in the
>> transaction. And I would like code touching bookmark to have to
>> explicitly pass a transaction object so that they have to think about
>> the transaction life time when doing so.
>
> BTW, would you plan to put updating bookmark via "hg update" into a
> transaction scope, too ?

Yep.

> FYI, 'bmstore.write()' are invoked also from functions below, and
> these may not be inside transaction at runtime, AFAIK:
>
> - histedit.movebookmarks
>    at the end of histedit-ing (via _histedit)
>
> - mq.queue.refresh
>    at the end of refreshing for qrefresh/qfold
>
> - rebase.rebase
>    at the end of rebasing (mainly focused by this patch)
>
> - repair.repair
> - strip
>    - strip
>    - stripcmd
>
>    refactoring bookmark operations between them may be able to put
>    bookmark updating into a transaction scope in 'repair.repair'.

Yes, Laurent started working on that be he is away for probably an extra 
month.

>> However this seems like a step forward in all cases so I'll probably
>> take that patch anyway.
>
> To implement "transactional dirstate" at first, I'll post this patch
> as it is also in V2 series (but with some additional comment), because
> discarding 'bmstore.write()' needs some more complicated working as
> described above.

I'm definitely not requiring bookmark.write to be dead before we move 
forward here, I was mentioning it to make sure the change we have to 
make here is in line with our final goal in this area.

If we need such patch after the shelve rework (do we need the shelve 
rework?) consider it accepted.

Patch

diff --git a/hgext/shelve.py b/hgext/shelve.py
--- a/hgext/shelve.py
+++ b/hgext/shelve.py
@@ -242,12 +242,11 @@ 
 
     name = opts['name']
 
-    wlock = lock = tr = bms = None
+    wlock = lock = tr = None
     try:
         wlock = repo.wlock()
         lock = repo.lock()
 
-        bms = repo._bookmarks.copy()
         # use an uncommitted transaction to generate the bundle to avoid
         # pull races. ensure we don't print the abort message to stderr.
         tr = repo.transaction('commit', report=lambda x: None)
@@ -303,10 +302,6 @@ 
         ui.status(_('shelved as %s\n') % name)
         hg.update(repo, parent.node())
     finally:
-        if bms:
-            # restore old bookmarks
-            repo._bookmarks.update(bms)
-            repo._bookmarks.write()
         if tr:
             tr.abort()
         lockmod.release(lock, wlock)
diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
--- a/mercurial/bookmarks.py
+++ b/mercurial/bookmarks.py
@@ -95,6 +95,14 @@ 
             l = repo._wlockref and repo._wlockref()
             if l is None or not l.held:
                 repo.ui.develwarn('bookmarks write with no wlock')
+
+        tr = repo.currenttransaction()
+        if tr:
+            self.recordchange(tr)
+            # invalidatevolatilesets() is omitted because this doesn't
+            # write changes out actually
+            return
+
         self._writerepo(repo)
         repo.invalidatevolatilesets()
 
diff --git a/tests/test-shelve.t b/tests/test-shelve.t
--- a/tests/test-shelve.t
+++ b/tests/test-shelve.t
@@ -534,8 +534,12 @@ 
   0 files updated, 0 files merged, 1 files removed, 0 files unresolved
   $ hg --config extensions.mq=! shelve --list
   test            (*)    changes to 'create conflict' (glob)
+  $ hg bookmark
+   * test                      4:33f7f61e6c5e
   $ hg --config extensions.mq=! unshelve
   unshelving change 'test'
+  $ hg bookmark
+   * test                      4:33f7f61e6c5e
 
 shelve should leave dirstate clean (issue4055)
 
@@ -796,6 +800,8 @@ 
   $ hg up test
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (activating bookmark test)
+  $ hg bookmark
+   * test                      4:33f7f61e6c5e
   $ hg unshelve
   unshelving change 'default'
   rebasing shelved changes
@@ -805,6 +811,8 @@ 
   merging a/a incomplete! (edit conflicts, then use 'hg resolve --mark')
   unresolved conflicts (see 'hg resolve', then 'hg unshelve --continue')
   [1]
+  $ hg bookmark
+     test                      4:33f7f61e6c5e
 
 Test that resolving all conflicts in one direction (so that the rebase
 is a no-op), works (issue4398)
@@ -817,6 +825,8 @@ 
   rebasing 5:4b555fdb4e96 "changes to 'second'" (tip)
   note: rebase of 5:4b555fdb4e96 created no changes to commit
   unshelve of 'default' complete
+  $ hg bookmark
+   * test                      4:33f7f61e6c5e
   $ hg diff
   $ hg status
   ? a/a.orig
@@ -900,12 +910,16 @@ 
   $ hg st
   M a/a
   ? foo/foo
+  $ hg bookmark
+   * test                      4:33f7f61e6c5e
   $ hg unshelve
   unshelving change 'test'
   temporarily committing pending changes (restore with 'hg unshelve --abort')
   rebasing shelved changes
   rebasing 6:65b5d1c34c34 "changes to 'create conflict'" (tip)
   merging a/a
+  $ hg bookmark
+   * test                      4:33f7f61e6c5e
   $ cat a/a
   a
   a
@@ -917,6 +931,7 @@ 
 
   $ hg up --clean .
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  (leaving bookmark test)
   $ hg shelve --list
   $ echo 'patch a' > shelf-patch-a
   $ hg add shelf-patch-a