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

[Martin Pool]

(mbp) give a clearer warning about cross-format conversions earlier in fetch

[Martin Pool (mbp)]

This fixes https://bugs.edge.launchpad.net/bzr/+bug/456077 properly by giving a message about cross-format conversions earlier in fetch, and https://bugs.edge.launchpad.net/bzr/+bug/515356 by showing the remote repository network name.

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

4741. By Martin Pool on 2010-02-24

Branching that does an implicit conversion now shows the fetch warning

4742. By Martin Pool on 2010-02-24

Also show conversion warning for InterDifferingSerializer

[Martin Pool (mbp)]

https://bugs.edge.launchpad.net/bzr/+bug/513157 causes some test failures and probably needs to be fixed to land this.

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

4743. By Martin Pool on 2010-02-25

Generalize to ui_factory.show_user_warning

4744. By Martin Pool on 2010-02-25

Suppress user warnings about cross-format fetch during upgrade

4745. By Martin Pool on 2010-02-25

Update test_ui for warning suppression

4746. By Martin Pool on 2010-02-25

Rename squelched to suppressed

4747. By Martin Pool on 2010-02-25

update news

4748. By Martin Pool on 2010-02-25

Restore UIFactory.warn_cross_format_fetch in case it's used by an API client

[Andrew Bennetts (spiv)]

On IRC Martin asked me to look at the UIFactory changes, i.e. the new show_user_warning/suppressed_warnings API. It looks fine to me. I like the paranoia when looking up the warning string and trying to format it. I think the API to temporarily suppress a particular warning is a bit cumbersome, but good enough for now (perhaps we'll think of improvements later if we use it more heavily). So, for that aspect of the patch, +1.

I haven't yet really looked at the part of the patch that emits warnings about slow fetches, although removing code from bzrlib/smart is a promising sign :)

review: Approve (uifactory)

[Martin Pool (mbp)]

On 25 February 2010 17:12, Andrew Bennetts
<email address hidden> wrote:
> Review: Approve uifactory
> On IRC Martin asked me to look at the UIFactory changes, i.e. the new show_user_warning/suppressed_warnings API.  It looks fine to me.  I like the paranoia when looking up the warning string and trying to format it.  I think the API to temporarily suppress a particular warning is a bit cumbersome, but good enough for now (perhaps we'll think of improvements later if we use it more heavily).  So, for that aspect of the patch, +1.

I was planning to build on addCleanup etc, but this isn't in 2.0 where
this is targeted.

It might be nice if there was a contextmanager-like object for "with a
warning suppressed".

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

[Martin Pool (mbp)]

As a followon, when this merges to trunk, it can take https://bugs.edge.launchpad.net/bzr/+bug/507655 into account. In 2.1 and later we have a config.suppress_warnings option that will let you turn them on/off per location; presumably that should be checked before going into the uifactory.

[Vincent Ladeuil (vila)]

>>>>> Martin Pool <email address hidden> writes:

    > As a followon, when this merges to trunk, it can take
    > https://bugs.edge.launchpad.net/bzr/+bug/507655 into account. In
    > 2.1 and later we have a config.suppress_warnings option that will
    > let you turn them on/off per location; presumably that should be
    > checked before going into the uifactory.

On a semi-related subject, we miss a config object that can
search either bazaar.conf or locations.conf.

So far, we can do either:
- global config,
- global + locations + branch.

  Vincent

[Andrew Bennetts (spiv)]

Vincent Ladeuil wrote:
> >>>>> Martin Pool <email address hidden> writes:
>
> > As a followon, when this merges to trunk, it can take
> > https://bugs.edge.launchpad.net/bzr/+bug/507655 into account. In
> > 2.1 and later we have a config.suppress_warnings option that will
> > let you turn them on/off per location; presumably that should be
> > checked before going into the uifactory.
>
> On a semi-related subject, we miss a config object that can
> search either bazaar.conf or locations.conf.
>
> So far, we can do either:
> - global config,
> - global + locations + branch.

That's true. I actually have code more-or-less like this in bzr-pqm of all
places, but have lacked the tuits to extract it into something appropriate for
bzrlib. See the StackedConfig class in pqm_submit.py:
<http://bazaar.launchpad.net/~bzr-pqm-devel/bzr-pqm/devel/annotate/head%3A/pqm_submit.py#L198>

-Andrew.

[John A Meinel (jameinel)]

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

Vincent Ladeuil wrote:
>>>>>> Martin Pool <email address hidden> writes:
>
> > As a followon, when this merges to trunk, it can take
> > https://bugs.edge.launchpad.net/bzr/+bug/507655 into account. In
> > 2.1 and later we have a config.suppress_warnings option that will
> > let you turn them on/off per location; presumably that should be
> > checked before going into the uifactory.
>
> On a semi-related subject, we miss a config object that can
> search either bazaar.conf or locations.conf.
>
> So far, we can do either:
> - global config,
> - global + locations + branch.
>
> Vincent

Not true, LocationConfig is global + locations without Branch. We just
don't have it obviously exposed.

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

iEYEARECAAYFAkuHA+4ACgkQJdeBCYSNAANaGACfXNLiwILJCKO9KTfIhKV+2+x7
VMkAoL82qA72xaHAv6d8cfjZbliujMKA
=FcJC
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

@Martin, is this ready to land ?

[Martin Pool (mbp)]

Fed to pqm

star-merge http://bazaar.launchpad.net/~mbp/bzr/456077-cross-format-fetch http://bazaar.launchpad.net/~bzr-pqm/bzr/2.0

[Martin Pool (mbp)]

Fed to pqm

star-merge http://bazaar.launchpad.net/~mbp/bzr/456077-cross-format-fetch http://bazaar.launchpad.net/~bzr-pqm/bzr/2.0

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

4749. By Martin Pool on 2010-03-17

merge news

=== modified file 'NEWS'
--- NEWS	2010-03-03 09:21:47 +0000
+++ NEWS	2010-03-17 04:31:23 +0000
@@ -29,6 +29,12 @@
   a lock.
   (Martin Pool, #185103)
 
+* Give the warning about potentially slow cross-format fetches much
+  earlier on in the fetch operation.  Don't show this message during
+  upgrades, and show the correct format indication for remote
+  repositories.
+  (Martin Pool, #456077, #515356, #513157)
+
 * Handle renames correctly when there are files or directories that 
   differ only in case.  (Chris Jones, Martin Pool, #368931)
 

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py	2010-01-14 07:34:37 +0000
+++ bzrlib/repository.py	2010-03-17 04:31:23 +0000
@@ -3067,8 +3067,8 @@
     # Does the repository inventory storage understand references to trees?
     supports_tree_reference = None
 
-    def __str__(self):
-        return "<%s>" % self.__class__.__name__
+    def __repr__(self):
+        return "%s()" % self.__class__.__name__
 
     def __eq__(self, other):
         # format objects are generally stateless
@@ -3407,6 +3407,11 @@
         :return: None.
         """
         from bzrlib.fetch import RepoFetcher
+        # See <https://launchpad.net/bugs/456077> asking for a warning here
+        if self.source._format.network_name() != self.target._format.network_name():
+            ui.ui_factory.show_user_warning('cross_format_fetch',
+                from_format=self.source._format,
+                to_format=self.target._format)
         f = RepoFetcher(to_repository=self.target,
                                from_repository=self.source,
                                last_revision=revision_id,
@@ -3980,18 +3985,17 @@
         """See InterRepository.fetch()."""
         if fetch_spec is not None:
             raise AssertionError("Not implemented yet...")
-        # See <https://launchpad.net/bugs/456077> asking for a warning here
-        #
-        # nb this is only active for local-local fetches; other things using
-        # streaming.
-        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
             self._revision_id_to_root_id = {}
         else:
             self._converting_to_rich_root = False
+        # See <https://launchpad.net/bugs/456077> asking for a warning here
+        if self.source._format.network_name() != self.target._format.network_name():
+            ui.ui_factory.show_user_warning('cross_format_fetch',
+                from_format=self.source._format,
+                to_format=self.target._format)
         revision_ids = self.target.search_missing_revision_ids(self.source,
             revision_id, find_ghosts=find_ghosts).get_keys()
         if not revision_ids:
@@ -4284,8 +4288,6 @@
                     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':

=== modified file 'bzrlib/smart/repository.py'
--- bzrlib/smart/repository.py	2010-01-14 07:34:37 +0000
+++ bzrlib/smart/repository.py	2010-03-17 04:31:23 +0000
@@ -503,13 +503,6 @@
     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_branch.py'
--- bzrlib/tests/blackbox/test_branch.py	2009-08-20 12:30:49 +0000
+++ bzrlib/tests/blackbox/test_branch.py	2010-03-17 04:31:23 +0000
@@ -326,6 +326,8 @@
             '  Packs 5 (adds stacking support, requires bzr 1.6)\n'
             'Source branch format does not support stacking, using format:\n'
             '  Branch format 7\n'
+            'Doing on-the-fly conversion from RepositoryFormatKnitPack1() to RepositoryFormatKnitPack5().\n'
+            'This may take some time. Upgrade the repositories to the same format for better performance.\n'
             'Created new stacked branch referring to %s.\n' % (trunk.base,),
             err)
 
@@ -339,6 +341,8 @@
             '  Packs 5 rich-root (adds stacking support, requires bzr 1.6.1)\n'
             'Source branch format does not support stacking, using format:\n'
             '  Branch format 7\n'
+            'Doing on-the-fly conversion from RepositoryFormatKnitPack4() to RepositoryFormatKnitPack5RichRoot().\n'
+            'This may take some time. Upgrade the repositories to the same format for better performance.\n'
             'Created new stacked branch referring to %s.\n' % (trunk.base,),
             err)
 

=== modified file 'bzrlib/tests/test_ui.py'
--- bzrlib/tests/test_ui.py	2009-08-03 05:54:52 +0000
+++ bzrlib/tests/test_ui.py	2010-03-17 04:31:23 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2008, 2009 Canonical Ltd
+# Copyright (C) 2005, 2008, 2009, 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
@@ -25,6 +25,8 @@
 
 from bzrlib import (
     errors,
+    remote,
+    repository,
     tests,
     ui as _mod_ui,
     )
@@ -300,6 +302,32 @@
         finally:
             pb.finished()
 
+    def test_text_ui_show_user_warning(self):
+        from bzrlib.repofmt.groupcompress_repo import RepositoryFormat2a
+        from bzrlib.repofmt.pack_repo import RepositoryFormatKnitPack5
+        err = StringIO()
+        out = StringIO()
+        ui = tests.TextUIFactory(stdin=None, stdout=out, stderr=err)
+        remote_fmt = remote.RemoteRepositoryFormat()
+        remote_fmt._network_name = RepositoryFormatKnitPack5().network_name()
+        ui.show_user_warning('cross_format_fetch', from_format=RepositoryFormat2a(),
+            to_format=remote_fmt)
+        self.assertEquals('', out.getvalue())
+        self.assertEquals("Doing on-the-fly conversion from RepositoryFormat2a() to "
+            "RemoteRepositoryFormat(_network_name='Bazaar RepositoryFormatKnitPack5 "
+            "(bzr 1.6)\\n').\nThis may take some time. Upgrade the repositories to "
+            "the same format for better performance.\n",
+            err.getvalue())
+        # and now with it suppressed please
+        err = StringIO()
+        out = StringIO()
+        ui = tests.TextUIFactory(stdin=None, stdout=out, stderr=err)
+        ui.suppressed_warnings.add('cross_format_fetch')
+        ui.show_user_warning('cross_format_fetch', from_format=RepositoryFormat2a(),
+            to_format=remote_fmt)
+        self.assertEquals('', out.getvalue())
+        self.assertEquals('', err.getvalue())
+
 
 class CLIUITests(TestCase):
 

=== modified file 'bzrlib/ui/__init__.py'
--- bzrlib/ui/__init__.py	2010-01-15 02:41:11 +0000
+++ bzrlib/ui/__init__.py	2010-03-17 04:31:23 +0000
@@ -105,10 +105,22 @@
 
     This tells the library how to display things to the user.  Through this
     layer different applications can choose the style of UI.
+
+    :ivar suppressed_warnings: Identifiers for user warnings that should 
+        no be emitted.
     """
 
+    _user_warning_templates = dict(
+        cross_format_fetch=("Doing on-the-fly conversion from "
+            "%(from_format)s to %(to_format)s.\n"
+            "This may take some time. Upgrade the repositories to the "
+            "same format for better performance."
+            )
+        )
+
     def __init__(self):
         self._task_stack = []
+        self.suppressed_warnings = set()
 
     def get_password(self, prompt='', **kwargs):
         """Prompt the user for a password.
@@ -170,6 +182,21 @@
         """
         pass
 
+    def format_user_warning(self, warning_id, message_args):
+        try:
+            template = self._user_warning_templates[warning_id]
+        except KeyError:
+            fail = "failed to format warning %r, %r" % (warning_id, message_args)
+            warnings.warn(fail)   # so tests will fail etc
+            return fail
+        try:
+            return template % message_args
+        except ValueError, e:
+            fail = "failed to format warning %r, %r: %s" % (
+                warning_id, message_args, e)
+            warnings.warn(fail)   # so tests will fail etc
+            return fail
+
     def get_boolean(self, prompt):
         """Get a boolean question answered from the user.
 
@@ -190,8 +217,11 @@
     def recommend_upgrade(self,
         current_format_name,
         basedir):
-        # this should perhaps be in the TextUIFactory and the default can do
+        # XXX: this should perhaps be in the TextUIFactory and the default can do
         # nothing
+        #
+        # XXX: Change to show_user_warning - that will accomplish the previous
+        # xxx. -- mbp 2010-02-25
         trace.warning("%s is deprecated "
             "and a better format is available.\n"
             "It is recommended that you upgrade by "
@@ -208,13 +238,32 @@
         """
         pass
 
+    def show_user_warning(self, warning_id, **message_args):
+        """Show a warning to the user.
+
+        This is specifically for things that are under the user's control (eg
+        outdated formats), not for internal program warnings like deprecated
+        APIs.
+
+        This can be overridden by UIFactory subclasses to show it in some 
+        appropriate way; the default UIFactory is noninteractive and does
+        nothing.  format_user_warning maps it to a string, though other
+        presentations can be used for particular UIs.
+
+        :param warning_id: An identifier like 'cross_format_fetch' used to 
+            check if the message is suppressed and to look up the string.
+        :param message_args: Arguments to be interpolated into the message.
+        """
+        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))
+        """Warn about a potentially slow cross-format transfer.
+        
+        This is deprecated in favor of show_user_warning, but retained for api
+        compatibility in 2.0 and 2.1.
+        """
+        self.show_user_warning('cross_format_fetch', from_format=from_format,
+            to_format=to_format)
 
 
 class CLIUIFactory(UIFactory):

=== modified file 'bzrlib/ui/text.py'
--- bzrlib/ui/text.py	2009-08-06 02:23:37 +0000
+++ bzrlib/ui/text.py	2010-03-17 04:31:23 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2008, 2009 Canonical Ltd
+# Copyright (C) 2005, 2008, 2009, 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
@@ -31,6 +31,7 @@
     progress,
     osutils,
     symbol_versioning,
+    trace,
     )
 
 """)
@@ -190,6 +191,18 @@
     def _progress_all_finished(self):
         self._progress_view.clear()
 
+    def show_user_warning(self, warning_id, **message_args):
+        """Show a text message to the user.
+
+        Explicitly not for warnings about bzr apis, deprecations or internals.
+        """
+        # eventually trace.warning should migrate here, to avoid logging and
+        # be easier to test; that has a lot of test fallout so for now just
+        # new code can call this
+        if warning_id not in self.suppressed_warnings:
+            self.stderr.write(self.format_user_warning(warning_id, message_args) +
+                '\n')
+
 
 class TextProgressView(object):
     """Display of progress bar and other information on a tty.

=== modified file 'bzrlib/upgrade.py'
--- bzrlib/upgrade.py	2009-05-07 05:08:46 +0000
+++ bzrlib/upgrade.py	2010-03-17 04:31:23 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005, 2008, 2009 Canonical Ltd
+# Copyright (C) 2005, 2008, 2009, 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
@@ -29,6 +29,9 @@
     def __init__(self, url, format=None):
         self.format = format
         self.bzrdir = BzrDir.open_unsupported(url)
+        # XXX: Change to cleanup
+        warning_id = 'cross_format_fetch'
+        saved_warning = warning_id in ui.ui_factory.suppressed_warnings
         if isinstance(self.bzrdir, RemoteBzrDir):
             self.bzrdir._ensure_real()
             self.bzrdir = self.bzrdir._real_bzrdir
@@ -36,10 +39,13 @@
             raise errors.UpgradeReadonly
         self.transport = self.bzrdir.root_transport
         self.pb = ui.ui_factory.nested_progress_bar()
+        ui.ui_factory.suppressed_warnings.add(warning_id)
         try:
             self.convert()
         finally:
             self.pb.finished()
+            if not saved_warning:
+                ui.ui_factory.suppressed_warnings.remove(warning_id)
 
     def convert(self):
         try: