Merge lp:~mwhudson/launchpad/puller-import-hack into lp:launchpad
- puller-import-hack
- Merge into devel
Proposed by
Michael Hudson-Doyle
| Status: | Merged |
|---|---|
| Approved by: | Aaron Bentley |
| Approved revision: | no longer in the source branch. |
| Merged at revision: | not available |
| Proposed branch: | lp:~mwhudson/launchpad/puller-import-hack |
| Merge into: | lp:launchpad |
| Diff against target: |
324 lines (+90/-80) 6 files modified
configs/testrunner/launchpad-lazr.conf (+0/-1) lib/lp/codehosting/codeimport/worker.py (+12/-8) lib/lp/codehosting/puller/tests/__init__.py (+1/-0) lib/lp/codehosting/puller/tests/test_acceptance.py (+10/-26) lib/lp/codehosting/puller/worker.py (+4/-36) lib/lp/codehosting/vfs/branchfs.py (+63/-9) |
| To merge this branch: | bzr merge lp:~mwhudson/launchpad/puller-import-hack |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Aaron Bentley (community) | Approve | ||
|
Review via email:
|
|||
Commit message
Use vfs-level copies when moving import branches around -- faster than 2a fetch, and safe because we control how the branches are made.
Description of the change
Hi,
This branch adds a few hacks to the handling of code imports to avoid the problems of cpu intensive slow fetch.
In particular, when getting the branch from the central area to the slave and when pulling an imported branch to the codehost for the first time, just copy the branch at the vfs level.
Deploying this on production depends on https:/
Cheers,
mwh
To post a comment you must log in.
Revision history for this message
| Michael Hudson-Doyle (mwhudson) wrote : | # |
On 15/04/10 03:41, Aaron Bentley wrote:
> Review: Needs Information
> What is the impact of removing the testrunner bzr_imports_
Yes, the value in the development config is a file:/// url.
> Instead of remote_
Ah, cool, thanks.
> Some of the test changes look like drive-bys, e.g. not wanting it to leave directories behind. Is that right?
I don't think I've really changed anything here, just folded the
addCleanup that followed every invocation of makeCleanDirectory into the
method itself.
> Is there a reason to join config.
No, none. Fixed.
> Did you consider using more specific name than _is_import? e.g. _vfs_initial_clone ? Or what about providing an initial_clone method on the BranchPolicy itself?
I thought about that name for about a microsecond, I have to admit.
Making it a method on the policy makes sense, partly because then it can
be tested separately...
> What about using sets to represent files_before and files_after? That's closer to the abstract representation, because order isn't important, and entries can only appear once. I don't really understand the purpose of this loop, either.
Yes, good point.
The point of the loop is to cope with the not-quite-
possibility that the branch being copied might be being written to as we
grab it. I think it can be written a bit more clearly now you mention
it, take a look.
> It might be nice to turn _runWithTransfo
Please no :-)
I think I have a strategy for getting rid of using the transform
fallback hook entirely now bzr has an ignore_fallbacks argument for
branch opening, but I'm more likely to do that in my no-hosted-area work
than here :)
Interdiff attached, although just looking at the preview diff again is
likely just as clear.
Cheers,
mwh
| 1 | === modified file 'lib/lp/codehosting/puller/tests/test_acceptance.py' |
| 2 | --- lib/lp/codehosting/puller/tests/test_acceptance.py 2010-04-14 03:07:42 +0000 |
| 3 | +++ lib/lp/codehosting/puller/tests/test_acceptance.py 2010-04-14 20:57:52 +0000 |
| 4 | @@ -20,7 +20,7 @@ |
| 5 | from bzrlib.tests import HttpServer |
| 6 | from bzrlib.transport import get_transport |
| 7 | from bzrlib.upgrade import upgrade |
| 8 | -from bzrlib.urlutils import local_path_from_url |
| 9 | +from bzrlib.urlutils import join as urljoin, local_path_from_url |
| 10 | |
| 11 | from zope.component import getUtility |
| 12 | from zope.security.proxy import removeSecurityProxy |
| 13 | @@ -397,7 +397,7 @@ |
| 14 | transaction.commit() |
| 15 | |
| 16 | # Create the Bazaar branch in the expected location. |
| 17 | - branch_url = os.path.join( |
| 18 | + branch_url = urljoin( |
| 19 | config.launchpad.bzr_imports_root_url, '%08x' % db_branch.id) |
| 20 | branch = BzrDir.create_branch_convenience(branch_url) |
| 21 | tree = branch.bzrdir.open_workingtree() |
| 22 | |
| 23 | === modified file 'lib/lp/codehosting/puller/worker.py' |
| 24 | --- lib/lp/codehosting/puller/worker.py 2010-04-14 03:18:02 +0000 |
| 25 | +++ lib/lp/codehosting/puller/worker.py 2010-04-15 01:52:27 +0000 |
| 26 | @@ -11,9 +11,6 @@ |
| 27 | from bzrlib.branch import Branch |
| 28 | from bzrlib.bzrdir import BzrDir |
| 29 | from bzrlib import errors |
| 30 | -from bzrlib.plugins.loom.branch import LoomSupport |
| 31 | -from bzrlib.transport import get_transport |
| 32 | -from bzrlib import urlutils |
| 33 | from bzrlib.ui import SilentUIFactory |
| 34 | import bzrlib.ui |
| 35 | |
| 36 | @@ -227,51 +224,14 @@ |
| 37 | 'source_branch'. Any content already at 'destination_url' will be |
| 38 | deleted. |
| 39 | |
| 40 | - If 'source_branch' is stacked, then the destination branch will be |
| 41 | - stacked on the same URL, relative to 'destination_url'. |
| 42 | - |
| 43 | :param source_branch: The Bazaar branch that will be mirrored. |
| 44 | :param destination_url: The place to make the destination branch. This |
| 45 | URL must point to a writable location. |
| 46 | :return: The destination branch. |
| 47 | """ |
| 48 | - dest_transport = get_transport(destination_url) |
| 49 | - if dest_transport.has('.'): |
| 50 | - dest_transport.delete_tree('.') |
| 51 | - bzrdir = source_branch.bzrdir |
| 52 | - if self.policy._is_import: |
| 53 | - source_transport = source_branch.bzrdir.transport.clone('..') |
| 54 | - while True: |
| 55 | - files_before = sorted(source_transport.iter_files_recursive()) |
| 56 | - source_transport.copy_tree_to_transport(dest_transport) |
| 57 | - files_after = sorted(source_transport.iter_files_recursive()) |
| 58 | - if files_before == files_after: |
| 59 | - break |
| 60 | - return Branch.open_from_transport(dest_transport) |
| 61 | - else: |
| 62 | - # We check to see if the stacked on branch exists in the mirrored |
| 63 | - # area so that we can nicely signal to the scheduler that the |
| 64 | - # pulling of this branch should be deferred before we even create |
| 65 | - # the branch in the mirrored area. |
| 66 | - stacked_on_url = ( |
| 67 | - self.policy.getStackedOnURLForDestinationBranch( |
| 68 | - source_branch, destination_url)) |
| 69 | - if stacked_on_url is not None: |
| 70 | - stacked_on_url = urlutils.join(destination_url, stacked_on_url) |
| 71 | - try: |
| 72 | - Branch.open(stacked_on_url) |
| 73 | - except errors.NotBranchError: |
| 74 | - raise StackedOnBranchNotFound() |
| 75 | - if isinstance(source_branch, LoomSupport): |
| 76 | - # Looms suck. |
| 77 | - revision_id = None |
| 78 | - else: |
| 79 | - revision_id = 'null:' |
| 80 | - self._runWithTransformFallbackLocationHookInstalled( |
| 81 | - bzrdir.clone_on_transport, dest_transport, |
| 82 | - revision_id=revision_id) |
| 83 | - branch = Branch.open(destination_url) |
| 84 | - return branch |
| 85 | + return self._runWithTransformFallbackLocationHookInstalled( |
| 86 | + self.policy.createDestinationBranch, source_branch, |
| 87 | + destination_url) |
| 88 | |
| 89 | def openDestinationBranch(self, source_branch, destination_url): |
| 90 | """Open or create the destination branch at 'destination_url'. |
| 91 | @@ -279,9 +239,7 @@ |
| 92 | :param source_branch: The Bazaar branch that will be mirrored. |
| 93 | :param destination_url: The place to make the destination branch. This |
| 94 | URL must point to a writable location. |
| 95 | - :return: (branch, up_to_date), where 'branch' is the destination |
| 96 | - branch, and 'up_to_date' is a boolean saying whether the returned |
| 97 | - branch is up-to-date with the source branch. |
| 98 | + :return: The opened or created branch. |
| 99 | """ |
| 100 | try: |
| 101 | branch = Branch.open(destination_url) |
| 102 | |
| 103 | === modified file 'lib/lp/codehosting/vfs/branchfs.py' |
| 104 | --- lib/lp/codehosting/vfs/branchfs.py 2010-04-13 01:40:03 +0000 |
| 105 | +++ lib/lp/codehosting/vfs/branchfs.py 2010-04-15 01:49:32 +0000 |
| 106 | @@ -66,10 +66,12 @@ |
| 107 | |
| 108 | import xmlrpclib |
| 109 | |
| 110 | +from bzrlib.branch import Branch |
| 111 | from bzrlib.bzrdir import BzrDir, BzrDirFormat |
| 112 | from bzrlib.errors import ( |
| 113 | NoSuchFile, NotBranchError, NotStacked, PermissionDenied, |
| 114 | TransportNotPossible, UnstackableBranchFormat) |
| 115 | +from bzrlib.plugins.loom.branch import LoomSupport |
| 116 | from bzrlib.transport import get_transport |
| 117 | from bzrlib.transport.memory import MemoryServer |
| 118 | from bzrlib import urlutils |
| 119 | @@ -697,7 +699,45 @@ |
| 120 | stacked. |
| 121 | """ |
| 122 | |
| 123 | - _is_import = False |
| 124 | + def createDestinationBranch(self, source_branch, destination_url): |
| 125 | + """Create a destination branch for 'source_branch'. |
| 126 | + |
| 127 | + Creates a branch at 'destination_url' that is has the same format as |
| 128 | + 'source_branch'. Any content already at 'destination_url' will be |
| 129 | + deleted. Generally the new branch will have no revisions, but they |
| 130 | + will be copied for import branches, because this can be done safely |
| 131 | + and efficiently with a vfs-level copy (see `ImportedBranchPolicy`, |
| 132 | + below). |
| 133 | + |
| 134 | + :param source_branch: The Bazaar branch that will be mirrored. |
| 135 | + :param destination_url: The place to make the destination branch. This |
| 136 | + URL must point to a writable location. |
| 137 | + :return: The destination branch. |
| 138 | + :raises StackedOnBranchNotFound: if the branch that the destination |
| 139 | + branch will be stacked on does not yet exist in the mirrored area. |
| 140 | + """ |
| 141 | + dest_transport = get_transport(destination_url) |
| 142 | + if dest_transport.has('.'): |
| 143 | + dest_transport.delete_tree('.') |
| 144 | + stacked_on_url = ( |
| 145 | + self.getStackedOnURLForDestinationBranch( |
| 146 | + source_branch, destination_url)) |
| 147 | + if stacked_on_url is not None: |
| 148 | + stacked_on_url = urlutils.join(destination_url, stacked_on_url) |
| 149 | + try: |
| 150 | + Branch.open(stacked_on_url) |
| 151 | + except NotBranchError: |
| 152 | + from lp.codehosting.codeimport.worker import ( |
| 153 | + StackedOnBranchNotFound) |
| 154 | + raise StackedOnBranchNotFound() |
| 155 | + if isinstance(source_branch, LoomSupport): |
| 156 | + # Looms suck. |
| 157 | + revision_id = None |
| 158 | + else: |
| 159 | + revision_id = 'null:' |
| 160 | + source_branch.bzrdir.clone_on_transport( |
| 161 | + dest_transport, revision_id=revision_id) |
| 162 | + return Branch.open(destination_url) |
| 163 | |
| 164 | def getStackedOnURLForDestinationBranch(self, source_branch, |
| 165 | destination_url): |
| 166 | @@ -768,15 +808,6 @@ |
| 167 | - assert we're pulling from a lp-hosted:/// URL. |
| 168 | """ |
| 169 | |
| 170 | - def shouldFollowReferences(self): |
| 171 | - """See `BranchPolicy.shouldFollowReferences`. |
| 172 | - |
| 173 | - We do not traverse references for HOSTED branches because that may |
| 174 | - cause us to connect to remote locations, which we do not allow because |
| 175 | - we want hosted branches to be mirrored quickly. |
| 176 | - """ |
| 177 | - return False |
| 178 | - |
| 179 | def _bzrdirExists(self, url): |
| 180 | """Return whether a BzrDir exists at `url`.""" |
| 181 | try: |
| 182 | @@ -926,7 +957,26 @@ |
| 183 | - assert the URLs start with the prefix we expect for imported branches. |
| 184 | """ |
| 185 | |
| 186 | - _is_import = True |
| 187 | + def createDestinationBranch(self, source_branch, destination_url): |
| 188 | + """See `BranchPolicy.createDestinationBranch`. |
| 189 | + |
| 190 | + Because we control the process that creates import branches, a |
| 191 | + vfs-level copy is safe and more efficient than a bzr fetch. |
| 192 | + """ |
| 193 | + source_transport = source_branch.bzrdir.root_transport |
| 194 | + dest_transport = get_transport(destination_url) |
| 195 | + while True: |
| 196 | + # We loop until the remote file list before and after the copy is |
| 197 | + # the same to catch the case where the remote side is being |
| 198 | + # mutated as we copy it. |
| 199 | + if dest_transport.has('.'): |
| 200 | + dest_transport.delete_tree('.') |
| 201 | + files_before = set(source_transport.iter_files_recursive()) |
| 202 | + source_transport.copy_tree_to_transport(dest_transport) |
| 203 | + files_after = set(source_transport.iter_files_recursive()) |
| 204 | + if files_before == files_after: |
| 205 | + break |
| 206 | + return Branch.open_from_transport(dest_transport) |
| 207 | |
| 208 | def shouldFollowReferences(self): |
| 209 | """See `BranchPolicy.shouldFollowReferences`. |
Revision history for this message
| Aaron Bentley (abentley) wrote : | # |
There was still another spot where you could use root_transport in lib/lp/
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
| 1 | === modified file 'configs/testrunner/launchpad-lazr.conf' |
| 2 | --- configs/testrunner/launchpad-lazr.conf 2010-03-30 17:25:52 +0000 |
| 3 | +++ configs/testrunner/launchpad-lazr.conf 2010-04-15 06:06:18 +0000 |
| 4 | @@ -127,7 +127,6 @@ |
| 5 | |
| 6 | [launchpad] |
| 7 | max_attachment_size: 1024 |
| 8 | -bzr_imports_root_url: http://localhost:10899 |
| 9 | geoip_database: /usr/share/GeoIP/GeoLiteCity.dat |
| 10 | |
| 11 | [launchpad_session] |
| 12 | |
| 13 | === modified file 'lib/lp/codehosting/codeimport/worker.py' |
| 14 | --- lib/lp/codehosting/codeimport/worker.py 2010-04-12 01:42:34 +0000 |
| 15 | +++ lib/lp/codehosting/codeimport/worker.py 2010-04-15 06:06:18 +0000 |
| 16 | @@ -89,14 +89,18 @@ |
| 17 | except NoSuchFile: |
| 18 | pass |
| 19 | upgrade(remote_url, required_format) |
| 20 | - local_branch = remote_bzr_dir.sprout( |
| 21 | - target_path, create_tree_if_local=needs_tree).open_branch() |
| 22 | - # Because of the way we do incremental imports, there may be revisions |
| 23 | - # in the branch's repo that are not in the ancestry of the branch tip. |
| 24 | - # We need to transfer them too. |
| 25 | - local_branch.repository.fetch( |
| 26 | - remote_bzr_dir.open_repository()) |
| 27 | - return local_branch |
| 28 | + # The proper thing to do here would be to call |
| 29 | + # "remote_bzr_dir.sprout()". But 2a fetch slowly checks which |
| 30 | + # revisions are in the ancestry of the tip of the remote branch, which |
| 31 | + # we strictly don't care about, so we just copy the whole thing down |
| 32 | + # at the vfs level. |
| 33 | + target = get_transport(target_path) |
| 34 | + target.ensure_base() |
| 35 | + remote_bzr_dir.root_transport.copy_tree_to_transport(target) |
| 36 | + local_bzr_dir = BzrDir.open_from_transport(target) |
| 37 | + if needs_tree: |
| 38 | + local_bzr_dir.create_workingtree() |
| 39 | + return local_bzr_dir.open_branch() |
| 40 | |
| 41 | def push(self, db_branch_id, bzr_branch, required_format): |
| 42 | """Push up `bzr_branch` as the Bazaar branch for `code_import`. |
| 43 | |
| 44 | === modified file 'lib/lp/codehosting/puller/tests/__init__.py' |
| 45 | --- lib/lp/codehosting/puller/tests/__init__.py 2009-11-21 00:28:10 +0000 |
| 46 | +++ lib/lp/codehosting/puller/tests/__init__.py 2010-04-15 06:06:18 +0000 |
| 47 | @@ -135,6 +135,7 @@ |
| 48 | if os.path.exists(path): |
| 49 | shutil.rmtree(path) |
| 50 | os.makedirs(path) |
| 51 | + self.addCleanup(shutil.rmtree, path) |
| 52 | |
| 53 | def pushToBranch(self, branch, tree): |
| 54 | """Push a Bazaar branch to a given Launchpad branch's hosted area. |
| 55 | |
| 56 | === modified file 'lib/lp/codehosting/puller/tests/test_acceptance.py' |
| 57 | --- lib/lp/codehosting/puller/tests/test_acceptance.py 2010-02-24 04:24:01 +0000 |
| 58 | +++ lib/lp/codehosting/puller/tests/test_acceptance.py 2010-04-15 06:06:18 +0000 |
| 59 | @@ -8,10 +8,8 @@ |
| 60 | |
| 61 | |
| 62 | import os |
| 63 | -import shutil |
| 64 | from subprocess import PIPE, Popen |
| 65 | import unittest |
| 66 | -from urlparse import urlparse |
| 67 | |
| 68 | import transaction |
| 69 | |
| 70 | @@ -22,6 +20,7 @@ |
| 71 | from bzrlib.tests import HttpServer |
| 72 | from bzrlib.transport import get_transport |
| 73 | from bzrlib.upgrade import upgrade |
| 74 | +from bzrlib.urlutils import join as urljoin, local_path_from_url |
| 75 | |
| 76 | from zope.component import getUtility |
| 77 | from zope.security.proxy import removeSecurityProxy |
| 78 | @@ -49,11 +48,9 @@ |
| 79 | self._puller_script = os.path.join( |
| 80 | config.root, 'cronscripts', 'supermirror-pull.py') |
| 81 | self.makeCleanDirectory(config.codehosting.hosted_branches_root) |
| 82 | - self.addCleanup( |
| 83 | - shutil.rmtree, config.codehosting.hosted_branches_root) |
| 84 | self.makeCleanDirectory(config.codehosting.mirrored_branches_root) |
| 85 | - self.addCleanup( |
| 86 | - shutil.rmtree, config.codehosting.mirrored_branches_root) |
| 87 | + self.makeCleanDirectory( |
| 88 | + local_path_from_url(config.launchpad.bzr_imports_root_url)) |
| 89 | |
| 90 | def assertMirrored(self, db_branch, source_branch=None, |
| 91 | accessing_user=None): |
| 92 | @@ -136,10 +133,9 @@ |
| 93 | retcode, output, error = self.runSubprocess(command) |
| 94 | return command, retcode, output, error |
| 95 | |
| 96 | - def serveOverHTTP(self, port=0): |
| 97 | + def serveOverHTTP(self): |
| 98 | """Serve the current directory over HTTP, returning the server URL.""" |
| 99 | http_server = HttpServer() |
| 100 | - http_server.port = port |
| 101 | http_server.start_server() |
| 102 | # Join cleanup added before the tearDown so the tearDown is executed |
| 103 | # first as this tells the thread to die. We then join explicitly as |
| 104 | @@ -392,18 +388,6 @@ |
| 105 | self.assertRaises( |
| 106 | errors.NotStacked, mirrored_branch.get_stacked_on_url) |
| 107 | |
| 108 | - def _getImportMirrorPort(self): |
| 109 | - """Return the port used to serve imported branches, as specified in |
| 110 | - config.launchpad.bzr_imports_root_url. |
| 111 | - """ |
| 112 | - address = urlparse(config.launchpad.bzr_imports_root_url)[1] |
| 113 | - host, port = address.split(':') |
| 114 | - self.assertEqual( |
| 115 | - 'localhost', host, |
| 116 | - 'bzr_imports_root_url must be configured on localhost: %s' |
| 117 | - % (config.launchpad.bzr_imports_root_url,)) |
| 118 | - return int(port) |
| 119 | - |
| 120 | def test_mirror_imported_branch(self): |
| 121 | # Run the puller on a populated imported branch pull queue. |
| 122 | # Create the branch in the database. |
| 123 | @@ -412,18 +396,18 @@ |
| 124 | db_branch.requestMirror() |
| 125 | transaction.commit() |
| 126 | |
| 127 | - # Create the Bazaar branch and serve it in the expected location. |
| 128 | - branch_path = '%08x' % db_branch.id |
| 129 | - os.mkdir(branch_path) |
| 130 | - tree = self.make_branch_and_tree(branch_path) |
| 131 | + # Create the Bazaar branch in the expected location. |
| 132 | + branch_url = urljoin( |
| 133 | + config.launchpad.bzr_imports_root_url, '%08x' % db_branch.id) |
| 134 | + branch = BzrDir.create_branch_convenience(branch_url) |
| 135 | + tree = branch.bzrdir.open_workingtree() |
| 136 | tree.commit('rev1') |
| 137 | - self.serveOverHTTP(self._getImportMirrorPort()) |
| 138 | |
| 139 | # Run the puller. |
| 140 | command, retcode, output, error = self.runPuller() |
| 141 | self.assertRanSuccessfully(command, retcode, output, error) |
| 142 | |
| 143 | - self.assertMirrored(db_branch, source_branch=tree.branch) |
| 144 | + self.assertMirrored(db_branch, source_branch=branch) |
| 145 | |
| 146 | def test_mirror_empty(self): |
| 147 | # Run the puller on an empty pull queue. |
| 148 | |
| 149 | === modified file 'lib/lp/codehosting/puller/worker.py' |
| 150 | --- lib/lp/codehosting/puller/worker.py 2010-01-20 20:56:29 +0000 |
| 151 | +++ lib/lp/codehosting/puller/worker.py 2010-04-15 06:06:18 +0000 |
| 152 | @@ -11,9 +11,6 @@ |
| 153 | from bzrlib.branch import Branch |
| 154 | from bzrlib.bzrdir import BzrDir |
| 155 | from bzrlib import errors |
| 156 | -from bzrlib.plugins.loom.branch import LoomSupport |
| 157 | -from bzrlib.transport import get_transport |
| 158 | -from bzrlib import urlutils |
| 159 | from bzrlib.ui import SilentUIFactory |
| 160 | import bzrlib.ui |
| 161 | |
| 162 | @@ -227,41 +224,14 @@ |
| 163 | 'source_branch'. Any content already at 'destination_url' will be |
| 164 | deleted. |
| 165 | |
| 166 | - If 'source_branch' is stacked, then the destination branch will be |
| 167 | - stacked on the same URL, relative to 'destination_url'. |
| 168 | - |
| 169 | :param source_branch: The Bazaar branch that will be mirrored. |
| 170 | :param destination_url: The place to make the destination branch. This |
| 171 | URL must point to a writable location. |
| 172 | :return: The destination branch. |
| 173 | """ |
| 174 | - dest_transport = get_transport(destination_url) |
| 175 | - if dest_transport.has('.'): |
| 176 | - dest_transport.delete_tree('.') |
| 177 | - bzrdir = source_branch.bzrdir |
| 178 | - # We check to see if the stacked on branch exists in the mirrored area |
| 179 | - # so that we can nicely signal to the scheduler that the pulling of |
| 180 | - # this branch should be deferred before we even create the branch in |
| 181 | - # the mirrored area. |
| 182 | - stacked_on_url = ( |
| 183 | - self.policy.getStackedOnURLForDestinationBranch( |
| 184 | - source_branch, destination_url)) |
| 185 | - if stacked_on_url is not None: |
| 186 | - stacked_on_url = urlutils.join(destination_url, stacked_on_url) |
| 187 | - try: |
| 188 | - Branch.open(stacked_on_url) |
| 189 | - except errors.NotBranchError: |
| 190 | - raise StackedOnBranchNotFound() |
| 191 | - if isinstance(source_branch, LoomSupport): |
| 192 | - # Looms suck. |
| 193 | - revision_id = None |
| 194 | - else: |
| 195 | - revision_id = 'null:' |
| 196 | - self._runWithTransformFallbackLocationHookInstalled( |
| 197 | - bzrdir.clone_on_transport, dest_transport, |
| 198 | - revision_id=revision_id) |
| 199 | - branch = Branch.open(destination_url) |
| 200 | - return branch |
| 201 | + return self._runWithTransformFallbackLocationHookInstalled( |
| 202 | + self.policy.createDestinationBranch, source_branch, |
| 203 | + destination_url) |
| 204 | |
| 205 | def openDestinationBranch(self, source_branch, destination_url): |
| 206 | """Open or create the destination branch at 'destination_url'. |
| 207 | @@ -269,9 +239,7 @@ |
| 208 | :param source_branch: The Bazaar branch that will be mirrored. |
| 209 | :param destination_url: The place to make the destination branch. This |
| 210 | URL must point to a writable location. |
| 211 | - :return: (branch, up_to_date), where 'branch' is the destination |
| 212 | - branch, and 'up_to_date' is a boolean saying whether the returned |
| 213 | - branch is up-to-date with the source branch. |
| 214 | + :return: The opened or created branch. |
| 215 | """ |
| 216 | try: |
| 217 | branch = Branch.open(destination_url) |
| 218 | |
| 219 | === modified file 'lib/lp/codehosting/vfs/branchfs.py' |
| 220 | --- lib/lp/codehosting/vfs/branchfs.py 2010-04-09 12:39:23 +0000 |
| 221 | +++ lib/lp/codehosting/vfs/branchfs.py 2010-04-15 06:06:18 +0000 |
| 222 | @@ -66,10 +66,12 @@ |
| 223 | |
| 224 | import xmlrpclib |
| 225 | |
| 226 | +from bzrlib.branch import Branch |
| 227 | from bzrlib.bzrdir import BzrDir, BzrDirFormat |
| 228 | from bzrlib.errors import ( |
| 229 | NoSuchFile, NotBranchError, NotStacked, PermissionDenied, |
| 230 | TransportNotPossible, UnstackableBranchFormat) |
| 231 | +from bzrlib.plugins.loom.branch import LoomSupport |
| 232 | from bzrlib.transport import get_transport |
| 233 | from bzrlib.transport.memory import MemoryServer |
| 234 | from bzrlib import urlutils |
| 235 | @@ -697,6 +699,46 @@ |
| 236 | stacked. |
| 237 | """ |
| 238 | |
| 239 | + def createDestinationBranch(self, source_branch, destination_url): |
| 240 | + """Create a destination branch for 'source_branch'. |
| 241 | + |
| 242 | + Creates a branch at 'destination_url' that is has the same format as |
| 243 | + 'source_branch'. Any content already at 'destination_url' will be |
| 244 | + deleted. Generally the new branch will have no revisions, but they |
| 245 | + will be copied for import branches, because this can be done safely |
| 246 | + and efficiently with a vfs-level copy (see `ImportedBranchPolicy`, |
| 247 | + below). |
| 248 | + |
| 249 | + :param source_branch: The Bazaar branch that will be mirrored. |
| 250 | + :param destination_url: The place to make the destination branch. This |
| 251 | + URL must point to a writable location. |
| 252 | + :return: The destination branch. |
| 253 | + :raises StackedOnBranchNotFound: if the branch that the destination |
| 254 | + branch will be stacked on does not yet exist in the mirrored area. |
| 255 | + """ |
| 256 | + dest_transport = get_transport(destination_url) |
| 257 | + if dest_transport.has('.'): |
| 258 | + dest_transport.delete_tree('.') |
| 259 | + stacked_on_url = ( |
| 260 | + self.getStackedOnURLForDestinationBranch( |
| 261 | + source_branch, destination_url)) |
| 262 | + if stacked_on_url is not None: |
| 263 | + stacked_on_url = urlutils.join(destination_url, stacked_on_url) |
| 264 | + try: |
| 265 | + Branch.open(stacked_on_url) |
| 266 | + except NotBranchError: |
| 267 | + from lp.codehosting.puller.worker import ( |
| 268 | + StackedOnBranchNotFound) |
| 269 | + raise StackedOnBranchNotFound() |
| 270 | + if isinstance(source_branch, LoomSupport): |
| 271 | + # Looms suck. |
| 272 | + revision_id = None |
| 273 | + else: |
| 274 | + revision_id = 'null:' |
| 275 | + source_branch.bzrdir.clone_on_transport( |
| 276 | + dest_transport, revision_id=revision_id) |
| 277 | + return Branch.open(destination_url) |
| 278 | + |
| 279 | def getStackedOnURLForDestinationBranch(self, source_branch, |
| 280 | destination_url): |
| 281 | """Return the URL of the branch to stack the mirrored copy on. |
| 282 | @@ -766,15 +808,6 @@ |
| 283 | - assert we're pulling from a lp-hosted:/// URL. |
| 284 | """ |
| 285 | |
| 286 | - def shouldFollowReferences(self): |
| 287 | - """See `BranchPolicy.shouldFollowReferences`. |
| 288 | - |
| 289 | - We do not traverse references for HOSTED branches because that may |
| 290 | - cause us to connect to remote locations, which we do not allow because |
| 291 | - we want hosted branches to be mirrored quickly. |
| 292 | - """ |
| 293 | - return False |
| 294 | - |
| 295 | def _bzrdirExists(self, url): |
| 296 | """Return whether a BzrDir exists at `url`.""" |
| 297 | try: |
| 298 | @@ -924,6 +957,27 @@ |
| 299 | - assert the URLs start with the prefix we expect for imported branches. |
| 300 | """ |
| 301 | |
| 302 | + def createDestinationBranch(self, source_branch, destination_url): |
| 303 | + """See `BranchPolicy.createDestinationBranch`. |
| 304 | + |
| 305 | + Because we control the process that creates import branches, a |
| 306 | + vfs-level copy is safe and more efficient than a bzr fetch. |
| 307 | + """ |
| 308 | + source_transport = source_branch.bzrdir.root_transport |
| 309 | + dest_transport = get_transport(destination_url) |
| 310 | + while True: |
| 311 | + # We loop until the remote file list before and after the copy is |
| 312 | + # the same to catch the case where the remote side is being |
| 313 | + # mutated as we copy it. |
| 314 | + if dest_transport.has('.'): |
| 315 | + dest_transport.delete_tree('.') |
| 316 | + files_before = set(source_transport.iter_files_recursive()) |
| 317 | + source_transport.copy_tree_to_transport(dest_transport) |
| 318 | + files_after = set(source_transport.iter_files_recursive()) |
| 319 | + if files_before == files_after: |
| 320 | + break |
| 321 | + return Branch.open_from_transport(dest_transport) |
| 322 | + |
| 323 | def shouldFollowReferences(self): |
| 324 | """See `BranchPolicy.shouldFollowReferences`. |
| 325 |
What is the impact of removing the testrunner bzr_imports_
Instead of remote_
Some of the test changes look like drive-bys, e.g. not wanting it to leave directories behind. Is that right?
Is there a reason to join config.
Did you consider using more specific name than _is_import? e.g. _vfs_initial_clone ? Or what about providing an initial_clone method on the BranchPolicy itself?
What about using sets to represent files_before and files_after? That's closer to the abstract representation, because order isn't important, and entries can only appear once. I don't really understand the purpose of this loop, either.
It might be nice to turn _runWithTransfo