Launchpad itself

Merge lp:~mwhudson/launchpad/code-import-paranoia into lp:launchpad

  1. code-import-paranoia
  2. Merge into devel
Proposed by Michael Hudson-Doyle
Status: Merged
Approved by: Tim Penhey
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~mwhudson/launchpad/code-import-paranoia
Merge into: lp:launchpad
Diff against target: 305 lines (+96/-33)
2 files modified
lib/lp/codehosting/codeimport/tests/test_worker.py (+65/-30)
lib/lp/codehosting/codeimport/worker.py (+31/-3)
To merge this branch: bzr merge lp:~mwhudson/launchpad/code-import-paranoia
Reviewer Review Type Date Requested Status
Tim Penhey (community) Approve
Review via email: mp+15467@code.launchpad.net

Commit message

Only allow the PullingImportWorker to open branches of the appropriate foreign format.

To post a comment you must log in.
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Hi,

This branch makes the code import worker code much more careful about the formats that it might open. It's not a real problem so far because we only accept git:// urls for git imports, but for bzr-svn and for git over http, it will be.

Most of the changes by diff-volume are to do with how the tests are parameterized, the actual change is fairly simple.

It's a shame that only the LocalGitBzrDirFormat format is exercised in the tests whereas only the RemoteGitBzrDirFormat will be used in production. Not sure what we can do about that though.

Cheers,
mwh

Revision history for this message
Tim Penhey (thumper) wrote :

  review approve

> It's a shame that only the LocalGitBzrDirFormat format is exercised in the
> tests whereas only the RemoteGitBzrDirFormat will be used in production.
> Not sure what we can do about that though.

Yeah, that's a bit of a question that needs an answer...

> === modified file 'lib/lp/codehosting/codeimport/worker.py'
> --- lib/lp/codehosting/codeimport/worker.py 2009-11-19 04:22:10 +0000
> +++ lib/lp/codehosting/codeimport/worker.py 2009-12-01 03:20:33 +0000
> @@ -504,8 +509,16 @@
> bzrlib.ui.ui_factory = LoggingUIFactory(
> writer=lambda m: self._logger.info('%s', m))
> try:
> - bazaar_tree.branch.pull(
> - Branch.open(self.pull_url), overwrite=True)
> + transport = get_transport(self.pull_url)
> + for format_class in self.format_classes:
> + try:
> + format = format_class.probe_transport(transport)
> + except NotBranchError:
> + pass
> + else:
> + raise NotBranchError(self.pull_url)
> + foreign_branch = format.open(transport).open_branch()
> + bazaar_tree.branch.pull(foreign_branch, overwrite=True)
> finally:
> bzrlib.ui.ui_factory = saved_factory
> self.pushBazaarWorkingTree(bazaar_tree)

This hunk looks wrong. The else clause is hanging wrongly.

review: Approve

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 2009-11-23 21:20:38 +0000
3+++ lib/lp/codehosting/codeimport/tests/test_worker.py 2009-12-06 23:05:29 +0000
4@@ -14,15 +14,15 @@
5 import time
6 import unittest
7
8-from bzrlib.branch import Branch
9+from bzrlib.branch import Branch, BranchReferenceFormat
10 from bzrlib.bzrdir import BzrDir, BzrDirFormat, format_registry
11-from bzrlib.errors import NoSuchFile
12+from bzrlib.errors import NoSuchFile, NotBranchError
13 from bzrlib.tests import TestCaseWithTransport
14 from bzrlib.transport import get_transport
15 from bzrlib.upgrade import upgrade
16 from bzrlib.urlutils import join as urljoin
17
18-from CVS import Repository, tree
19+from CVS import Repository, tree as CVSTree
20
21 from canonical.cachedproperty import cachedproperty
22 from canonical.config import config
23@@ -609,10 +609,8 @@
24 self.bazaar_store = BazaarBranchStore(
25 self.get_transport('bazaar_store'))
26 self.foreign_commit_count = 0
27- self.source_details = self.makeSourceDetails(
28- 'trunk', [('README', 'Original contents')])
29
30- def makeImportWorker(self):
31+ def makeImportWorker(self, source_details):
32 """Make a new `ImportWorker`.
33
34 Override this in your subclass.
35@@ -620,7 +618,7 @@
36 raise NotImplementedError(
37 "Override this with a VCS-specific implementation.")
38
39- def makeForeignCommit(self):
40+ def makeForeignCommit(self, source_details):
41 """Commit a revision to the repo described by `self.source_details`.
42
43 Increment `self.foreign_commit_count` as appropriate.
44@@ -650,7 +648,8 @@
45 def test_import(self):
46 # Running the worker on a branch that hasn't been imported yet imports
47 # the branch.
48- worker = self.makeImportWorker()
49+ worker = self.makeImportWorker(self.makeSourceDetails(
50+ 'trunk', [('README', 'Original contents')]))
51 worker.run()
52 branch = self.getStoredBazaarBranch(worker)
53 self.assertEqual(
54@@ -658,14 +657,15 @@
55
56 def test_sync(self):
57 # Do an import.
58- worker = self.makeImportWorker()
59+ worker = self.makeImportWorker(self.makeSourceDetails(
60+ 'trunk', [('README', 'Original contents')]))
61 worker.run()
62 branch = self.getStoredBazaarBranch(worker)
63 self.assertEqual(
64 self.foreign_commit_count, len(branch.revision_history()))
65
66 # Change the remote branch.
67- self.makeForeignCommit()
68+ self.makeForeignCommit(worker.source_details)
69
70 # Run the same worker again.
71 worker.run()
72@@ -678,14 +678,16 @@
73 def test_import_script(self):
74 # Like test_import, but using the code-import-worker.py script
75 # to perform the import.
76+ source_details = self.makeSourceDetails(
77+ 'trunk', [('README', 'Original contents')])
78
79- clean_up_default_stores_for_import(self.source_details)
80+ clean_up_default_stores_for_import(source_details)
81
82 script_path = os.path.join(
83 config.root, 'scripts', 'code-import-worker.py')
84 output = tempfile.TemporaryFile()
85 retcode = subprocess.call(
86- [script_path] + self.source_details.asArguments(),
87+ [script_path] + source_details.asArguments(),
88 stderr=output, stdout=output)
89 self.assertEqual(retcode, 0)
90
91@@ -697,13 +699,13 @@
92 self.assertPositive(output.tell())
93
94 self.addCleanup(
95- lambda : clean_up_default_stores_for_import(self.source_details))
96+ lambda : clean_up_default_stores_for_import(source_details))
97
98 tree_path = tempfile.mkdtemp()
99 self.addCleanup(lambda: shutil.rmtree(tree_path))
100
101 branch_url = get_default_bazaar_branch_store()._getMirrorURL(
102- self.source_details.branch_id)
103+ source_details.branch_id)
104 branch = Branch.open(branch_url)
105
106 self.assertEqual(
107@@ -720,10 +722,10 @@
108 """
109 TestActualImportMixin.setUpImport(self)
110
111- def makeImportWorker(self):
112+ def makeImportWorker(self, source_details):
113 """Make a new `ImportWorker`."""
114 return CSCVSImportWorker(
115- self.source_details, self.get_transport('foreign_store'),
116+ source_details, self.get_transport('foreign_store'),
117 self.bazaar_store, logging.getLogger())
118
119
120@@ -734,13 +736,13 @@
121 super(TestCVSImport, self).setUp()
122 self.setUpImport()
123
124- def makeForeignCommit(self):
125+ def makeForeignCommit(self, source_details):
126 # If you write to a file in the same second as the previous commit,
127 # CVS will not think that it has changed.
128 time.sleep(1)
129- repo = Repository(self.source_details.cvs_root, QuietFakeLogger())
130- repo.get(self.source_details.cvs_module, 'working_dir')
131- wt = tree('working_dir')
132+ repo = Repository(source_details.cvs_root, QuietFakeLogger())
133+ repo.get(source_details.cvs_module, 'working_dir')
134+ wt = CVSTree('working_dir')
135 self.build_tree_contents([('working_dir/README', 'New content')])
136 wt.commit(log='Log message')
137 self.foreign_commit_count += 1
138@@ -765,10 +767,10 @@
139 """Implementations of `makeForeignCommit` and `makeSourceDetails` for svn.
140 """
141
142- def makeForeignCommit(self):
143+ def makeForeignCommit(self, source_details):
144 """Change the foreign tree."""
145 client = pysvn.Client()
146- client.checkout(self.source_details.svn_branch_url, 'working_tree')
147+ client.checkout(source_details.svn_branch_url, 'working_tree')
148 file = open('working_tree/newfile', 'w')
149 file.write('No real content\n')
150 file.close()
151@@ -802,7 +804,40 @@
152 self.setUpImport()
153
154
155-class TestGitImport(WorkerTest, TestActualImportMixin):
156+class PullingImportWorkerTests:
157+ """Tests for the PullingImportWorker subclasses."""
158+
159+ def createBranchReference(self):
160+ """Create a pure branch reference that points to a branch.
161+ """
162+ branch = self.make_branch('branch')
163+ t = get_transport(self.get_url('.'))
164+ t.mkdir('reference')
165+ a_bzrdir = BzrDir.create(self.get_url('reference'))
166+ BranchReferenceFormat().initialize(a_bzrdir, branch)
167+ return a_bzrdir.root_transport.base
168+
169+ def test_reject_branch_reference(self):
170+ # URLs that point to other branch types than that expected by the
171+ # import should be rejected.
172+ args = {'rcstype': self.rcstype}
173+ reference_url = self.createBranchReference()
174+ if self.rcstype == 'git':
175+ args['git_repo_url'] = reference_url
176+ elif self.rcstype == 'bzr-svn':
177+ args['svn_branch_url'] = reference_url
178+ else:
179+ raise AssertionError("unexpected rcs_type %r" % self.rcs_type)
180+ source_details = self.factory.makeCodeImportSourceDetails(**args)
181+
182+ worker = self.makeImportWorker(source_details)
183+ self.assertRaises(NotBranchError, worker.run)
184+
185+
186+class TestGitImport(WorkerTest, TestActualImportMixin,
187+ PullingImportWorkerTests):
188+
189+ rcstype = 'git'
190
191 def setUp(self):
192 super(TestGitImport, self).setUp()
193@@ -821,17 +856,17 @@
194 mapdbs().clear()
195 WorkerTest.tearDown(self)
196
197- def makeImportWorker(self):
198+ def makeImportWorker(self, source_details):
199 """Make a new `ImportWorker`."""
200 return GitImportWorker(
201- self.source_details, self.get_transport('import_data'),
202+ source_details, self.get_transport('import_data'),
203 self.bazaar_store, logging.getLogger())
204
205- def makeForeignCommit(self):
206+ def makeForeignCommit(self, source_details):
207 """Change the foreign tree, generating exactly one commit."""
208 from bzrlib.plugins.git.tests import run_git
209 wd = os.getcwd()
210- os.chdir(self.source_details.git_repo_url)
211+ os.chdir(source_details.git_repo_url)
212 try:
213 run_git('config', 'user.name', 'Joe Random Hacker')
214 run_git('commit', '-m', 'dsadas')
215@@ -855,7 +890,7 @@
216
217
218 class TestBzrSvnImport(WorkerTest, SubversionImportHelpers,
219- TestActualImportMixin):
220+ TestActualImportMixin, PullingImportWorkerTests):
221
222 rcstype = 'bzr-svn'
223
224@@ -872,10 +907,10 @@
225 os.environ['HOME'] = os.environ['HOME'].encode(
226 sys.getfilesystemencoding())
227
228- def makeImportWorker(self):
229+ def makeImportWorker(self, source_details):
230 """Make a new `ImportWorker`."""
231 return BzrSvnImportWorker(
232- self.source_details, self.get_transport('import_data'),
233+ source_details, self.get_transport('import_data'),
234 self.bazaar_store, logging.getLogger())
235
236
237
238=== modified file 'lib/lp/codehosting/codeimport/worker.py'
239--- lib/lp/codehosting/codeimport/worker.py 2009-11-19 04:22:10 +0000
240+++ lib/lp/codehosting/codeimport/worker.py 2009-12-06 23:05:29 +0000
241@@ -489,13 +489,18 @@
242 class PullingImportWorker(ImportWorker):
243 """An import worker for imports that can be done by a bzr plugin.
244
245- Subclasses need to implement `pull_url`.
246+ Subclasses need to implement `pull_url` and `format_classes`.
247 """
248 @property
249 def pull_url(self):
250 """Return the URL that should be pulled from."""
251 raise NotImplementedError
252
253+ @property
254+ def format_classes(self):
255+ """The format classes that should be tried for this import."""
256+ raise NotImplementedError
257+
258 def _doImport(self):
259 bazaar_tree = self.getBazaarWorkingTree()
260 self.bazaar_branch_store.push(
261@@ -504,8 +509,17 @@
262 bzrlib.ui.ui_factory = LoggingUIFactory(
263 writer=lambda m: self._logger.info('%s', m))
264 try:
265- bazaar_tree.branch.pull(
266- Branch.open(self.pull_url), overwrite=True)
267+ transport = get_transport(self.pull_url)
268+ for format_class in self.format_classes:
269+ try:
270+ format = format_class.probe_transport(transport)
271+ break
272+ except NotBranchError:
273+ pass
274+ else:
275+ raise NotBranchError(self.pull_url)
276+ foreign_branch = format.open(transport).open_branch()
277+ bazaar_tree.branch.pull(foreign_branch, overwrite=True)
278 finally:
279 bzrlib.ui.ui_factory = saved_factory
280 self.pushBazaarWorkingTree(bazaar_tree)
281@@ -522,6 +536,14 @@
282 """See `PullingImportWorker.pull_url`."""
283 return self.source_details.git_repo_url
284
285+ @property
286+ def format_classes(self):
287+ """See `PullingImportWorker.opening_format`."""
288+ # We only return LocalGitBzrDirFormat for tests.
289+ from bzrlib.plugins.git import (
290+ LocalGitBzrDirFormat, RemoteGitBzrDirFormat)
291+ return [LocalGitBzrDirFormat, RemoteGitBzrDirFormat]
292+
293 def getBazaarWorkingTree(self):
294 """See `ImportWorker.getBazaarWorkingTree`.
295
296@@ -553,3 +575,9 @@
297 def pull_url(self):
298 """See `PullingImportWorker.pull_url`."""
299 return self.source_details.svn_branch_url
300+
301+ @property
302+ def format_classes(self):
303+ """See `PullingImportWorker.opening_format`."""
304+ from bzrlib.plugins.svn.format import SvnRemoteFormat
305+ return [SvnRemoteFormat]