Bazaar

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

  1. 456077-cross-format-fetch
  2. Merge into 2.0
Proposed by Martin Pool
Status: Merged
Approved by: John A Meinel
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~mbp/bzr/456077-cross-format-fetch
Merge into: lp:bzr/2.0
Diff against target: 340 lines (+128/-27)
8 files modified
NEWS (+6/-0)
bzrlib/repository.py (+12/-10)
bzrlib/smart/repository.py (+0/-7)
bzrlib/tests/blackbox/test_branch.py (+4/-0)
bzrlib/tests/test_ui.py (+29/-1)
bzrlib/ui/__init__.py (+56/-7)
bzrlib/ui/text.py (+14/-1)
bzrlib/upgrade.py (+7/-1)
To merge this branch: bzr merge lp:~mbp/bzr/456077-cross-format-fetch
Reviewer Review Type Date Requested Status
Andrew Bennetts uifactory Approve
bzr-core Pending
Review via email: mp+20041@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

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.

Revision history for this message
Martin Pool (mbp) wrote :

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

Revision history for this message
Andrew Bennetts (spiv) wrote :

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)
Revision history for this message
Martin Pool (mbp) wrote :

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/>

Revision history for this message
Martin Pool (mbp) wrote :

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.

Revision history for this message
Vincent Ladeuil (vila) 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

Revision history for this message
Andrew Bennetts (spiv) wrote :

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.

Revision history for this message
John A Meinel (jameinel) wrote :

-----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-----

Revision history for this message
Vincent Ladeuil (vila) wrote :

@Martin, is this ready to land ?

Revision history for this message
Martin Pool (mbp) wrote :

Fed to pqm

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

Revision history for this message
Martin Pool (mbp) wrote :

Fed to pqm

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'NEWS'
--- NEWS 2010-03-03 09:21:47 +0000
+++ NEWS 2010-03-17 04:31:23 +0000
@@ -29,6 +29,12 @@
29 a lock.29 a lock.
30 (Martin Pool, #185103)30 (Martin Pool, #185103)
3131
32* Give the warning about potentially slow cross-format fetches much
33 earlier on in the fetch operation. Don't show this message during
34 upgrades, and show the correct format indication for remote
35 repositories.
36 (Martin Pool, #456077, #515356, #513157)
37
32* Handle renames correctly when there are files or directories that 38* Handle renames correctly when there are files or directories that
33 differ only in case. (Chris Jones, Martin Pool, #368931)39 differ only in case. (Chris Jones, Martin Pool, #368931)
3440
3541
=== 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 @@
3067 # Does the repository inventory storage understand references to trees?3067 # Does the repository inventory storage understand references to trees?
3068 supports_tree_reference = None3068 supports_tree_reference = None
30693069
3070 def __str__(self):3070 def __repr__(self):
3071 return "<%s>" % self.__class__.__name__3071 return "%s()" % self.__class__.__name__
30723072
3073 def __eq__(self, other):3073 def __eq__(self, other):
3074 # format objects are generally stateless3074 # format objects are generally stateless
@@ -3407,6 +3407,11 @@
3407 :return: None.3407 :return: None.
3408 """3408 """
3409 from bzrlib.fetch import RepoFetcher3409 from bzrlib.fetch import RepoFetcher
3410 # See <https://launchpad.net/bugs/456077> asking for a warning here
3411 if self.source._format.network_name() != self.target._format.network_name():
3412 ui.ui_factory.show_user_warning('cross_format_fetch',
3413 from_format=self.source._format,
3414 to_format=self.target._format)
3410 f = RepoFetcher(to_repository=self.target,3415 f = RepoFetcher(to_repository=self.target,
3411 from_repository=self.source,3416 from_repository=self.source,
3412 last_revision=revision_id,3417 last_revision=revision_id,
@@ -3980,18 +3985,17 @@
3980 """See InterRepository.fetch()."""3985 """See InterRepository.fetch()."""
3981 if fetch_spec is not None:3986 if fetch_spec is not None:
3982 raise AssertionError("Not implemented yet...")3987 raise AssertionError("Not implemented yet...")
3983 # See <https://launchpad.net/bugs/456077> asking for a warning here
3984 #
3985 # nb this is only active for local-local fetches; other things using
3986 # streaming.
3987 ui.ui_factory.warn_cross_format_fetch(self.source._format,
3988 self.target._format)
3989 if (not self.source.supports_rich_root()3988 if (not self.source.supports_rich_root()
3990 and self.target.supports_rich_root()):3989 and self.target.supports_rich_root()):
3991 self._converting_to_rich_root = True3990 self._converting_to_rich_root = True
3992 self._revision_id_to_root_id = {}3991 self._revision_id_to_root_id = {}
3993 else:3992 else:
3994 self._converting_to_rich_root = False3993 self._converting_to_rich_root = False
3994 # See <https://launchpad.net/bugs/456077> asking for a warning here
3995 if self.source._format.network_name() != self.target._format.network_name():
3996 ui.ui_factory.show_user_warning('cross_format_fetch',
3997 from_format=self.source._format,
3998 to_format=self.target._format)
3995 revision_ids = self.target.search_missing_revision_ids(self.source,3999 revision_ids = self.target.search_missing_revision_ids(self.source,
3996 revision_id, find_ghosts=find_ghosts).get_keys()4000 revision_id, find_ghosts=find_ghosts).get_keys()
3997 if not revision_ids:4001 if not revision_ids:
@@ -4284,8 +4288,6 @@
4284 self._extract_and_insert_inventories(4288 self._extract_and_insert_inventories(
4285 substream, src_serializer)4289 substream, src_serializer)
4286 elif substream_type == 'inventory-deltas':4290 elif substream_type == 'inventory-deltas':
4287 ui.ui_factory.warn_cross_format_fetch(src_format,
4288 self.target_repo._format)
4289 self._extract_and_insert_inventory_deltas(4291 self._extract_and_insert_inventory_deltas(
4290 substream, src_serializer)4292 substream, src_serializer)
4291 elif substream_type == 'chk_bytes':4293 elif substream_type == 'chk_bytes':
42924294
=== 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 @@
503 yield pack_writer.begin()503 yield pack_writer.begin()
504 yield pack_writer.bytes_record(src_format.network_name(), '')504 yield pack_writer.bytes_record(src_format.network_name(), '')
505 for substream_type, substream in stream:505 for substream_type, substream in stream:
506 if substream_type == 'inventory-deltas':
507 # This doesn't feel like the ideal place to issue this warning;
508 # however we don't want to do it in the Repository that's
509 # generating the stream, because that might be on the server.
510 # Instead we try to observe it as the stream goes by.
511 ui.ui_factory.warn_cross_format_fetch(src_format,
512 '(remote)')
513 for record in substream:506 for record in substream:
514 if record.storage_kind in ('chunked', 'fulltext'):507 if record.storage_kind in ('chunked', 'fulltext'):
515 serialised = record_to_fulltext_bytes(record)508 serialised = record_to_fulltext_bytes(record)
516509
=== 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 @@
326 ' Packs 5 (adds stacking support, requires bzr 1.6)\n'326 ' Packs 5 (adds stacking support, requires bzr 1.6)\n'
327 'Source branch format does not support stacking, using format:\n'327 'Source branch format does not support stacking, using format:\n'
328 ' Branch format 7\n'328 ' Branch format 7\n'
329 'Doing on-the-fly conversion from RepositoryFormatKnitPack1() to RepositoryFormatKnitPack5().\n'
330 'This may take some time. Upgrade the repositories to the same format for better performance.\n'
329 'Created new stacked branch referring to %s.\n' % (trunk.base,),331 'Created new stacked branch referring to %s.\n' % (trunk.base,),
330 err)332 err)
331333
@@ -339,6 +341,8 @@
339 ' Packs 5 rich-root (adds stacking support, requires bzr 1.6.1)\n'341 ' Packs 5 rich-root (adds stacking support, requires bzr 1.6.1)\n'
340 'Source branch format does not support stacking, using format:\n'342 'Source branch format does not support stacking, using format:\n'
341 ' Branch format 7\n'343 ' Branch format 7\n'
344 'Doing on-the-fly conversion from RepositoryFormatKnitPack4() to RepositoryFormatKnitPack5RichRoot().\n'
345 'This may take some time. Upgrade the repositories to the same format for better performance.\n'
342 'Created new stacked branch referring to %s.\n' % (trunk.base,),346 'Created new stacked branch referring to %s.\n' % (trunk.base,),
343 err)347 err)
344348
345349
=== 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 @@
1# Copyright (C) 2005, 2008, 2009 Canonical Ltd1# Copyright (C) 2005, 2008, 2009, 2010 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -25,6 +25,8 @@
2525
26from bzrlib import (26from bzrlib import (
27 errors,27 errors,
28 remote,
29 repository,
28 tests,30 tests,
29 ui as _mod_ui,31 ui as _mod_ui,
30 )32 )
@@ -300,6 +302,32 @@
300 finally:302 finally:
301 pb.finished()303 pb.finished()
302304
305 def test_text_ui_show_user_warning(self):
306 from bzrlib.repofmt.groupcompress_repo import RepositoryFormat2a
307 from bzrlib.repofmt.pack_repo import RepositoryFormatKnitPack5
308 err = StringIO()
309 out = StringIO()
310 ui = tests.TextUIFactory(stdin=None, stdout=out, stderr=err)
311 remote_fmt = remote.RemoteRepositoryFormat()
312 remote_fmt._network_name = RepositoryFormatKnitPack5().network_name()
313 ui.show_user_warning('cross_format_fetch', from_format=RepositoryFormat2a(),
314 to_format=remote_fmt)
315 self.assertEquals('', out.getvalue())
316 self.assertEquals("Doing on-the-fly conversion from RepositoryFormat2a() to "
317 "RemoteRepositoryFormat(_network_name='Bazaar RepositoryFormatKnitPack5 "
318 "(bzr 1.6)\\n').\nThis may take some time. Upgrade the repositories to "
319 "the same format for better performance.\n",
320 err.getvalue())
321 # and now with it suppressed please
322 err = StringIO()
323 out = StringIO()
324 ui = tests.TextUIFactory(stdin=None, stdout=out, stderr=err)
325 ui.suppressed_warnings.add('cross_format_fetch')
326 ui.show_user_warning('cross_format_fetch', from_format=RepositoryFormat2a(),
327 to_format=remote_fmt)
328 self.assertEquals('', out.getvalue())
329 self.assertEquals('', err.getvalue())
330
303331
304class CLIUITests(TestCase):332class CLIUITests(TestCase):
305333
306334
=== 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 @@
105105
106 This tells the library how to display things to the user. Through this106 This tells the library how to display things to the user. Through this
107 layer different applications can choose the style of UI.107 layer different applications can choose the style of UI.
108
109 :ivar suppressed_warnings: Identifiers for user warnings that should
110 no be emitted.
108 """111 """
109112
113 _user_warning_templates = dict(
114 cross_format_fetch=("Doing on-the-fly conversion from "
115 "%(from_format)s to %(to_format)s.\n"
116 "This may take some time. Upgrade the repositories to the "
117 "same format for better performance."
118 )
119 )
120
110 def __init__(self):121 def __init__(self):
111 self._task_stack = []122 self._task_stack = []
123 self.suppressed_warnings = set()
112124
113 def get_password(self, prompt='', **kwargs):125 def get_password(self, prompt='', **kwargs):
114 """Prompt the user for a password.126 """Prompt the user for a password.
@@ -170,6 +182,21 @@
170 """182 """
171 pass183 pass
172184
185 def format_user_warning(self, warning_id, message_args):
186 try:
187 template = self._user_warning_templates[warning_id]
188 except KeyError:
189 fail = "failed to format warning %r, %r" % (warning_id, message_args)
190 warnings.warn(fail) # so tests will fail etc
191 return fail
192 try:
193 return template % message_args
194 except ValueError, e:
195 fail = "failed to format warning %r, %r: %s" % (
196 warning_id, message_args, e)
197 warnings.warn(fail) # so tests will fail etc
198 return fail
199
173 def get_boolean(self, prompt):200 def get_boolean(self, prompt):
174 """Get a boolean question answered from the user.201 """Get a boolean question answered from the user.
175202
@@ -190,8 +217,11 @@
190 def recommend_upgrade(self,217 def recommend_upgrade(self,
191 current_format_name,218 current_format_name,
192 basedir):219 basedir):
193 # this should perhaps be in the TextUIFactory and the default can do220 # XXX: this should perhaps be in the TextUIFactory and the default can do
194 # nothing221 # nothing
222 #
223 # XXX: Change to show_user_warning - that will accomplish the previous
224 # xxx. -- mbp 2010-02-25
195 trace.warning("%s is deprecated "225 trace.warning("%s is deprecated "
196 "and a better format is available.\n"226 "and a better format is available.\n"
197 "It is recommended that you upgrade by "227 "It is recommended that you upgrade by "
@@ -208,13 +238,32 @@
208 """238 """
209 pass239 pass
210240
241 def show_user_warning(self, warning_id, **message_args):
242 """Show a warning to the user.
243
244 This is specifically for things that are under the user's control (eg
245 outdated formats), not for internal program warnings like deprecated
246 APIs.
247
248 This can be overridden by UIFactory subclasses to show it in some
249 appropriate way; the default UIFactory is noninteractive and does
250 nothing. format_user_warning maps it to a string, though other
251 presentations can be used for particular UIs.
252
253 :param warning_id: An identifier like 'cross_format_fetch' used to
254 check if the message is suppressed and to look up the string.
255 :param message_args: Arguments to be interpolated into the message.
256 """
257 pass
258
211 def warn_cross_format_fetch(self, from_format, to_format):259 def warn_cross_format_fetch(self, from_format, to_format):
212 """Warn about a potentially slow cross-format transfer"""260 """Warn about a potentially slow cross-format transfer.
213 # See <https://launchpad.net/bugs/456077> asking for a warning here261
214 trace.warning("Doing on-the-fly conversion from %s to %s.\n"262 This is deprecated in favor of show_user_warning, but retained for api
215 "This may take some time. Upgrade the repositories to the "263 compatibility in 2.0 and 2.1.
216 "same format for better performance.\n" %264 """
217 (from_format, to_format))265 self.show_user_warning('cross_format_fetch', from_format=from_format,
266 to_format=to_format)
218267
219268
220class CLIUIFactory(UIFactory):269class CLIUIFactory(UIFactory):
221270
=== 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 @@
1# Copyright (C) 2005, 2008, 2009 Canonical Ltd1# Copyright (C) 2005, 2008, 2009, 2010 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -31,6 +31,7 @@
31 progress,31 progress,
32 osutils,32 osutils,
33 symbol_versioning,33 symbol_versioning,
34 trace,
34 )35 )
3536
36""")37""")
@@ -190,6 +191,18 @@
190 def _progress_all_finished(self):191 def _progress_all_finished(self):
191 self._progress_view.clear()192 self._progress_view.clear()
192193
194 def show_user_warning(self, warning_id, **message_args):
195 """Show a text message to the user.
196
197 Explicitly not for warnings about bzr apis, deprecations or internals.
198 """
199 # eventually trace.warning should migrate here, to avoid logging and
200 # be easier to test; that has a lot of test fallout so for now just
201 # new code can call this
202 if warning_id not in self.suppressed_warnings:
203 self.stderr.write(self.format_user_warning(warning_id, message_args) +
204 '\n')
205
193206
194class TextProgressView(object):207class TextProgressView(object):
195 """Display of progress bar and other information on a tty.208 """Display of progress bar and other information on a tty.
196209
=== 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 @@
1# Copyright (C) 2005, 2008, 2009 Canonical Ltd1# Copyright (C) 2005, 2008, 2009, 2010 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -29,6 +29,9 @@
29 def __init__(self, url, format=None):29 def __init__(self, url, format=None):
30 self.format = format30 self.format = format
31 self.bzrdir = BzrDir.open_unsupported(url)31 self.bzrdir = BzrDir.open_unsupported(url)
32 # XXX: Change to cleanup
33 warning_id = 'cross_format_fetch'
34 saved_warning = warning_id in ui.ui_factory.suppressed_warnings
32 if isinstance(self.bzrdir, RemoteBzrDir):35 if isinstance(self.bzrdir, RemoteBzrDir):
33 self.bzrdir._ensure_real()36 self.bzrdir._ensure_real()
34 self.bzrdir = self.bzrdir._real_bzrdir37 self.bzrdir = self.bzrdir._real_bzrdir
@@ -36,10 +39,13 @@
36 raise errors.UpgradeReadonly39 raise errors.UpgradeReadonly
37 self.transport = self.bzrdir.root_transport40 self.transport = self.bzrdir.root_transport
38 self.pb = ui.ui_factory.nested_progress_bar()41 self.pb = ui.ui_factory.nested_progress_bar()
42 ui.ui_factory.suppressed_warnings.add(warning_id)
39 try:43 try:
40 self.convert()44 self.convert()
41 finally:45 finally:
42 self.pb.finished()46 self.pb.finished()
47 if not saved_warning:
48 ui.ui_factory.suppressed_warnings.remove(warning_id)
4349
44 def convert(self):50 def convert(self):
45 try:51 try:

Subscribers

People subscribed via source and target branches