Merge lp:~spiv/bzr/hpss-diff-locking-bug into lp:bzr/2.0

[Andrew Bennetts]

[Andrew Bennetts (spiv)]

This fixes bug 389413. Various code paths assume that Branch.get_rev_id will implicitly lock_read the branch (and repository), but RemoteBranch's get_rev_id didn't have the @needs_read_lock decorator. So this branch fixes that, and adds a test about it. The sneaky part is that RemoteBranch would get away with it if the server is new enough, which is probably why the existing test suite didn't catch this.

Also, it's not so great that callers such as revisionspec.py expect get_rev_id to acquire a read lock for them, as we are almost certainly wasting some effort, but fixing the performance is separate to fixing the correctness. (And fixing the performance is considerable knottier.)

I think this fix is a good candidate for the 2.0 branch.

[Vincent Ladeuil (vila)]

Clear, concise, nice job :)
I agree that should be backported to 2.0.x.

review: Approve

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Review: Approve
> Clear, concise, nice job :)
> I agree that should be backported to 2.0.x.

Overall, I would like to see us get away from @needs_read_lock
decorators, because they inherently hide performance issues.

That said, good enough for a bugfix, I guess.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrMmIQACgkQJdeBCYSNAANJbgCgwHUcc3Xwb/ReidSTi3fF8xPL
J7EAnjNf4uixV+lKLkqKtk4KXvtzrGWI
=ARY2
-----END PGP SIGNATURE-----

=== modified file 'NEWS'
--- NEWS	2009-10-07 00:46:40 +0000
+++ NEWS	2009-10-07 05:45:22 +0000
@@ -26,6 +26,9 @@
   with some combinations of remote and local formats.  This was causing
   "unknown object type identifier 60" errors.  (Andrew Bennetts, #427736)
 
+* Fixed ``ObjectNotLocked`` errors when doing some log and diff operations
+  on branches via a smart server.  (Andrew Bennetts, #389413)
+
 * Occasional IndexError on renamed files have been fixed. Operations that
   set a full inventory in the working tree will now go via the
   apply_inventory_delta code path which is simpler and easier to

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2009-10-02 03:07:22 +0000
+++ bzrlib/remote.py	2009-10-07 05:45:22 +0000
@@ -2392,6 +2392,7 @@
             raise NotImplementedError(self.dont_leave_lock_in_place)
         self._leave_lock = False
 
+    @needs_read_lock
     def get_rev_id(self, revno, history=None):
         if revno == 0:
             return _mod_revision.NULL_REVISION

=== modified file 'bzrlib/tests/test_remote.py'
--- bzrlib/tests/test_remote.py	2009-09-10 06:32:42 +0000
+++ bzrlib/tests/test_remote.py	2009-10-07 05:45:22 +0000
@@ -2147,6 +2147,26 @@
             repo.get_rev_id_for_revno, 5, (42, 'rev-foo'))
         self.assertFinished(client)
 
+    def test_branch_fallback_locking(self):
+        """RemoteBranch.get_rev_id takes a read lock, and tries to call the
+        get_rev_id_for_revno verb.  If the verb is unknown the VFS fallback
+        will be invoked, which will fail if the repo is unlocked.
+        """
+        self.setup_smart_server_with_call_log()
+        tree = self.make_branch_and_memory_tree('.')
+        tree.lock_write()
+        rev1 = tree.commit('First')
+        rev2 = tree.commit('Second')
+        tree.unlock()
+        branch = tree.branch
+        self.assertFalse(branch.is_locked())
+        self.reset_smart_call_log()
+        verb = 'Repository.get_rev_id_for_revno'
+        self.disable_verb(verb)
+        self.assertEqual(rev1, branch.get_rev_id(1))
+        self.assertLength(1, [call for call in self.hpss_calls if
+                              call.call.method == verb])
+
 
 class TestRepositoryIsShared(TestRemoteRepository):