Merge lp:~mbp/bzr/456077-cross-format-fetch into lp:bzr/2.0

[Martin Pool]

[Martin Pool (mbp)]

* Give warnings about cross-format fetch for pushes and pulls across the network.
* Put the actual warning into the UI object.

[John A Meinel (jameinel)]

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

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/456077-cross-format-fetch into lp:bzr/2.0.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #456077 MYSQL/BZR P3: bzr doesn't explain it's doing a slow cross-format fetch
> https://bugs.launchpad.net/bugs/456077
>
>
> * Give warnings about cross-format fetch for pushes and pulls across the network.
> * Put the actual warning into the UI object.
>
>

This seems ok. My main concern is that I don't see a dedup counter. So
if there are multiple 'inventory-delta' substreams (allowed by the
protocol, may not happen in practice) then you'll get the warning
multiple times.

Otherwise the change to a common warning helper is nice. I guess on
ui_factory is reasonable as long as it is on the base class so that
other factories don't *have* to implement it. (potentially a 2.0-stable
compatibility issue.)

"Upgrade the branches" also isn't 100% accurate, given that it is the
repository format that matters...

I'm happy enough with this, but you may want to tweak it before landing.

 review: approve
 merge: approve

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

iEYEARECAAYFAktPLQIACgkQJdeBCYSNAAP7IQCdG/GXeDjqN6sFzukMpnluf0/U
SsQAniso3Xu+z/pjbvaFhnS0yftZr7ga
=dfAR
-----END PGP SIGNATURE-----

review: Approve

[Martin Pool (mbp)]

> This seems ok. My main concern is that I don't see a dedup counter. So
> if there are multiple 'inventory-delta' substreams (allowed by the
> protocol, may not happen in practice) then you'll get the warning
> multiple times.

It's a fair point, but it doesn't seem to happen at the moment.

Should the de-duping be local? Maybe the warning suppression code should allow it to be global just once, as for Python warnings.

> "Upgrade the branches" also isn't 100% accurate, given that it is the
> repository format that matters...

Good point.

>
> I'm happy enough with this, but you may want to tweak it before landing.

I'll just correct the phrasing and send it. Bug 507655 is a followon for being able to suppress the warning.

[lp:~mbp/bzr/456077-cross-format-fetch]

4727. By Canonical.com Patch Queue Manager <email address hidden> on 2010-01-15

(mbp) warning on cross-format stream transfers

=== modified file 'NEWS'
--- NEWS	2010-01-13 16:31:34 +0000
+++ NEWS	2010-01-15 03:36:28 +0000
@@ -41,10 +41,10 @@
 * Give a clearer message if the lockdir disappears after being apparently
   successfully taken.  (Martin Pool, #498378)
 
-* Give a warning when fetching between local repositories with
+* Give a warning when fetching between repositories (local or remote) with
   sufficiently different formats that the content will need to be
-  serialized (ie ``InterDifferingSerializer``) so the user has a clue that
-  upgrading could make it faster.
+  serialized (ie ``InterDifferingSerializer`` or ``inventory-deltas``), so
+  the user has a clue that upgrading could make it faster.
   (Martin Pool, #456077)
 
 * If we fail to open ``~/.bzr.log`` write a clear message to stderr rather

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py	2010-01-12 06:12:31 +0000
+++ bzrlib/repository.py	2010-01-15 03:36:28 +0000
@@ -3984,11 +3984,8 @@
         #
         # nb this is only active for local-local fetches; other things using
         # streaming.
-        trace.warning("Fetching between repositories with different formats\n"
-            "from %s to %s.\n"
-            "This may take some time. Upgrade the branches to the same format \n"
-            "for better results.\n"
-            % (self.source._format, self.target._format))
+        ui.ui_factory.warn_cross_format_fetch(self.source._format,
+            self.target._format)
         if (not self.source.supports_rich_root()
             and self.target.supports_rich_root()):
             self._converting_to_rich_root = True
@@ -4287,6 +4284,8 @@
                     self._extract_and_insert_inventories(
                         substream, src_serializer)
             elif substream_type == 'inventory-deltas':
+                ui.ui_factory.warn_cross_format_fetch(src_format,
+                    self.target_repo._format)
                 self._extract_and_insert_inventory_deltas(
                     substream, src_serializer)
             elif substream_type == 'chk_bytes':
@@ -4591,8 +4590,10 @@
 
     def _get_convertable_inventory_stream(self, revision_ids,
                                           delta_versus_null=False):
-        # The source is using CHKs, but the target either doesn't or it has a
-        # different serializer.  The StreamSink code expects to be able to
+        # The two formats are sufficiently different that there is no fast
+        # path, so we need to send just inventorydeltas, which any
+        # sufficiently modern client can insert into any repository.
+        # The StreamSink code expects to be able to
         # convert on the target, so we need to put bytes-on-the-wire that can
         # be converted.  That means inventory deltas (if the remote is <1.19,
         # RemoteStreamSink will fallback to VFS to insert the deltas).

=== modified file 'bzrlib/smart/repository.py'
--- bzrlib/smart/repository.py	2009-09-03 00:33:35 +0000
+++ bzrlib/smart/repository.py	2010-01-15 03:36:28 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006, 2007 Canonical Ltd
+# Copyright (C) 2006, 2007, 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
@@ -30,6 +30,7 @@
     graph,
     osutils,
     pack,
+    ui,
     versionedfile,
     )
 from bzrlib.bzrdir import BzrDir
@@ -502,6 +503,13 @@
     yield pack_writer.begin()
     yield pack_writer.bytes_record(src_format.network_name(), '')
     for substream_type, substream in stream:
+        if substream_type == 'inventory-deltas':
+            # This doesn't feel like the ideal place to issue this warning;
+            # however we don't want to do it in the Repository that's
+            # generating the stream, because that might be on the server.
+            # Instead we try to observe it as the stream goes by.
+            ui.ui_factory.warn_cross_format_fetch(src_format,
+                '(remote)')
         for record in substream:
             if record.storage_kind in ('chunked', 'fulltext'):
                 serialised = record_to_fulltext_bytes(record)

=== modified file 'bzrlib/tests/blackbox/test_pull.py'
--- bzrlib/tests/blackbox/test_pull.py	2010-01-13 16:27:22 +0000
+++ bzrlib/tests/blackbox/test_pull.py	2010-01-15 03:36:28 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005-2010 Canonical Ltd
+# Copyright (C) 2005, 2006, 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
@@ -20,13 +20,18 @@
 import os
 import sys
 
+from bzrlib import (
+    debug,
+    remote,
+    urlutils,
+    )
+
 from bzrlib.branch import Branch
 from bzrlib.directory_service import directories
 from bzrlib.osutils import pathjoin
 from bzrlib.tests.blackbox import ExternalBase
 from bzrlib.uncommit import uncommit
 from bzrlib.workingtree import WorkingTree
-from bzrlib import urlutils
 
 
 class TestPull(ExternalBase):
@@ -394,10 +399,37 @@
     def test_pull_cross_format_warning(self):
         """You get a warning for probably slow cross-format pulls.
         """
-
-        from_tree = self.make_branch_and_tree('from', format='2a')
-        to_tree = self.make_branch_and_tree('to', format='1.14-rich-root')
-        from_tree.commit(message='first commit')
-        out, err = self.run_bzr(['pull', '-d', 'to', 'from'])
-        self.assertContainsRe(err,
-            "(?m)Fetching between repositories with different formats.*")
+        # this is assumed to be going through InterDifferingSerializer
+        from_tree = self.make_branch_and_tree('from', format='2a')
+        to_tree = self.make_branch_and_tree('to', format='1.14-rich-root')
+        from_tree.commit(message='first commit')
+        out, err = self.run_bzr(['pull', '-d', 'to', 'from'])
+        self.assertContainsRe(err,
+            "(?m)Doing on-the-fly conversion")
+
+    def test_pull_cross_format_warning_no_IDS(self):
+        """You get a warning for probably slow cross-format pulls.
+        """
+        # this simulates what would happen across the network, where
+        # interdifferingserializer is not active
+
+        debug.debug_flags.add('IDS_never')
+        # TestCase take care of restoring them
+
+        from_tree = self.make_branch_and_tree('from', format='2a')
+        to_tree = self.make_branch_and_tree('to', format='1.14-rich-root')
+        from_tree.commit(message='first commit')
+        out, err = self.run_bzr(['pull', '-d', 'to', 'from'])
+        self.assertContainsRe(err,
+            "(?m)Doing on-the-fly conversion")
+
+    def test_pull_cross_format_from_network(self):
+        self.setup_smart_server_with_call_log()
+        from_tree = self.make_branch_and_tree('from', format='2a')
+        to_tree = self.make_branch_and_tree('to', format='1.14-rich-root')
+        self.assertIsInstance(from_tree.branch, remote.RemoteBranch)
+        from_tree.commit(message='first commit')
+        out, err = self.run_bzr(['pull', '-d', 'to',
+            from_tree.branch.bzrdir.root_transport.base])
+        self.assertContainsRe(err,
+            "(?m)Doing on-the-fly conversion")

=== modified file 'bzrlib/ui/__init__.py'
--- bzrlib/ui/__init__.py	2009-07-22 07:34:08 +0000
+++ bzrlib/ui/__init__.py	2010-01-15 03:36:28 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2006, 2007, 2008, 2009 Canonical Ltd
+# Copyright (C) 2005-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
@@ -208,6 +208,13 @@
         """
         pass
 
+    def warn_cross_format_fetch(self, from_format, to_format):
+        """Warn about a potentially slow cross-format transfer"""
+        # See <https://launchpad.net/bugs/456077> asking for a warning here
+        trace.warning("Doing on-the-fly conversion from %s to %s.\n"
+            "This may take some time. Upgrade the repositories to the "
+            "same format for better performance.\n" %
+            (from_format, to_format))
 
 
 class CLIUIFactory(UIFactory):