Patchwork [6,of,6] bmstore: add handling of the active bookmark

login
register
mail settings
Submitter Augie Fackler
Date Dec. 2, 2015, 4:19 p.m.
Message ID <9d01df9152da4f0e1c3f.1449073152@imladris.local>
Download mbox | patch
Permalink /patch/11752/
State Superseded
Headers show

Comments

Augie Fackler - Dec. 2, 2015, 4:19 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1447294682 18000
#      Wed Nov 11 21:18:02 2015 -0500
# Node ID 9d01df9152da4f0e1c3fb1beae465c9bb936fbc8
# Parent  32b95ec9058de8565b64ddcae4f8d44b915c87ce
bmstore: add handling of the active bookmark

This further centralizes the handling of bookmark storage, and will
help get some lingering bookmarks business out of localrepo. Right
now, this change implies reading of the active bookmark to also imply
reading all bookmarks from disk - for users with many many bookmarks
this may be a measurable performance hit. In that case, we should
migrate bmstore to be able to lazy-read its properties from disk
rather than having to eagerly read them, but I decided to avoid doing
that to try and avoid some potentially complicated filecache decorator
issues.

Patch

diff --git a/mercurial/bookmarks.py b/mercurial/bookmarks.py
--- a/mercurial/bookmarks.py
+++ b/mercurial/bookmarks.py
@@ -44,16 +44,15 @@  def _getbkfile(repo):
 class bmstore(dict):
     """Storage for bookmarks.
 
-    This object should do all bookmark reads and writes, so that it's
-    fairly simple to replace the storage underlying bookmarks without
-    having to clone the logic surrounding bookmarks.
+    This object should do all bookmark-related reads and writes, so
+    that it's fairly simple to replace the storage underlying
+    bookmarks without having to clone the logic surrounding
+    bookmarks. This type also should manage the active bookmark, if
+    any.
 
     This particular bmstore implementation stores bookmarks as
     {hash}\s{name}\n (the same format as localtags) in
     .hg/bookmarks. The mapping is stored as {name: nodeid}.
-
-    This class does NOT handle the "active" bookmark state at this
-    time.
     """
 
     def __init__(self, repo):
@@ -79,6 +78,20 @@  class bmstore(dict):
             if inst.errno != errno.ENOENT:
                 raise
         self._clean = True
+        self._active = _readactive(repo, self)
+        self._aclean = True
+
+    def _getactive(self):
+        return self._active
+
+    def _setactive(self, mark):
+        if mark is not None and mark not in self:
+            raise AssertionError('bookmark %s does not exist!' % mark)
+
+        self._active = mark
+        self._aclean = False
+
+    active = property(_getactive, _setactive)
 
     def __setitem__(self, *args, **kwargs):
         self._clean = False
@@ -105,6 +118,7 @@  class bmstore(dict):
         We also store a backup of the previous state in undo.bookmarks that
         can be copied back on rollback.
         '''
+        self._writeactive()
         if self._clean:
             return
         repo = self._repo
@@ -126,8 +140,10 @@  class bmstore(dict):
 
     def _writerepo(self, repo):
         """Factored out for extensibility"""
-        if repo._activebookmark not in self:
-            deactivate(repo)
+        rbm = repo._bookmarks
+        if rbm.active not in self:
+            rbm.active = None
+            rbm._writeactive()
 
         wlock = repo.wlock()
         try:
@@ -141,12 +157,32 @@  class bmstore(dict):
         finally:
             wlock.release()
 
+    def _writeactive(self):
+        if self._aclean:
+            return
+        wlock = self._repo.wlock()
+        try:
+            if self._active is not None:
+                f = self._repo.vfs('bookmarks.current', 'w', atomictemp=True)
+                try:
+                    f.write(encoding.fromlocal(self._active))
+                finally:
+                    f.close()
+            else:
+                try:
+                    self._repo.vfs.unlink('bookmarks.current')
+                except OSError as inst:
+                    if inst.errno != errno.ENOENT:
+                        raise
+        finally:
+            wlock.release()
+
     def _write(self, fp):
         for name, node in self.iteritems():
             fp.write("%s %s\n" % (hex(node), encoding.fromlocal(name)))
         self._clean = True
 
-def readactive(repo):
+def _readactive(repo, marks):
     """
     Get the active bookmark. We can have an active bookmark that updates
     itself as we commit. This function returns the name of that bookmark.
@@ -162,7 +198,7 @@  def readactive(repo):
     try:
         # No readline() in osutil.posixfile, reading everything is cheap
         mark = encoding.tolocal((file.readlines() or [''])[0])
-        if mark == '' or mark not in repo._bookmarks:
+        if mark == '' or mark not in marks:
             mark = None
     except IOError as inst:
         if inst.errno != errno.ENOENT:
@@ -178,35 +214,15 @@  def activate(repo, mark):
     follow new commits that are made.
     The name is recorded in .hg/bookmarks.current
     """
-    if mark not in repo._bookmarks:
-        raise AssertionError('bookmark %s does not exist!' % mark)
-
-    active = repo._activebookmark
-    if active == mark:
-        return
-
-    wlock = repo.wlock()
-    try:
-        file = repo.vfs('bookmarks.current', 'w', atomictemp=True)
-        file.write(encoding.fromlocal(mark))
-        file.close()
-    finally:
-        wlock.release()
-    repo._activebookmark = mark
+    repo._bookmarks.active = mark
+    repo._bookmarks.write()
 
 def deactivate(repo):
     """
     Unset the active bookmark in this repository.
     """
-    wlock = repo.wlock()
-    try:
-        repo.vfs.unlink('bookmarks.current')
-        repo._activebookmark = None
-    except OSError as inst:
-        if inst.errno != errno.ENOENT:
-            raise
-    finally:
-        wlock.release()
+    repo._bookmarks.active = None
+    repo._bookmarks.write()
 
 def isactivewdirparent(repo):
     """
@@ -256,7 +272,7 @@  def update(repo, parents, node):
     deletefrom = parents
     marks = repo._bookmarks
     update = False
-    active = repo._activebookmark
+    active = marks.active
     if not active:
         return False
 
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2521,13 +2521,14 @@  def amend(ui, repo, commitfunc, old, ext
             # First, do a regular commit to record all changes in the working
             # directory (if there are any)
             ui.callhooks = False
-            activebookmark = repo._activebookmark
+            activebookmark = repo._bookmarks.active
             try:
-                repo._activebookmark = None
+                repo._bookmarks.active = None
                 opts['message'] = 'temporary amend commit for %s' % old
                 node = commit(ui, repo, commitfunc, pats, opts)
             finally:
-                repo._activebookmark = activebookmark
+                repo._bookmarks.active = activebookmark
+                repo._bookmarks.write()
                 ui.callhooks = True
             ctx = repo[node]
 
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -418,13 +418,13 @@  class localrepository(object):
             pass
         return proxycls(self, name)
 
-    @repofilecache('bookmarks')
+    @repofilecache('bookmarks', 'bookmarks.current')
     def _bookmarks(self):
         return bookmarks.bmstore(self)
 
-    @repofilecache('bookmarks.current')
+    @property
     def _activebookmark(self):
-        return bookmarks.readactive(self)
+        return self._bookmarks.active
 
     def bookmarkheads(self, bookmark):
         name = bookmark.split('@', 1)[0]