Merge lp:~mwhudson/launchpad/code-import-optimizations into lp:launchpad
- code-import-optimizations
- Merge into devel
Proposed by
Michael Hudson-Doyle
| Status: | Merged |
|---|---|
| Approved by: | Tim Penhey |
| Approved revision: | 10680 |
| Merged at revision: | not available |
| Proposed branch: | lp:~mwhudson/launchpad/code-import-optimizations |
| Merge into: | lp:launchpad |
| Diff against target: |
555 lines (+176/-126) 2 files modified
lib/lp/codehosting/codeimport/tests/test_worker.py (+87/-52) lib/lp/codehosting/codeimport/worker.py (+89/-74) |
| To merge this branch: | bzr merge lp:~mwhudson/launchpad/code-import-optimizations |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Tim Penhey (community) | Approve | ||
|
Review via email:
|
|||
Commit message
Improve logging during code imports and don't build a working tree if not needed, both of which will help with large imports timing out on startup
Description of the change
Hi,
This branch should fix some very large foreign branch imports timing out in two ways:
1) There was no progress reporting during fetching the branch from the store or building the tree, so I moved the initial fetch into the block that has our custom ui factory installed.
2) Building the working tree was a waste of time anyway for foreign branch imports, so we no longer do that. This required the bulk of the changes from a textual point of view, to talk about branches where previously we talked about trees, but I think the result is better even if it wasn't faster.
I also add a few logging calls as suggested in bug 559808.
Cheers,
mwh
To post a comment you must log in.
Revision history for this message
| Michael Hudson-Doyle (mwhudson) wrote : | # |
On 12/04/10 14:16, Tim Penhey wrote:
> Review: Approve
> The change looks fine.
Thanks.
> It does get me wondering a bit why we didn't seem to get any logging when it was pushing the branch to the central store before. Thanks for adding the job started and finished logging bits too.
I think we do actually get logging when pushing to the central store,
but the new log lines should let us see what comes from where :-)
> I agree that it makes more sense to use branch rather than working tree in the workers.
Cool.
Cheers,
mwh
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
| 1 | === modified file 'lib/lp/codehosting/codeimport/tests/test_worker.py' |
| 2 | --- lib/lp/codehosting/codeimport/tests/test_worker.py 2010-04-09 04:39:38 +0000 |
| 3 | +++ lib/lp/codehosting/codeimport/tests/test_worker.py 2010-04-12 01:54:17 +0000 |
| 4 | @@ -19,7 +19,7 @@ |
| 5 | from bzrlib.tests import TestCaseWithTransport |
| 6 | from bzrlib.transport import get_transport |
| 7 | from bzrlib.upgrade import upgrade |
| 8 | -from bzrlib.urlutils import join as urljoin |
| 9 | +from bzrlib.urlutils import join as urljoin, local_path_from_url |
| 10 | |
| 11 | from CVS import Repository, tree as CVSTree |
| 12 | |
| 13 | @@ -120,22 +120,57 @@ |
| 14 | |
| 15 | def test_getNewBranch(self): |
| 16 | # If there's no Bazaar branch of this id, then pull creates a new |
| 17 | - # Bazaar working tree. |
| 18 | + # Bazaar branch. |
| 19 | store = self.makeBranchStore() |
| 20 | - bzr_working_tree = store.pull( |
| 21 | + bzr_branch = store.pull( |
| 22 | self.arbitrary_branch_id, self.temp_dir, default_format) |
| 23 | - self.assertEqual([], bzr_working_tree.branch.revision_history()) |
| 24 | + self.assertEqual([], bzr_branch.revision_history()) |
| 25 | + |
| 26 | + def test_getNewBranch_without_tree(self): |
| 27 | + # If pull() with needs_tree=False creates a new branch, it doesn't |
| 28 | + # create a working tree. |
| 29 | + store = self.makeBranchStore() |
| 30 | + bzr_branch = store.pull( |
| 31 | + self.arbitrary_branch_id, self.temp_dir, default_format, False) |
| 32 | + self.assertFalse(bzr_branch.bzrdir.has_workingtree()) |
| 33 | + |
| 34 | + def test_getNewBranch_with_tree(self): |
| 35 | + # If pull() with needs_tree=True creates a new branch, it creates a |
| 36 | + # working tree. |
| 37 | + store = self.makeBranchStore() |
| 38 | + bzr_branch = store.pull( |
| 39 | + self.arbitrary_branch_id, self.temp_dir, default_format, True) |
| 40 | + self.assertTrue(bzr_branch.bzrdir.has_workingtree()) |
| 41 | |
| 42 | def test_pushBranchThenPull(self): |
| 43 | # After we've pushed up a branch to the store, we can then pull it |
| 44 | # from the store. |
| 45 | store = self.makeBranchStore() |
| 46 | tree = create_branch_with_one_revision('original') |
| 47 | - store.push(self.arbitrary_branch_id, tree, default_format) |
| 48 | - new_tree = store.pull( |
| 49 | + store.push(self.arbitrary_branch_id, tree.branch, default_format) |
| 50 | + new_branch = store.pull( |
| 51 | self.arbitrary_branch_id, self.temp_dir, default_format) |
| 52 | self.assertEqual( |
| 53 | - tree.branch.last_revision(), new_tree.branch.last_revision()) |
| 54 | + tree.branch.last_revision(), new_branch.last_revision()) |
| 55 | + |
| 56 | + def test_pull_without_needs_tree_doesnt_create_tree(self): |
| 57 | + # pull with needs_tree=False doesn't spend the time to create a |
| 58 | + # working tree. |
| 59 | + store = self.makeBranchStore() |
| 60 | + tree = create_branch_with_one_revision('original') |
| 61 | + store.push(self.arbitrary_branch_id, tree.branch, default_format) |
| 62 | + new_branch = store.pull( |
| 63 | + self.arbitrary_branch_id, self.temp_dir, default_format, False) |
| 64 | + self.assertFalse(new_branch.bzrdir.has_workingtree()) |
| 65 | + |
| 66 | + def test_pull_needs_tree_creates_tree(self): |
| 67 | + # pull with needs_tree=True creates a working tree. |
| 68 | + store = self.makeBranchStore() |
| 69 | + tree = create_branch_with_one_revision('original') |
| 70 | + store.push(self.arbitrary_branch_id, tree.branch, default_format) |
| 71 | + new_branch = store.pull( |
| 72 | + self.arbitrary_branch_id, self.temp_dir, default_format, True) |
| 73 | + self.assertTrue(new_branch.bzrdir.has_workingtree()) |
| 74 | |
| 75 | # XXX Tim Penhey 2009-09-18 bug 432217 Automatic upgrade of import |
| 76 | # branches disabled. Need an orderly upgrade process. |
| 77 | @@ -188,21 +223,21 @@ |
| 78 | # store. |
| 79 | store = self.makeBranchStore() |
| 80 | tree = create_branch_with_one_revision('original') |
| 81 | - store.push(self.arbitrary_branch_id, tree, default_format) |
| 82 | - store.push(self.arbitrary_branch_id, tree, default_format) |
| 83 | - new_tree = store.pull( |
| 84 | + store.push(self.arbitrary_branch_id, tree.branch, default_format) |
| 85 | + store.push(self.arbitrary_branch_id, tree.branch, default_format) |
| 86 | + new_branch = store.pull( |
| 87 | self.arbitrary_branch_id, self.temp_dir, default_format) |
| 88 | self.assertEqual( |
| 89 | - tree.branch.last_revision(), new_tree.branch.last_revision()) |
| 90 | + tree.branch.last_revision(), new_branch.last_revision()) |
| 91 | |
| 92 | def test_push_divergant_branches(self): |
| 93 | # push() uses overwrite=True, so divergent branches (rebased) can be |
| 94 | # pushed. |
| 95 | store = self.makeBranchStore() |
| 96 | tree = create_branch_with_one_revision('original') |
| 97 | - store.push(self.arbitrary_branch_id, tree, default_format) |
| 98 | + store.push(self.arbitrary_branch_id, tree.branch, default_format) |
| 99 | tree = create_branch_with_one_revision('divergant') |
| 100 | - store.push(self.arbitrary_branch_id, tree, default_format) |
| 101 | + store.push(self.arbitrary_branch_id, tree.branch, default_format) |
| 102 | |
| 103 | def fetchBranch(self, from_url, target_path): |
| 104 | """Pull a branch from `from_url` to `target_path`. |
| 105 | @@ -221,7 +256,7 @@ |
| 106 | # doesn't already exist. |
| 107 | store = BazaarBranchStore(self.get_transport('doesntexist')) |
| 108 | tree = create_branch_with_one_revision('original') |
| 109 | - store.push(self.arbitrary_branch_id, tree, default_format) |
| 110 | + store.push(self.arbitrary_branch_id, tree.branch, default_format) |
| 111 | self.assertIsDirectory('doesntexist', self.get_transport()) |
| 112 | |
| 113 | def test_storedLocation(self): |
| 114 | @@ -229,12 +264,12 @@ |
| 115 | # the BazaarBranchStore's transport. |
| 116 | store = self.makeBranchStore() |
| 117 | tree = create_branch_with_one_revision('original') |
| 118 | - store.push(self.arbitrary_branch_id, tree, default_format) |
| 119 | - new_tree = self.fetchBranch( |
| 120 | + store.push(self.arbitrary_branch_id, tree.branch, default_format) |
| 121 | + new_branch = self.fetchBranch( |
| 122 | urljoin(store.transport.base, '%08x' % self.arbitrary_branch_id), |
| 123 | 'new_tree') |
| 124 | self.assertEqual( |
| 125 | - tree.branch.last_revision(), new_tree.branch.last_revision()) |
| 126 | + tree.branch.last_revision(), new_branch.last_revision()) |
| 127 | |
| 128 | def test_sftpPrefix(self): |
| 129 | # Since branches are mirrored by importd via sftp, _getMirrorURL must |
| 130 | @@ -270,15 +305,14 @@ |
| 131 | None, None, [('add', ('', 'root-id', 'directory', ''))]) |
| 132 | revid1 = builder.build_snapshot(None, [revid], []) |
| 133 | revid2 = builder.build_snapshot(None, [revid], []) |
| 134 | - branch = builder.get_branch() |
| 135 | - source_tree = branch.bzrdir.create_workingtree() |
| 136 | store = self.makeBranchStore() |
| 137 | - store.push(self.arbitrary_branch_id, source_tree, default_format) |
| 138 | - retrieved_tree = store.pull( |
| 139 | + store.push( |
| 140 | + self.arbitrary_branch_id, builder.get_branch(), default_format) |
| 141 | + retrieved_branch = store.pull( |
| 142 | self.arbitrary_branch_id, 'pulled', default_format) |
| 143 | self.assertEqual( |
| 144 | set([revid, revid1, revid2]), |
| 145 | - set(retrieved_tree.branch.repository.all_revision_ids())) |
| 146 | + set(retrieved_branch.repository.all_revision_ids())) |
| 147 | |
| 148 | |
| 149 | class TestImportDataStore(WorkerTest): |
| 150 | @@ -532,21 +566,21 @@ |
| 151 | worker = self.makeImportWorker() |
| 152 | self.assertEqual(self.source_details, worker.source_details) |
| 153 | |
| 154 | - def test_getBazaarWorkingTreeMakesEmptyTree(self): |
| 155 | - # getBazaarWorkingTree returns a brand-new working tree for an initial |
| 156 | + def test_getBazaarWorkingBranchMakesEmptyBranch(self): |
| 157 | + # getBazaarBranch returns a brand-new working tree for an initial |
| 158 | # import. |
| 159 | worker = self.makeImportWorker() |
| 160 | - bzr_working_tree = worker.getBazaarWorkingTree() |
| 161 | - self.assertEqual([], bzr_working_tree.branch.revision_history()) |
| 162 | + bzr_branch = worker.getBazaarBranch() |
| 163 | + self.assertEqual([], bzr_branch.revision_history()) |
| 164 | |
| 165 | - def test_bazaarWorkingTreeLocation(self): |
| 166 | - # getBazaarWorkingTree makes the working tree under the current |
| 167 | - # working directory. |
| 168 | + def test_bazaarBranchLocation(self): |
| 169 | + # getBazaarBranch makes the working tree under the current working |
| 170 | + # directory. |
| 171 | worker = self.makeImportWorker() |
| 172 | - bzr_working_tree = worker.getBazaarWorkingTree() |
| 173 | + bzr_branch = worker.getBazaarBranch() |
| 174 | self.assertIsSameRealPath( |
| 175 | - os.path.abspath(worker.BZR_WORKING_TREE_PATH), |
| 176 | - os.path.abspath(bzr_working_tree.basedir)) |
| 177 | + os.path.abspath(worker.BZR_BRANCH_PATH), |
| 178 | + os.path.abspath(local_path_from_url(bzr_branch.base))) |
| 179 | |
| 180 | |
| 181 | class TestCSCVSWorker(WorkerTest): |
| 182 | @@ -591,22 +625,22 @@ |
| 183 | source_details, self.get_transport('import_data'), |
| 184 | self.makeBazaarBranchStore(), logging.getLogger("silent")) |
| 185 | |
| 186 | - def test_pushBazaarWorkingTree_saves_git_cache(self): |
| 187 | - # GitImportWorker.pushBazaarWorkingTree saves a tarball of the git |
| 188 | - # cache from the tree's repository in the worker's ImportDataStore. |
| 189 | + def test_pushBazaarBranch_saves_git_cache(self): |
| 190 | + # GitImportWorker.pushBazaarBranch saves a tarball of the git cache |
| 191 | + # from the tree's repository in the worker's ImportDataStore. |
| 192 | content = self.factory.getUniqueString() |
| 193 | - tree = self.make_branch_and_tree('.') |
| 194 | - tree.branch.repository._transport.mkdir('git') |
| 195 | - tree.branch.repository._transport.put_bytes('git/cache', content) |
| 196 | + branch = self.make_branch('.') |
| 197 | + branch.repository._transport.mkdir('git') |
| 198 | + branch.repository._transport.put_bytes('git/cache', content) |
| 199 | import_worker = self.makeImportWorker() |
| 200 | - import_worker.pushBazaarWorkingTree(tree) |
| 201 | + import_worker.pushBazaarBranch(branch) |
| 202 | import_worker.import_data_store.fetch('git-cache.tar.gz') |
| 203 | extract_tarball('git-cache.tar.gz', '.') |
| 204 | self.assertEqual(content, open('cache').read()) |
| 205 | |
| 206 | - def test_getBazaarWorkingTree_fetches_legacy_git_db(self): |
| 207 | - # GitImportWorker.getBazaarWorkingTree fetches the legacy git.db file, |
| 208 | - # if present, from the worker's ImportDataStore into the tree's |
| 209 | + def test_getBazaarBranch_fetches_legacy_git_db(self): |
| 210 | + # GitImportWorker.getBazaarBranch fetches the legacy git.db file, if |
| 211 | + # present, from the worker's ImportDataStore into the tree's |
| 212 | # repository. |
| 213 | import_worker = self.makeImportWorker() |
| 214 | # Store the git.db file in the store. |
| 215 | @@ -614,15 +648,15 @@ |
| 216 | open('git.db', 'w').write(content) |
| 217 | import_worker.import_data_store.put('git.db') |
| 218 | # Make sure there's a Bazaar branch in the branch store. |
| 219 | - tree = self.make_branch_and_tree('tree') |
| 220 | - ImportWorker.pushBazaarWorkingTree(import_worker, tree) |
| 221 | + branch = self.make_branch('branch') |
| 222 | + ImportWorker.pushBazaarBranch(import_worker, branch) |
| 223 | # Finally, fetching the tree gets the git.db file too. |
| 224 | - tree = import_worker.getBazaarWorkingTree() |
| 225 | + branch = import_worker.getBazaarBranch() |
| 226 | self.assertEqual( |
| 227 | - content, tree.branch.repository._transport.get('git.db').read()) |
| 228 | + content, branch.repository._transport.get('git.db').read()) |
| 229 | |
| 230 | - def test_getBazaarWorkingTree_fetches_git_cache(self): |
| 231 | - # GitImportWorker.getBazaarWorkingTree fetches the tarball of the git |
| 232 | + def test_getBazaarBranch_fetches_git_cache(self): |
| 233 | + # GitImportWorker.getBazaarBranch fetches the tarball of the git |
| 234 | # cache from the worker's ImportDataStore and expands it into the |
| 235 | # tree's repository. |
| 236 | import_worker = self.makeImportWorker() |
| 237 | @@ -633,12 +667,13 @@ |
| 238 | create_tarball('cache', 'git-cache.tar.gz') |
| 239 | import_worker.import_data_store.put('git-cache.tar.gz') |
| 240 | # Make sure there's a Bazaar branch in the branch store. |
| 241 | - tree = self.make_branch_and_tree('tree') |
| 242 | - ImportWorker.pushBazaarWorkingTree(import_worker, tree) |
| 243 | + branch = self.make_branch('branch') |
| 244 | + ImportWorker.pushBazaarBranch(import_worker, branch) |
| 245 | # Finally, fetching the tree gets the git.db file too. |
| 246 | - tree = import_worker.getBazaarWorkingTree() |
| 247 | + new_branch = import_worker.getBazaarBranch() |
| 248 | self.assertEqual( |
| 249 | - content, tree.branch.repository._transport.get('git/git-cache').read()) |
| 250 | + content, |
| 251 | + new_branch.repository._transport.get('git/git-cache').read()) |
| 252 | |
| 253 | |
| 254 | def clean_up_default_stores_for_import(source_details): |
| 255 | |
| 256 | === modified file 'lib/lp/codehosting/codeimport/worker.py' |
| 257 | --- lib/lp/codehosting/codeimport/worker.py 2010-04-09 04:39:38 +0000 |
| 258 | +++ lib/lp/codehosting/codeimport/worker.py 2010-04-12 01:54:17 +0000 |
| 259 | @@ -64,17 +64,22 @@ |
| 260 | """Return the URL that `db_branch` is stored at.""" |
| 261 | return urljoin(self.transport.base, '%08x' % db_branch_id) |
| 262 | |
| 263 | - def pull(self, db_branch_id, target_path, required_format): |
| 264 | - """Pull down the Bazaar branch for `code_import` to `target_path`. |
| 265 | + def pull(self, db_branch_id, target_path, required_format, |
| 266 | + needs_tree=False): |
| 267 | + """Pull down the Bazaar branch of an import to `target_path`. |
| 268 | |
| 269 | - :return: A Bazaar working tree for the branch of `code_import`. |
| 270 | + :return: A Bazaar branch for the code import corresponding to the |
| 271 | + database branch with id `db_branch_id`. |
| 272 | """ |
| 273 | remote_url = self._getMirrorURL(db_branch_id) |
| 274 | try: |
| 275 | remote_bzr_dir = BzrDir.open(remote_url) |
| 276 | except NotBranchError: |
| 277 | - return BzrDir.create_standalone_workingtree( |
| 278 | - target_path, required_format) |
| 279 | + local_branch = BzrDir.create_branch_and_repo( |
| 280 | + target_path, format=required_format) |
| 281 | + if needs_tree: |
| 282 | + local_branch.bzrdir.create_workingtree() |
| 283 | + return local_branch |
| 284 | # XXX Tim Penhey 2009-09-18 bug 432217 Automatic upgrade of import |
| 285 | # branches disabled. Need an orderly upgrade process. |
| 286 | if False and remote_bzr_dir.needs_format_conversion( |
| 287 | @@ -84,33 +89,33 @@ |
| 288 | except NoSuchFile: |
| 289 | pass |
| 290 | upgrade(remote_url, required_format) |
| 291 | - local_bzr_dir = remote_bzr_dir.sprout(target_path) |
| 292 | + local_branch = remote_bzr_dir.sprout( |
| 293 | + target_path, create_tree_if_local=needs_tree).open_branch() |
| 294 | # Because of the way we do incremental imports, there may be revisions |
| 295 | # in the branch's repo that are not in the ancestry of the branch tip. |
| 296 | # We need to transfer them too. |
| 297 | - local_bzr_dir.open_repository().fetch( |
| 298 | + local_branch.repository.fetch( |
| 299 | remote_bzr_dir.open_repository()) |
| 300 | - return local_bzr_dir.open_workingtree() |
| 301 | + return local_branch |
| 302 | |
| 303 | - def push(self, db_branch_id, bzr_tree, required_format): |
| 304 | - """Push up `bzr_tree` as the Bazaar branch for `code_import`. |
| 305 | + def push(self, db_branch_id, bzr_branch, required_format): |
| 306 | + """Push up `bzr_branch` as the Bazaar branch for `code_import`. |
| 307 | |
| 308 | :return: A boolean that is true if the push was non-trivial |
| 309 | (i.e. actually transferred revisions). |
| 310 | """ |
| 311 | self.transport.create_prefix() |
| 312 | - branch_from = bzr_tree.branch |
| 313 | target_url = self._getMirrorURL(db_branch_id) |
| 314 | try: |
| 315 | - branch_to = Branch.open(target_url) |
| 316 | + remote_branch = Branch.open(target_url) |
| 317 | except NotBranchError: |
| 318 | - branch_to = BzrDir.create_branch_and_repo( |
| 319 | + remote_branch = BzrDir.create_branch_and_repo( |
| 320 | target_url, format=required_format) |
| 321 | - pull_result = branch_to.pull(branch_from, overwrite=True) |
| 322 | + pull_result = remote_branch.pull(bzr_branch, overwrite=True) |
| 323 | # Because of the way we do incremental imports, there may be revisions |
| 324 | # in the branch's repo that are not in the ancestry of the branch tip. |
| 325 | # We need to transfer them too. |
| 326 | - branch_to.repository.fetch(branch_from.repository) |
| 327 | + remote_branch.repository.fetch(bzr_branch.repository) |
| 328 | return pull_result.old_revid != pull_result.new_revid |
| 329 | |
| 330 | |
| 331 | @@ -352,7 +357,10 @@ |
| 332 | """Oversees the actual work of a code import.""" |
| 333 | |
| 334 | # Where the Bazaar working tree will be stored. |
| 335 | - BZR_WORKING_TREE_PATH = 'bzr_working_tree' |
| 336 | + BZR_BRANCH_PATH = 'bzr_branch' |
| 337 | + |
| 338 | + # Should `getBazaarBranch` create a working tree? |
| 339 | + needs_bzr_tree = True |
| 340 | |
| 341 | required_format = BzrDirFormat.get_default_format() |
| 342 | |
| 343 | @@ -372,21 +380,22 @@ |
| 344 | import_data_transport, self.source_details) |
| 345 | self._logger = logger |
| 346 | |
| 347 | - def getBazaarWorkingTree(self): |
| 348 | - """Return the Bazaar `WorkingTree` that we are importing into.""" |
| 349 | - if os.path.isdir(self.BZR_WORKING_TREE_PATH): |
| 350 | - shutil.rmtree(self.BZR_WORKING_TREE_PATH) |
| 351 | + def getBazaarBranch(self): |
| 352 | + """Return the Bazaar `Branch` that we are importing into.""" |
| 353 | + if os.path.isdir(self.BZR_BRANCH_PATH): |
| 354 | + shutil.rmtree(self.BZR_BRANCH_PATH) |
| 355 | return self.bazaar_branch_store.pull( |
| 356 | - self.source_details.branch_id, self.BZR_WORKING_TREE_PATH, |
| 357 | - self.required_format) |
| 358 | + self.source_details.branch_id, self.BZR_BRANCH_PATH, |
| 359 | + self.required_format, self.needs_bzr_tree) |
| 360 | |
| 361 | - def pushBazaarWorkingTree(self, bazaar_tree): |
| 362 | - """Push the updated Bazaar working tree to the server. |
| 363 | + def pushBazaarBranch(self, bazaar_branch): |
| 364 | + """Push the updated Bazaar branch to the server. |
| 365 | |
| 366 | :return: True if revisions were transferred. |
| 367 | """ |
| 368 | return self.bazaar_branch_store.push( |
| 369 | - self.source_details.branch_id, bazaar_tree, self.required_format) |
| 370 | + self.source_details.branch_id, bazaar_branch, |
| 371 | + self.required_format) |
| 372 | |
| 373 | def getWorkingDirectory(self): |
| 374 | """The directory we should change to and store all scratch files in. |
| 375 | @@ -455,14 +464,15 @@ |
| 376 | os.mkdir(self.FOREIGN_WORKING_TREE_PATH) |
| 377 | return self.foreign_tree_store.fetch(self.FOREIGN_WORKING_TREE_PATH) |
| 378 | |
| 379 | - def importToBazaar(self, foreign_tree, bazaar_tree): |
| 380 | - """Actually import `foreign_tree` into `bazaar_tree`. |
| 381 | + def importToBazaar(self, foreign_tree, bazaar_branch): |
| 382 | + """Actually import `foreign_tree` into `bazaar_branch`. |
| 383 | |
| 384 | :param foreign_tree: A `SubversionWorkingTree` or a `CVSWorkingTree`. |
| 385 | - :param bazaar_tree: A `bzrlib.workingtree.WorkingTree`. |
| 386 | + :param bazaar_tree: A `bzrlib.branch.Branch`, which must have a |
| 387 | + colocated working tree. |
| 388 | """ |
| 389 | foreign_directory = foreign_tree.local_path |
| 390 | - bzr_directory = str(bazaar_tree.basedir) |
| 391 | + bzr_directory = str(bazaar_branch.bzrdir.open_workingtree().basedir) |
| 392 | |
| 393 | scm_branch = SCM.branch(bzr_directory) |
| 394 | last_commit = cscvs.findLastCscvsCommit(scm_branch) |
| 395 | @@ -500,9 +510,9 @@ |
| 396 | |
| 397 | def _doImport(self): |
| 398 | foreign_tree = self.getForeignTree() |
| 399 | - bazaar_tree = self.getBazaarWorkingTree() |
| 400 | - self.importToBazaar(foreign_tree, bazaar_tree) |
| 401 | - non_trivial = self.pushBazaarWorkingTree(bazaar_tree) |
| 402 | + bazaar_branch = self.getBazaarBranch() |
| 403 | + self.importToBazaar(foreign_tree, bazaar_branch) |
| 404 | + non_trivial = self.pushBazaarBranch(bazaar_branch) |
| 405 | self.foreign_tree_store.archive(foreign_tree) |
| 406 | if non_trivial: |
| 407 | return CodeImportWorkerExitCode.SUCCESS |
| 408 | @@ -516,6 +526,8 @@ |
| 409 | Subclasses need to implement `format_classes`. |
| 410 | """ |
| 411 | |
| 412 | + needs_bzr_tree = False |
| 413 | + |
| 414 | @property |
| 415 | def format_classes(self): |
| 416 | """The format classes that should be tried for this import.""" |
| 417 | @@ -531,13 +543,14 @@ |
| 418 | return {} |
| 419 | |
| 420 | def _doImport(self): |
| 421 | - bazaar_tree = self.getBazaarWorkingTree() |
| 422 | - self.bazaar_branch_store.push( |
| 423 | - self.source_details.branch_id, bazaar_tree, self.required_format) |
| 424 | + self._logger.info("Starting job.") |
| 425 | saved_factory = bzrlib.ui.ui_factory |
| 426 | bzrlib.ui.ui_factory = LoggingUIFactory( |
| 427 | writer=lambda m: self._logger.info('%s', m)) |
| 428 | try: |
| 429 | + self._logger.info( |
| 430 | + "Getting exising bzr branch from central store.") |
| 431 | + bazaar_branch = self.getBazaarBranch() |
| 432 | transport = get_transport(self.source_details.url) |
| 433 | for format_class in self.format_classes: |
| 434 | try: |
| 435 | @@ -549,11 +562,14 @@ |
| 436 | raise NotBranchError(self.source_details.url) |
| 437 | foreign_branch = format.open(transport).open_branch() |
| 438 | foreign_branch_tip = foreign_branch.last_revision() |
| 439 | - inter_branch = InterBranch.get(foreign_branch, bazaar_tree.branch) |
| 440 | + inter_branch = InterBranch.get(foreign_branch, bazaar_branch) |
| 441 | + self._logger.info("Importing foreign branch.") |
| 442 | pull_result = inter_branch.pull( |
| 443 | overwrite=True, **self.getExtraPullArgs()) |
| 444 | - self.pushBazaarWorkingTree(bazaar_tree) |
| 445 | - last_imported_revison = bazaar_tree.branch.last_revision() |
| 446 | + self._logger.info("Pushing foreign branch to central store.") |
| 447 | + self.pushBazaarBranch(bazaar_branch) |
| 448 | + last_imported_revison = bazaar_branch.last_revision() |
| 449 | + self._logger.info("Job complete.") |
| 450 | if last_imported_revison == foreign_branch_tip: |
| 451 | if pull_result.old_revid != pull_result.new_revid: |
| 452 | return CodeImportWorkerExitCode.SUCCESS |
| 453 | @@ -583,39 +599,38 @@ |
| 454 | """See `PullingImportWorker.getExtraPullArgs`.""" |
| 455 | return {'limit': config.codeimport.git_revisions_import_limit} |
| 456 | |
| 457 | - def getBazaarWorkingTree(self): |
| 458 | - """See `ImportWorker.getBazaarWorkingTree`. |
| 459 | + def getBazaarBranch(self): |
| 460 | + """See `ImportWorker.getBazaarBranch`. |
| 461 | |
| 462 | - In addition to the superclass' behaviour, we retrieve the 'git.db' |
| 463 | - shamap from the import data store and put it where bzr-git will find |
| 464 | - it in the Bazaar tree, that is at '.bzr/repository/git.db'. |
| 465 | + In addition to the superclass' behaviour, we retrieve bzr-git's |
| 466 | + caches, both legacy and modern, from the import data store and put |
| 467 | + them where bzr-git will find them in the Bazaar tree, that is at |
| 468 | + '.bzr/repository/git.db' and '.bzr/repository/git'. |
| 469 | """ |
| 470 | - tree = PullingImportWorker.getBazaarWorkingTree(self) |
| 471 | + branch = PullingImportWorker.getBazaarBranch(self) |
| 472 | # Fetch the legacy cache from the store, if present. |
| 473 | self.import_data_store.fetch( |
| 474 | - 'git.db', tree.branch.repository._transport) |
| 475 | + 'git.db', branch.repository._transport) |
| 476 | # The cache dir from newer bzr-gits is stored as a tarball. |
| 477 | local_name = 'git-cache.tar.gz' |
| 478 | if self.import_data_store.fetch(local_name): |
| 479 | - repo_transport = tree.branch.repository._transport |
| 480 | + repo_transport = branch.repository._transport |
| 481 | repo_transport.mkdir('git') |
| 482 | git_db_dir = os.path.join( |
| 483 | local_path_from_url(repo_transport.base), 'git') |
| 484 | extract_tarball(local_name, git_db_dir) |
| 485 | - return tree |
| 486 | - |
| 487 | - def pushBazaarWorkingTree(self, bazaar_tree): |
| 488 | - """See `ImportWorker.pushBazaarWorkingTree`. |
| 489 | - |
| 490 | - In addition to the superclass' behaviour, we store the 'git.db' shamap |
| 491 | - that bzr-git will have created at .bzr/repository/bzr.git into the |
| 492 | - import data store. |
| 493 | + return branch |
| 494 | + |
| 495 | + def pushBazaarBranch(self, bazaar_branch): |
| 496 | + """See `ImportWorker.pushBazaarBranch`. |
| 497 | + |
| 498 | + In addition to the superclass' behaviour, we store bzr-git's cache |
| 499 | + directory at .bzr/repository/git in the import data store. |
| 500 | """ |
| 501 | - non_trivial = PullingImportWorker.pushBazaarWorkingTree( |
| 502 | - self, bazaar_tree) |
| 503 | - repo_transport = bazaar_tree.branch.repository._transport |
| 504 | - git_db_dir = os.path.join( |
| 505 | - local_path_from_url(repo_transport.base), 'git') |
| 506 | + non_trivial = PullingImportWorker.pushBazaarBranch( |
| 507 | + self, bazaar_branch) |
| 508 | + repo_base = bazaar_branch.repository._transport.base |
| 509 | + git_db_dir = os.path.join(local_path_from_url(repo_base), 'git') |
| 510 | local_name = 'git-cache.tar.gz' |
| 511 | create_tarball(git_db_dir, local_name) |
| 512 | self.import_data_store.put(local_name) |
| 513 | @@ -637,29 +652,29 @@ |
| 514 | from bzrlib.plugins.hg import HgBzrDirFormat |
| 515 | return [HgBzrDirFormat] |
| 516 | |
| 517 | - def getBazaarWorkingTree(self): |
| 518 | - """See `ImportWorker.getBazaarWorkingTree`. |
| 519 | + def getBazaarBranch(self): |
| 520 | + """See `ImportWorker.getBazaarBranch`. |
| 521 | |
| 522 | In addition to the superclass' behaviour, we retrieve the 'hg-v2.db' |
| 523 | map from the import data store and put it where bzr-hg will find |
| 524 | it in the Bazaar tree, that is at '.bzr/repository/hg-v2.db'. |
| 525 | """ |
| 526 | - tree = PullingImportWorker.getBazaarWorkingTree(self) |
| 527 | + branch = PullingImportWorker.getBazaarBranch(self) |
| 528 | self.import_data_store.fetch( |
| 529 | - self.db_file, tree.branch.repository._transport) |
| 530 | - return tree |
| 531 | - |
| 532 | - def pushBazaarWorkingTree(self, bazaar_tree): |
| 533 | - """See `ImportWorker.pushBazaarWorkingTree`. |
| 534 | - |
| 535 | - In addition to the superclass' behaviour, we store the 'hg-v2.db' shamap |
| 536 | - that bzr-hg will have created at .bzr/repository/hg-v2.db into the |
| 537 | - import data store. |
| 538 | + self.db_file, branch.repository._transport) |
| 539 | + return branch |
| 540 | + |
| 541 | + def pushBazaarBranch(self, bazaar_branch): |
| 542 | + """See `ImportWorker.pushBazaarBranch`. |
| 543 | + |
| 544 | + In addition to the superclass' behaviour, we store the 'hg-v2.db' |
| 545 | + shamap that bzr-hg will have created at .bzr/repository/hg-v2.db into |
| 546 | + the import data store. |
| 547 | """ |
| 548 | - non_trivial = PullingImportWorker.pushBazaarWorkingTree( |
| 549 | - self, bazaar_tree) |
| 550 | + non_trivial = PullingImportWorker.pushBazaarBranch( |
| 551 | + self, bazaar_branch) |
| 552 | self.import_data_store.put( |
| 553 | - self.db_file, bazaar_tree.branch.repository._transport) |
| 554 | + self.db_file, bazaar_branch.repository._transport) |
| 555 | return non_trivial |
| 556 | |
| 557 |
The change looks fine.
It does get me wondering a bit why we didn't seem to get any logging when it was pushing the branch to the central store before. Thanks for adding the job started and finished logging bits too.
I agree that it makes more sense to use branch rather than working tree in the workers.