[MERGE] Default InterRepository.fetch raises IncompatibleRepositories
John Arbash Meinel
john at arbash-meinel.com
Mon Jul 28 23:56:19 BST 2008
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Martin Pool wrote:
| https://bugs.edge.launchpad.net/bzr/+bug/206258
| incompatible repository error messages are unhelpful [edit]
|
| This raises a better error when failing to fetch from one repository
to the
| other, tests it, and adds the start of some documentation about
InterObject.
|
| A couple of general points:
|
| I think we should reserve NotImplementedError for cases where we intend to
| write the code and it's not done - comparable to an AssertionError. It is
| potentially confusing to also use it for expected conditions.
|
| Also, when we're doing a 'check' type operation I think it may be
better to
| have the core code raise an exception if the condition is not true,
and then
| possibly there can be a convenience method to turn it into a boolean.
If the
| api is defined as just a boolean it's hard to pass back extra information.
| (There are tradeoffs; this is just a thought not a hard-and-fast rule.)
|
|
| ------------------------------------------------------------------------
|
| # Bazaar merge directive format 2 (Bazaar 0.90)
| # revision_id: mbp at sourcefrog.net-20080728084303-rjroyiv5oe2jto26
| # target_branch: http://bazaar-vcs.org/bzr/bzr.dev
| # testament_sha1: 23ef9745080c880a0feaa2b483ae53bf795e30a5
| # timestamp: 2008-07-28 19:56:00 +1000
| # source_branch: http://sourcefrog.net/bzr/check_compatible
| # base_revision_id: pqm at pqm.ubuntu.com-20080728024856-nbikndmfq06firuo
| #
| # Begin patch
| === modified file 'bzrlib/errors.py'
| --- bzrlib/errors.py 2008-07-25 09:39:26 +0000
| +++ bzrlib/errors.py 2008-07-28 08:43:03 +0000
| @@ -774,11 +774,15 @@
|
| class IncompatibleRepositories(BzrError):
|
| - _fmt = "Repository %(target)s is not compatible with repository"\
| - " %(source)s"
| + _fmt = "%(target)s\n" \
| + "is not compatible with\n" \
| + "%(source)s\n" \
| + "%(details)s"
|
v- This should clearly be "if details *is* None"
| - def __init__(self, source, target):
| - BzrError.__init__(self, target=target, source=source)
| + def __init__(self, source, target, details=None):
| + if details == None:
| + details = "(no details)"
| + BzrError.__init__(self, target=target, source=source,
details=details)
|
|
| class IncompatibleRevision(BzrError):
|
| === modified file 'bzrlib/repository.py'
| --- bzrlib/repository.py 2008-07-22 09:54:03 +0000
| +++ bzrlib/repository.py 2008-07-28 08:43:03 +0000
| @@ -2350,10 +2350,16 @@
| :param pb: optional progress bar to use for progress reports.
If not
| provided a default one will be created.
|
| - Returns the copied revision count and the failed revisions in
a tuple:
| - (copied, failures).
| + :returns: (copied_revision_count, failures).
| """
| - raise NotImplementedError(self.fetch)
| + # Normally we should find a specific InterRepository subclass
to do
| + # the fetch; if nothing else then at least
InterSameDataRepository.
| + # If none of them is suitable it looks like fetching is not
possible;
| + # we try to give a good message why. _assert_same_model will
probably
| + # give a helpful message; otherwise a generic one.
| + self._assert_same_model(self.source, self.target)
| + raise errors.IncompatibleRepositories(self.source, self.target,
| + "no suitableInterRepository found")
|
| def _walk_to_common_revisions(self, revision_ids):
| """Walk out from revision_ids in source to revisions target has.
| @@ -2433,12 +2439,27 @@
|
| @staticmethod
| def _same_model(source, target):
| - """True if source and target have the same data
representation."""
| + """True if source and target have the same data representation.
| +
| + Note: this is always called on the base class; overriding it in a
| + subclass will have no effect.
| + """
| + try:
| + InterRepository._assert_same_model(source, target)
| + return True
| + except errors.IncompatibleRepositories, e:
| + return False
| +
| + @staticmethod
| + def _assert_same_model(source, target):
| + """Raise an exception if two repositories do not use the same
model.
| + """
| if source.supports_rich_root() != target.supports_rich_root():
| - return False
| + raise errors.IncompatibleRepositories(source, target,
| + "different rich-root support")
| if source._serializer != target._serializer:
| - return False
| - return True
| + raise errors.IncompatibleRepositories(source, target,
| + "different serializers")
|
^- This seems like it might be better layered as _assert_same_model
raising an exception based on the return value of _same_model. So rather
than catching the exception and returning False, you do:
if not ..._same_model():
~ raise IncompatibleRepositories
Is it just that _assert has better details about *why*? You could return
that from _same_model.
|
| class InterSameDataRepository(InterRepository):
|
| === modified file 'bzrlib/tests/test_fetch.py'
| --- bzrlib/tests/test_fetch.py 2008-06-11 04:20:16 +0000
| +++ bzrlib/tests/test_fetch.py 2008-07-28 08:43:03 +0000
| @@ -170,8 +170,11 @@
| knit3_tree = self.make_branch_and_tree('knit3',
| format='dirstate-with-subtree')
| knit3_tree.commit('blah')
| - self.assertRaises(errors.IncompatibleRepositories,
| - knit_tree.branch.fetch, knit3_tree.branch)
| + e = self.assertRaises(errors.IncompatibleRepositories,
| + knit_tree.branch.fetch, knit3_tree.branch)
| + self.assertContainsRe(str(e),
| + r"(?m).*/knit.*\nis not compatible with\n.*/knit3/.*\n"
| + r"different rich-root support")
|
|
| class TestMergeFetch(TestCaseWithTransport):
|
| === modified file 'doc/developers/HACKING.txt'
| --- doc/developers/HACKING.txt 2008-07-15 05:06:13 +0000
| +++ doc/developers/HACKING.txt 2008-07-28 07:20:06 +0000
| @@ -895,6 +895,27 @@
| associated information such as a help string or description.
|
|
| +InterObject and multiple dispatch
| +=================================
| +
| +The ``InterObject`` provides for two-way `multiple dispatch`__: matching
| +up for example a source and destination repository to find the right way
| +to transfer data between them.
| +
| +There is a subclass ``InterObject`` classes for each type of object
that is
| +dispatched this way, e.g. ``InterRepository``. Calling ``.get()`` on
this
| +class will return an ``InterObject`` instance providing the best
match for
| +those parameters, and this instance then has methods for operations
| +between the objects.
| +
| + inter = InterRepository.get(source_repo, target_repo)
| + inter.fetch(revision_id)
| +
| +``InterRepository`` also acts as a registry-like object for its
| +subclasses, and they can be added through ``.register_optimizer``. The
| +right one to run is selected by asking each class, in reverse order of
| +registration, whether it ``.is_compatible`` with the relevant objects.
| +
| Lazy Imports
| ============
BB:tweak
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkiOTpMACgkQJdeBCYSNAAPpkgCgh6QwxLCQ7mP4NbRaVKH0EuYT
p3kAn3Na9K1VeTZ9JPzyWJ7nWnpG8n7T
=wN0N
-----END PGP SIGNATURE-----
More information about the bazaar
mailing list