lock: turn prepinherit/reacquire into a single context manager

Review feedback from Pierre-Yves David. This makes the overall code cleaner and
less error-prone, and makes a previously explicitly checked error state
impossible.

mercurial/lock.py

@@ -7,6 +7,7 b''
 
 from __future__ import absolute_import
 
+import contextlib
 import errno
 import os
 import socket
@@ -171,19 +172,20 b' class lock(object):'
         locker = self._readlock()
         return self._testlock(locker)
 
-    def prepinherit(self):
-        """prepare for the lock to be inherited by a Mercurial subprocess
+    @contextlib.contextmanager
+    def inherit(self):
+        """context for the lock to be inherited by a Mercurial subprocess.
 
-        Returns a string that will be recognized by the lock in the
-        subprocess. Communicating this string to the subprocess needs to be done
-        separately -- typically by an environment variable.
+        Yields a string that will be recognized by the lock in the subprocess.
+        Communicating this string to the subprocess needs to be done separately
+        -- typically by an environment variable.
         """
         if not self.held:
             raise error.LockInheritanceContractViolation(
-                'prepinherit can only be called while lock is held')
+                'inherit can only be called while lock is held')
         if self._inherited:
             raise error.LockInheritanceContractViolation(
-                'prepinherit cannot be called while lock is already inherited')
+                'inherit cannot be called while lock is already inherited')
         if self.releasefn:
             self.releasefn()
         if self._parentheld:
@@ -191,12 +193,9 b' class lock(object):'
         else:
             lockname = '%s:%s' % (lock._host, self.pid)
         self._inherited = True
-        return lockname
-
-    def reacquire(self):
-        if not self._inherited:
-            raise error.LockInheritanceContractViolation(
-                'reacquire can only be called after prepinherit')
+        try:
+            yield lockname
+        finally:
         if self.acquirefn:
             self.acquirefn()
         self._inherited = False

tests/test-lock.py

@@ -158,7 +158,7 b' class testlock(unittest.TestCase):'
         parentstate.assertacquirecalled(True)
 
         # set up lock inheritance
-        lockname = parentlock.prepinherit()
+        with parentlock.inherit() as lockname:
         parentstate.assertreleasecalled(True)
         parentstate.assertpostreleasecalled(False)
         parentstate.assertlockexists(True)
@@ -167,14 +167,13 b' class testlock(unittest.TestCase):'
         childlock = childstate.makelock(parentlock=lockname)
         childstate.assertacquirecalled(True)
 
-        # release the child lock -- the lock file should still exist on disk
         childlock.release()
         childstate.assertreleasecalled(True)
         childstate.assertpostreleasecalled(True)
         childstate.assertlockexists(True)
 
         parentstate.resetacquirefn()
-        parentlock.reacquire()
+
         parentstate.assertacquirecalled(True)
 
         parentlock.release()
@@ -188,7 +187,7 b' class testlock(unittest.TestCase):'
         lock0 = state0.makelock()
         state0.assertacquirecalled(True)
 
-        lock0name = lock0.prepinherit()
+        with lock0.inherit() as lock0name:
         state0.assertreleasecalled(True)
         state0.assertpostreleasecalled(False)
         state0.assertlockexists(True)
@@ -198,8 +197,9 b' class testlock(unittest.TestCase):'
         state1.assertacquirecalled(True)
 
         # from within lock1, acquire another lock
-        lock1name = lock1.prepinherit()
-        # since the file on disk is lock0's this should have the same name
+            with lock1.inherit() as lock1name:
+                # since the file on disk is lock0's this should have the same
+                # name
         self.assertEqual(lock0name, lock1name)
 
         state2 = teststate(self, d, pidoffset=2)
@@ -212,7 +212,7 b' class testlock(unittest.TestCase):'
         state2.assertlockexists(True)
 
         state1.resetacquirefn()
-        lock1.reacquire()
+
         state1.assertacquirecalled(True)
 
         lock1.release()
@@ -220,7 +220,6 b' class testlock(unittest.TestCase):'
         state1.assertpostreleasecalled(True)
         state1.assertlockexists(True)
 
-        lock0.reacquire()
         lock0.release()
 
     def testinheritlockfork(self):
@@ -230,7 +229,7 b' class testlock(unittest.TestCase):'
         parentstate.assertacquirecalled(True)
 
         # set up lock inheritance
-        lockname = parentlock.prepinherit()
+        with parentlock.inherit() as lockname:
         childstate = teststate(self, d, pidoffset=1)
         childlock = childstate.makelock(parentlock=lockname)
         childstate.assertacquirecalled(True)
@@ -249,7 +248,6 b' class testlock(unittest.TestCase):'
         childstate.assertpostreleasecalled(True)
         childstate.assertlockexists(True)
 
-        parentlock.reacquire()
         parentlock.release()
 
 if __name__ == '__main__':