# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 2015-10-07 16:41:30
# Node ID 46dec89fe88821df819ee54a15e7f919095b10fc
# Parent  48476c6129a2492d264a09b60cf520c620718220

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()

Even though 'bmstore.write()' itself may have to be discarded by
putting bookmark operations into transaction scope, this patch chose
fixing it to implement "transactional dirstate" at first.

diff --git a/hgext/shelve.py b/hgext/shelve.py
--- a/hgext/shelve.py
+++ b/hgext/shelve.py
@@ -253,12 +253,11 @@ def createcmd(ui, repo, pats, opts):
 
     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)
@@ -313,10 +312,6 @@ def createcmd(ui, repo, pats, opts):
         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 @@ class bmstore(dict):
             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 @@ shelve should still work even if mq is d
   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 @@ Recreate some conflict again
   $ 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 @@ Recreate some conflict again
   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 @@ is a no-op), works (issue4398)
   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 @@ Test interactive shelve
   $ 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 @@ shelve --patch and shelve --stat should 
 
   $ 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