Merge lp:~jelmer/launchpad/bzr-code-imports into lp:launchpad

[Jelmer Vernooij]

[r=mwhudson][no-qa] Add support for importing Bazaar branches using the code import mechanism.

Add support for importing code from Bazaar branches.

At the moment mirrors of remote Bazaar branches are created with completely different infrastructure as the code imports. This is confusing for users (bug 611837) and duplicates a lot of code. Several features are only available for code imports (bug 362622, bug 193607, bug 193607, bug 371484) and vice versa. Having shared infrastructure would also make it easier to fix several open bugs that affect both code imports and code mirrors (bug 519159, bug 136939)

Code imports are a bit heavier than mirrors at the moment, as they run on a separate machine and require an extra copy of the branch that is imported.

This branch only adds backend support for Bazaar branches, it does not yet add a UI which can add code imports of this kind nor does it migrate any of the existing code mirrors.

[Gavin Panella (allenap)]

I don't know the ins and outs of bzrlib or codehosting - for that I
guess that's why you've asked Michael to review - but the rest looks
good.

[1]

+ def test_partial(self):
+ # Skip tests of partial tests, as they are disabled for native imports
+ # at the moment.
+ return

You could use TestCase.skip() here:

    def test_partial(self):
        self.skip("Disabled for native imports at the moment.")

review: Approve

[Jelmer Vernooij (jelmer)]

> I don't know the ins and outs of bzrlib or codehosting - for that I
> guess that's why you've asked Michael to review - but the rest looks
> good.
Thanks :)

I'm pretty confident about this code change (especially since nothing actually triggers the new code yet), but I'd like to get some feedback from more people (Michael, Jono?, Rob?) on to confirm this is the right direction to go in.

> + def test_partial(self):
> + # Skip tests of partial tests, as they are disabled for native
> imports
> + # at the moment.
> + return
>
> You could use TestCase.skip() here:
>
> def test_partial(self):
> self.skip("Disabled for native imports at the moment.")
I looked for things that raised SkipTest or TestSkipped and couldn't find any. I didn't know about TestCase.skip - thanks, fixed.

[Michael Hudson-Doyle (mwhudson)]

Some random comments: it would have been nice to see the things you had to do wrt the bzr upgrade

The fact that you put this line in suggests that the tests aren't very well isolated:

        branch.get_config().set_user_option("create_signatures", "never")

Does something need to inherit from bzrlib's TestCase? It's all a complete tangle though.

Is simply prohibiting branch references the right thing to do? The branch puller goes to some lengths to support them safely -- being able to dump all that code would surely be very nice. Relatedly, and on thinking about it I think this is a bit more serious, I think you might need to be careful about stacking -- it's contrived but you might be able to construct a branch that was stacked on some DC-internal branch and have that be imported so you can grab it.

In terms of overall direction, I'm all for removing duplication. I think the UI will require some effort (e.g. do we want to count bzr mirrors as "imported branches" on the code.launchpad.net frontpage?) but engineering wise, this looks fine, modulo the above comments.

review: Approve

[Michael Hudson-Doyle (mwhudson)]

"Some random comments: it would have been nice to see the things you had to do wrt the bzr upgrade " ... in a separate branch, I meant to say.

[Jelmer Vernooij (jelmer)]

On 06/24/2011 05:35 AM, Michael Hudson-Doyle wrote:
> Review: Approve
> Some random comments: it would have been nice to see the things you had to do wrt the bzr upgrade
This branch has two prerequisite branches, but I can only set one in
Launchpad. Hopefully the diff will get smaller when the update to the
newer bzr lands on lp:launchpad...
>
> The fact that you put this line in suggests that the tests aren't very well isolated:
>
> branch.get_config().set_user_option("create_signatures", "never")
>
> Does something need to inherit from bzrlib's TestCase? It's all a complete tangle though.
Perhaps; bzr's TestCase does a lot though, and I'm kindof worried that
mixing it in will break other things. It should do more than just this
ad-hoc override though. Perhaps reset the global bzr configuration in setUp?

>
> Is simply prohibiting branch references the right thing to do? The branch puller goes to some lengths to support them safely -- being able to dump all that code would surely be very nice. Relatedly, and on thinking about it I think this is a bit more serious, I think you might need to be careful about stacking -- it's contrived but you might be able to construct a branch that was stacked on some DC-internal branch and have that be imported so you can grab it.
Stacking is a very good point, and one that I had not considered -
thanks. I should probably also have another, closer, look at the branch
puller to see what it does and why.

For branch references, the easiest thing to do for the moment seemed to
be to just refuse to mirror them. If code mirrors support branch
references at the moment, we should keep that support.

>
> In terms of overall direction, I'm all for removing duplication. I think the UI will require some effort (e.g. do we want to count bzr mirrors as "imported branches" on the code.launchpad.net frontpage?) but engineering wise, this looks fine, modulo the above comments.
Thanks for having a look at this. I'm only just getting into this code
and am not very familiar with it yet.

Cheers,

Jelmer

[Michael Hudson-Doyle (mwhudson)]

On Fri, 24 Jun 2011 19:55:45 +0200, Jelmer Vernooij <email address hidden> wrote:
> On 06/24/2011 05:35 AM, Michael Hudson-Doyle wrote:
> > Review: Approve
> > Some random comments: it would have been nice to see the things you had to do wrt the bzr upgrade
> This branch has two prerequisite branches, but I can only set one in
> Launchpad. Hopefully the diff will get smaller when the update to the
> newer bzr lands on lp:launchpad...

Ah! I guess you could have created a branch with both of the
prerequisites merged in, but that's getting pretty tedious...

> >
> > The fact that you put this line in suggests that the tests aren't very well isolated:
> >
> > branch.get_config().set_user_option("create_signatures", "never")
> >
> > Does something need to inherit from bzrlib's TestCase? It's all a complete tangle though.
> Perhaps; bzr's TestCase does a lot though, and I'm kindof worried that
> mixing it in will break other things.

I'd be surprised if it broke other stuff on past experience, but you
might be right.

> It should do more than just this ad-hoc override though. Perhaps reset
> the global bzr configuration in setUp?

I guess what you want is a test fixture that just does the environment
isolation in bzrlib you can reuse here... but yes, doing something more
generic than just clearing create_signatures would be better, I think.

> >
> > Is simply prohibiting branch references the right thing to do? The branch puller goes to some lengths to support them safely -- being able to dump all that code would surely be very nice. Relatedly, and on thinking about it I think this is a bit more serious, I think you might need to be careful about stacking -- it's contrived but you might be able to construct a branch that was stacked on some DC-internal branch and have that be imported so you can grab it.
> Stacking is a very good point, and one that I had not considered -
> thanks. I should probably also have another, closer, look at the branch
> puller to see what it does and why.

For reasons I should probably be ashamed of, there seem to be two
implementations of 'safe opening' in Launchpad; one is around
lp.codehosting.bzrutils.safe_open and the other around
lp.codehosting.puller.worker.BranchMirrorer.

(looking at worker.py makes me realize how much code we can delete once
this branch is done, never mind how much can be deleted when the puller
is gone completely).

> For branch references, the easiest thing to do for the moment seemed to
> be to just refuse to mirror them. If code mirrors support branch
> references at the moment, we should keep that support.

Yeah, they do.

> >
> > In terms of overall direction, I'm all for removing duplication. I think the UI will require some effort (e.g. do we want to count bzr mirrors as "imported branches" on the code.launchpad.net frontpage?) but engineering wise, this looks fine, modulo the above comments.
> Thanks for having a look at this. I'm only just getting into this code
> and am not very familiar with it yet.

You seem to be doing fine :)

Cheers,
mwh

[Michael Hudson-Doyle (mwhudson)]

I don't see anything to do with stacking safety and/or supporting branch references in here yet -- is this really ready for review again?

[Jelmer Vernooij (jelmer)]

> I don't see anything to do with stacking safety and/or supporting branch
> references in here yet -- is this really ready for review again?
No, it indeed isn't - I was merely trying to generate a proper diff. Sorry.

[Robert Collins (lifeless)]

Does this permit imports from lp hosted branches? if so, thats likely to permit bypassing of private branch ACLs - can you check that that isn't the please?

review: Needs Information

[Gavin Panella (allenap)]

review: Abstain

[Jelmer Vernooij (jelmer)]

On Sun, 2011-08-07 at 22:41 +0000, Robert Collins wrote:
> Review: Needs Information
> Does this permit imports from lp hosted branches? if so, thats likely to permit bypassing of private branch ACLs - can you check that that isn't the please?
It does not allow imports from lp hosted branches, importing of branch
references to launchpad branches or even importing of branches stacked
on Launchpad branches.

It also limits imports to a specific (whitelist) set of schemes, which
excludes bzr+ssh://, sftp:// and file://.

This is the same policy as is currently in place for code mirrors at the
moment.

Cheers,

Jelmer

[Gavin Panella (allenap)]

review: Abstain

[Michael Hudson-Doyle (mwhudson)]

This (finally!) looks fine to me. Thanks for plugging away. Maybe a positive test for a branch reference being imported would be nice?

BzrServer._use_server appears to always be false, so maybe some code can be deleted?

review: Approve

[Jelmer Vernooij (jelmer)]

> This (finally!) looks fine to me. Thanks for plugging away. Maybe a positive
> test for a branch reference being imported would be nice?
I'll add one.

> BzrServer._use_server appears to always be false, so maybe some code can be
> deleted?
I'd like to keep it around for the moment. Hopefully we can switch to running actual VCS servers at some point, so we can use the proper database constrains for code import URLs.

Thanks for all your reviews :)

=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf	2011-08-15 08:47:36 +0000
+++ lib/canonical/config/schema-lazr.conf	2011-08-27 23:26:24 +0000
@@ -473,6 +473,11 @@
 # datatype: integer
 default_interval_cvs: 43200
 
+# The default value of the update interval of a code import from
+# Bazaar, in seconds.
+# datatype: integer
+default_interval_bzr: 21600
+
 # Where the tarballs of foreign branches are uploaded for storage.
 # datatype: string
 foreign_tree_store: sftp://hoover@escudero/srv/importd/sources/

=== modified file 'lib/lp/code/mail/codeimport.py'
--- lib/lp/code/mail/codeimport.py	2011-08-12 14:39:51 +0000
+++ lib/lp/code/mail/codeimport.py	2011-08-27 23:26:24 +0000
@@ -51,6 +51,7 @@
         RevisionControlSystems.BZR_SVN: 'subversion',
         RevisionControlSystems.GIT: 'git',
         RevisionControlSystems.HG: 'mercurial',
+        RevisionControlSystems.BZR: 'bazaar',
         }
     body = get_email_template('new-code-import.txt') % {
         'person': code_import.registrant.displayname,
@@ -123,7 +124,8 @@
     elif code_import.rcs_type in (RevisionControlSystems.SVN,
                                   RevisionControlSystems.BZR_SVN,
                                   RevisionControlSystems.GIT,
-                                  RevisionControlSystems.HG):
+                                  RevisionControlSystems.HG,
+                                  RevisionControlSystems.BZR):
         if CodeImportEventDataType.OLD_URL in event_data:
             old_url = event_data[CodeImportEventDataType.OLD_URL]
             body.append(

=== modified file 'lib/lp/code/model/codeimport.py'
--- lib/lp/code/model/codeimport.py	2011-04-27 01:42:46 +0000
+++ lib/lp/code/model/codeimport.py	2011-08-27 23:26:24 +0000
@@ -116,6 +116,8 @@
                 config.codeimport.default_interval_git,
             RevisionControlSystems.HG:
                 config.codeimport.default_interval_hg,
+            RevisionControlSystems.BZR:
+                config.codeimport.default_interval_bzr,
             }
         seconds = default_interval_dict[self.rcs_type]
         return timedelta(seconds=seconds)
@@ -133,7 +135,8 @@
             RevisionControlSystems.SVN,
             RevisionControlSystems.GIT,
             RevisionControlSystems.BZR_SVN,
-            RevisionControlSystems.HG):
+            RevisionControlSystems.HG,
+            RevisionControlSystems.BZR):
             return self.url
         else:
             raise AssertionError(
@@ -252,7 +255,8 @@
         elif rcs_type in (RevisionControlSystems.SVN,
                           RevisionControlSystems.BZR_SVN,
                           RevisionControlSystems.GIT,
-                          RevisionControlSystems.HG):
+                          RevisionControlSystems.HG,
+                          RevisionControlSystems.BZR):
             assert cvs_root is None and cvs_module is None
             assert url is not None
         else:
@@ -262,7 +266,8 @@
         if review_status is None:
             # Auto approve git and hg imports.
             if rcs_type in (
-                RevisionControlSystems.GIT, RevisionControlSystems.HG):
+                RevisionControlSystems.GIT, RevisionControlSystems.HG,
+                RevisionControlSystems.BZR):
                 review_status = CodeImportReviewStatus.REVIEWED
             else:
                 review_status = CodeImportReviewStatus.NEW

=== modified file 'lib/lp/code/model/codeimportevent.py'
--- lib/lp/code/model/codeimportevent.py	2010-10-17 22:51:50 +0000
+++ lib/lp/code/model/codeimportevent.py	2011-08-27 23:26:24 +0000
@@ -269,7 +269,8 @@
         if code_import.rcs_type in (RevisionControlSystems.SVN,
                                     RevisionControlSystems.BZR_SVN,
                                     RevisionControlSystems.GIT,
-                                    RevisionControlSystems.HG):
+                                    RevisionControlSystems.HG,
+                                    RevisionControlSystems.BZR):
             yield 'URL', code_import.url
         elif code_import.rcs_type == RevisionControlSystems.CVS:
             yield 'CVS_ROOT', code_import.cvs_root

=== modified file 'lib/lp/code/model/tests/test_codeimport.py'
--- lib/lp/code/model/tests/test_codeimport.py	2011-08-12 11:37:08 +0000
+++ lib/lp/code/model/tests/test_codeimport.py	2011-08-27 23:26:24 +0000
@@ -70,6 +70,20 @@
         # No job is created for the import.
         self.assertIs(None, code_import.import_job)
 
+    def test_new_svn_import_svn_scheme(self):
+        """A subversion import can use the svn:// scheme."""
+        code_import = CodeImportSet().new(
+            registrant=self.factory.makePerson(),
+            target=IBranchTarget(self.factory.makeProduct()),
+            branch_name='imported',
+            rcs_type=RevisionControlSystems.SVN,
+            url=self.factory.getUniqueURL(scheme="svn"))
+        self.assertEqual(
+            CodeImportReviewStatus.NEW,
+            code_import.review_status)
+        # No job is created for the import.
+        self.assertIs(None, code_import.import_job)
+
     def test_reviewed_svn_import(self):
         """A specific review status can be set for a new import."""
         code_import = CodeImportSet().new(
@@ -116,6 +130,21 @@
         # A job is created for the import.
         self.assertIsNot(None, code_import.import_job)
 
+    def test_git_import_git_scheme(self):
+        """A git import can have a git:// style URL."""
+        code_import = CodeImportSet().new(
+            registrant=self.factory.makePerson(),
+            target=IBranchTarget(self.factory.makeProduct()),
+            branch_name='imported',
+            rcs_type=RevisionControlSystems.GIT,
+            url=self.factory.getUniqueURL(scheme="git"),
+            review_status=None)
+        self.assertEqual(
+            CodeImportReviewStatus.REVIEWED,
+            code_import.review_status)
+        # A job is created for the import.
+        self.assertIsNot(None, code_import.import_job)
+
     def test_git_import_reviewed(self):
         """A new git import is always reviewed by default."""
         code_import = CodeImportSet().new(
@@ -146,6 +175,21 @@
         # A job is created for the import.
         self.assertIsNot(None, code_import.import_job)
 
+    def test_bzr_import_reviewed(self):
+        """A new bzr import is always reviewed by default."""
+        code_import = CodeImportSet().new(
+            registrant=self.factory.makePerson(),
+            target=IBranchTarget(self.factory.makeProduct()),
+            branch_name='mirrored',
+            rcs_type=RevisionControlSystems.BZR,
+            url=self.factory.getUniqueURL(),
+            review_status=None)
+        self.assertEqual(
+            CodeImportReviewStatus.REVIEWED,
+            code_import.review_status)
+        # A job is created for the import.
+        self.assertIsNot(None, code_import.import_job)
+
     def test_junk_code_import_rejected(self):
         """You are not allowed to create code imports targetting +junk."""
         registrant = self.factory.makePerson()

=== modified file 'lib/lp/codehosting/codeimport/tests/servers.py'
--- lib/lp/codehosting/codeimport/tests/servers.py	2011-08-22 11:56:26 +0000
+++ lib/lp/codehosting/codeimport/tests/servers.py	2011-08-27 23:26:24 +0000
@@ -4,6 +4,7 @@
 """Server classes that know how to create various kinds of foreign archive."""
 
 __all__ = [
+    'BzrServer',
     'CVSServer',
     'GitServer',
     'MercurialServer',
@@ -23,7 +24,14 @@
 import time
 import threading
 
+from bzrlib.branch import Branch
+from bzrlib.branchbuilder import BranchBuilder
+from bzrlib.bzrdir import BzrDir
 from bzrlib.tests.treeshape import build_tree_contents
+from bzrlib.tests.test_server import (
+    ReadonlySmartTCPServer_for_testing,
+    TestServer,
+    )
 from bzrlib.transport import Server
 from bzrlib.urlutils import (
     escape,
@@ -348,3 +356,50 @@
                 f.close()
             repo[None].add([filename])
         repo.commit(text='<The commit message>', user='jane Foo <[e-mail]>')
+
+
+class BzrServer(Server):
+
+    def __init__(self, repository_path, use_server=False):
+        super(BzrServer, self).__init__()
+        self.repository_path = repository_path
+        self._use_server = use_server
+
+    def createRepository(self, path):
+        BzrDir.create_branch_convenience(path)
+
+    def makeRepo(self, tree_contents):
+        branch = Branch.open(self.repository_path)
+        branch.get_config().set_user_option("create_signatures", "never")
+        builder = BranchBuilder(branch=branch)
+        actions = [('add', ('', 'tree-root', 'directory', None))]
+        actions += [('add', (path, path+'-id', 'file', content)) for (path,
+            content) in tree_contents]
+        builder.build_snapshot(None, None,
+                actions, committer='Joe Foo <[e-mail]>',
+                message=u'<The commit message>')
+
+    def get_url(self):
+        if self._use_server:
+            return self._bzrserver.get_url()
+        else:
+            return local_path_to_url(self.repository_path)
+
+    def start_server(self):
+        super(BzrServer, self).start_server()
+        self.createRepository(self.repository_path)
+        class LocalURLServer(TestServer):
+            def __init__(self, repository_path):
+                self.repository_path = repository_path
+            def start_server(self): pass
+            def get_url(self):
+                return local_path_to_url(self.repository_path)
+        if self._use_server:
+            self._bzrserver = ReadonlySmartTCPServer_for_testing()
+            self._bzrserver.start_server(
+                LocalURLServer(self.repository_path))
+
+    def stop_server(self):
+        super(BzrServer, self).stop_server()
+        if self._use_server:
+            self._bzrserver.stop_server()

=== modified file 'lib/lp/codehosting/codeimport/tests/test_worker.py'
--- lib/lp/codehosting/codeimport/tests/test_worker.py	2011-08-24 19:39:15 +0000
+++ lib/lp/codehosting/codeimport/tests/test_worker.py	2011-08-27 23:26:24 +0000
@@ -16,6 +16,9 @@
     Branch,
     BranchReferenceFormat,
     )
+from bzrlib.branchbuilder import (
+    BranchBuilder,
+    )
 from bzrlib.bzrdir import (
     BzrDir,
     BzrDirFormat,
@@ -45,6 +48,7 @@
 from lp.codehosting import load_optional_plugin
 from lp.codehosting.safe_open import (
     AcceptAnythingPolicy,
+    BlacklistPolicy,
     BadUrl,
     SafeBranchOpener,
     )
@@ -53,6 +57,7 @@
     extract_tarball,
     )
 from lp.codehosting.codeimport.tests.servers import (
+    BzrServer,
     CVSServer,
     GitServer,
     MercurialServer,
@@ -60,6 +65,7 @@
     )
 from lp.codehosting.codeimport.worker import (
     BazaarBranchStore,
+    BzrImportWorker,
     BzrSvnImportWorker,
     CodeImportBranchOpenPolicy,
     CodeImportWorkerExitCode,
@@ -1012,13 +1018,13 @@
         t.mkdir('reference')
         a_bzrdir = BzrDir.create(self.get_url('reference'))
         BranchReferenceFormat().initialize(a_bzrdir, target_branch=branch)
-        return a_bzrdir.root_transport.base
+        return a_bzrdir.root_transport.base, branch.base
 
     def test_reject_branch_reference(self):
         # URLs that point to other branch types than that expected by the
         # import should be rejected.
         args = {'rcstype': self.rcstype}
-        reference_url = self.createBranchReference()
+        reference_url, target_url = self.createBranchReference()
         if self.rcstype in ('git', 'bzr-svn', 'hg'):
             args['url'] = reference_url
         else:
@@ -1195,6 +1201,83 @@
             opener_policy=opener_policy)
 
 
+class TestBzrImport(WorkerTest, TestActualImportMixin,
+                    PullingImportWorkerTests):
+
+    rcstype = 'bzr'
+
+    def setUp(self):
+        super(TestBzrImport, self).setUp()
+        self.setUpImport()
+
+    def makeImportWorker(self, source_details, opener_policy):
+        """Make a new `ImportWorker`."""
+        return BzrImportWorker(
+            source_details, self.get_transport('import_data'),
+            self.bazaar_store, logging.getLogger(), opener_policy)
+
+    def makeForeignCommit(self, source_details):
+        """Change the foreign tree, generating exactly one commit."""
+        branch = Branch.open(source_details.url)
+        builder = BranchBuilder(branch=branch)
+        builder.build_commit(message=self.factory.getUniqueString(),
+            committer="Joe Random Hacker <[e-mail]>")
+        self.foreign_commit_count += 1
+
+    def makeSourceDetails(self, branch_name, files):
+        """Make Bzr `CodeImportSourceDetails` pointing at a real Bzr repo.
+        """
+        repository_path = self.makeTemporaryDirectory()
+        bzr_server = BzrServer(repository_path)
+        bzr_server.start_server()
+        self.addCleanup(bzr_server.stop_server)
+
+        bzr_server.makeRepo(files)
+        self.foreign_commit_count = 1
+
+        return self.factory.makeCodeImportSourceDetails(
+            rcstype='bzr', url=bzr_server.get_url())
+
+    def test_partial(self):
+        self.skip(
+            "Partial fetching is not supported for native bzr branches "
+            "at the moment.")
+
+    def test_unsupported_feature(self):
+        self.skip("All Bazaar features are supported by Bazaar.")
+
+    def test_reject_branch_reference(self):
+        # Branch references are allowed in the BzrImporter, but their URL
+        # should be checked.
+        reference_url, target_url = self.createBranchReference()
+        source_details = self.factory.makeCodeImportSourceDetails(
+            url=reference_url, rcstype='bzr')
+        policy = BlacklistPolicy(True, [target_url])
+        worker = self.makeImportWorker(source_details, opener_policy=policy)
+        self.assertEqual(
+            CodeImportWorkerExitCode.FAILURE_FORBIDDEN, worker.run())
+
+    def test_support_branch_reference(self):
+        # Branch references are allowed in the BzrImporter.
+        reference_url, target_url = self.createBranchReference()
+        target_branch = Branch.open(target_url)
+        builder = BranchBuilder(branch=target_branch)
+        builder.build_commit(message=self.factory.getUniqueString(),
+            committer="Some Random Hacker <[e-mail]>")
+        source_details = self.factory.makeCodeImportSourceDetails(
+            url=reference_url, rcstype='bzr')
+        worker = self.makeImportWorker(source_details,
+            opener_policy=AcceptAnythingPolicy())
+        self.assertEqual(
+            CodeImportWorkerExitCode.SUCCESS, worker.run())
+        branch = self.getStoredBazaarBranch(worker)
+        self.assertEqual(
+            1, len(branch.revision_history()))
+        self.assertEqual(
+            "Some Random Hacker <[e-mail]>",
+            branch.repository.get_revision(branch.last_revision()).committer)
+
+
 class CodeImportBranchOpenPolicyTests(TestCase):
 
     def setUp(self):

=== modified file 'lib/lp/codehosting/codeimport/tests/test_workermonitor.py'
--- lib/lp/codehosting/codeimport/tests/test_workermonitor.py	2011-08-27 03:35:53 +0000
+++ lib/lp/codehosting/codeimport/tests/test_workermonitor.py	2011-08-27 23:26:24 +0000
@@ -49,6 +49,7 @@
 from lp.code.model.codeimportjob import CodeImportJob
 from lp.codehosting import load_optional_plugin
 from lp.codehosting.codeimport.tests.servers import (
+    BzrServer,
     CVSServer,
     GitServer,
     MercurialServer,
@@ -67,6 +68,7 @@
     CodeImportWorkerMonitorProtocol,
     ExitQuietly,
     )
+from lp.codehosting.safe_open import AcceptAnythingPolicy
 from lp.services.log.logger import BufferLogger
 from lp.services.twistedsupport import suppress_stderr
 from lp.services.twistedsupport.tests.test_processmonitor import (
@@ -704,6 +706,17 @@
         return self.factory.makeCodeImport(
             hg_repo_url=self.hg_server.get_url())
 
+    def makeBzrCodeImport(self):
+        """Make a `CodeImport` that points to a real Bazaar branch."""
+        self.bzr_server = BzrServer(self.repo_path)
+        self.bzr_server.start_server()
+        self.addCleanup(self.bzr_server.stop_server)
+
+        self.bzr_server.makeRepo([('README', 'contents')])
+        self.foreign_commit_count = 1
+        return self.factory.makeCodeImport(
+            bzr_branch_url=self.bzr_server.get_url())
+
     def getStartedJobForImport(self, code_import):
         """Get a started `CodeImportJob` for `code_import`.
 
@@ -806,6 +819,15 @@
         result = self.performImport(job_id)
         return result.addCallback(self.assertImported, code_import_id)
 
+    def test_import_bzr(self):
+        # Create a Bazaar CodeImport and import it.
+        job = self.getStartedJobForImport(self.makeBzrCodeImport())
+        code_import_id = job.code_import.id
+        job_id = job.id
+        self.layer.txn.commit()
+        result = self.performImport(job_id)
+        return result.addCallback(self.assertImported, code_import_id)
+
     def test_import_bzrsvn(self):
         # Create a Subversion-via-bzr-svn CodeImport and import it.
         job = self.getStartedJobForImport(self.makeBzrSvnCodeImport())

=== modified file 'lib/lp/codehosting/codeimport/worker.py'
--- lib/lp/codehosting/codeimport/worker.py	2011-08-26 10:00:49 +0000
+++ lib/lp/codehosting/codeimport/worker.py	2011-08-27 23:26:24 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 __all__ = [
     'BazaarBranchStore',
+    'BzrImportWorker',
     'BzrSvnImportWorker',
     'CSCVSImportWorker',
     'CodeImportBranchOpenPolicy',
@@ -33,8 +34,8 @@
 from bzrlib.errors import (
     ConnectionError,
     InvalidEntryName,
+    NoRepositoryPresent,
     NoSuchFile,
-    NoRepositoryPresent,
     NotBranchError,
     )
 from bzrlib.transport import get_transport
@@ -249,9 +250,9 @@
 
     :ivar branch_id: The id of the branch associated to this code import, used
         for locating the existing import and the foreign tree.
-    :ivar rcstype: 'svn', 'cvs', 'hg', 'git', 'bzr-svn', as appropriate.
+    :ivar rcstype: 'svn', 'cvs', 'hg', 'git', 'bzr-svn', 'bzr' as appropriate.
     :ivar url: The branch URL if rcstype in ['svn', 'bzr-svn',
-        'git', 'hg'], None otherwise.
+        'git', 'hg', 'bzr'], None otherwise.
     :ivar cvs_root: The $CVSROOT if rcstype == 'cvs', None otherwise.
     :ivar cvs_module: The CVS module if rcstype == 'cvs', None otherwise.
     """
@@ -269,7 +270,7 @@
         """Convert command line-style arguments to an instance."""
         branch_id = int(arguments.pop(0))
         rcstype = arguments.pop(0)
-        if rcstype in ['svn', 'bzr-svn', 'git', 'hg']:
+        if rcstype in ['svn', 'bzr-svn', 'git', 'hg', 'bzr']:
             [url] = arguments
             cvs_root = cvs_module = None
         elif rcstype == 'cvs':
@@ -296,6 +297,8 @@
             return cls(branch_id, 'git', str(code_import.url))
         elif code_import.rcs_type == RevisionControlSystems.HG:
             return cls(branch_id, 'hg', str(code_import.url))
+        elif code_import.rcs_type == RevisionControlSystems.BZR:
+            return cls(branch_id, 'bzr', str(code_import.url))
         else:
             raise AssertionError("Unknown rcstype %r." % code_import.rcs_type)
 
@@ -303,7 +306,7 @@
         """Return a list of arguments suitable for passing to a child process.
         """
         result = [str(self.branch_id), self.rcstype]
-        if self.rcstype in ['svn', 'bzr-svn', 'git', 'hg']:
+        if self.rcstype in ['svn', 'bzr-svn', 'git', 'hg', 'bzr']:
             result.append(self.url)
         elif self.rcstype == 'cvs':
             result.append(self.cvs_root)
@@ -708,7 +711,12 @@
                 remote_branch_tip = remote_branch.last_revision()
                 inter_branch = InterBranch.get(remote_branch, bazaar_branch)
                 self._logger.info("Importing branch.")
-                inter_branch.fetch(limit=self.getRevisionLimit())
+                revision_limit = self.getRevisionLimit()
+                if revision_limit is None:
+                    # bzr < 2.4 does not support InterBranch.fetch()
+                    bazaar_branch.fetch(remote_branch)
+                else:
+                    inter_branch.fetch(limit=revision_limit)
                 if bazaar_branch.repository.has_revision(remote_branch_tip):
                     pull_result = inter_branch.pull(overwrite=True)
                     if pull_result.old_revid != pull_result.new_revid:
@@ -922,3 +930,25 @@
         """See `PullingImportWorker.probers`."""
         from bzrlib.plugins.svn import SvnRemoteProber
         return [SvnRemoteProber]
+
+
+class BzrImportWorker(PullingImportWorker):
+    """An import worker for importing Bazaar branches."""
+
+    invalid_branch_exceptions = [
+        NotBranchError,
+        ConnectionError,
+        ]
+    unsupported_feature_exceptions = []
+
+    def getRevisionLimit(self):
+        """See `PullingImportWorker.getRevisionLimit`."""
+        # For now, just grab the whole branch at once.
+        # bzr does support fetch(limit=) but it isn't very efficient at the moment.
+        return None
+
+    @property
+    def probers(self):
+        """See `PullingImportWorker.probers`."""
+        from bzrlib.bzrdir import BzrProber, RemoteBzrProber
+        return [BzrProber, RemoteBzrProber]

=== modified file 'lib/lp/registry/browser/productseries.py'
--- lib/lp/registry/browser/productseries.py	2011-05-27 21:12:25 +0000
+++ lib/lp/registry/browser/productseries.py	2011-08-27 23:26:24 +0000
@@ -827,15 +827,6 @@
     ))
 
 
-class RevisionControlSystemsExtended(RevisionControlSystems):
-    """External RCS plus Bazaar."""
-    BZR = DBItem(99, """
-        Bazaar
-
-        External Bazaar branch.
-        """)
-
-
 class SetBranchForm(Interface):
     """The fields presented on the form for setting a branch."""
 
@@ -844,7 +835,7 @@
         ['cvs_module'])
 
     rcs_type = Choice(title=_("Type of RCS"),
-        required=False, vocabulary=RevisionControlSystemsExtended,
+        required=False, vocabulary=RevisionControlSystems,
         description=_(
             "The version control system to import from. "))
 
@@ -908,7 +899,7 @@
     @property
     def initial_values(self):
         return dict(
-            rcs_type=RevisionControlSystemsExtended.BZR,
+            rcs_type=RevisionControlSystems.BZR,
             branch_type=LINK_LP_BZR,
             branch_location=self.context.branch)
 
@@ -989,7 +980,7 @@
             self.setFieldError(
                 'rcs_type',
                 'You must specify the type of RCS for the remote host.')
-        elif rcs_type == RevisionControlSystemsExtended.CVS:
+        elif rcs_type == RevisionControlSystems.CVS:
             if 'cvs_module' not in data:
                 self.setFieldError(
                     'cvs_module',
@@ -1022,8 +1013,9 @@
         # Extend the allowed schemes for the repository URL based on
         # rcs_type.
         extra_schemes = {
-            RevisionControlSystemsExtended.BZR_SVN: ['svn'],
-            RevisionControlSystemsExtended.GIT: ['git'],
+            RevisionControlSystems.BZR_SVN: ['svn'],
+            RevisionControlSystems.GIT: ['git'],
+            RevisionControlSystems.BZR: ['bzr'],
             }
         schemes.update(extra_schemes.get(rcs_type, []))
         return schemes
@@ -1050,7 +1042,7 @@
             # The branch location is not required for validation.
             self._setRequired(['branch_location'], False)
             # The cvs_module is required if it is a CVS import.
-            if rcs_type == RevisionControlSystemsExtended.CVS:
+            if rcs_type == RevisionControlSystems.CVS:
                 self._setRequired(['cvs_module'], True)
         else:
             raise AssertionError("Unknown branch type %s" % branch_type)
@@ -1110,7 +1102,7 @@
                 # Either create an externally hosted bzr branch
                 # (a.k.a. 'mirrored') or create a new code import.
                 rcs_type = data.get('rcs_type')
-                if rcs_type == RevisionControlSystemsExtended.BZR:
+                if rcs_type == RevisionControlSystems.BZR:
                     branch = self._createBzrBranch(
                         BranchType.MIRRORED, branch_name, branch_owner,
                         data['repo_url'])
@@ -1123,7 +1115,7 @@
                         'the series.')
                 else:
                     # We need to create an import request.
-                    if rcs_type == RevisionControlSystemsExtended.CVS:
+                    if rcs_type == RevisionControlSystems.CVS:
                         cvs_root = data.get('repo_url')
                         cvs_module = data.get('cvs_module')
                         url = None

=== modified file 'lib/lp/registry/browser/tests/productseries-setbranch-view.txt'
--- lib/lp/registry/browser/tests/productseries-setbranch-view.txt	2010-04-30 05:21:19 +0000
+++ lib/lp/registry/browser/tests/productseries-setbranch-view.txt	2011-08-27 23:26:24 +0000
@@ -178,7 +178,7 @@
     ...     'field.rcs_type': 'BZR',
     ...     'field.branch_name': 'blazer-branch',
     ...     'field.branch_owner': product.owner.name,
-    ...     'field.repo_url': 'bzr://bzr.com/foo',
+    ...     'field.repo_url': 'bzr+ssh://bzr.com/foo',
     ...     'field.actions.update': 'Update',
     ...     }
     >>> view = create_initialized_view(series, name='+setbranch', principal=product.owner, form=form)
@@ -186,8 +186,9 @@
     ...     print notification.message
     >>> for error in view.errors:
     ...     print error
-    ('repo_url'...The URI scheme "bzr" is not allowed.  Only URIs with the following schemes may be
-    used: http, https'))
+    ('repo_url'...The URI scheme "bzr+ssh" is not allowed.  Only URIs with the following schemes may be
+    used: bzr, http, https'))
+
 
 A correct URL is accepted.
 

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2011-08-26 04:15:38 +0000
+++ lib/lp/testing/factory.py	2011-08-27 23:26:24 +0000
@@ -474,7 +474,7 @@
             branch_id = self.getUniqueInteger()
         if rcstype is None:
             rcstype = 'svn'
-        if rcstype in ['svn', 'bzr-svn', 'hg']:
+        if rcstype in ['svn', 'bzr-svn', 'hg', 'bzr']:
             assert cvs_root is cvs_module is None
             if url is None:
                 url = self.getUniqueURL()
@@ -2123,7 +2123,8 @@
 
     def makeCodeImport(self, svn_branch_url=None, cvs_root=None,
                        cvs_module=None, target=None, branch_name=None,
-                       git_repo_url=None, hg_repo_url=None, registrant=None,
+                       git_repo_url=None, hg_repo_url=None,
+                       bzr_branch_url=None, registrant=None,
                        rcs_type=None, review_status=None):
         """Create and return a new, arbitrary code import.
 
@@ -2132,7 +2133,7 @@
         unique URL.
         """
         if (svn_branch_url is cvs_root is cvs_module is git_repo_url is
-            hg_repo_url is None):
+            hg_repo_url is bzr_branch_url is None):
             svn_branch_url = self.getUniqueURL()
 
         if target is None:
@@ -2163,6 +2164,11 @@
                 registrant, target, branch_name,
                 rcs_type=RevisionControlSystems.HG,
                 url=hg_repo_url)
+        elif bzr_branch_url is not None:
+            code_import = code_import_set.new(
+                registrant, target, branch_name,
+                rcs_type=RevisionControlSystems.BZR,
+                url=bzr_branch_url)
         else:
             assert rcs_type in (None, RevisionControlSystems.CVS)
             code_import = code_import_set.new(

=== modified file 'scripts/code-import-worker.py'
--- scripts/code-import-worker.py	2011-08-24 21:36:52 +0000
+++ scripts/code-import-worker.py	2011-08-27 23:26:24 +0000
@@ -26,9 +26,9 @@
 from canonical.config import config
 from lp.codehosting import load_optional_plugin
 from lp.codehosting.codeimport.worker import (
-    BzrSvnImportWorker, CSCVSImportWorker, CodeImportBranchOpenPolicy,
-    CodeImportSourceDetails, GitImportWorker, HgImportWorker,
-    get_default_bazaar_branch_store)
+    BzrImportWorker, BzrSvnImportWorker, CSCVSImportWorker,
+    CodeImportBranchOpenPolicy, CodeImportSourceDetails, GitImportWorker,
+    HgImportWorker, get_default_bazaar_branch_store)
 from lp.codehosting.safe_open import AcceptAnythingPolicy
 from canonical.launchpad import scripts
 
@@ -77,6 +77,9 @@
         elif source_details.rcstype == 'hg':
             load_optional_plugin('hg')
             import_worker_cls = HgImportWorker
+        elif source_details.rcstype == 'bzr':
+            load_optional_plugin('loom')
+            import_worker_cls = BzrImportWorker
         elif source_details.rcstype in ['cvs', 'svn']:
             import_worker_cls = CSCVSImportWorker
         else: