From patchwork Sun Oct 4 12:44:17 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [1, of, 4] bookmarks: use recordchange instead of writing if transaction is active From: Katsunori FUJIWARA X-Patchwork-Id: 10775 Message-Id: <1655b19c19bdd94da85d.1443962657@feefifofum> To: mercurial-devel@selenic.com Date: Sun, 04 Oct 2015 21:44:17 +0900 # HG changeset patch # User FUJIWARA Katsunori # 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() 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