Merge lp:~danny.vanheumen/bzr/bug-498409-bzr-revert-takes-a-branch-lock into lp:bzr/2.0

[Danny van Heumen]

(andrew, for Danny van Heumen) 'bzr revert' no longer takes a branch lock (#498409)

Fix for bug 498409 bzr revert takes a branch lock including a test.

[Andrew Bennetts (spiv)]

Overall this looks very good, and only needs a few tweaks I think.

Thanks for writing that documentation! I was a bit surprised to see a documentation addition in the diff because you didn't mention it in the merge proposal description, but it was a welcome surprise :)

126 +Note that the representation of LockResult can be confusing. As can be seen in
127 +the sample below::
128 +
129 + LockResult(file:///tmp/testbzr-J2pcy2.tmp/.bzr/branch/lockh8c6t28rcjdkgxtndbje)
130 +
131 +The last 20 characters, in this case `h8c6t28rcjdkgxtndbje`, are used internally
132 +in the locking mechanism. The locking URL is everything before that.

That is confusing. Rather than explain the confusing, I think we should just fix LockResult's __repr__ method so that it doesn't confuse, e.g.:

     def __repr__(self):
- return '%s(%s%s)' % (self.__class__.__name__,
+ return '%s(%s, %s)' % (self.__class__.__name__,

(in bzrlib/lock.py)

147 +Note: There is one special case of a lock that is created during the creation of
148 +the '.bzr' directory, for example when a new branch is initialized.
149 +If this is the case, the LockResult will look like this::
150 +
151 + LockResult(file:///tmp/testbzr-J2pcy2.tmp/.bzr/branch-lockc4au55ppz8wdym11z1aq)
152 +
153 +or `/branch-lock`

I don't think this is a special case, really. I'd just list this as "BzrDir: `/branch-lock`" in your list of lock types.

83 + # make sure that only one lock is acquired and released.
84 + self.assertEqual(1, len(locks_acquired))
85 + self.assertEqual(1, len(locks_released))

Use "self.assertLength(1, locks_acquired)" instead; it's a little short and clearer, and will give more informative errors if it fails.

65 + self.start_logging_connections()

I don't think you need to call this, you don't seem to use it. For that matter, I think you should simply use TestCaseInTempDir rather than the more complicated TestCaseWithConnectionHookedTransport.

You should add an entry to NEWS for this fix — you deserve the credit!

Finally, it appears you haven't signed a contributors agreement yet, please follow the instructions at <http://www.canonical.com/contributors>.

Thanks!

review: Needs Fixing

[Danny van Heumen (danny.vanheumen)]

Processed all suggested tweaks.

[Andrew Bennetts (spiv)]

Thanks, that looks good to me. All that's left is a review from a second person, and the contributor agreement.

review: Approve

[Danny van Heumen (danny.vanheumen)]

> Thanks, that looks good to me. All that's left is a review from a second
> person, and the contributor agreement.
I mailed the Contributor Agreement yesterday.

[Martin Pool (mbp)]

On 18 March 2010 11:19, Danny van Heumen <email address hidden> wrote:
>> Thanks, that looks good to me.  All that's left is a review from a second
>> person, and the contributor agreement.
> I mailed the Contributor Agreement yesterday.

Hi Danny,

I got your questions about it (obviously, since I replied) and I saw
you say you sent it, but I didn't actually get the agreement. Did you
cc me?

thanks,

--
Martin <http://launchpad.net/~mbp/>

[Ian Clatworthy (ian-clatworthy)]

Looks good to me. In particular, thanks for the documentation explaining how to test lock behaviour.

review: Approve

[Andrew Bennetts (spiv)]

Fed to pqm

star-merge http://bazaar.launchpad.net/~danny.vanheumen/bzr/bug-498409-bzr-revert-takes-a-branch-lock http://bazaar.launchpad.net/~bzr-pqm/bzr/2.0

[Vincent Ladeuil (vila)]

Watch out ! This has been merged after 2.0.5 tarball has been cut but still mentions
the bugfix in the 2.0.5 NEWS section.

[Andrew Bennetts (spiv)]

Vincent Ladeuil wrote:
> Watch out ! This has been merged after 2.0.5 tarball has been cut but still mentions
> the bugfix in the 2.0.5 NEWS section.

Ouch! Good catch, I'll fix that now.

-Andrew.

=== modified file 'NEWS'
--- NEWS	2010-03-17 04:26:48 +0000
+++ NEWS	2010-03-18 00:01:02 +0000
@@ -54,6 +54,10 @@
   errors.
   (Inada Naoki, #524560)
 
+* ``bzr revert`` now only takes write lock on working tree, instead of on 
+  both working tree and branch.
+  (Danny van Heumen, #498409)
+
 Documentation
 *************
 

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2009-12-16 22:26:56 +0000
+++ bzrlib/builtins.py	2010-03-18 00:01:02 +0000
@@ -4045,7 +4045,7 @@
     def run(self, revision=None, no_backup=False, file_list=None,
             forget_merges=None):
         tree, file_list = tree_files(file_list)
-        tree.lock_write()
+        tree.lock_tree_write()
         try:
             if forget_merges:
                 tree.set_parent_ids(tree.get_parent_ids()[:1])

=== modified file 'bzrlib/lock.py'
--- bzrlib/lock.py	2009-07-31 16:51:48 +0000
+++ bzrlib/lock.py	2010-03-18 00:01:02 +0000
@@ -84,7 +84,7 @@
         return self.lock_url == other.lock_url and self.details == other.details
 
     def __repr__(self):
-        return '%s(%s%s)' % (self.__class__.__name__,
+        return '%s(%s, %s)' % (self.__class__.__name__,
                              self.lock_url, self.details)
 
 

=== modified file 'bzrlib/tests/commands/__init__.py'
--- bzrlib/tests/commands/__init__.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/commands/__init__.py	2010-03-18 00:01:02 +0000
@@ -41,6 +41,7 @@
         'bzrlib.tests.commands.test_pull',
         'bzrlib.tests.commands.test_push',
         'bzrlib.tests.commands.test_update',
+        'bzrlib.tests.commands.test_revert',
         ]
     # add the tests for the sub modules
     suite.addTests(loader.loadTestsFromModuleNames(testmod_names))

=== added file 'bzrlib/tests/commands/test_revert.py'
--- bzrlib/tests/commands/test_revert.py	1970-01-01 00:00:00 +0000
+++ bzrlib/tests/commands/test_revert.py	2010-03-18 00:01:02 +0000
@@ -0,0 +1,61 @@
+# Copyright (C) 2010 Canonical Ltd
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+import os
+from bzrlib import (
+    branch,
+    builtins,
+    errors,
+    lock,
+    )
+from bzrlib.tests import (
+    transport_util,
+    TestCaseInTempDir,
+    )
+
+
+class TestRevert(TestCaseInTempDir):
+
+    def setUp(self):
+        super(TestRevert, self).setUp()
+
+    def test_revert_tree_write_lock_and_branch_read_lock(self):
+
+        # install lock hooks to find out about cmd_revert's locking actions
+        locks_acquired = []
+        locks_released = []
+        lock.Lock.hooks.install_named_hook('lock_acquired',
+            locks_acquired.append, None)
+        lock.Lock.hooks.install_named_hook('lock_released',
+            locks_released.append, None)
+
+        # execute the revert command (There is nothing to actually revert,
+        # but locks are acquired either way.)
+        revert = builtins.cmd_revert()
+        revert.run()
+
+        # make sure that only one lock is acquired and released.
+        self.assertLength(1, locks_acquired)
+        self.assertLength(1, locks_released)
+
+        # make sure that the nonces are the same, since otherwise
+        # this would not be the same lock.
+        self.assertEqual(locks_acquired[0].details, locks_released[0].details)
+
+        # make sure that the locks are checkout locks.
+        self.assertEndsWith(locks_acquired[0].lock_url, "/checkout/lock")
+        self.assertEndsWith(locks_released[0].lock_url, "/checkout/lock")
+

=== modified file 'doc/developers/testing.txt'
--- doc/developers/testing.txt	2009-09-09 02:21:47 +0000
+++ doc/developers/testing.txt	2010-03-18 00:01:02 +0000
@@ -201,6 +201,43 @@
 .. ~~~~~~~~~~~~
 
 
+Testing locking behaviour
+-------------------------
+
+In order to test the locking behaviour of commands, it is possible to install
+a hook that is called when a write lock is: acquired, released or broken.
+(Read locks also exist, they cannot be discovered in this way.)
+
+A hook can be installed by calling bzrlib.lock.Lock.hooks.install_named_hook.
+The three valid hooks are: `lock_acquired`, `lock_released` and `lock_broken`.
+
+Example::
+
+    locks_acquired = []
+    locks_released = []
+
+    lock.Lock.hooks.install_named_hook('lock_acquired',
+        locks_acquired.append, None)
+    lock.Lock.hooks.install_named_hook('lock_released',
+        locks_released.append, None)
+
+`locks_acquired` will now receive a LockResult instance for all locks acquired
+since the time the hook is installed.
+
+The last part of the `lock_url` allows you to identify the type of object that is locked.
+
+- BzrDir: `/branch-lock`
+- Working tree: `/checkout/lock`
+- Branch: `/branch/lock`
+- Repository: `/repository/lock`
+
+To test if a lock is a write lock on a working tree, one can do the following::
+
+    self.assertEndsWith(locks_acquired[0].lock_url, "/checkout/lock")
+
+See bzrlib/tests/commands/test_revert.py for an example of how to use this for
+testing locks.
+
 
 Skipping tests
 --------------