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: 188 lines (+69/-21)
5 files modified
NEWS (+3/-3)
bzrlib/repository.py (+8/-7)
bzrlib/smart/repository.py (+9/-1)
bzrlib/tests/blackbox/test_pull.py (+41/-9)
bzrlib/ui/__init__.py (+8/-1)
To merge this branch: bzr merge lp:~mbp/bzr/456077-cross-format-fetch
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+17354@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

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

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

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

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'NEWS'
--- NEWS 2010-01-13 16:31:34 +0000
+++ NEWS 2010-01-15 03:36:28 +0000
@@ -41,10 +41,10 @@
41* Give a clearer message if the lockdir disappears after being apparently41* Give a clearer message if the lockdir disappears after being apparently
42 successfully taken. (Martin Pool, #498378)42 successfully taken. (Martin Pool, #498378)
4343
44* Give a warning when fetching between local repositories with44* Give a warning when fetching between repositories (local or remote) with
45 sufficiently different formats that the content will need to be45 sufficiently different formats that the content will need to be
46 serialized (ie ``InterDifferingSerializer``) so the user has a clue that46 serialized (ie ``InterDifferingSerializer`` or ``inventory-deltas``), so
47 upgrading could make it faster.47 the user has a clue that upgrading could make it faster.
48 (Martin Pool, #456077)48 (Martin Pool, #456077)
4949
50* If we fail to open ``~/.bzr.log`` write a clear message to stderr rather50* If we fail to open ``~/.bzr.log`` write a clear message to stderr rather
5151
=== 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 @@
3984 #3984 #
3985 # nb this is only active for local-local fetches; other things using3985 # nb this is only active for local-local fetches; other things using
3986 # streaming.3986 # streaming.
3987 trace.warning("Fetching between repositories with different formats\n"3987 ui.ui_factory.warn_cross_format_fetch(self.source._format,
3988 "from %s to %s.\n"3988 self.target._format)
3989 "This may take some time. Upgrade the branches to the same format \n"
3990 "for better results.\n"
3991 % (self.source._format, self.target._format))
3992 if (not self.source.supports_rich_root()3989 if (not self.source.supports_rich_root()
3993 and self.target.supports_rich_root()):3990 and self.target.supports_rich_root()):
3994 self._converting_to_rich_root = True3991 self._converting_to_rich_root = True
@@ -4287,6 +4284,8 @@
4287 self._extract_and_insert_inventories(4284 self._extract_and_insert_inventories(
4288 substream, src_serializer)4285 substream, src_serializer)
4289 elif substream_type == 'inventory-deltas':4286 elif substream_type == 'inventory-deltas':
4287 ui.ui_factory.warn_cross_format_fetch(src_format,
4288 self.target_repo._format)
4290 self._extract_and_insert_inventory_deltas(4289 self._extract_and_insert_inventory_deltas(
4291 substream, src_serializer)4290 substream, src_serializer)
4292 elif substream_type == 'chk_bytes':4291 elif substream_type == 'chk_bytes':
@@ -4591,8 +4590,10 @@
45914590
4592 def _get_convertable_inventory_stream(self, revision_ids,4591 def _get_convertable_inventory_stream(self, revision_ids,
4593 delta_versus_null=False):4592 delta_versus_null=False):
4594 # The source is using CHKs, but the target either doesn't or it has a4593 # The two formats are sufficiently different that there is no fast
4595 # different serializer. The StreamSink code expects to be able to4594 # path, so we need to send just inventorydeltas, which any
4595 # sufficiently modern client can insert into any repository.
4596 # The StreamSink code expects to be able to
4596 # convert on the target, so we need to put bytes-on-the-wire that can4597 # convert on the target, so we need to put bytes-on-the-wire that can
4597 # be converted. That means inventory deltas (if the remote is <1.19,4598 # be converted. That means inventory deltas (if the remote is <1.19,
4598 # RemoteStreamSink will fallback to VFS to insert the deltas).4599 # RemoteStreamSink will fallback to VFS to insert the deltas).
45994600
=== 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 @@
1# Copyright (C) 2006, 2007 Canonical Ltd1# Copyright (C) 2006, 2007, 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
@@ -30,6 +30,7 @@
30 graph,30 graph,
31 osutils,31 osutils,
32 pack,32 pack,
33 ui,
33 versionedfile,34 versionedfile,
34 )35 )
35from bzrlib.bzrdir import BzrDir36from bzrlib.bzrdir import BzrDir
@@ -502,6 +503,13 @@
502 yield pack_writer.begin()503 yield pack_writer.begin()
503 yield pack_writer.bytes_record(src_format.network_name(), '')504 yield pack_writer.bytes_record(src_format.network_name(), '')
504 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)')
505 for record in substream:513 for record in substream:
506 if record.storage_kind in ('chunked', 'fulltext'):514 if record.storage_kind in ('chunked', 'fulltext'):
507 serialised = record_to_fulltext_bytes(record)515 serialised = record_to_fulltext_bytes(record)
508516
=== 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 @@
1# Copyright (C) 2005-2010 Canonical Ltd1# Copyright (C) 2005, 2006, 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
@@ -20,13 +20,18 @@
20import os20import os
21import sys21import sys
2222
23from bzrlib import (
24 debug,
25 remote,
26 urlutils,
27 )
28
23from bzrlib.branch import Branch29from bzrlib.branch import Branch
24from bzrlib.directory_service import directories30from bzrlib.directory_service import directories
25from bzrlib.osutils import pathjoin31from bzrlib.osutils import pathjoin
26from bzrlib.tests.blackbox import ExternalBase32from bzrlib.tests.blackbox import ExternalBase
27from bzrlib.uncommit import uncommit33from bzrlib.uncommit import uncommit
28from bzrlib.workingtree import WorkingTree34from bzrlib.workingtree import WorkingTree
29from bzrlib import urlutils
3035
3136
32class TestPull(ExternalBase):37class TestPull(ExternalBase):
@@ -394,10 +399,37 @@
394 def test_pull_cross_format_warning(self):399 def test_pull_cross_format_warning(self):
395 """You get a warning for probably slow cross-format pulls.400 """You get a warning for probably slow cross-format pulls.
396 """401 """
397402 # this is assumed to be going through InterDifferingSerializer
398 from_tree = self.make_branch_and_tree('from', format='2a')403 from_tree = self.make_branch_and_tree('from', format='2a')
399 to_tree = self.make_branch_and_tree('to', format='1.14-rich-root')404 to_tree = self.make_branch_and_tree('to', format='1.14-rich-root')
400 from_tree.commit(message='first commit')405 from_tree.commit(message='first commit')
401 out, err = self.run_bzr(['pull', '-d', 'to', 'from'])406 out, err = self.run_bzr(['pull', '-d', 'to', 'from'])
402 self.assertContainsRe(err,407 self.assertContainsRe(err,
403 "(?m)Fetching between repositories with different formats.*")408 "(?m)Doing on-the-fly conversion")
409
410 def test_pull_cross_format_warning_no_IDS(self):
411 """You get a warning for probably slow cross-format pulls.
412 """
413 # this simulates what would happen across the network, where
414 # interdifferingserializer is not active
415
416 debug.debug_flags.add('IDS_never')
417 # TestCase take care of restoring them
418
419 from_tree = self.make_branch_and_tree('from', format='2a')
420 to_tree = self.make_branch_and_tree('to', format='1.14-rich-root')
421 from_tree.commit(message='first commit')
422 out, err = self.run_bzr(['pull', '-d', 'to', 'from'])
423 self.assertContainsRe(err,
424 "(?m)Doing on-the-fly conversion")
425
426 def test_pull_cross_format_from_network(self):
427 self.setup_smart_server_with_call_log()
428 from_tree = self.make_branch_and_tree('from', format='2a')
429 to_tree = self.make_branch_and_tree('to', format='1.14-rich-root')
430 self.assertIsInstance(from_tree.branch, remote.RemoteBranch)
431 from_tree.commit(message='first commit')
432 out, err = self.run_bzr(['pull', '-d', 'to',
433 from_tree.branch.bzrdir.root_transport.base])
434 self.assertContainsRe(err,
435 "(?m)Doing on-the-fly conversion")
404436
=== 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 @@
1# Copyright (C) 2005, 2006, 2007, 2008, 2009 Canonical Ltd1# Copyright (C) 2005-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
@@ -208,6 +208,13 @@
208 """208 """
209 pass209 pass
210210
211 def warn_cross_format_fetch(self, from_format, to_format):
212 """Warn about a potentially slow cross-format transfer"""
213 # See <https://launchpad.net/bugs/456077> asking for a warning here
214 trace.warning("Doing on-the-fly conversion from %s to %s.\n"
215 "This may take some time. Upgrade the repositories to the "
216 "same format for better performance.\n" %
217 (from_format, to_format))
211218
212219
213class CLIUIFactory(UIFactory):220class CLIUIFactory(UIFactory):

Subscribers

People subscribed via source and target branches