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
1=== modified file 'NEWS'
2--- NEWS 2010-03-03 09:21:47 +0000
3+++ NEWS 2010-03-17 04:31:23 +0000
4@@ -29,6 +29,12 @@
5 a lock.
6 (Martin Pool, #185103)
7
8+* Give the warning about potentially slow cross-format fetches much
9+ earlier on in the fetch operation. Don't show this message during
10+ upgrades, and show the correct format indication for remote
11+ repositories.
12+ (Martin Pool, #456077, #515356, #513157)
13+
14 * Handle renames correctly when there are files or directories that
15 differ only in case. (Chris Jones, Martin Pool, #368931)
16
17
18=== modified file 'bzrlib/repository.py'
19--- bzrlib/repository.py 2010-01-14 07:34:37 +0000
20+++ bzrlib/repository.py 2010-03-17 04:31:23 +0000
21@@ -3067,8 +3067,8 @@
22 # Does the repository inventory storage understand references to trees?
23 supports_tree_reference = None
24
25- def __str__(self):
26- return "<%s>" % self.__class__.__name__
27+ def __repr__(self):
28+ return "%s()" % self.__class__.__name__
29
30 def __eq__(self, other):
31 # format objects are generally stateless
32@@ -3407,6 +3407,11 @@
33 :return: None.
34 """
35 from bzrlib.fetch import RepoFetcher
36+ # See <https://launchpad.net/bugs/456077> asking for a warning here
37+ if self.source._format.network_name() != self.target._format.network_name():
38+ ui.ui_factory.show_user_warning('cross_format_fetch',
39+ from_format=self.source._format,
40+ to_format=self.target._format)
41 f = RepoFetcher(to_repository=self.target,
42 from_repository=self.source,
43 last_revision=revision_id,
44@@ -3980,18 +3985,17 @@
45 """See InterRepository.fetch()."""
46 if fetch_spec is not None:
47 raise AssertionError("Not implemented yet...")
48- # See <https://launchpad.net/bugs/456077> asking for a warning here
49- #
50- # nb this is only active for local-local fetches; other things using
51- # streaming.
52- ui.ui_factory.warn_cross_format_fetch(self.source._format,
53- self.target._format)
54 if (not self.source.supports_rich_root()
55 and self.target.supports_rich_root()):
56 self._converting_to_rich_root = True
57 self._revision_id_to_root_id = {}
58 else:
59 self._converting_to_rich_root = False
60+ # See <https://launchpad.net/bugs/456077> asking for a warning here
61+ if self.source._format.network_name() != self.target._format.network_name():
62+ ui.ui_factory.show_user_warning('cross_format_fetch',
63+ from_format=self.source._format,
64+ to_format=self.target._format)
65 revision_ids = self.target.search_missing_revision_ids(self.source,
66 revision_id, find_ghosts=find_ghosts).get_keys()
67 if not revision_ids:
68@@ -4284,8 +4288,6 @@
69 self._extract_and_insert_inventories(
70 substream, src_serializer)
71 elif substream_type == 'inventory-deltas':
72- ui.ui_factory.warn_cross_format_fetch(src_format,
73- self.target_repo._format)
74 self._extract_and_insert_inventory_deltas(
75 substream, src_serializer)
76 elif substream_type == 'chk_bytes':
77
78=== modified file 'bzrlib/smart/repository.py'
79--- bzrlib/smart/repository.py 2010-01-14 07:34:37 +0000
80+++ bzrlib/smart/repository.py 2010-03-17 04:31:23 +0000
81@@ -503,13 +503,6 @@
82 yield pack_writer.begin()
83 yield pack_writer.bytes_record(src_format.network_name(), '')
84 for substream_type, substream in stream:
85- if substream_type == 'inventory-deltas':
86- # This doesn't feel like the ideal place to issue this warning;
87- # however we don't want to do it in the Repository that's
88- # generating the stream, because that might be on the server.
89- # Instead we try to observe it as the stream goes by.
90- ui.ui_factory.warn_cross_format_fetch(src_format,
91- '(remote)')
92 for record in substream:
93 if record.storage_kind in ('chunked', 'fulltext'):
94 serialised = record_to_fulltext_bytes(record)
95
96=== modified file 'bzrlib/tests/blackbox/test_branch.py'
97--- bzrlib/tests/blackbox/test_branch.py 2009-08-20 12:30:49 +0000
98+++ bzrlib/tests/blackbox/test_branch.py 2010-03-17 04:31:23 +0000
99@@ -326,6 +326,8 @@
100 ' Packs 5 (adds stacking support, requires bzr 1.6)\n'
101 'Source branch format does not support stacking, using format:\n'
102 ' Branch format 7\n'
103+ 'Doing on-the-fly conversion from RepositoryFormatKnitPack1() to RepositoryFormatKnitPack5().\n'
104+ 'This may take some time. Upgrade the repositories to the same format for better performance.\n'
105 'Created new stacked branch referring to %s.\n' % (trunk.base,),
106 err)
107
108@@ -339,6 +341,8 @@
109 ' Packs 5 rich-root (adds stacking support, requires bzr 1.6.1)\n'
110 'Source branch format does not support stacking, using format:\n'
111 ' Branch format 7\n'
112+ 'Doing on-the-fly conversion from RepositoryFormatKnitPack4() to RepositoryFormatKnitPack5RichRoot().\n'
113+ 'This may take some time. Upgrade the repositories to the same format for better performance.\n'
114 'Created new stacked branch referring to %s.\n' % (trunk.base,),
115 err)
116
117
118=== modified file 'bzrlib/tests/test_ui.py'
119--- bzrlib/tests/test_ui.py 2009-08-03 05:54:52 +0000
120+++ bzrlib/tests/test_ui.py 2010-03-17 04:31:23 +0000
121@@ -1,4 +1,4 @@
122-# Copyright (C) 2005, 2008, 2009 Canonical Ltd
123+# Copyright (C) 2005, 2008, 2009, 2010 Canonical Ltd
124 #
125 # This program is free software; you can redistribute it and/or modify
126 # it under the terms of the GNU General Public License as published by
127@@ -25,6 +25,8 @@
128
129 from bzrlib import (
130 errors,
131+ remote,
132+ repository,
133 tests,
134 ui as _mod_ui,
135 )
136@@ -300,6 +302,32 @@
137 finally:
138 pb.finished()
139
140+ def test_text_ui_show_user_warning(self):
141+ from bzrlib.repofmt.groupcompress_repo import RepositoryFormat2a
142+ from bzrlib.repofmt.pack_repo import RepositoryFormatKnitPack5
143+ err = StringIO()
144+ out = StringIO()
145+ ui = tests.TextUIFactory(stdin=None, stdout=out, stderr=err)
146+ remote_fmt = remote.RemoteRepositoryFormat()
147+ remote_fmt._network_name = RepositoryFormatKnitPack5().network_name()
148+ ui.show_user_warning('cross_format_fetch', from_format=RepositoryFormat2a(),
149+ to_format=remote_fmt)
150+ self.assertEquals('', out.getvalue())
151+ self.assertEquals("Doing on-the-fly conversion from RepositoryFormat2a() to "
152+ "RemoteRepositoryFormat(_network_name='Bazaar RepositoryFormatKnitPack5 "
153+ "(bzr 1.6)\\n').\nThis may take some time. Upgrade the repositories to "
154+ "the same format for better performance.\n",
155+ err.getvalue())
156+ # and now with it suppressed please
157+ err = StringIO()
158+ out = StringIO()
159+ ui = tests.TextUIFactory(stdin=None, stdout=out, stderr=err)
160+ ui.suppressed_warnings.add('cross_format_fetch')
161+ ui.show_user_warning('cross_format_fetch', from_format=RepositoryFormat2a(),
162+ to_format=remote_fmt)
163+ self.assertEquals('', out.getvalue())
164+ self.assertEquals('', err.getvalue())
165+
166
167 class CLIUITests(TestCase):
168
169
170=== modified file 'bzrlib/ui/__init__.py'
171--- bzrlib/ui/__init__.py 2010-01-15 02:41:11 +0000
172+++ bzrlib/ui/__init__.py 2010-03-17 04:31:23 +0000
173@@ -105,10 +105,22 @@
174
175 This tells the library how to display things to the user. Through this
176 layer different applications can choose the style of UI.
177+
178+ :ivar suppressed_warnings: Identifiers for user warnings that should
179+ no be emitted.
180 """
181
182+ _user_warning_templates = dict(
183+ cross_format_fetch=("Doing on-the-fly conversion from "
184+ "%(from_format)s to %(to_format)s.\n"
185+ "This may take some time. Upgrade the repositories to the "
186+ "same format for better performance."
187+ )
188+ )
189+
190 def __init__(self):
191 self._task_stack = []
192+ self.suppressed_warnings = set()
193
194 def get_password(self, prompt='', **kwargs):
195 """Prompt the user for a password.
196@@ -170,6 +182,21 @@
197 """
198 pass
199
200+ def format_user_warning(self, warning_id, message_args):
201+ try:
202+ template = self._user_warning_templates[warning_id]
203+ except KeyError:
204+ fail = "failed to format warning %r, %r" % (warning_id, message_args)
205+ warnings.warn(fail) # so tests will fail etc
206+ return fail
207+ try:
208+ return template % message_args
209+ except ValueError, e:
210+ fail = "failed to format warning %r, %r: %s" % (
211+ warning_id, message_args, e)
212+ warnings.warn(fail) # so tests will fail etc
213+ return fail
214+
215 def get_boolean(self, prompt):
216 """Get a boolean question answered from the user.
217
218@@ -190,8 +217,11 @@
219 def recommend_upgrade(self,
220 current_format_name,
221 basedir):
222- # this should perhaps be in the TextUIFactory and the default can do
223+ # XXX: this should perhaps be in the TextUIFactory and the default can do
224 # nothing
225+ #
226+ # XXX: Change to show_user_warning - that will accomplish the previous
227+ # xxx. -- mbp 2010-02-25
228 trace.warning("%s is deprecated "
229 "and a better format is available.\n"
230 "It is recommended that you upgrade by "
231@@ -208,13 +238,32 @@
232 """
233 pass
234
235+ def show_user_warning(self, warning_id, **message_args):
236+ """Show a warning to the user.
237+
238+ This is specifically for things that are under the user's control (eg
239+ outdated formats), not for internal program warnings like deprecated
240+ APIs.
241+
242+ This can be overridden by UIFactory subclasses to show it in some
243+ appropriate way; the default UIFactory is noninteractive and does
244+ nothing. format_user_warning maps it to a string, though other
245+ presentations can be used for particular UIs.
246+
247+ :param warning_id: An identifier like 'cross_format_fetch' used to
248+ check if the message is suppressed and to look up the string.
249+ :param message_args: Arguments to be interpolated into the message.
250+ """
251+ pass
252+
253 def warn_cross_format_fetch(self, from_format, to_format):
254- """Warn about a potentially slow cross-format transfer"""
255- # See <https://launchpad.net/bugs/456077> asking for a warning here
256- trace.warning("Doing on-the-fly conversion from %s to %s.\n"
257- "This may take some time. Upgrade the repositories to the "
258- "same format for better performance.\n" %
259- (from_format, to_format))
260+ """Warn about a potentially slow cross-format transfer.
261+
262+ This is deprecated in favor of show_user_warning, but retained for api
263+ compatibility in 2.0 and 2.1.
264+ """
265+ self.show_user_warning('cross_format_fetch', from_format=from_format,
266+ to_format=to_format)
267
268
269 class CLIUIFactory(UIFactory):
270
271=== modified file 'bzrlib/ui/text.py'
272--- bzrlib/ui/text.py 2009-08-06 02:23:37 +0000
273+++ bzrlib/ui/text.py 2010-03-17 04:31:23 +0000
274@@ -1,4 +1,4 @@
275-# Copyright (C) 2005, 2008, 2009 Canonical Ltd
276+# Copyright (C) 2005, 2008, 2009, 2010 Canonical Ltd
277 #
278 # This program is free software; you can redistribute it and/or modify
279 # it under the terms of the GNU General Public License as published by
280@@ -31,6 +31,7 @@
281 progress,
282 osutils,
283 symbol_versioning,
284+ trace,
285 )
286
287 """)
288@@ -190,6 +191,18 @@
289 def _progress_all_finished(self):
290 self._progress_view.clear()
291
292+ def show_user_warning(self, warning_id, **message_args):
293+ """Show a text message to the user.
294+
295+ Explicitly not for warnings about bzr apis, deprecations or internals.
296+ """
297+ # eventually trace.warning should migrate here, to avoid logging and
298+ # be easier to test; that has a lot of test fallout so for now just
299+ # new code can call this
300+ if warning_id not in self.suppressed_warnings:
301+ self.stderr.write(self.format_user_warning(warning_id, message_args) +
302+ '\n')
303+
304
305 class TextProgressView(object):
306 """Display of progress bar and other information on a tty.
307
308=== modified file 'bzrlib/upgrade.py'
309--- bzrlib/upgrade.py 2009-05-07 05:08:46 +0000
310+++ bzrlib/upgrade.py 2010-03-17 04:31:23 +0000
311@@ -1,4 +1,4 @@
312-# Copyright (C) 2005, 2008, 2009 Canonical Ltd
313+# Copyright (C) 2005, 2008, 2009, 2010 Canonical Ltd
314 #
315 # This program is free software; you can redistribute it and/or modify
316 # it under the terms of the GNU General Public License as published by
317@@ -29,6 +29,9 @@
318 def __init__(self, url, format=None):
319 self.format = format
320 self.bzrdir = BzrDir.open_unsupported(url)
321+ # XXX: Change to cleanup
322+ warning_id = 'cross_format_fetch'
323+ saved_warning = warning_id in ui.ui_factory.suppressed_warnings
324 if isinstance(self.bzrdir, RemoteBzrDir):
325 self.bzrdir._ensure_real()
326 self.bzrdir = self.bzrdir._real_bzrdir
327@@ -36,10 +39,13 @@
328 raise errors.UpgradeReadonly
329 self.transport = self.bzrdir.root_transport
330 self.pb = ui.ui_factory.nested_progress_bar()
331+ ui.ui_factory.suppressed_warnings.add(warning_id)
332 try:
333 self.convert()
334 finally:
335 self.pb.finished()
336+ if not saved_warning:
337+ ui.ui_factory.suppressed_warnings.remove(warning_id)
338
339 def convert(self):
340 try:

Subscribers

People subscribed via source and target branches