Merge lp:~spiv/bzr/hpss-compat-528041 into lp:bzr/2.1

[Andrew Bennetts]

This fixes the AssertionError from _remember_remote_is_before when opening a branch from a 1.5 (or older) server. The right fix is to simply not raise an error there, all it does is inflict pain on users for a minor (at worst) performance bug. The longer analysis is at <https://bugs.edge.launchpad.net/bzr/+bug/528041/comments/11> if you're curious.

I'm sure Martin was hoping for this method to be gone entirely, but hopefully this smaller step will still make him at least a little be happy :)

[Vincent Ladeuil (vila)]

Sounds good.

review: Approve

[lp:~spiv/bzr/hpss-compat-528041]

4841. By Andrew Bennetts on 2010-04-30

Merge lp:bzr/2.1

=== modified file 'NEWS'
--- NEWS	2010-04-30 06:27:15 +0000
+++ NEWS	2010-04-30 08:16:29 +0000
@@ -16,12 +16,26 @@
 * ``bzr switch`` does not die if a ConfigurableFileMerger is used.
   (Aaron Bentley, #559436)
 
+* Fixed ``AssertionError`` when accessing smart servers running Bazaar
+  versions before 1.6.
+  (Andrew Bennetts, #528041)
+
 * Reset ``siginterrupt`` flag to False every time we handle a signal
   installed with ``set_signal_handler(..., restart_syscall=True)`` (from
   ``bzrlib.osutils``.  Reduces the likelihood of "Interrupted System Call"
   errors after two window resizes.
   (Andrew Bennetts)
 
+Internals
+*********
+
+* ``_remember_remote_is_before`` no longer raises AssertionError when
+  suboptimal network behaviour is noticed; instead it just mutters to the
+  log file (and warns the user if they have set the ``hpss`` debug flag).
+  This was causing unnecessary aborts for performance bugs that are minor
+  at worst.
+  (Andrew Bennetts, #528041)
+
 
 bzr 2.1.1
 #########

=== modified file 'bzrlib/smart/medium.py'
--- bzrlib/smart/medium.py	2010-02-17 17:11:16 +0000
+++ bzrlib/smart/medium.py	2010-04-30 08:16:29 +0000
@@ -609,10 +609,16 @@
             # which is newer than a previously supplied older-than version.
             # This indicates that some smart verb call is not guarded
             # appropriately (it should simply not have been tried).
-            raise AssertionError(
+            trace.mutter(
                 "_remember_remote_is_before(%r) called, but "
                 "_remember_remote_is_before(%r) was called previously."
-                % (version_tuple, self._remote_version_is_before))
+                , version_tuple, self._remote_version_is_before)
+            if 'hpss' in debug.debug_flags:
+                ui.ui_factory.show_warning(
+                    "_remember_remote_is_before(%r) called, but "
+                    "_remember_remote_is_before(%r) was called previously."
+                    % (version_tuple, self._remote_version_is_before))
+            return
         self._remote_version_is_before = version_tuple
 
     def protocol_version(self):

=== modified file 'bzrlib/tests/test_remote.py'
--- bzrlib/tests/test_remote.py	2010-04-23 05:28:35 +0000
+++ bzrlib/tests/test_remote.py	2010-04-30 08:16:29 +0000
@@ -417,9 +417,12 @@
         # Calling _remember_remote_is_before again with a lower value works.
         client_medium._remember_remote_is_before((1, 5))
         self.assertTrue(client_medium._is_remote_before((1, 5)))
-        # You cannot call _remember_remote_is_before with a larger value.
-        self.assertRaises(
-            AssertionError, client_medium._remember_remote_is_before, (1, 9))
+        # If you call _remember_remote_is_before with a higher value it logs a
+        # warning, and continues to remember the lower value.
+        self.assertNotContainsRe(self.get_log(), '_remember_remote_is_before')
+        client_medium._remember_remote_is_before((1, 9))
+        self.assertContainsRe(self.get_log(), '_remember_remote_is_before')
+        self.assertTrue(client_medium._is_remote_before((1, 5)))
 
 
 class TestBzrDirCloningMetaDir(TestRemote):
@@ -527,6 +530,28 @@
         self.assertIsInstance(bd, RemoteBzrDir)
         self.assertFinished(client)
 
+    def test_backwards_compat_hpss_v2(self):
+        client, transport = self.make_fake_client_and_transport()
+        # Monkey-patch fake client to simulate real-world behaviour with v2
+        # server: upon first RPC call detect the protocol version, and because
+        # the version is 2 also do _remember_remote_is_before((1, 6)) before
+        # continuing with the RPC.
+        orig_check_call = client._check_call
+        def check_call(method, args):
+            client._medium._protocol_version = 2
+            client._medium._remember_remote_is_before((1, 6))
+            client._check_call = orig_check_call
+            client._check_call(method, args)
+        client._check_call = check_call
+        client.add_expected_call(
+            'BzrDir.open_2.1', ('quack/',), 'unknown', ('BzrDir.open_2.1',))
+        client.add_expected_call(
+            'BzrDir.open', ('quack/',), 'success', ('yes',))
+        bd = RemoteBzrDir(transport, remote.RemoteBzrDirFormat(),
+            _client=client, _force_probe=True)
+        self.assertIsInstance(bd, RemoteBzrDir)
+        self.assertFinished(client)
+
 
 class TestBzrDirOpenBranch(TestRemote):