[PATCH 2 of 4] commands.bookmarks: hold wlock for write operations

[Siddharth Agarwal]

# HG changeset patch
# User Siddharth Agarwal <sid0 at fb.com>
# Date 1384893194 28800
#      Tue Nov 19 12:33:14 2013 -0800
# Node ID d018c7466c66d710dccc2ede1b10e79b02de01c1
# Parent  87229e6e9924304a060d573bade13faf77e9080e
commands.bookmarks: hold wlock for write operations

Any invocations of bookmarks other than a plain 'hg bookmarks' will likely
cause a write to the bookmark store. These should be guarded by the wlock.

The repo._bookmarks read should be similarly guarded by the wlock if we're
going to be subsequently writing to it.

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -808,7 +808,6 @@
     inactive = opts.get('inactive')
 
     hexfn = ui.debugflag and hex or short
-    marks = repo._bookmarks
     cur   = repo.changectx('.').node()
 
     def checkformat(mark):
@@ -860,59 +859,66 @@
     if not names and (delete or rev):
         raise util.Abort(_("bookmark name required"))
 
-    if delete:
-        for mark in names:
-            if mark not in marks:
-                raise util.Abort(_("bookmark '%s' does not exist") % mark)
-            if mark == repo._bookmarkcurrent:
-                bookmarks.setcurrent(repo, None)
-            del marks[mark]
-        marks.write()
-
-    elif rename:
-        if not names:
-            raise util.Abort(_("new bookmark name required"))
-        elif len(names) > 1:
-            raise util.Abort(_("only one new bookmark name allowed"))
-        mark = checkformat(names[0])
-        if rename not in marks:
-            raise util.Abort(_("bookmark '%s' does not exist") % rename)
-        checkconflict(repo, mark, force)
-        marks[mark] = marks[rename]
-        if repo._bookmarkcurrent == rename and not inactive:
-            bookmarks.setcurrent(repo, mark)
-        del marks[rename]
-        marks.write()
-
-    elif names:
-        newact = None
-        for mark in names:
-            mark = checkformat(mark)
-            if newact is None:
-                newact = mark
-            if inactive and mark == repo._bookmarkcurrent:
-                bookmarks.setcurrent(repo, None)
-                return
-            tgt = cur
-            if rev:
-                tgt = scmutil.revsingle(repo, rev).node()
-            checkconflict(repo, mark, force, tgt)
-            marks[mark] = tgt
-        if not inactive and cur == marks[newact] and not rev:
-            bookmarks.setcurrent(repo, newact)
-        elif cur != tgt and newact == repo._bookmarkcurrent:
-            bookmarks.setcurrent(repo, None)
-        marks.write()
-
-    elif inactive:
-        if len(marks) == 0:
-            ui.status(_("no bookmarks set\n"))
-        elif not repo._bookmarkcurrent:
-            ui.status(_("no active bookmark\n"))
-        else:
-            bookmarks.setcurrent(repo, None)
-
+    if delete or rename or names or inactive:
+        wlock = repo.wlock()
+        try:
+            marks = repo._bookmarks
+            if delete:
+                for mark in names:
+                    if mark not in marks:
+                        raise util.Abort(_("bookmark '%s' does not exist") %
+                                         mark)
+                    if mark == repo._bookmarkcurrent:
+                        bookmarks.setcurrent(repo, None)
+                    del marks[mark]
+                marks.write()
+
+            elif rename:
+                if not names:
+                    raise util.Abort(_("new bookmark name required"))
+                elif len(names) > 1:
+                    raise util.Abort(_("only one new bookmark name allowed"))
+                mark = checkformat(names[0])
+                if rename not in marks:
+                    raise util.Abort(_("bookmark '%s' does not exist") % rename)
+                checkconflict(repo, mark, force)
+                marks[mark] = marks[rename]
+                if repo._bookmarkcurrent == rename and not inactive:
+                    bookmarks.setcurrent(repo, mark)
+                del marks[rename]
+                marks.write()
+
+            elif names:
+                newact = None
+                for mark in names:
+                    mark = checkformat(mark)
+                    if newact is None:
+                        newact = mark
+                    if inactive and mark == repo._bookmarkcurrent:
+                        bookmarks.setcurrent(repo, None)
+                        return
+                    tgt = cur
+                    if rev:
+                        tgt = scmutil.revsingle(repo, rev).node()
+                    checkconflict(repo, mark, force, tgt)
+                    marks[mark] = tgt
+                if not inactive and cur == marks[newact] and not rev:
+                    bookmarks.setcurrent(repo, newact)
+                elif cur != tgt and newact == repo._bookmarkcurrent:
+                    bookmarks.setcurrent(repo, None)
+                marks.write()
+
+            elif inactive:
+                if len(marks) == 0:
+                    ui.status(_("no bookmarks set\n"))
+                elif not repo._bookmarkcurrent:
+                    ui.status(_("no active bookmark\n"))
+                else:
+                    bookmarks.setcurrent(repo, None)
+        finally:
+            wlock.release()
     else: # show bookmarks
+        marks = repo._bookmarks
         if len(marks) == 0:
             ui.status(_("no bookmarks set\n"))
         else: