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
=== modified file 'lib/lp/codehosting/codeimport/tests/test_worker.py'
--- lib/lp/codehosting/codeimport/tests/test_worker.py 2009-11-23 21:20:38 +0000
+++ lib/lp/codehosting/codeimport/tests/test_worker.py 2009-12-06 23:05:29 +0000
@@ -14,15 +14,15 @@
14import time14import time
15import unittest15import unittest
1616
17from bzrlib.branch import Branch17from bzrlib.branch import Branch, BranchReferenceFormat
18from bzrlib.bzrdir import BzrDir, BzrDirFormat, format_registry18from bzrlib.bzrdir import BzrDir, BzrDirFormat, format_registry
19from bzrlib.errors import NoSuchFile19from bzrlib.errors import NoSuchFile, NotBranchError
20from bzrlib.tests import TestCaseWithTransport20from bzrlib.tests import TestCaseWithTransport
21from bzrlib.transport import get_transport21from bzrlib.transport import get_transport
22from bzrlib.upgrade import upgrade22from bzrlib.upgrade import upgrade
23from bzrlib.urlutils import join as urljoin23from bzrlib.urlutils import join as urljoin
2424
25from CVS import Repository, tree25from CVS import Repository, tree as CVSTree
2626
27from canonical.cachedproperty import cachedproperty27from canonical.cachedproperty import cachedproperty
28from canonical.config import config28from canonical.config import config
@@ -609,10 +609,8 @@
609 self.bazaar_store = BazaarBranchStore(609 self.bazaar_store = BazaarBranchStore(
610 self.get_transport('bazaar_store'))610 self.get_transport('bazaar_store'))
611 self.foreign_commit_count = 0611 self.foreign_commit_count = 0
612 self.source_details = self.makeSourceDetails(
613 'trunk', [('README', 'Original contents')])
614612
615 def makeImportWorker(self):613 def makeImportWorker(self, source_details):
616 """Make a new `ImportWorker`.614 """Make a new `ImportWorker`.
617615
618 Override this in your subclass.616 Override this in your subclass.
@@ -620,7 +618,7 @@
620 raise NotImplementedError(618 raise NotImplementedError(
621 "Override this with a VCS-specific implementation.")619 "Override this with a VCS-specific implementation.")
622620
623 def makeForeignCommit(self):621 def makeForeignCommit(self, source_details):
624 """Commit a revision to the repo described by `self.source_details`.622 """Commit a revision to the repo described by `self.source_details`.
625623
626 Increment `self.foreign_commit_count` as appropriate.624 Increment `self.foreign_commit_count` as appropriate.
@@ -650,7 +648,8 @@
650 def test_import(self):648 def test_import(self):
651 # Running the worker on a branch that hasn't been imported yet imports649 # Running the worker on a branch that hasn't been imported yet imports
652 # the branch.650 # the branch.
653 worker = self.makeImportWorker()651 worker = self.makeImportWorker(self.makeSourceDetails(
652 'trunk', [('README', 'Original contents')]))
654 worker.run()653 worker.run()
655 branch = self.getStoredBazaarBranch(worker)654 branch = self.getStoredBazaarBranch(worker)
656 self.assertEqual(655 self.assertEqual(
@@ -658,14 +657,15 @@
658657
659 def test_sync(self):658 def test_sync(self):
660 # Do an import.659 # Do an import.
661 worker = self.makeImportWorker()660 worker = self.makeImportWorker(self.makeSourceDetails(
661 'trunk', [('README', 'Original contents')]))
662 worker.run()662 worker.run()
663 branch = self.getStoredBazaarBranch(worker)663 branch = self.getStoredBazaarBranch(worker)
664 self.assertEqual(664 self.assertEqual(
665 self.foreign_commit_count, len(branch.revision_history()))665 self.foreign_commit_count, len(branch.revision_history()))
666666
667 # Change the remote branch.667 # Change the remote branch.
668 self.makeForeignCommit()668 self.makeForeignCommit(worker.source_details)
669669
670 # Run the same worker again.670 # Run the same worker again.
671 worker.run()671 worker.run()
@@ -678,14 +678,16 @@
678 def test_import_script(self):678 def test_import_script(self):
679 # Like test_import, but using the code-import-worker.py script679 # Like test_import, but using the code-import-worker.py script
680 # to perform the import.680 # to perform the import.
681 source_details = self.makeSourceDetails(
682 'trunk', [('README', 'Original contents')])
681683
682 clean_up_default_stores_for_import(self.source_details)684 clean_up_default_stores_for_import(source_details)
683685
684 script_path = os.path.join(686 script_path = os.path.join(
685 config.root, 'scripts', 'code-import-worker.py')687 config.root, 'scripts', 'code-import-worker.py')
686 output = tempfile.TemporaryFile()688 output = tempfile.TemporaryFile()
687 retcode = subprocess.call(689 retcode = subprocess.call(
688 [script_path] + self.source_details.asArguments(),690 [script_path] + source_details.asArguments(),
689 stderr=output, stdout=output)691 stderr=output, stdout=output)
690 self.assertEqual(retcode, 0)692 self.assertEqual(retcode, 0)
691693
@@ -697,13 +699,13 @@
697 self.assertPositive(output.tell())699 self.assertPositive(output.tell())
698700
699 self.addCleanup(701 self.addCleanup(
700 lambda : clean_up_default_stores_for_import(self.source_details))702 lambda : clean_up_default_stores_for_import(source_details))
701703
702 tree_path = tempfile.mkdtemp()704 tree_path = tempfile.mkdtemp()
703 self.addCleanup(lambda: shutil.rmtree(tree_path))705 self.addCleanup(lambda: shutil.rmtree(tree_path))
704706
705 branch_url = get_default_bazaar_branch_store()._getMirrorURL(707 branch_url = get_default_bazaar_branch_store()._getMirrorURL(
706 self.source_details.branch_id)708 source_details.branch_id)
707 branch = Branch.open(branch_url)709 branch = Branch.open(branch_url)
708710
709 self.assertEqual(711 self.assertEqual(
@@ -720,10 +722,10 @@
720 """722 """
721 TestActualImportMixin.setUpImport(self)723 TestActualImportMixin.setUpImport(self)
722724
723 def makeImportWorker(self):725 def makeImportWorker(self, source_details):
724 """Make a new `ImportWorker`."""726 """Make a new `ImportWorker`."""
725 return CSCVSImportWorker(727 return CSCVSImportWorker(
726 self.source_details, self.get_transport('foreign_store'),728 source_details, self.get_transport('foreign_store'),
727 self.bazaar_store, logging.getLogger())729 self.bazaar_store, logging.getLogger())
728730
729731
@@ -734,13 +736,13 @@
734 super(TestCVSImport, self).setUp()736 super(TestCVSImport, self).setUp()
735 self.setUpImport()737 self.setUpImport()
736738
737 def makeForeignCommit(self):739 def makeForeignCommit(self, source_details):
738 # If you write to a file in the same second as the previous commit,740 # If you write to a file in the same second as the previous commit,
739 # CVS will not think that it has changed.741 # CVS will not think that it has changed.
740 time.sleep(1)742 time.sleep(1)
741 repo = Repository(self.source_details.cvs_root, QuietFakeLogger())743 repo = Repository(source_details.cvs_root, QuietFakeLogger())
742 repo.get(self.source_details.cvs_module, 'working_dir')744 repo.get(source_details.cvs_module, 'working_dir')
743 wt = tree('working_dir')745 wt = CVSTree('working_dir')
744 self.build_tree_contents([('working_dir/README', 'New content')])746 self.build_tree_contents([('working_dir/README', 'New content')])
745 wt.commit(log='Log message')747 wt.commit(log='Log message')
746 self.foreign_commit_count += 1748 self.foreign_commit_count += 1
@@ -765,10 +767,10 @@
765 """Implementations of `makeForeignCommit` and `makeSourceDetails` for svn.767 """Implementations of `makeForeignCommit` and `makeSourceDetails` for svn.
766 """768 """
767769
768 def makeForeignCommit(self):770 def makeForeignCommit(self, source_details):
769 """Change the foreign tree."""771 """Change the foreign tree."""
770 client = pysvn.Client()772 client = pysvn.Client()
771 client.checkout(self.source_details.svn_branch_url, 'working_tree')773 client.checkout(source_details.svn_branch_url, 'working_tree')
772 file = open('working_tree/newfile', 'w')774 file = open('working_tree/newfile', 'w')
773 file.write('No real content\n')775 file.write('No real content\n')
774 file.close()776 file.close()
@@ -802,7 +804,40 @@
802 self.setUpImport()804 self.setUpImport()
803805
804806
805class TestGitImport(WorkerTest, TestActualImportMixin):807class PullingImportWorkerTests:
808 """Tests for the PullingImportWorker subclasses."""
809
810 def createBranchReference(self):
811 """Create a pure branch reference that points to a branch.
812 """
813 branch = self.make_branch('branch')
814 t = get_transport(self.get_url('.'))
815 t.mkdir('reference')
816 a_bzrdir = BzrDir.create(self.get_url('reference'))
817 BranchReferenceFormat().initialize(a_bzrdir, branch)
818 return a_bzrdir.root_transport.base
819
820 def test_reject_branch_reference(self):
821 # URLs that point to other branch types than that expected by the
822 # import should be rejected.
823 args = {'rcstype': self.rcstype}
824 reference_url = self.createBranchReference()
825 if self.rcstype == 'git':
826 args['git_repo_url'] = reference_url
827 elif self.rcstype == 'bzr-svn':
828 args['svn_branch_url'] = reference_url
829 else:
830 raise AssertionError("unexpected rcs_type %r" % self.rcs_type)
831 source_details = self.factory.makeCodeImportSourceDetails(**args)
832
833 worker = self.makeImportWorker(source_details)
834 self.assertRaises(NotBranchError, worker.run)
835
836
837class TestGitImport(WorkerTest, TestActualImportMixin,
838 PullingImportWorkerTests):
839
840 rcstype = 'git'
806841
807 def setUp(self):842 def setUp(self):
808 super(TestGitImport, self).setUp()843 super(TestGitImport, self).setUp()
@@ -821,17 +856,17 @@
821 mapdbs().clear()856 mapdbs().clear()
822 WorkerTest.tearDown(self)857 WorkerTest.tearDown(self)
823858
824 def makeImportWorker(self):859 def makeImportWorker(self, source_details):
825 """Make a new `ImportWorker`."""860 """Make a new `ImportWorker`."""
826 return GitImportWorker(861 return GitImportWorker(
827 self.source_details, self.get_transport('import_data'),862 source_details, self.get_transport('import_data'),
828 self.bazaar_store, logging.getLogger())863 self.bazaar_store, logging.getLogger())
829864
830 def makeForeignCommit(self):865 def makeForeignCommit(self, source_details):
831 """Change the foreign tree, generating exactly one commit."""866 """Change the foreign tree, generating exactly one commit."""
832 from bzrlib.plugins.git.tests import run_git867 from bzrlib.plugins.git.tests import run_git
833 wd = os.getcwd()868 wd = os.getcwd()
834 os.chdir(self.source_details.git_repo_url)869 os.chdir(source_details.git_repo_url)
835 try:870 try:
836 run_git('config', 'user.name', 'Joe Random Hacker')871 run_git('config', 'user.name', 'Joe Random Hacker')
837 run_git('commit', '-m', 'dsadas')872 run_git('commit', '-m', 'dsadas')
@@ -855,7 +890,7 @@
855890
856891
857class TestBzrSvnImport(WorkerTest, SubversionImportHelpers,892class TestBzrSvnImport(WorkerTest, SubversionImportHelpers,
858 TestActualImportMixin):893 TestActualImportMixin, PullingImportWorkerTests):
859894
860 rcstype = 'bzr-svn'895 rcstype = 'bzr-svn'
861896
@@ -872,10 +907,10 @@
872 os.environ['HOME'] = os.environ['HOME'].encode(907 os.environ['HOME'] = os.environ['HOME'].encode(
873 sys.getfilesystemencoding())908 sys.getfilesystemencoding())
874909
875 def makeImportWorker(self):910 def makeImportWorker(self, source_details):
876 """Make a new `ImportWorker`."""911 """Make a new `ImportWorker`."""
877 return BzrSvnImportWorker(912 return BzrSvnImportWorker(
878 self.source_details, self.get_transport('import_data'),913 source_details, self.get_transport('import_data'),
879 self.bazaar_store, logging.getLogger())914 self.bazaar_store, logging.getLogger())
880915
881916
882917
=== 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-06 23:05:29 +0000
@@ -489,13 +489,18 @@
489class PullingImportWorker(ImportWorker):489class PullingImportWorker(ImportWorker):
490 """An import worker for imports that can be done by a bzr plugin.490 """An import worker for imports that can be done by a bzr plugin.
491491
492 Subclasses need to implement `pull_url`.492 Subclasses need to implement `pull_url` and `format_classes`.
493 """493 """
494 @property494 @property
495 def pull_url(self):495 def pull_url(self):
496 """Return the URL that should be pulled from."""496 """Return the URL that should be pulled from."""
497 raise NotImplementedError497 raise NotImplementedError
498498
499 @property
500 def format_classes(self):
501 """The format classes that should be tried for this import."""
502 raise NotImplementedError
503
499 def _doImport(self):504 def _doImport(self):
500 bazaar_tree = self.getBazaarWorkingTree()505 bazaar_tree = self.getBazaarWorkingTree()
501 self.bazaar_branch_store.push(506 self.bazaar_branch_store.push(
@@ -504,8 +509,17 @@
504 bzrlib.ui.ui_factory = LoggingUIFactory(509 bzrlib.ui.ui_factory = LoggingUIFactory(
505 writer=lambda m: self._logger.info('%s', m))510 writer=lambda m: self._logger.info('%s', m))
506 try:511 try:
507 bazaar_tree.branch.pull(512 transport = get_transport(self.pull_url)
508 Branch.open(self.pull_url), overwrite=True)513 for format_class in self.format_classes:
514 try:
515 format = format_class.probe_transport(transport)
516 break
517 except NotBranchError:
518 pass
519 else:
520 raise NotBranchError(self.pull_url)
521 foreign_branch = format.open(transport).open_branch()
522 bazaar_tree.branch.pull(foreign_branch, overwrite=True)
509 finally:523 finally:
510 bzrlib.ui.ui_factory = saved_factory524 bzrlib.ui.ui_factory = saved_factory
511 self.pushBazaarWorkingTree(bazaar_tree)525 self.pushBazaarWorkingTree(bazaar_tree)
@@ -522,6 +536,14 @@
522 """See `PullingImportWorker.pull_url`."""536 """See `PullingImportWorker.pull_url`."""
523 return self.source_details.git_repo_url537 return self.source_details.git_repo_url
524538
539 @property
540 def format_classes(self):
541 """See `PullingImportWorker.opening_format`."""
542 # We only return LocalGitBzrDirFormat for tests.
543 from bzrlib.plugins.git import (
544 LocalGitBzrDirFormat, RemoteGitBzrDirFormat)
545 return [LocalGitBzrDirFormat, RemoteGitBzrDirFormat]
546
525 def getBazaarWorkingTree(self):547 def getBazaarWorkingTree(self):
526 """See `ImportWorker.getBazaarWorkingTree`.548 """See `ImportWorker.getBazaarWorkingTree`.
527549
@@ -553,3 +575,9 @@
553 def pull_url(self):575 def pull_url(self):
554 """See `PullingImportWorker.pull_url`."""576 """See `PullingImportWorker.pull_url`."""
555 return self.source_details.svn_branch_url577 return self.source_details.svn_branch_url
578
579 @property
580 def format_classes(self):
581 """See `PullingImportWorker.opening_format`."""
582 from bzrlib.plugins.svn.format import SvnRemoteFormat
583 return [SvnRemoteFormat]