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

[Jelmer Vernooij]

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

=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf	2011-07-27 20:14:40 +0000
+++ lib/canonical/config/schema-lazr.conf	2011-08-07 20:42:25 +0000
@@ -488,6 +488,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/bzr.py'
--- lib/lp/code/bzr.py	2011-06-02 19:27:36 +0000
+++ lib/lp/code/bzr.py	2011-08-07 20:42:25 +0000
@@ -20,34 +20,38 @@
 
 from bzrlib.branch import (
     BranchReferenceFormat,
-    BzrBranchFormat4,
     BzrBranchFormat5,
     BzrBranchFormat6,
     BzrBranchFormat7,
     )
 from bzrlib.bzrdir import (
-    BzrDirFormat4,
-    BzrDirFormat5,
-    BzrDirFormat6,
     BzrDirMetaFormat1,
     )
 from bzrlib.plugins.loom.branch import (
     BzrBranchLoomFormat1,
     BzrBranchLoomFormat6,
     )
+from bzrlib.plugins.weave_fmt.branch import (
+    BzrBranchFormat4,
+    )
+from bzrlib.plugins.weave_fmt.bzrdir import (
+    BzrDirFormat4,
+    BzrDirFormat5,
+    BzrDirFormat6,
+    )
 from bzrlib.repofmt.groupcompress_repo import RepositoryFormat2a
 from bzrlib.repofmt.knitrepo import (
     RepositoryFormatKnit1,
     RepositoryFormatKnit3,
     RepositoryFormatKnit4,
     )
-from bzrlib.repofmt.pack_repo import (
+from bzrlib.repofmt.knitpack_repo import (
     RepositoryFormatKnitPack1,
     RepositoryFormatKnitPack3,
     RepositoryFormatKnitPack4,
     RepositoryFormatKnitPack5,
     )
-from bzrlib.repofmt.weaverepo import (
+from bzrlib.plugins.weave_fmt.repository import (
     RepositoryFormat4,
     RepositoryFormat5,
     RepositoryFormat6,

=== modified file 'lib/lp/code/mail/codeimport.py'
--- lib/lp/code/mail/codeimport.py	2011-05-27 21:12:25 +0000
+++ lib/lp/code/mail/codeimport.py	2011-08-07 20:42:25 +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-07 20:42:25 +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-07 20:42:25 +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_branchjob.py'
--- lib/lp/code/model/tests/test_branchjob.py	2011-05-23 11:06:50 +0000
+++ lib/lp/code/model/tests/test_branchjob.py	2011-08-07 20:42:25 +0000
@@ -17,7 +17,7 @@
     BzrBranchFormat7,
     )
 from bzrlib.bzrdir import BzrDirMetaFormat1
-from bzrlib.repofmt.pack_repo import RepositoryFormatKnitPack6
+from bzrlib.repofmt.knitpack_repo import RepositoryFormatKnitPack6
 from bzrlib.revision import NULL_REVISION
 from bzrlib.transport import get_transport
 import pytz

=== modified file 'lib/lp/code/model/tests/test_codeimport.py'
--- lib/lp/code/model/tests/test_codeimport.py	2011-05-27 21:12:25 +0000
+++ lib/lp/code/model/tests/test_codeimport.py	2011-08-07 20:42:25 +0000
@@ -71,6 +71,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(
@@ -117,6 +131,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(
@@ -147,6 +176,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/__init__.py'
--- lib/lp/codehosting/__init__.py	2011-03-30 15:16:35 +0000
+++ lib/lp/codehosting/__init__.py	2011-08-07 20:42:25 +0000
@@ -70,15 +70,13 @@
     __import__("bzrlib.plugins.%s" % plugin_name)
 
 
-def remove_hook(self, hook):
-    """Remove the hook from the HookPoint"""
-    self._callbacks.remove(hook)
-    for name, value in self._callback_names.iteritems():
-        if value is hook:
-            del self._callback_names[name]
-
-
-# XXX: JonathanLange 2011-03-30 bug=301472: Monkeypatch: Branch.hooks is a
-# list in bzr 1.13, so it supports remove.  It is a HookPoint in bzr 1.14, so
-# add HookPoint.remove.
-hooks.HookPoint.remove = remove_hook
+def load_bundled_plugin(plugin_name):
+    """Load a plugin bundled with Bazaar."""
+    from bzrlib.plugin import get_core_plugin_path
+    from bzrlib import plugins
+    if get_core_plugin_path() not in plugins.__path__:
+        plugins.__path__.append(get_core_plugin_path())
+    __import__("bzrlib.plugins.%s" % plugin_name)
+
+
+load_bundled_plugin("weave_fmt")

=== modified file 'lib/lp/codehosting/codeimport/tests/servers.py'
--- lib/lp/codehosting/codeimport/tests/servers.py	2011-06-02 10:48:54 +0000
+++ lib/lp/codehosting/codeimport/tests/servers.py	2011-08-07 20:42:25 +0000
@@ -4,6 +4,7 @@
 """Server classes that know how to create various kinds of foreign archive."""
 
 __all__ = [
+    'BzrServer',
     'CVSServer',
     'GitServer',
     'MercurialServer',
@@ -13,6 +14,7 @@
 __metaclass__ = type
 
 from cStringIO import StringIO
+import errno
 import os
 import shutil
 import signal
@@ -20,8 +22,16 @@
 import subprocess
 import tempfile
 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,
@@ -31,6 +41,17 @@
 import dulwich.index
 from dulwich.objects import Blob
 from dulwich.repo import Repo as GitRepo
+from dulwich.server import (
+    DictBackend,
+    TCPGitServer,
+    )
+from mercurial.ui import (
+    ui as hg_ui,
+    )
+from mercurial.hgweb import (
+    hgweb,
+    server as hgweb_server,
+    )
 import subvertpy.ra
 import svn_oo
 
@@ -107,8 +128,8 @@
             for i in range(10):
                 try:
                     ra = self._get_ra(self.get_url())
-                except subvertpy.SubversionException, e:
-                    if 'Connection refused' in str(e):
+                except OSError, e:
+                    if e.errno == errno.ECONNREFUSED:
                         time.sleep(delay)
                         delay *= 1.5
                         continue
@@ -204,45 +225,177 @@
         self._repository = self.createRepository(self._repository_path)
 
 
+class TCPGitServerThread(threading.Thread):
+    """TCP Git server that runs in a separate thread."""
+
+    def __init__(self, backend, address, port=None):
+        super(TCPGitServerThread, self).__init__()
+        self.setName("TCP Git server on %s:%s" % (address, port))
+        self.server = TCPGitServer(backend, address, port)
+
+    def run(self):
+        self.server.serve_forever()
+
+    def get_address(self):
+        return self.server.server_address
+
+    def stop(self):
+        self.server.shutdown()
+
+
 class GitServer(Server):
 
-    def __init__(self, repo_url):
+    def __init__(self, repository_path, use_server=False):
         super(GitServer, self).__init__()
-        self.repo_url = repo_url
+        self.repository_path = repository_path
+        self._use_server = use_server
+
+    def get_url(self):
+        """Return a URL to the Git repository."""
+        if self._use_server:
+            return 'git://%s:%d/' % self._server.get_address()
+        else:
+            return local_path_to_url(self.repository_path)
+
+    def createRepository(self, path):
+        GitRepo.init(path)
+
+    def start_server(self):
+        super(GitServer, self).start_server()
+        self.createRepository(self.repository_path)
+        if self._use_server:
+            repo = GitRepo(self.repository_path)
+            self._server = TCPGitServerThread(
+                DictBackend({"/": repo}), "localhost", 0)
+            self._server.start()
+
+    def stop_server(self):
+        super(GitServer, self).stop_server()
+        if self._use_server:
+            self._server.stop()
 
     def makeRepo(self, tree_contents):
-        wd = os.getcwd()
-        try:
-            os.chdir(self.repo_url)
-            repo = GitRepo.init(".")
-            blobs = [
-                (Blob.from_string(contents), filename) for (filename, contents)
-                in tree_contents]
-            repo.object_store.add_objects(blobs)
-            root_id = dulwich.index.commit_tree(repo.object_store, [
-                (filename, b.id, stat.S_IFREG | 0644)
-                for (b, filename) in blobs])
-            repo.do_commit(committer='Joe Foo <[e-mail]>',
-                message=u'<The commit message>', tree=root_id)
-        finally:
-            os.chdir(wd)
+        repo = GitRepo(self.repository_path)
+        blobs = [
+            (Blob.from_string(contents), filename) for (filename, contents)
+            in tree_contents]
+        repo.object_store.add_objects(blobs)
+        root_id = dulwich.index.commit_tree(repo.object_store, [
+            (filename, b.id, stat.S_IFREG | 0644)
+            for (b, filename) in blobs])
+        repo.do_commit(committer='Joe Foo <[e-mail]>',
+            message=u'<The commit message>', tree=root_id)
+
+
+class MercurialServerThread(threading.Thread):
+
+    def __init__(self, path, address, port=0):
+        super(MercurialServerThread, self).__init__()
+        self.ui = hg_ui()
+        self.ui.setconfig("web", "address", address)
+        self.ui.setconfig("web", "port", 0)
+        self.app = hgweb(path, baseui=self.ui)
+        self.httpd = hgweb_server.create_server(self.ui, self.app)
+        # By default the Mercurial server output goes to stdout
+        self.httpd.errorlog = StringIO()
+        self.httpd.accesslog = StringIO()
+
+    def get_address(self):
+        return (self.httpd.addr, self.httpd.port)
+
+    def run(self):
+        self.httpd.serve_forever()
+
+    def stop(self):
+        self.httpd.shutdown()
 
 
 class MercurialServer(Server):
 
-    def __init__(self, repo_url):
+    def __init__(self, repository_path, use_server=False):
         super(MercurialServer, self).__init__()
-        self.repo_url = repo_url
+        self.repository_path = repository_path
+        self._use_server = use_server
+
+    def get_url(self):
+        if self._use_server:
+            return "http://%s:%d/" % self._hgserver.get_address()
+        else:
+            return local_path_to_url(self.repository_path)
+
+    def start_server(self):
+        super(MercurialServer, self).start_server()
+        self.createRepository(self.repository_path)
+        if self._use_server:
+            self._hgserver = MercurialServerThread(self.repository_path, "localhost")
+            self._hgserver.start()
+
+    def stop_server(self):
+        super(MercurialServer, self).stop_server()
+        if self._use_server:
+            self._hgserver.stop()
+
+    def createRepository(self, path):
+        from mercurial.ui import ui
+        from mercurial.localrepo import localrepository
+        localrepository(ui(), self.repository_path, create=1)
 
     def makeRepo(self, tree_contents):
         from mercurial.ui import ui
         from mercurial.localrepo import localrepository
-        repo = localrepository(ui(), self.repo_url, create=1)
+        repo = localrepository(ui(), self.repository_path)
         for filename, contents in tree_contents:
-            f = open(os.path.join(self.repo_url, filename), 'w')
+            f = open(os.path.join(self.repository_path, filename), 'w')
             try:
                 f.write(contents)
             finally:
                 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-07-20 17:20:03 +0000
+++ lib/lp/codehosting/codeimport/tests/test_worker.py	2011-08-07 20:42:25 +0000
@@ -17,6 +17,9 @@
     Branch,
     BranchReferenceFormat,
     )
+from bzrlib.branchbuilder import (
+    BranchBuilder,
+    )
 from bzrlib.bzrdir import (
     BzrDir,
     BzrDirFormat,
@@ -47,6 +50,7 @@
     extract_tarball,
     )
 from lp.codehosting.codeimport.tests.servers import (
+    BzrServer,
     CVSServer,
     GitServer,
     MercurialServer,
@@ -54,6 +58,7 @@
     )
 from lp.codehosting.codeimport.worker import (
     BazaarBranchStore,
+    BzrImportWorker,
     BzrSvnImportWorker,
     CodeImportWorkerExitCode,
     CSCVSImportWorker,
@@ -959,7 +964,7 @@
     def makeSourceDetails(self, branch_name, files):
         """Make a SVN `CodeImportSourceDetails` pointing at a real SVN repo.
         """
-        svn_server = SubversionServer(self.makeTemporaryDirectory())
+        svn_server = SubversionServer(self.makeTemporaryDirectory(), True)
         svn_server.start_server()
         self.addCleanup(svn_server.stop_server)
 
@@ -999,10 +1004,10 @@
         # import should be rejected.
         args = {'rcstype': self.rcstype}
         reference_url = self.createBranchReference()
-        if self.rcstype in ('git', 'bzr-svn', 'hg'):
+        if self.rcstype in ('git', 'bzr-svn', 'hg', 'bzr'):
             args['url'] = reference_url
         else:
-            raise AssertionError("unexpected rcs_type %r" % self.rcs_type)
+            raise AssertionError("unexpected rcs_type %r" % self.rcstype)
         source_details = self.factory.makeCodeImportSourceDetails(**args)
         worker = self.makeImportWorker(source_details)
         self.assertEqual(
@@ -1011,7 +1016,21 @@
     def test_invalid(self):
         # If there is no branch in the target URL, exit with FAILURE_INVALID
         worker = self.makeImportWorker(self.factory.makeCodeImportSourceDetails(
-            rcstype=self.rcstype, url="file:///path/non/existant"))
+            rcstype=self.rcstype, url="http://localhost/path/non/existant"))
+        self.assertEqual(
+            CodeImportWorkerExitCode.FAILURE_INVALID, worker.run())
+
+    def test_bad_url(self):
+        # Local path URLs are not allowed
+        worker = self.makeImportWorker(self.factory.makeCodeImportSourceDetails(
+            rcstype=self.rcstype, url="file:///tmp/path/non/existant"))
+        self.assertEqual(
+            CodeImportWorkerExitCode.FAILURE_INVALID, worker.run())
+
+    def test_launchpad_url(self):
+        # Launchpad URLs are not allowed
+        worker = self.makeImportWorker(self.factory.makeCodeImportSourceDetails(
+            rcstype=self.rcstype, url="https://code.launchpad.net/linux/"))
         self.assertEqual(
             CodeImportWorkerExitCode.FAILURE_INVALID, worker.run())
 
@@ -1071,7 +1090,7 @@
 
     def makeForeignCommit(self, source_details):
         """Change the foreign tree, generating exactly one commit."""
-        repo = GitRepo(source_details.url)
+        repo = GitRepo(local_path_from_url(source_details.url))
         repo.do_commit(message=self.factory.getUniqueString(),
             committer="Joe Random Hacker <[e-mail]>")
         self.foreign_commit_count += 1
@@ -1080,7 +1099,7 @@
         """Make a Git `CodeImportSourceDetails` pointing at a real Git repo.
         """
         repository_path = self.makeTemporaryDirectory()
-        git_server = GitServer(repository_path)
+        git_server = GitServer(repository_path, use_server=True)
         git_server.start_server()
         self.addCleanup(git_server.stop_server)
 
@@ -1088,8 +1107,7 @@
         self.foreign_commit_count = 1
 
         return self.factory.makeCodeImportSourceDetails(
-            rcstype='git', url=repository_path)
-
+            rcstype='git', url=git_server.get_url())
 
 
 class TestMercurialImport(WorkerTest, TestActualImportMixin,
@@ -1124,7 +1142,7 @@
         """Change the foreign tree, generating exactly one commit."""
         from mercurial.ui import ui
         from mercurial.localrepo import localrepository
-        repo = localrepository(ui(), source_details.url)
+        repo = localrepository(ui(), local_path_from_url(source_details.url))
         repo.commit(text="hello world!", user="Jane Random Hacker", force=1)
         self.foreign_commit_count += 1
 
@@ -1132,7 +1150,7 @@
         """Make a Mercurial `CodeImportSourceDetails` pointing at a real repo.
         """
         repository_path = self.makeTemporaryDirectory()
-        hg_server = MercurialServer(repository_path)
+        hg_server = MercurialServer(repository_path, use_server=True)
         hg_server.start_server()
         self.addCleanup(hg_server.stop_server)
 
@@ -1140,7 +1158,7 @@
         self.foreign_commit_count = 1
 
         return self.factory.makeCodeImportSourceDetails(
-            rcstype='hg', url=repository_path)
+            rcstype='hg', url=hg_server.get_url())
 
 
 class TestBzrSvnImport(WorkerTest, SubversionImportHelpers,
@@ -1160,5 +1178,51 @@
             self.bazaar_store, logging.getLogger())
 
 
+class TestBzrImport(WorkerTest, TestActualImportMixin,
+                    PullingImportWorkerTests):
+
+    rcstype = 'bzr'
+
+    def setUp(self):
+        super(TestBzrImport, self).setUp()
+        self.setUpImport()
+
+    def makeImportWorker(self, source_details):
+        """Make a new `ImportWorker`."""
+        return BzrImportWorker(
+            source_details, self.get_transport('import_data'),
+            self.bazaar_store, logging.getLogger())
+
+    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, use_server=True)
+        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_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/codehosting/codeimport/tests/test_workermonitor.py'
--- lib/lp/codehosting/codeimport/tests/test_workermonitor.py	2011-06-16 23:43:04 +0000
+++ lib/lp/codehosting/codeimport/tests/test_workermonitor.py	2011-08-07 20:42:25 +0000
@@ -50,6 +50,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,
@@ -661,26 +662,38 @@
     def makeGitCodeImport(self):
         """Make a `CodeImport` that points to a real Git repository."""
         load_optional_plugin('git')
-        self.git_server = GitServer(self.repo_path)
+        self.git_server = GitServer(self.repo_path, use_server=True)
         self.git_server.start_server()
         self.addCleanup(self.git_server.stop_server)
 
         self.git_server.makeRepo([('README', 'contents')])
         self.foreign_commit_count = 1
 
-        return self.factory.makeCodeImport(git_repo_url=self.repo_path)
+        return self.factory.makeCodeImport(
+            git_repo_url=self.git_server.get_url())
 
     def makeHgCodeImport(self):
         """Make a `CodeImport` that points to a real Mercurial repository."""
         load_optional_plugin('hg')
-        self.hg_server = MercurialServer(self.repo_path)
+        self.hg_server = MercurialServer(self.repo_path, use_server=True)
         self.hg_server.start_server()
         self.addCleanup(self.hg_server.stop_server)
 
         self.hg_server.makeRepo([('README', 'contents')])
         self.foreign_commit_count = 1
 
-        return self.factory.makeCodeImport(hg_repo_url=self.repo_path)
+        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.repo_path)
 
     def getStartedJobForImport(self, code_import):
         """Get a started `CodeImportJob` for `code_import`.
@@ -782,6 +795,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)
+
     # XXX 2010-03-24 MichaelHudson, bug=541526: This test fails intermittently
     # in EC2.
     def DISABLED_test_import_bzrsvn(self):

=== modified file 'lib/lp/codehosting/codeimport/worker.py'
--- lib/lp/codehosting/codeimport/worker.py	2011-08-02 11:28:46 +0000
+++ lib/lp/codehosting/codeimport/worker.py	2011-08-07 20:42:25 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 __all__ = [
     'BazaarBranchStore',
+    'BzrImportWorker',
     'BzrSvnImportWorker',
     'CSCVSImportWorker',
     'CodeImportSourceDetails',
@@ -21,6 +22,7 @@
 import os
 import shutil
 
+from bzrlib.upgrade import upgrade
 from bzrlib.branch import (
     Branch,
     InterBranch,
@@ -38,7 +40,6 @@
     )
 from bzrlib.transport import get_transport
 import bzrlib.ui
-from bzrlib.upgrade import upgrade
 from bzrlib.urlutils import (
     join as urljoin,
     local_path_from_url,
@@ -49,11 +50,22 @@
 import SCM
 
 from canonical.config import config
+
+from lazr.uri import (
+    URI,
+    )
+
 from lp.code.enums import RevisionControlSystems
+from lp.code.interfaces.branch import get_blacklisted_hostnames
 from lp.codehosting.codeimport.foreigntree import (
     CVSWorkingTree,
     SubversionWorkingTree,
     )
+from lp.codehosting.safe_open import (
+    BadUrl,
+    BranchOpenPolicy,
+    SafeBranchOpener,
+    )
 from lp.codehosting.codeimport.tarball import (
     create_tarball,
     extract_tarball,
@@ -62,6 +74,52 @@
 from lp.services.propertycache import cachedproperty
 
 
+class CodeImportBranchOpenPolicy(BranchOpenPolicy):
+    """Branch open policy for code imports.
+
+    In summary:
+     - follow references,
+     - only open non-Launchpad URLs
+     - only open the allowed schemes
+    """
+
+    allowed_schemes = ['http', 'https', 'svn', 'git', 'ftp']
+
+    def shouldFollowReferences(self):
+        """See `BranchOpenPolicy.shouldFollowReferences`.
+
+        We traverse branch references for MIRRORED branches because they
+        provide a useful redirection mechanism and we want to be consistent
+        with the bzr command line.
+        """
+        return True
+
+    def transformFallbackLocation(self, branch, url):
+        """See `BranchOpenPolicy.transformFallbackLocation`.
+
+        For mirrored branches, we stack on whatever the remote branch claims
+        to stack on, but this URL still needs to be checked.
+        """
+        return urljoin(branch.base, url), True
+
+    def checkOneURL(self, url):
+        """See `BranchOpenPolicy.checkOneURL`.
+
+        We refuse to mirror from Launchpad or a ssh-like or file URL.
+        """
+        uri = URI(url)
+        launchpad_domain = config.vhost.mainsite.hostname
+        if uri.underDomain(launchpad_domain):
+            raise BadUrl(url)
+        for hostname in get_blacklisted_hostnames():
+            if uri.underDomain(hostname):
+                raise BadUrl(url)
+        if uri.scheme in ['sftp', 'bzr+ssh']:
+            raise BadUrl(url)
+        elif uri.scheme not in self.allowed_schemes:
+            raise BadUrl(url)
+
+
 class CodeImportWorkerExitCode:
     """Exit codes used by the code import worker script."""
 
@@ -187,9 +245,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' or 'cvs' 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'], 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.
     """
@@ -207,7 +265,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':
@@ -234,6 +292,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)
 
@@ -241,7 +301,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)
@@ -601,11 +661,18 @@
     def _doImport(self):
         self._logger.info("Starting job.")
         saved_factory = bzrlib.ui.ui_factory
+        opener_policy = CodeImportBranchOpenPolicy()
+        opener = SafeBranchOpener(opener_policy)
         bzrlib.ui.ui_factory = LoggingUIFactory(logger=self._logger)
         try:
             self._logger.info(
                 "Getting exising bzr branch from central store.")
             bazaar_branch = self.getBazaarBranch()
+            try:
+                opener_policy.checkOneURL(self.source_details.url)
+            except BadUrl, e:
+                self._logger.info("Invalid URL: %s" % e)
+                return CodeImportWorkerExitCode.FAILURE_INVALID
             transport = get_transport(self.source_details.url)
             for prober_kls in self.probers:
                 prober = prober_kls()
@@ -617,7 +684,13 @@
             else:
                 self._logger.info("No branch found at remote location.")
                 return CodeImportWorkerExitCode.FAILURE_INVALID
-            remote_branch = format.open(transport).open_branch()
+            remote_dir = format.open(transport)
+            try:
+                remote_branch = opener.runWithTransformFallbackLocationHookInstalled(
+                    remote_dir.open_branch)
+            except BadUrl, e:
+                self._logger.info("Invalid URL: %s" % e)
+                return CodeImportWorkerExitCode.FAILURE_INVALID
             remote_branch_tip = remote_branch.last_revision()
             inter_branch = InterBranch.get(remote_branch, bazaar_branch)
             self._logger.info("Importing branch.")
@@ -820,3 +893,21 @@
         """See `PullingImportWorker.probers`."""
         from bzrlib.plugins.svn import SvnRemoteProber
         return [SvnRemoteProber]
+
+
+class BzrImportWorker(PullingImportWorker):
+    """An import worker for importing Bazaar branches."""
+
+    invalid_branch_exceptions = []
+    unsupported_feature_exceptions = []
+
+    def getRevisionLimit(self):
+        """See `PullingImportWorker.getRevisionLimit`."""
+        # For now, just grab the whole branch at once
+        return None
+
+    @property
+    def probers(self):
+        """See `PullingImportWorker.probers`."""
+        from bzrlib.bzrdir import BzrProber, RemoteBzrProber
+        return [BzrProber, RemoteBzrProber]

=== modified file 'lib/lp/codehosting/puller/tests/__init__.py'
--- lib/lp/codehosting/puller/tests/__init__.py	2011-06-02 11:16:47 +0000
+++ lib/lp/codehosting/puller/tests/__init__.py	2011-08-07 20:42:25 +0000
@@ -25,75 +25,19 @@
 from canonical.config import config
 from lp.codehosting.puller.worker import (
     BranchMirrorer,
+    BranchMirrorerPolicy,
     PullerWorker,
     PullerWorkerProtocol,
     )
 from lp.codehosting.tests.helpers import LoomTestMixin
+from lp.codehosting.safe_open import AcceptAnythingPolicy
 from lp.codehosting.vfs import branch_id_to_path
-from lp.codehosting.vfs.branchfs import (
-    BadUrl,
-    BranchPolicy,
-    )
 from lp.testing import TestCaseWithFactory
 
 
-class BlacklistPolicy(BranchPolicy):
-    """Branch policy that forbids certain URLs."""
-
-    def __init__(self, should_follow_references, unsafe_urls=None):
-        if unsafe_urls is None:
-            unsafe_urls = set()
-        self._unsafe_urls = unsafe_urls
-        self._should_follow_references = should_follow_references
-
-    def shouldFollowReferences(self):
-        return self._should_follow_references
-
-    def checkOneURL(self, url):
-        if url in self._unsafe_urls:
-            raise BadUrl(url)
-
-    def transformFallbackLocation(self, branch, url):
-        """See `BranchPolicy.transformFallbackLocation`.
-
-        This class is not used for testing our smarter stacking features so we
-        just do the simplest thing: return the URL that would be used anyway
-        and don't check it.
-        """
-        return urlutils.join(branch.base, url), False
-
-
-class AcceptAnythingPolicy(BlacklistPolicy):
-    """Accept anything, to make testing easier."""
-
-    def __init__(self):
-        super(AcceptAnythingPolicy, self).__init__(True, set())
-
-
-class WhitelistPolicy(BranchPolicy):
-    """Branch policy that only allows certain URLs."""
-
-    def __init__(self, should_follow_references, allowed_urls=None,
-                 check=False):
-        if allowed_urls is None:
-            allowed_urls = []
-        self.allowed_urls = set(url.rstrip('/') for url in allowed_urls)
-        self.check = check
-
-    def shouldFollowReferences(self):
-        return self._should_follow_references
-
-    def checkOneURL(self, url):
-        if url.rstrip('/') not in self.allowed_urls:
-            raise BadUrl(url)
-
-    def transformFallbackLocation(self, branch, url):
-        """See `BranchPolicy.transformFallbackLocation`.
-
-        Here we return the URL that would be used anyway and optionally check
-        it.
-        """
-        return urlutils.join(branch.base, url), self.check
+class AcceptAnythingBranchMirrorerPolicy(AcceptAnythingPolicy,
+    BranchMirrorerPolicy): pass
+
 
 
 class PullerWorkerMixin:
@@ -114,7 +58,7 @@
             oops_prefix = ''
         if branch_type is None:
             if policy is None:
-                policy = AcceptAnythingPolicy()
+                policy = AcceptAnythingBranchMirrorerPolicy()
             opener = BranchMirrorer(policy, protocol)
         else:
             opener = None

=== modified file 'lib/lp/codehosting/puller/tests/test_errors.py'
--- lib/lp/codehosting/puller/tests/test_errors.py	2010-08-20 20:31:18 +0000
+++ lib/lp/codehosting/puller/tests/test_errors.py	2011-08-07 20:42:25 +0000
@@ -23,17 +23,15 @@
 
 from lp.code.enums import BranchType
 from lp.codehosting.puller.worker import (
+    BadUrlLaunchpad,
+    BadUrlScheme,
+    BadUrlSsh,
     BranchLoopError,
     BranchMirrorer,
     BranchReferenceForbidden,
     PullerWorker,
     PullerWorkerProtocol,
     )
-from lp.codehosting.vfs.branchfs import (
-    BadUrlLaunchpad,
-    BadUrlScheme,
-    BadUrlSsh,
-    )
 
 
 class StubbedPullerWorkerProtocol(PullerWorkerProtocol):

=== modified file 'lib/lp/codehosting/puller/tests/test_worker.py'
--- lib/lp/codehosting/puller/tests/test_worker.py	2010-10-15 08:47:20 +0000
+++ lib/lp/codehosting/puller/tests/test_worker.py	2011-08-07 20:42:25 +0000
@@ -14,18 +14,15 @@
 import bzrlib.branch
 from bzrlib.branch import (
     BranchReferenceFormat,
-    BzrBranchFormat7,
     )
 from bzrlib.bzrdir import (
     BzrDir,
-    BzrDirMetaFormat1,
     )
 from bzrlib.errors import (
     IncompatibleRepositories,
     NotBranchError,
     NotStacked,
     )
-from bzrlib.repofmt.pack_repo import RepositoryFormatKnitPack1
 from bzrlib.revision import NULL_REVISION
 from bzrlib.tests import (
     TestCaseInTempDir,
@@ -35,28 +32,25 @@
 
 from lp.code.enums import BranchType
 from lp.codehosting.puller.tests import (
-    AcceptAnythingPolicy,
-    BlacklistPolicy,
     FixedHttpServer,
     PullerWorkerMixin,
-    WhitelistPolicy,
     )
 from lp.codehosting.puller.worker import (
-    BranchLoopError,
-    BranchMirrorer,
-    BranchReferenceForbidden,
-    install_worker_ui_factory,
-    PullerWorkerProtocol,
-    WORKER_ACTIVITY_NETWORK,
-    )
-from lp.codehosting.vfs.branchfs import (
-    BadUrl,
     BadUrlLaunchpad,
     BadUrlScheme,
     BadUrlSsh,
-    BranchPolicy,
+    BranchMirrorerPolicy,
     ImportedBranchPolicy,
+    install_worker_ui_factory,
     MirroredBranchPolicy,
+    PullerWorkerProtocol,
+    SafeBranchOpener,
+    WORKER_ACTIVITY_NETWORK,
+    )
+from lp.codehosting.safe_open import (
+    AcceptAnythingPolicy,
+    BadUrl,
+    BranchOpenPolicy,
     )
 from lp.testing import TestCase
 from lp.testing.factory import (
@@ -81,7 +75,8 @@
     return strings
 
 
-class PrearrangedStackedBranchPolicy(AcceptAnythingPolicy):
+class PrearrangedStackedBranchPolicy(BranchMirrorerPolicy,
+    AcceptAnythingPolicy):
     """A branch policy that returns a pre-configurable stack-on URL."""
 
     def __init__(self, stack_on_url):
@@ -280,199 +275,8 @@
         self.assertEqual('', stacked_on_url)
 
 
-class TestBranchMirrorerCheckAndFollowBranchReference(TestCase):
-    """Unit tests for `BranchMirrorer.checkAndFollowBranchReference`."""
-
-    class StubbedBranchMirrorer(BranchMirrorer):
-        """BranchMirrorer that provides canned answers.
-
-        We implement the methods we need to to be able to control all the
-        inputs to the `BranchMirrorer.checkSource` method, which is what is
-        being tested in this class.
-        """
-
-        def __init__(self, references, policy):
-            parent_cls = TestBranchMirrorerCheckAndFollowBranchReference
-            super(parent_cls.StubbedBranchMirrorer, self).__init__(policy)
-            self._reference_values = {}
-            for i in range(len(references) - 1):
-                self._reference_values[references[i]] = references[i+1]
-            self.follow_reference_calls = []
-
-        def followReference(self, url):
-            self.follow_reference_calls.append(url)
-            return self._reference_values[url]
-
-    def makeBranchMirrorer(self, should_follow_references, references,
-                         unsafe_urls=None):
-        policy = BlacklistPolicy(should_follow_references, unsafe_urls)
-        opener = self.StubbedBranchMirrorer(references, policy)
-        return opener
-
-    def testCheckInitialURL(self):
-        # checkSource rejects all URLs that are not allowed.
-        opener = self.makeBranchMirrorer(None, [], set(['a']))
-        self.assertRaises(BadUrl, opener.checkAndFollowBranchReference, 'a')
-
-    def testNotReference(self):
-        # When branch references are forbidden, checkAndFollowBranchReference
-        # does not raise on non-references.
-        opener = self.makeBranchMirrorer(False, ['a', None])
-        self.assertEquals('a', opener.checkAndFollowBranchReference('a'))
-        self.assertEquals(['a'], opener.follow_reference_calls)
-
-    def testBranchReferenceForbidden(self):
-        # checkAndFollowBranchReference raises BranchReferenceForbidden if
-        # branch references are forbidden and the source URL points to a
-        # branch reference.
-        opener = self.makeBranchMirrorer(False, ['a', 'b'])
-        self.assertRaises(
-            BranchReferenceForbidden,
-            opener.checkAndFollowBranchReference, 'a')
-        self.assertEquals(['a'], opener.follow_reference_calls)
-
-    def testAllowedReference(self):
-        # checkAndFollowBranchReference does not raise if following references
-        # is allowed and the source URL points to a branch reference to a
-        # permitted location.
-        opener = self.makeBranchMirrorer(True, ['a', 'b', None])
-        self.assertEquals('b', opener.checkAndFollowBranchReference('a'))
-        self.assertEquals(['a', 'b'], opener.follow_reference_calls)
-
-    def testCheckReferencedURLs(self):
-        # checkAndFollowBranchReference checks if the URL a reference points
-        # to is safe.
-        opener = self.makeBranchMirrorer(
-            True, ['a', 'b', None], unsafe_urls=set('b'))
-        self.assertRaises(BadUrl, opener.checkAndFollowBranchReference, 'a')
-        self.assertEquals(['a'], opener.follow_reference_calls)
-
-    def testSelfReferencingBranch(self):
-        # checkAndFollowBranchReference raises BranchReferenceLoopError if
-        # following references is allowed and the source url points to a
-        # self-referencing branch reference.
-        opener = self.makeBranchMirrorer(True, ['a', 'a'])
-        self.assertRaises(
-            BranchLoopError, opener.checkAndFollowBranchReference, 'a')
-        self.assertEquals(['a'], opener.follow_reference_calls)
-
-    def testBranchReferenceLoop(self):
-        # checkAndFollowBranchReference raises BranchReferenceLoopError if
-        # following references is allowed and the source url points to a loop
-        # of branch references.
-        references = ['a', 'b', 'a']
-        opener = self.makeBranchMirrorer(True, references)
-        self.assertRaises(
-            BranchLoopError, opener.checkAndFollowBranchReference, 'a')
-        self.assertEquals(['a', 'b'], opener.follow_reference_calls)
-
-
-class TestBranchMirrorerStacking(TestCaseWithTransport):
-
-    def makeBranchMirrorer(self, allowed_urls):
-        policy = WhitelistPolicy(True, allowed_urls, True)
-        return BranchMirrorer(policy)
-
-    def makeBranch(self, path, branch_format, repository_format):
-        """Make a Bazaar branch at 'path' with the given formats."""
-        bzrdir_format = BzrDirMetaFormat1()
-        bzrdir_format.set_branch_format(branch_format)
-        bzrdir = self.make_bzrdir(path, format=bzrdir_format)
-        repository_format.initialize(bzrdir)
-        return bzrdir.create_branch()
-
-    def testAllowedURL(self):
-        # checkSource does not raise an exception for branches stacked on
-        # branches with allowed URLs.
-        stacked_on_branch = self.make_branch('base-branch', format='1.6')
-        stacked_branch = self.make_branch('stacked-branch', format='1.6')
-        stacked_branch.set_stacked_on_url(stacked_on_branch.base)
-        opener = self.makeBranchMirrorer(
-            [stacked_branch.base, stacked_on_branch.base])
-        # This doesn't raise an exception.
-        opener.open(stacked_branch.base)
-
-    def testUnstackableRepository(self):
-        # checkSource treats branches with UnstackableRepositoryFormats as
-        # being not stacked.
-        branch = self.makeBranch(
-            'unstacked', BzrBranchFormat7(), RepositoryFormatKnitPack1())
-        opener = self.makeBranchMirrorer([branch.base])
-        # This doesn't raise an exception.
-        opener.open(branch.base)
-
-    def testAllowedRelativeURL(self):
-        # checkSource passes on absolute urls to checkOneURL, even if the
-        # value of stacked_on_location in the config is set to a relative URL.
-        stacked_on_branch = self.make_branch('base-branch', format='1.6')
-        stacked_branch = self.make_branch('stacked-branch', format='1.6')
-        stacked_branch.set_stacked_on_url('../base-branch')
-        opener = self.makeBranchMirrorer(
-            [stacked_branch.base, stacked_on_branch.base])
-        # Note that stacked_on_branch.base is not '../base-branch', it's an
-        # absolute URL.
-        self.assertNotEqual('../base-branch', stacked_on_branch.base)
-        # This doesn't raise an exception.
-        opener.open(stacked_branch.base)
-
-    def testAllowedRelativeNested(self):
-        # Relative URLs are resolved relative to the stacked branch.
-        self.get_transport().mkdir('subdir')
-        a = self.make_branch('subdir/a', format='1.6')
-        b = self.make_branch('b', format='1.6')
-        b.set_stacked_on_url('../subdir/a')
-        c = self.make_branch('subdir/c', format='1.6')
-        c.set_stacked_on_url('../../b')
-        opener = self.makeBranchMirrorer([c.base, b.base, a.base])
-        # This doesn't raise an exception.
-        opener.open(c.base)
-
-    def testForbiddenURL(self):
-        # checkSource raises a BadUrl exception if a branch is stacked on a
-        # branch with a forbidden URL.
-        stacked_on_branch = self.make_branch('base-branch', format='1.6')
-        stacked_branch = self.make_branch('stacked-branch', format='1.6')
-        stacked_branch.set_stacked_on_url(stacked_on_branch.base)
-        opener = self.makeBranchMirrorer([stacked_branch.base])
-        self.assertRaises(BadUrl, opener.open, stacked_branch.base)
-
-    def testForbiddenURLNested(self):
-        # checkSource raises a BadUrl exception if a branch is stacked on a
-        # branch that is in turn stacked on a branch with a forbidden URL.
-        a = self.make_branch('a', format='1.6')
-        b = self.make_branch('b', format='1.6')
-        b.set_stacked_on_url(a.base)
-        c = self.make_branch('c', format='1.6')
-        c.set_stacked_on_url(b.base)
-        opener = self.makeBranchMirrorer([c.base, b.base])
-        self.assertRaises(BadUrl, opener.open, c.base)
-
-    def testSelfStackedBranch(self):
-        # checkSource raises StackingLoopError if a branch is stacked on
-        # itself. This avoids infinite recursion errors.
-        a = self.make_branch('a', format='1.6')
-        # Bazaar 1.17 and up make it harder to create branches like this.
-        # It's still worth testing that we don't blow up in the face of them,
-        # so we grovel around a bit to create one anyway.
-        a.get_config().set_user_option('stacked_on_location', a.base)
-        opener = self.makeBranchMirrorer([a.base])
-        self.assertRaises(BranchLoopError, opener.open, a.base)
-
-    def testLoopStackedBranch(self):
-        # checkSource raises StackingLoopError if a branch is stacked in such
-        # a way so that it is ultimately stacked on itself. e.g. a stacked on
-        # b stacked on a.
-        a = self.make_branch('a', format='1.6')
-        b = self.make_branch('b', format='1.6')
-        a.set_stacked_on_url(b.base)
-        b.set_stacked_on_url(a.base)
-        opener = self.makeBranchMirrorer([a.base, b.base])
-        self.assertRaises(BranchLoopError, opener.open, a.base)
-        self.assertRaises(BranchLoopError, opener.open, b.base)
-
-
-class TestReferenceMirroring(TestCaseWithTransport):
-    """Feature tests for mirroring of branch references."""
+class TestReferenceOpener(TestCaseWithTransport):
+    """Feature tests for safe opening of branch references."""
 
     def createBranchReference(self, url):
         """Create a pure branch reference that points to the specified URL.
@@ -513,19 +317,19 @@
         self.assertEqual(opened_branch.base, target_branch.base)
 
     def testFollowReferenceValue(self):
-        # BranchMirrorer.followReference gives the reference value for
+        # SafeBranchOpener.followReference gives the reference value for
         # a branch reference.
-        opener = BranchMirrorer(BranchPolicy())
+        opener = SafeBranchOpener(BranchOpenPolicy())
         reference_value = 'http://example.com/branch'
         reference_url = self.createBranchReference(reference_value)
         self.assertEqual(
             reference_value, opener.followReference(reference_url))
 
     def testFollowReferenceNone(self):
-        # BranchMirrorer.followReference gives None for a normal branch.
+        # SafeBranchOpener.followReference gives None for a normal branch.
         self.make_branch('repo')
         branch_url = self.get_url('repo')
-        opener = BranchMirrorer(BranchPolicy())
+        opener = SafeBranchOpener(BranchOpenPolicy())
         self.assertIs(None, opener.followReference(branch_url))
 
 

=== modified file 'lib/lp/codehosting/puller/tests/test_worker_formats.py'
--- lib/lp/codehosting/puller/tests/test_worker_formats.py	2010-08-20 20:31:18 +0000
+++ lib/lp/codehosting/puller/tests/test_worker_formats.py	2011-08-07 20:42:25 +0000
@@ -7,14 +7,18 @@
 
 import unittest
 
+import lp.codehosting # for bzr plugins
+
 from bzrlib.branch import Branch
 from bzrlib.bzrdir import (
-    BzrDirFormat6,
     BzrDirMetaFormat1,
     )
 from bzrlib.repofmt.knitrepo import RepositoryFormatKnit1
-from bzrlib.repofmt.pack_repo import RepositoryFormatKnitPack5
-from bzrlib.repofmt.weaverepo import (
+from bzrlib.repofmt.knitpack_repo import RepositoryFormatKnitPack5
+from bzrlib.plugins.weave_fmt.bzrdir import (
+    BzrDirFormat6,
+    )
+from bzrlib.plugins.weave_fmt.repository import (
     RepositoryFormat6,
     RepositoryFormat7,
     )

=== modified file 'lib/lp/codehosting/puller/worker.py'
--- lib/lp/codehosting/puller/worker.py	2011-06-03 01:00:53 +0000
+++ lib/lp/codehosting/puller/worker.py	2011-08-07 20:42:25 +0000
@@ -8,20 +8,32 @@
 import sys
 import urllib2
 
-from bzrlib import errors
+import lp.codehosting # to load bzr plugins
+
+from bzrlib import (
+    errors,
+    urlutils,
+    )
 from bzrlib.branch import (
     Branch,
+    )
+from bzrlib.plugins.weave_fmt.branch import (
     BzrBranchFormat4,
     )
-from bzrlib.bzrdir import BzrDir
-from bzrlib.repofmt.weaverepo import (
+from bzrlib.plugins.weave_fmt.repository import (
     RepositoryFormat4,
     RepositoryFormat5,
     RepositoryFormat6,
     )
+
 import bzrlib.ui
+from bzrlib.plugins.loom.branch import LoomSupport
+from bzrlib.transport import get_transport
 from bzrlib.ui import SilentUIFactory
-from lazr.uri import InvalidURIError
+from lazr.uri import (
+    InvalidURIError,
+    URI,
+    )
 
 from canonical.config import config
 from canonical.launchpad.webapp import errorlog
@@ -32,18 +44,21 @@
 from lp.code.enums import BranchType
 from lp.codehosting.bzrutils import identical_formats
 from lp.codehosting.puller import get_lock_id_for_branch_id
-from lp.codehosting.vfs.branchfs import (
-    BadUrlLaunchpad,
-    BadUrlScheme,
-    BadUrlSsh,
-    make_branch_mirrorer,
+from lp.codehosting.safe_open import (
+    BadUrl,
+    BranchLoopError,
+    BranchOpenPolicy,
+    BranchReferenceForbidden,
+    SafeBranchOpener,
     )
 
 
 __all__ = [
+    'BadUrlLaunchpad',
+    'BadUrlScheme',
+    'BadUrlSsh',
     'BranchMirrorer',
-    'BranchLoopError',
-    'BranchReferenceForbidden',
+    'BranchMirrorerPolicy',
     'get_canonical_url_for_branch_name',
     'install_worker_ui_factory',
     'PullerWorker',
@@ -51,19 +66,22 @@
     ]
 
 
-class BranchReferenceForbidden(Exception):
-    """Trying to mirror a branch reference and the branch type does not allow
-    references.
-    """
-
-
-class BranchLoopError(Exception):
-    """Encountered a branch cycle.
-
-    A URL may point to a branch reference or it may point to a stacked branch.
-    In either case, it's possible for there to be a cycle in these references,
-    and this exception is raised when we detect such a cycle.
-    """
+class BadUrlSsh(BadUrl):
+    """Tried to access a branch from sftp or bzr+ssh."""
+
+
+class BadUrlLaunchpad(BadUrl):
+    """Tried to access a branch from launchpad.net."""
+
+
+class BadUrlScheme(BadUrl):
+    """Found a URL with an untrusted scheme."""
+
+    def __init__(self, scheme, url):
+        BadUrl.__init__(self, scheme, url)
+        self.scheme = scheme
+
+
 
 
 def get_canonical_url_for_branch_name(unique_name):
@@ -120,12 +138,50 @@
         self.sendEvent('log', fmt % args)
 
 
+class BranchMirrorerPolicy(BranchOpenPolicy):
+    """The policy for what branches to open and how to stack them."""
+
+    def createDestinationBranch(self, source_branch, destination_url):
+        """Create a destination branch for 'source_branch'.
+
+        Creates a branch at 'destination_url' that is has the same format as
+        'source_branch'.  Any content already at 'destination_url' will be
+        deleted.  Generally the new branch will have no revisions, but they
+        will be copied for import branches, because this can be done safely
+        and efficiently with a vfs-level copy (see `ImportedBranchPolicy`).
+
+        :param source_branch: The Bazaar branch that will be mirrored.
+        :param destination_url: The place to make the destination branch. This
+            URL must point to a writable location.
+        :return: The destination branch.
+        """
+        dest_transport = get_transport(destination_url)
+        if dest_transport.has('.'):
+            dest_transport.delete_tree('.')
+        if isinstance(source_branch, LoomSupport):
+            # Looms suck.
+            revision_id = None
+        else:
+            revision_id = 'null:'
+        source_branch.bzrdir.clone_on_transport(
+            dest_transport, revision_id=revision_id)
+        return Branch.open(destination_url)
+
+    def getStackedOnURLForDestinationBranch(self, source_branch,
+                                            destination_url):
+        """Get the stacked on URL for `source_branch`.
+
+        In particular, the URL it should be stacked on when it is mirrored to
+        `destination_url`.
+        """
+        return None
+
+
 class BranchMirrorer(object):
     """A `BranchMirrorer` safely makes mirrors of branches.
 
-    A `BranchMirrorer` has a `BranchPolicy` to tell it which URLs are safe to
-    accesss, whether or not to follow branch references and how to stack
-    branches when they are mirrored.
+    A `BranchMirrorer` has a `BranchOpenPolicy` to tell it which URLs are safe
+    to accesss and whether or not to follow branch references.
 
     The mirrorer knows how to follow branch references, create new mirrors,
     update existing mirrors, determine stacked-on branches and the like.
@@ -136,92 +192,20 @@
     def __init__(self, policy, protocol=None, log=None):
         """Construct a branch opener with 'policy'.
 
-        :param policy: A `BranchPolicy` that tells us what URLs are valid and
-            similar things.
+        :param policy: A `BranchOpenPolicy` that tells us what URLs are valid
+            and similar things.
         :param log: A callable which can be called with a format string and
             arguments to log messages in the scheduler, or None, in which case
             log messages are discarded.
         """
-        self._seen_urls = set()
         self.policy = policy
         self.protocol = protocol
+        self.opener = SafeBranchOpener(policy)
         if log is not None:
             self.log = log
         else:
             self.log = lambda *args: None
 
-    def _runWithTransformFallbackLocationHookInstalled(
-            self, callable, *args, **kw):
-        Branch.hooks.install_named_hook(
-            'transform_fallback_location', self.transformFallbackLocationHook,
-            'BranchMirrorer.transformFallbackLocationHook')
-        try:
-            return callable(*args, **kw)
-        finally:
-            # XXX 2008-11-24 MichaelHudson, bug=301472: This is the hacky way
-            # to remove a hook.  The linked bug report asks for an API to do
-            # it.
-            Branch.hooks['transform_fallback_location'].remove(
-                self.transformFallbackLocationHook)
-            # We reset _seen_urls here to avoid multiple calls to open giving
-            # spurious loop exceptions.
-            self._seen_urls = set()
-
-    def open(self, url):
-        """Open the Bazaar branch at url, first checking for safety.
-
-        What safety means is defined by a subclasses `followReference` and
-        `checkOneURL` methods.
-        """
-        url = self.checkAndFollowBranchReference(url)
-        return self._runWithTransformFallbackLocationHookInstalled(
-            Branch.open, url)
-
-    def transformFallbackLocationHook(self, branch, url):
-        """Installed as the 'transform_fallback_location' Branch hook.
-
-        This method calls `transformFallbackLocation` on the policy object and
-        either returns the url it provides or passes it back to
-        checkAndFollowBranchReference.
-        """
-        new_url, check = self.policy.transformFallbackLocation(branch, url)
-        if check:
-            return self.checkAndFollowBranchReference(new_url)
-        else:
-            return new_url
-
-    def followReference(self, url):
-        """Get the branch-reference value at the specified url.
-
-        This exists as a separate method only to be overriden in unit tests.
-        """
-        bzrdir = BzrDir.open(url)
-        return bzrdir.get_branch_reference()
-
-    def checkAndFollowBranchReference(self, url):
-        """Check URL (and possibly the referenced URL) for safety.
-
-        This method checks that `url` passes the policy's `checkOneURL`
-        method, and if `url` refers to a branch reference, it checks whether
-        references are allowed and whether the reference's URL passes muster
-        also -- recursively, until a real branch is found.
-
-        :raise BranchLoopError: If the branch references form a loop.
-        :raise BranchReferenceForbidden: If this opener forbids branch
-            references.
-        """
-        while True:
-            if url in self._seen_urls:
-                raise BranchLoopError()
-            self._seen_urls.add(url)
-            self.policy.checkOneURL(url)
-            next_url = self.followReference(url)
-            if next_url is None:
-                return url
-            url = next_url
-            if not self.policy.shouldFollowReferences():
-                raise BranchReferenceForbidden(url)
-
     def createDestinationBranch(self, source_branch, destination_url):
         """Create a destination branch for 'source_branch'.
 
@@ -234,7 +218,7 @@
             URL must point to a writable location.
         :return: The destination branch.
         """
-        return self._runWithTransformFallbackLocationHookInstalled(
+        return self.opener.runWithTransformFallbackLocationHookInstalled(
             self.policy.createDestinationBranch, source_branch,
             destination_url)
 
@@ -296,6 +280,9 @@
         stacked_on_url = self.updateBranch(source_branch, branch)
         return branch, revid_before, stacked_on_url
 
+    def open(self, url):
+        return self.opener.open(url)
+
 
 class PullerWorker:
     """This class represents a single branch that needs mirroring.
@@ -305,7 +292,7 @@
     """
 
     def _checkerForBranchType(self, branch_type):
-        """Return a `BranchMirrorer` with an appropriate `BranchPolicy`.
+        """Return a `BranchMirrorer` with an appropriate policy.
 
         :param branch_type: A `BranchType`. The policy of the mirrorer will
             be based on this.
@@ -546,3 +533,151 @@
        created by another puller worker process.
     """
     bzrlib.ui.ui_factory = PullerWorkerUIFactory(puller_worker_protocol)
+
+
+class MirroredBranchPolicy(BranchMirrorerPolicy):
+    """Mirroring policy for MIRRORED branches.
+
+    In summary:
+
+     - follow references,
+     - only open non-Launchpad http: and https: URLs.
+    """
+
+    def __init__(self, stacked_on_url=None):
+        self.stacked_on_url = stacked_on_url
+
+    def getStackedOnURLForDestinationBranch(self, source_branch,
+                                            destination_url):
+        """Return the stacked on URL for the destination branch.
+
+        Mirrored branches are stacked on the default stacked-on branch of
+        their product, except when we're mirroring the default stacked-on
+        branch itself.
+        """
+        if self.stacked_on_url is None:
+            return None
+        stacked_on_url = urlutils.join(destination_url, self.stacked_on_url)
+        if destination_url == stacked_on_url:
+            return None
+        return self.stacked_on_url
+
+    def shouldFollowReferences(self):
+        """See `BranchOpenPolicy.shouldFollowReferences`.
+
+        We traverse branch references for MIRRORED branches because they
+        provide a useful redirection mechanism and we want to be consistent
+        with the bzr command line.
+        """
+        return True
+
+    def transformFallbackLocation(self, branch, url):
+        """See `BranchOpenPolicy.transformFallbackLocation`.
+
+        For mirrored branches, we stack on whatever the remote branch claims
+        to stack on, but this URL still needs to be checked.
+        """
+        return urlutils.join(branch.base, url), True
+
+    def checkOneURL(self, url):
+        """See `BranchOpenPolicy.checkOneURL`.
+
+        We refuse to mirror from Launchpad or a ssh-like or file URL.
+        """
+        # Avoid circular import
+        from lp.code.interfaces.branch import get_blacklisted_hostnames
+        uri = URI(url)
+        launchpad_domain = config.vhost.mainsite.hostname
+        if uri.underDomain(launchpad_domain):
+            raise BadUrlLaunchpad(url)
+        for hostname in get_blacklisted_hostnames():
+            if uri.underDomain(hostname):
+                raise BadUrl(url)
+        if uri.scheme in ['sftp', 'bzr+ssh']:
+            raise BadUrlSsh(url)
+        elif uri.scheme not in ['http', 'https']:
+            raise BadUrlScheme(uri.scheme, url)
+
+
+class ImportedBranchPolicy(BranchMirrorerPolicy):
+    """Mirroring policy for IMPORTED branches.
+
+    In summary:
+
+     - don't follow references,
+     - assert the URLs start with the prefix we expect for imported branches.
+    """
+
+    def createDestinationBranch(self, source_branch, destination_url):
+        """See `BranchOpenPolicy.createDestinationBranch`.
+
+        Because we control the process that creates import branches, a
+        vfs-level copy is safe and more efficient than a bzr fetch.
+        """
+        source_transport = source_branch.bzrdir.root_transport
+        dest_transport = get_transport(destination_url)
+        while True:
+            # We loop until the remote file list before and after the copy is
+            # the same to catch the case where the remote side is being
+            # mutated as we copy it.
+            if dest_transport.has('.'):
+                dest_transport.delete_tree('.')
+            files_before = set(source_transport.iter_files_recursive())
+            source_transport.copy_tree_to_transport(dest_transport)
+            files_after = set(source_transport.iter_files_recursive())
+            if files_before == files_after:
+                break
+        return Branch.open_from_transport(dest_transport)
+
+    def shouldFollowReferences(self):
+        """See `BranchOpenerPolicy.shouldFollowReferences`.
+
+        We do not traverse references for IMPORTED branches because the
+        code-import system should never produce branch references.
+        """
+        return False
+
+    def transformFallbackLocation(self, branch, url):
+        """See `BranchOpenerPolicy.transformFallbackLocation`.
+
+        Import branches should not be stacked, ever.
+        """
+        raise AssertionError("Import branch unexpectedly stacked!")
+
+    def checkOneURL(self, url):
+        """See `BranchOpenerPolicy.checkOneURL`.
+
+        If the URL we are mirroring from does not start how we expect the pull
+        URLs of import branches to start, something has gone badly wrong, so
+        we raise AssertionError if that's happened.
+        """
+        if not url.startswith(config.launchpad.bzr_imports_root_url):
+            raise AssertionError(
+                "Bogus URL for imported branch: %r" % url)
+
+
+def make_branch_mirrorer(branch_type, protocol=None,
+                         mirror_stacked_on_url=None):
+    """Create a `BranchMirrorer` with the appropriate `BranchOpenerPolicy`.
+
+    :param branch_type: A `BranchType` to select a policy by.
+    :param protocol: Optional protocol for the mirrorer to work with.
+        If given, its log will also be used.
+    :param mirror_stacked_on_url: For mirrored branches, the default URL
+        to stack on.  Ignored for other branch types.
+    :return: A `BranchMirrorer`.
+    """
+    if branch_type == BranchType.MIRRORED:
+        policy = MirroredBranchPolicy(mirror_stacked_on_url)
+    elif branch_type == BranchType.IMPORTED:
+        policy = ImportedBranchPolicy()
+    else:
+        raise AssertionError(
+            "Unexpected branch type: %r" % branch_type)
+
+    if protocol is not None:
+        log_function = protocol.log
+    else:
+        log_function = None
+
+    return BranchMirrorer(policy, protocol, log_function)

=== added file 'lib/lp/codehosting/safe_open.py'
--- lib/lp/codehosting/safe_open.py	1970-01-01 00:00:00 +0000
+++ lib/lp/codehosting/safe_open.py	2011-08-07 20:42:25 +0000
@@ -0,0 +1,230 @@
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Safe branch opening."""
+
+__metaclass__ = type
+
+from bzrlib import urlutils
+from bzrlib.branch import Branch
+from bzrlib.bzrdir import BzrDir
+
+__all__ = [
+    'AcceptAnythingPolicy',
+    'BadUrl',
+    'BlacklistPolicy',
+    'BranchLoopError',
+    'BranchOpenPolicy',
+    'BranchReferenceForbidden',
+    'SafeBranchOpener',
+    'WhitelistPolicy',
+    ]
+
+
+# TODO JelmerVernooij 2011-08-06: This module is generic enough to be
+# in bzrlib, and may be of use to others.
+
+
+class BadUrl(Exception):
+    """Tried to access a branch from a bad URL."""
+
+
+class BranchReferenceForbidden(Exception):
+    """Trying to mirror a branch reference and the branch type does not allow
+    references.
+    """
+
+
+class BranchLoopError(Exception):
+    """Encountered a branch cycle.
+
+    A URL may point to a branch reference or it may point to a stacked branch.
+    In either case, it's possible for there to be a cycle in these references,
+    and this exception is raised when we detect such a cycle.
+    """
+
+
+class BranchOpenPolicy:
+    """Policy on how to open branches.
+
+    In particular, a policy determines which branches are safe to open by
+    checking their URLs and deciding whether or not to follow branch
+    references.
+    """
+
+    def shouldFollowReferences(self):
+        """Whether we traverse references when mirroring.
+
+        Subclasses must override this method.
+
+        If we encounter a branch reference and this returns false, an error is
+        raised.
+
+        :returns: A boolean to indicate whether to follow a branch reference.
+        """
+        raise NotImplementedError(self.shouldFollowReferences)
+
+    def transformFallbackLocation(self, branch, url):
+        """Validate, maybe modify, 'url' to be used as a stacked-on location.
+
+        :param branch:  The branch that is being opened.
+        :param url: The URL that the branch provides for its stacked-on
+            location.
+        :return: (new_url, check) where 'new_url' is the URL of the branch to
+            actually open and 'check' is true if 'new_url' needs to be
+            validated by checkAndFollowBranchReference.
+        """
+        raise NotImplementedError(self.transformFallbackLocation)
+
+    def checkOneURL(self, url):
+        """Check the safety of the source URL.
+
+        Subclasses must override this method.
+
+        :param url: The source URL to check.
+        :raise BadUrl: subclasses are expected to raise this or a subclass
+            when it finds a URL it deems to be unsafe.
+        """
+        raise NotImplementedError(self.checkOneURL)
+
+
+class BlacklistPolicy(BranchOpenPolicy):
+    """Branch policy that forbids certain URLs."""
+
+    def __init__(self, should_follow_references, unsafe_urls=None):
+        if unsafe_urls is None:
+            unsafe_urls = set()
+        self._unsafe_urls = unsafe_urls
+        self._should_follow_references = should_follow_references
+
+    def shouldFollowReferences(self):
+        return self._should_follow_references
+
+    def checkOneURL(self, url):
+        if url in self._unsafe_urls:
+            raise BadUrl(url)
+
+    def transformFallbackLocation(self, branch, url):
+        """See `BranchOpenPolicy.transformFallbackLocation`.
+
+        This class is not used for testing our smarter stacking features so we
+        just do the simplest thing: return the URL that would be used anyway
+        and don't check it.
+        """
+        return urlutils.join(branch.base, url), False
+
+
+class AcceptAnythingPolicy(BlacklistPolicy):
+    """Accept anything, to make testing easier."""
+
+    def __init__(self):
+        super(AcceptAnythingPolicy, self).__init__(True, set())
+
+
+class WhitelistPolicy(BranchOpenPolicy):
+    """Branch policy that only allows certain URLs."""
+
+    def __init__(self, should_follow_references, allowed_urls=None,
+                 check=False):
+        if allowed_urls is None:
+            allowed_urls = []
+        self.allowed_urls = set(url.rstrip('/') for url in allowed_urls)
+        self.check = check
+
+    def shouldFollowReferences(self):
+        return self._should_follow_references
+
+    def checkOneURL(self, url):
+        if url.rstrip('/') not in self.allowed_urls:
+            raise BadUrl(url)
+
+    def transformFallbackLocation(self, branch, url):
+        """See `BranchOpenPolicy.transformFallbackLocation`.
+
+        Here we return the URL that would be used anyway and optionally check
+        it.
+        """
+        return urlutils.join(branch.base, url), self.check
+
+
+class SafeBranchOpener(object):
+    """Safe branch opener.
+
+    The policy object is expected to have the following methods:
+    * checkOneURL
+    * shouldFollowReferences
+    * transformFallbackLocation
+    """
+
+    def __init__(self, policy):
+        self.policy = policy
+        self._seen_urls = set()
+
+    def checkAndFollowBranchReference(self, url):
+        """Check URL (and possibly the referenced URL) for safety.
+
+        This method checks that `url` passes the policy's `checkOneURL`
+        method, and if `url` refers to a branch reference, it checks whether
+        references are allowed and whether the reference's URL passes muster
+        also -- recursively, until a real branch is found.
+
+        :raise BranchLoopError: If the branch references form a loop.
+        :raise BranchReferenceForbidden: If this opener forbids branch
+            references.
+        """
+        while True:
+            if url in self._seen_urls:
+                raise BranchLoopError()
+            self._seen_urls.add(url)
+            self.policy.checkOneURL(url)
+            next_url = self.followReference(url)
+            if next_url is None:
+                return url
+            url = next_url
+            if not self.policy.shouldFollowReferences():
+                raise BranchReferenceForbidden(url)
+
+    def transformFallbackLocationHook(self, branch, url):
+        """Installed as the 'transform_fallback_location' Branch hook.
+
+        This method calls `transformFallbackLocation` on the policy object and
+        either returns the url it provides or passes it back to
+        checkAndFollowBranchReference.
+        """
+        new_url, check = self.policy.transformFallbackLocation(branch, url)
+        if check:
+            return self.checkAndFollowBranchReference(new_url)
+        else:
+            return new_url
+
+    def runWithTransformFallbackLocationHookInstalled(
+            self, callable, *args, **kw):
+        Branch.hooks.install_named_hook(
+            'transform_fallback_location', self.transformFallbackLocationHook,
+            'SafeBranchOpener.transformFallbackLocationHook')
+        try:
+            return callable(*args, **kw)
+        finally:
+            Branch.hooks.uninstall_named_hook('transform_fallback_location',
+                'SafeBranchOpener.transformFallbackLocationHook')
+            # We reset _seen_urls here to avoid multiple calls to open giving
+            # spurious loop exceptions.
+            self._seen_urls = set()
+
+    def followReference(self, url):
+        """Get the branch-reference value at the specified url.
+
+        This exists as a separate method only to be overriden in unit tests.
+        """
+        bzrdir = BzrDir.open(url)
+        return bzrdir.get_branch_reference()
+
+    def open(self, url):
+        """Open the Bazaar branch at url, first checking for safety.
+
+        What safety means is defined by a subclasses `followReference` and
+        `checkOneURL` methods.
+        """
+        url = self.checkAndFollowBranchReference(url)
+        return self.runWithTransformFallbackLocationHookInstalled(
+            Branch.open, url)

=== added file 'lib/lp/codehosting/tests/test_safe_open.py'
--- lib/lp/codehosting/tests/test_safe_open.py	1970-01-01 00:00:00 +0000
+++ lib/lp/codehosting/tests/test_safe_open.py	2011-08-07 20:42:25 +0000
@@ -0,0 +1,223 @@
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the safe branch open code."""
+
+
+__metaclass__ = type
+
+
+from lp.codehosting.puller.worker import (
+    BranchLoopError,
+    BranchReferenceForbidden,
+    SafeBranchOpener,
+    )
+from lp.codehosting.safe_open import (
+    BadUrl,
+    BlacklistPolicy,
+    WhitelistPolicy,
+    )
+
+from lp.testing import TestCase
+
+from bzrlib.branch import (
+    BzrBranchFormat7,
+    )
+from bzrlib.bzrdir import (
+    BzrDirMetaFormat1,
+    )
+from bzrlib.repofmt.pack_repo import RepositoryFormatKnitPack1
+from bzrlib.tests import (
+    TestCaseWithTransport,
+    )
+
+
+class TestSafeBranchOpenerCheckAndFollowBranchReference(TestCase):
+    """Unit tests for `SafeBranchOpener.checkAndFollowBranchReference`."""
+
+    class StubbedSafeBranchOpener(SafeBranchOpener):
+        """SafeBranchOpener that provides canned answers.
+
+        We implement the methods we need to to be able to control all the
+        inputs to the `BranchMirrorer.checkSource` method, which is what is
+        being tested in this class.
+        """
+
+        def __init__(self, references, policy):
+            parent_cls = TestSafeBranchOpenerCheckAndFollowBranchReference
+            super(parent_cls.StubbedSafeBranchOpener, self).__init__(policy)
+            self._reference_values = {}
+            for i in range(len(references) - 1):
+                self._reference_values[references[i]] = references[i+1]
+            self.follow_reference_calls = []
+
+        def followReference(self, url):
+            self.follow_reference_calls.append(url)
+            return self._reference_values[url]
+
+    def makeBranchOpener(self, should_follow_references, references,
+                         unsafe_urls=None):
+        policy = BlacklistPolicy(should_follow_references, unsafe_urls)
+        opener = self.StubbedSafeBranchOpener(references, policy)
+        return opener
+
+    def testCheckInitialURL(self):
+        # checkSource rejects all URLs that are not allowed.
+        opener = self.makeBranchOpener(None, [], set(['a']))
+        self.assertRaises(BadUrl, opener.checkAndFollowBranchReference, 'a')
+
+    def testNotReference(self):
+        # When branch references are forbidden, checkAndFollowBranchReference
+        # does not raise on non-references.
+        opener = self.makeBranchOpener(False, ['a', None])
+        self.assertEquals('a', opener.checkAndFollowBranchReference('a'))
+        self.assertEquals(['a'], opener.follow_reference_calls)
+
+    def testBranchReferenceForbidden(self):
+        # checkAndFollowBranchReference raises BranchReferenceForbidden if
+        # branch references are forbidden and the source URL points to a
+        # branch reference.
+        opener = self.makeBranchOpener(False, ['a', 'b'])
+        self.assertRaises(
+            BranchReferenceForbidden,
+            opener.checkAndFollowBranchReference, 'a')
+        self.assertEquals(['a'], opener.follow_reference_calls)
+
+    def testAllowedReference(self):
+        # checkAndFollowBranchReference does not raise if following references
+        # is allowed and the source URL points to a branch reference to a
+        # permitted location.
+        opener = self.makeBranchOpener(True, ['a', 'b', None])
+        self.assertEquals('b', opener.checkAndFollowBranchReference('a'))
+        self.assertEquals(['a', 'b'], opener.follow_reference_calls)
+
+    def testCheckReferencedURLs(self):
+        # checkAndFollowBranchReference checks if the URL a reference points
+        # to is safe.
+        opener = self.makeBranchOpener(
+            True, ['a', 'b', None], unsafe_urls=set('b'))
+        self.assertRaises(BadUrl, opener.checkAndFollowBranchReference, 'a')
+        self.assertEquals(['a'], opener.follow_reference_calls)
+
+    def testSelfReferencingBranch(self):
+        # checkAndFollowBranchReference raises BranchReferenceLoopError if
+        # following references is allowed and the source url points to a
+        # self-referencing branch reference.
+        opener = self.makeBranchOpener(True, ['a', 'a'])
+        self.assertRaises(
+            BranchLoopError, opener.checkAndFollowBranchReference, 'a')
+        self.assertEquals(['a'], opener.follow_reference_calls)
+
+    def testBranchReferenceLoop(self):
+        # checkAndFollowBranchReference raises BranchReferenceLoopError if
+        # following references is allowed and the source url points to a loop
+        # of branch references.
+        references = ['a', 'b', 'a']
+        opener = self.makeBranchOpener(True, references)
+        self.assertRaises(
+            BranchLoopError, opener.checkAndFollowBranchReference, 'a')
+        self.assertEquals(['a', 'b'], opener.follow_reference_calls)
+
+
+class TestSafeBranchOpenerStacking(TestCaseWithTransport):
+
+    def makeBranchOpener(self, allowed_urls):
+        policy = WhitelistPolicy(True, allowed_urls, True)
+        return SafeBranchOpener(policy)
+
+    def makeBranch(self, path, branch_format, repository_format):
+        """Make a Bazaar branch at 'path' with the given formats."""
+        bzrdir_format = BzrDirMetaFormat1()
+        bzrdir_format.set_branch_format(branch_format)
+        bzrdir = self.make_bzrdir(path, format=bzrdir_format)
+        repository_format.initialize(bzrdir)
+        return bzrdir.create_branch()
+
+    def testAllowedURL(self):
+        # checkSource does not raise an exception for branches stacked on
+        # branches with allowed URLs.
+        stacked_on_branch = self.make_branch('base-branch', format='1.6')
+        stacked_branch = self.make_branch('stacked-branch', format='1.6')
+        stacked_branch.set_stacked_on_url(stacked_on_branch.base)
+        opener = self.makeBranchOpener(
+            [stacked_branch.base, stacked_on_branch.base])
+        # This doesn't raise an exception.
+        opener.open(stacked_branch.base)
+
+    def testUnstackableRepository(self):
+        # checkSource treats branches with UnstackableRepositoryFormats as
+        # being not stacked.
+        branch = self.makeBranch(
+            'unstacked', BzrBranchFormat7(), RepositoryFormatKnitPack1())
+        opener = self.makeBranchOpener([branch.base])
+        # This doesn't raise an exception.
+        opener.open(branch.base)
+
+    def testAllowedRelativeURL(self):
+        # checkSource passes on absolute urls to checkOneURL, even if the
+        # value of stacked_on_location in the config is set to a relative URL.
+        stacked_on_branch = self.make_branch('base-branch', format='1.6')
+        stacked_branch = self.make_branch('stacked-branch', format='1.6')
+        stacked_branch.set_stacked_on_url('../base-branch')
+        opener = self.makeBranchOpener(
+            [stacked_branch.base, stacked_on_branch.base])
+        # Note that stacked_on_branch.base is not '../base-branch', it's an
+        # absolute URL.
+        self.assertNotEqual('../base-branch', stacked_on_branch.base)
+        # This doesn't raise an exception.
+        opener.open(stacked_branch.base)
+
+    def testAllowedRelativeNested(self):
+        # Relative URLs are resolved relative to the stacked branch.
+        self.get_transport().mkdir('subdir')
+        a = self.make_branch('subdir/a', format='1.6')
+        b = self.make_branch('b', format='1.6')
+        b.set_stacked_on_url('../subdir/a')
+        c = self.make_branch('subdir/c', format='1.6')
+        c.set_stacked_on_url('../../b')
+        opener = self.makeBranchOpener([c.base, b.base, a.base])
+        # This doesn't raise an exception.
+        opener.open(c.base)
+
+    def testForbiddenURL(self):
+        # checkSource raises a BadUrl exception if a branch is stacked on a
+        # branch with a forbidden URL.
+        stacked_on_branch = self.make_branch('base-branch', format='1.6')
+        stacked_branch = self.make_branch('stacked-branch', format='1.6')
+        stacked_branch.set_stacked_on_url(stacked_on_branch.base)
+        opener = self.makeBranchOpener([stacked_branch.base])
+        self.assertRaises(BadUrl, opener.open, stacked_branch.base)
+
+    def testForbiddenURLNested(self):
+        # checkSource raises a BadUrl exception if a branch is stacked on a
+        # branch that is in turn stacked on a branch with a forbidden URL.
+        a = self.make_branch('a', format='1.6')
+        b = self.make_branch('b', format='1.6')
+        b.set_stacked_on_url(a.base)
+        c = self.make_branch('c', format='1.6')
+        c.set_stacked_on_url(b.base)
+        opener = self.makeBranchOpener([c.base, b.base])
+        self.assertRaises(BadUrl, opener.open, c.base)
+
+    def testSelfStackedBranch(self):
+        # checkSource raises StackingLoopError if a branch is stacked on
+        # itself. This avoids infinite recursion errors.
+        a = self.make_branch('a', format='1.6')
+        # Bazaar 1.17 and up make it harder to create branches like this.
+        # It's still worth testing that we don't blow up in the face of them,
+        # so we grovel around a bit to create one anyway.
+        a.get_config().set_user_option('stacked_on_location', a.base)
+        opener = self.makeBranchOpener([a.base])
+        self.assertRaises(BranchLoopError, opener.open, a.base)
+
+    def testLoopStackedBranch(self):
+        # checkSource raises StackingLoopError if a branch is stacked in such
+        # a way so that it is ultimately stacked on itself. e.g. a stacked on
+        # b stacked on a.
+        a = self.make_branch('a', format='1.6')
+        b = self.make_branch('b', format='1.6')
+        a.set_stacked_on_url(b.base)
+        b.set_stacked_on_url(a.base)
+        opener = self.makeBranchOpener([a.base, b.base])
+        self.assertRaises(BranchLoopError, opener.open, a.base)
+        self.assertRaises(BranchLoopError, opener.open, b.base)

=== modified file 'lib/lp/codehosting/vfs/__init__.py'
--- lib/lp/codehosting/vfs/__init__.py	2010-09-22 17:16:19 +0000
+++ lib/lp/codehosting/vfs/__init__.py	2011-08-07 20:42:25 +0000
@@ -11,7 +11,6 @@
     'get_ro_server',
     'get_rw_server',
     'LaunchpadServer',
-    'make_branch_mirrorer',
     ]
 
 from lp.codehosting.vfs.branchfs import (
@@ -21,7 +20,6 @@
     get_ro_server,
     get_rw_server,
     LaunchpadServer,
-    make_branch_mirrorer,
     )
 from lp.codehosting.vfs.branchfsclient import (
     BranchFileSystemClient,

=== modified file 'lib/lp/codehosting/vfs/branchfs.py'
--- lib/lp/codehosting/vfs/branchfs.py	2011-05-25 19:57:07 +0000
+++ lib/lp/codehosting/vfs/branchfs.py	2011-08-07 20:42:25 +0000
@@ -46,17 +46,14 @@
 __metaclass__ = type
 __all__ = [
     'AsyncLaunchpadTransport',
-    'BadUrl',
     'BadUrlLaunchpad',
     'BadUrlScheme',
     'BadUrlSsh',
     'branch_id_to_path',
-    'BranchPolicy',
     'DirectDatabaseLaunchpadServer',
     'get_lp_server',
     'get_ro_server',
     'get_rw_server',
-    'make_branch_mirrorer',
     'LaunchpadInternalServer',
     'LaunchpadServer',
     ]
@@ -65,7 +62,6 @@
 import xmlrpclib
 
 from bzrlib import urlutils
-from bzrlib.branch import Branch
 from bzrlib.bzrdir import (
     BzrDir,
     BzrDirFormat,
@@ -76,7 +72,6 @@
     PermissionDenied,
     TransportNotPossible,
     )
-from bzrlib.plugins.loom.branch import LoomSupport
 from bzrlib.smart.request import jail_info
 from bzrlib.transport import get_transport
 from bzrlib.transport.memory import MemoryServer
@@ -98,7 +93,6 @@
 from canonical.config import config
 from canonical.launchpad.webapp import errorlog
 from canonical.launchpad.xmlrpc import faults
-from lp.code.enums import BranchType
 from lp.code.interfaces.branchlookup import IBranchLookup
 from lp.code.interfaces.codehosting import (
     BRANCH_TRANSPORT,
@@ -126,26 +120,6 @@
     )
 
 
-class BadUrl(Exception):
-    """Tried to access a branch from a bad URL."""
-
-
-class BadUrlSsh(BadUrl):
-    """Tried to access a branch from sftp or bzr+ssh."""
-
-
-class BadUrlLaunchpad(BadUrl):
-    """Tried to access a branch from launchpad.net."""
-
-
-class BadUrlScheme(BadUrl):
-    """Found a URL with an untrusted scheme."""
-
-    def __init__(self, scheme, url):
-        BadUrl.__init__(self, scheme, url)
-        self.scheme = scheme
-
-
 # The directories allowed directly beneath a branch directory. These are the
 # directories that Bazaar creates as part of regular operation. We support
 # only two numbered backups to avoid indefinite space usage.
@@ -774,234 +748,3 @@
         seen_new_branch_hook)
     return lp_server
 
-
-class BranchPolicy:
-    """Policy on how to mirror branches.
-
-    In particular, a policy determines which branches are safe to mirror by
-    checking their URLs and deciding whether or not to follow branch
-    references. A policy also determines how the mirrors of branches should be
-    stacked.
-    """
-
-    def createDestinationBranch(self, source_branch, destination_url):
-        """Create a destination branch for 'source_branch'.
-
-        Creates a branch at 'destination_url' that is has the same format as
-        'source_branch'.  Any content already at 'destination_url' will be
-        deleted.  Generally the new branch will have no revisions, but they
-        will be copied for import branches, because this can be done safely
-        and efficiently with a vfs-level copy (see `ImportedBranchPolicy`,
-        below).
-
-        :param source_branch: The Bazaar branch that will be mirrored.
-        :param destination_url: The place to make the destination branch. This
-            URL must point to a writable location.
-        :return: The destination branch.
-        """
-        dest_transport = get_transport(destination_url)
-        if dest_transport.has('.'):
-            dest_transport.delete_tree('.')
-        if isinstance(source_branch, LoomSupport):
-            # Looms suck.
-            revision_id = None
-        else:
-            revision_id = 'null:'
-        source_branch.bzrdir.clone_on_transport(
-            dest_transport, revision_id=revision_id)
-        return Branch.open(destination_url)
-
-    def getStackedOnURLForDestinationBranch(self, source_branch,
-                                            destination_url):
-        """Get the stacked on URL for `source_branch`.
-
-        In particular, the URL it should be stacked on when it is mirrored to
-        `destination_url`.
-        """
-        return None
-
-    def shouldFollowReferences(self):
-        """Whether we traverse references when mirroring.
-
-        Subclasses must override this method.
-
-        If we encounter a branch reference and this returns false, an error is
-        raised.
-
-        :returns: A boolean to indicate whether to follow a branch reference.
-        """
-        raise NotImplementedError(self.shouldFollowReferences)
-
-    def transformFallbackLocation(self, branch, url):
-        """Validate, maybe modify, 'url' to be used as a stacked-on location.
-
-        :param branch:  The branch that is being opened.
-        :param url: The URL that the branch provides for its stacked-on
-            location.
-        :return: (new_url, check) where 'new_url' is the URL of the branch to
-            actually open and 'check' is true if 'new_url' needs to be
-            validated by checkAndFollowBranchReference.
-        """
-        raise NotImplementedError(self.transformFallbackLocation)
-
-    def checkOneURL(self, url):
-        """Check the safety of the source URL.
-
-        Subclasses must override this method.
-
-        :param url: The source URL to check.
-        :raise BadUrl: subclasses are expected to raise this or a subclass
-            when it finds a URL it deems to be unsafe.
-        """
-        raise NotImplementedError(self.checkOneURL)
-
-
-class MirroredBranchPolicy(BranchPolicy):
-    """Mirroring policy for MIRRORED branches.
-
-    In summary:
-
-     - follow references,
-     - only open non-Launchpad http: and https: URLs.
-    """
-
-    def __init__(self, stacked_on_url=None):
-        self.stacked_on_url = stacked_on_url
-
-    def getStackedOnURLForDestinationBranch(self, source_branch,
-                                            destination_url):
-        """See `BranchPolicy.getStackedOnURLForDestinationBranch`.
-
-        Mirrored branches are stacked on the default stacked-on branch of
-        their product, except when we're mirroring the default stacked-on
-        branch itself.
-        """
-        if self.stacked_on_url is None:
-            return None
-        stacked_on_url = urlutils.join(destination_url, self.stacked_on_url)
-        if destination_url == stacked_on_url:
-            return None
-        return self.stacked_on_url
-
-    def shouldFollowReferences(self):
-        """See `BranchPolicy.shouldFollowReferences`.
-
-        We traverse branch references for MIRRORED branches because they
-        provide a useful redirection mechanism and we want to be consistent
-        with the bzr command line.
-        """
-        return True
-
-    def transformFallbackLocation(self, branch, url):
-        """See `BranchPolicy.transformFallbackLocation`.
-
-        For mirrored branches, we stack on whatever the remote branch claims
-        to stack on, but this URL still needs to be checked.
-        """
-        return urlutils.join(branch.base, url), True
-
-    def checkOneURL(self, url):
-        """See `BranchPolicy.checkOneURL`.
-
-        We refuse to mirror from Launchpad or a ssh-like or file URL.
-        """
-        # Avoid circular import
-        from lp.code.interfaces.branch import get_blacklisted_hostnames
-        uri = URI(url)
-        launchpad_domain = config.vhost.mainsite.hostname
-        if uri.underDomain(launchpad_domain):
-            raise BadUrlLaunchpad(url)
-        for hostname in get_blacklisted_hostnames():
-            if uri.underDomain(hostname):
-                raise BadUrl(url)
-        if uri.scheme in ['sftp', 'bzr+ssh']:
-            raise BadUrlSsh(url)
-        elif uri.scheme not in ['http', 'https']:
-            raise BadUrlScheme(uri.scheme, url)
-
-
-class ImportedBranchPolicy(BranchPolicy):
-    """Mirroring policy for IMPORTED branches.
-
-    In summary:
-
-     - don't follow references,
-     - assert the URLs start with the prefix we expect for imported branches.
-    """
-
-    def createDestinationBranch(self, source_branch, destination_url):
-        """See `BranchPolicy.createDestinationBranch`.
-
-        Because we control the process that creates import branches, a
-        vfs-level copy is safe and more efficient than a bzr fetch.
-        """
-        source_transport = source_branch.bzrdir.root_transport
-        dest_transport = get_transport(destination_url)
-        while True:
-            # We loop until the remote file list before and after the copy is
-            # the same to catch the case where the remote side is being
-            # mutated as we copy it.
-            if dest_transport.has('.'):
-                dest_transport.delete_tree('.')
-            files_before = set(source_transport.iter_files_recursive())
-            source_transport.copy_tree_to_transport(dest_transport)
-            files_after = set(source_transport.iter_files_recursive())
-            if files_before == files_after:
-                break
-        return Branch.open_from_transport(dest_transport)
-
-    def shouldFollowReferences(self):
-        """See `BranchPolicy.shouldFollowReferences`.
-
-        We do not traverse references for IMPORTED branches because the
-        code-import system should never produce branch references.
-        """
-        return False
-
-    def transformFallbackLocation(self, branch, url):
-        """See `BranchPolicy.transformFallbackLocation`.
-
-        Import branches should not be stacked, ever.
-        """
-        raise AssertionError("Import branch unexpectedly stacked!")
-
-    def checkOneURL(self, url):
-        """See `BranchPolicy.checkOneURL`.
-
-        If the URL we are mirroring from does not start how we expect the pull
-        URLs of import branches to start, something has gone badly wrong, so
-        we raise AssertionError if that's happened.
-        """
-        if not url.startswith(config.launchpad.bzr_imports_root_url):
-            raise AssertionError(
-                "Bogus URL for imported branch: %r" % url)
-
-
-def make_branch_mirrorer(branch_type, protocol=None,
-                         mirror_stacked_on_url=None):
-    """Create a `BranchMirrorer` with the appropriate `BranchPolicy`.
-
-    :param branch_type: A `BranchType` to select a policy by.
-    :param protocol: Optional protocol for the mirrorer to work with.
-        If given, its log will also be used.
-    :param mirror_stacked_on_url: For mirrored branches, the default URL
-        to stack on.  Ignored for other branch types.
-    :return: A `BranchMirrorer`.
-    """
-    # Avoid circular import
-    from lp.codehosting.puller.worker import BranchMirrorer
-
-    if branch_type == BranchType.MIRRORED:
-        policy = MirroredBranchPolicy(mirror_stacked_on_url)
-    elif branch_type == BranchType.IMPORTED:
-        policy = ImportedBranchPolicy()
-    else:
-        raise AssertionError(
-            "Unexpected branch type: %r" % branch_type)
-
-    if protocol is not None:
-        log_function = protocol.log
-    else:
-        log_function = None
-
-    return BranchMirrorer(policy, protocol, log_function)

=== 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-07 20:42:25 +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/testing/factory.py'
--- lib/lp/testing/factory.py	2011-08-05 06:40:47 +0000
+++ lib/lp/testing/factory.py	2011-08-07 20:42:25 +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()
@@ -2117,7 +2117,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.
 
@@ -2126,7 +2127,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:
@@ -2157,6 +2158,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 'lib/lp/translations/scripts/translations_to_branch.py'
--- lib/lp/translations/scripts/translations_to_branch.py	2011-05-27 13:36:02 +0000
+++ lib/lp/translations/scripts/translations_to_branch.py	2011-08-07 20:42:25 +0000
@@ -14,6 +14,7 @@
 import os.path
 
 from bzrlib.errors import NotBranchError
+from bzrlib.revision import NULL_REVISION
 import pytz
 from storm.expr import (
     And,
@@ -157,7 +158,9 @@
 
         revno, current_rev = branch.last_revision_info()
         repository = branch.repository
-        for rev_id in repository.iter_reverse_revision_history(current_rev):
+        graph = repository.get_graph()
+        for rev_id in graph.iter_lefthand_ancestry(
+                current_rev, (NULL_REVISION, )):
             revision = repository.get_revision(rev_id)
             revision_date = self._getRevisionTime(revision)
             if self._isTranslationsCommit(revision):

=== modified file 'scripts/code-import-worker.py'
--- scripts/code-import-worker.py	2011-06-16 23:43:04 +0000
+++ scripts/code-import-worker.py	2011-08-07 20:42:25 +0000
@@ -26,8 +26,9 @@
 from canonical.config import config
 from lp.codehosting import load_optional_plugin
 from lp.codehosting.codeimport.worker import (
-    BzrSvnImportWorker, CSCVSImportWorker, CodeImportSourceDetails,
-    GitImportWorker, HgImportWorker, get_default_bazaar_branch_store)
+    BzrImportWorker, BzrSvnImportWorker, CSCVSImportWorker,
+    CodeImportSourceDetails, GitImportWorker, HgImportWorker,
+    get_default_bazaar_branch_store)
 from canonical.launchpad import scripts
 
 
@@ -65,6 +66,10 @@
         elif source_details.rcstype == 'hg':
             load_optional_plugin('hg')
             import_worker_cls = HgImportWorker
+        elif source_details.rcstype == 'bzr':
+            load_optional_plugin('loom')
+            load_optional_plugin('weave_fmt')
+            import_worker_cls = BzrImportWorker
         elif source_details.rcstype in ['cvs', 'svn']:
             import_worker_cls = CSCVSImportWorker
         else:

=== modified file 'versions.cfg'
--- versions.cfg	2011-08-03 13:05:12 +0000
+++ versions.cfg	2011-08-07 20:42:25 +0000
@@ -7,7 +7,7 @@
 ampoule = 0.2.0
 amqplib = 0.6.1
 BeautifulSoup = 3.1.0.1
-bzr = 2.3.3
+bzr = 2.4b4-r6001
 chameleon.core = 1.0b35
 chameleon.zpt = 1.0b17
 ClientForm = 0.2.10