From c9a0ffa1cfaf171da8199f7fa79254e64ff6d0ae Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 21 Jan 2014 14:02:58 -0800 Subject: [PATCH] Verify that SVN repository roots really are repository roots Summary: Fixes T3238. Ref T4327. Although the instructions are fairly clear on this, it's easy to miss them. Make sure the root the user enters matches the real root. Test Plan: Added unit tests. Used `bin/repository discover` to hit the check explicitly. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T3238, T4327 Differential Revision: https://secure.phabricator.com/D8020 --- .../DiffusionRepositoryCreateController.php | 6 ++- .../PhabricatorRepositoryURINormalizer.php | 12 +++++ ...ricatorRepositoryURINormalizerTestCase.php | 20 +++++++- .../PhabricatorRepositoryDiscoveryEngine.php | 41 +++++++++++++++ .../PhabricatorWorkingCopyTestCase.php | 10 +++- .../PhabricatorChangeParserTestCase.php | 50 ++++++++++++++++++- 6 files changed, 134 insertions(+), 5 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionRepositoryCreateController.php b/src/applications/diffusion/controller/DiffusionRepositoryCreateController.php index 0486b248fe..417119704f 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryCreateController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryCreateController.php @@ -473,8 +473,10 @@ final class DiffusionRepositoryCreateController "| `svn://svn.example.net/svnroot/` |\n". "| `file:///local/path/to/svnroot/` |\n". "\n\n". - "Make sure you specify the root of the repository, not a ". - "subdirectory."); + "You **MUST** specify the root of the repository, not a ". + "subdirectory. (If you want to import only part of a Subversion ". + "repository, use the //Import Only// option at the end of this ". + "workflow.)"); } else { throw new Exception("Unsupported VCS!"); } diff --git a/src/applications/repository/data/PhabricatorRepositoryURINormalizer.php b/src/applications/repository/data/PhabricatorRepositoryURINormalizer.php index aa7c8d6520..75714f4f83 100644 --- a/src/applications/repository/data/PhabricatorRepositoryURINormalizer.php +++ b/src/applications/repository/data/PhabricatorRepositoryURINormalizer.php @@ -39,12 +39,15 @@ final class PhabricatorRepositoryURINormalizer extends Phobject { const TYPE_GIT = 'git'; + const TYPE_SVN = 'svn'; + private $type; private $uri; public function __construct($type, $uri) { switch ($type) { case self::TYPE_GIT: + case self::TYPE_SVN: break; default: throw new Exception(pht('Unknown URI type "%s"!')); @@ -74,6 +77,13 @@ final class PhabricatorRepositoryURINormalizer extends Phobject { return $uri->getPath(); } + return $this->uri; + case self::TYPE_SVN: + $uri = new PhutilURI($this->uri); + if ($uri->getProtocol()) { + return $uri->getPath(); + } + return $this->uri; } } @@ -90,6 +100,8 @@ final class PhabricatorRepositoryURINormalizer extends Phobject { case self::TYPE_GIT: $path = preg_replace('/\.git$/', '', $path); break; + case self::TYPE_SVN: + break; } return $path; diff --git a/src/applications/repository/data/__tests__/PhabricatorRepositoryURINormalizerTestCase.php b/src/applications/repository/data/__tests__/PhabricatorRepositoryURINormalizerTestCase.php index 32429ce244..d69d98bb54 100644 --- a/src/applications/repository/data/__tests__/PhabricatorRepositoryURINormalizerTestCase.php +++ b/src/applications/repository/data/__tests__/PhabricatorRepositoryURINormalizerTestCase.php @@ -25,7 +25,25 @@ final class PhabricatorRepositoryURINormalizerTestCase $this->assertEqual( $expect, $normal->getNormalizedPath(), - pht('Normalized path for "%s".', $input)); + pht('Normalized Git path for "%s".', $input)); } } + + public function testSVNURINormalizer() { + $cases = array( + 'file:///path/to/repo' => 'path/to/repo', + 'file:///path/to/repo/' => 'path/to/repo', + ); + + $type_svn = PhabricatorRepositoryURINormalizer::TYPE_SVN; + + foreach ($cases as $input => $expect) { + $normal = new PhabricatorRepositoryURINormalizer($type_svn, $input); + $this->assertEqual( + $expect, + $normal->getNormalizedPath(), + pht('Normalized SVN path for "%s".', $input)); + } + } + } diff --git a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php index b6f5fe4ba3..3744f09d11 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php @@ -225,6 +225,10 @@ final class PhabricatorRepositoryDiscoveryEngine private function discoverSubversionCommits() { $repository = $this->getRepository(); + if (!$repository->isHosted()) { + $this->verifySubversionRoot($repository); + } + $upper_bound = null; $limit = 1; $refs = array(); @@ -289,6 +293,43 @@ final class PhabricatorRepositoryDiscoveryEngine } + private function verifySubversionRoot(PhabricatorRepository $repository) { + list($xml) = $repository->execxRemoteCommand( + 'info --xml %s', + $repository->getSubversionPathURI()); + + $xml = phutil_utf8ize($xml); + $xml = new SimpleXMLElement($xml); + + $remote_root = (string)($xml->entry[0]->repository[0]->root[0]); + $expect_root = $repository->getSubversionPathURI(); + + $normal_type_svn = PhabricatorRepositoryURINormalizer::TYPE_SVN; + + $remote_normal = id(new PhabricatorRepositoryURINormalizer( + $normal_type_svn, + $remote_root))->getNormalizedPath(); + + $expect_normal = id(new PhabricatorRepositoryURINormalizer( + $normal_type_svn, + $expect_root))->getNormalizedPath(); + + if ($remote_normal != $expect_normal) { + throw new Exception( + pht( + 'Repository "%s" does not have a correctly configured remote URI. '. + 'The remote URI for a Subversion repository MUST point at the '. + 'repository root. The root for this repository is "%s", but the '. + 'configured URI is "%s". To resolve this error, set the remote URI '. + 'to point at the repository root. If you want to import only part '. + 'of a Subversion repository, use the "Import Only" option.', + $repository->getCallsign(), + $remote_root, + $expect_root)); + } + } + + /* -( Discovering Mercurial Repositories )--------------------------------- */ diff --git a/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyTestCase.php b/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyTestCase.php index ab1ea5ebca..40412e45d0 100644 --- a/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyTestCase.php +++ b/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyTestCase.php @@ -11,6 +11,14 @@ abstract class PhabricatorWorkingCopyTestCase extends PhabricatorTestCase { } protected function buildBareRepository($callsign) { + $existing_repository = id(new PhabricatorRepositoryQuery()) + ->withCallsigns(array($callsign)) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->executeOne(); + if ($existing_repository) { + $existing_repository->delete(); + } + $data_dir = dirname(__FILE__).'/data/'; $types = array( @@ -84,7 +92,7 @@ abstract class PhabricatorWorkingCopyTestCase extends PhabricatorTestCase { id(new PhabricatorRepositoryDiscoveryEngine()) ->setRepository($repository) - ->discoverCommits($repository); + ->discoverCommits(); return $repository; } diff --git a/src/applications/repository/worker/__tests__/PhabricatorChangeParserTestCase.php b/src/applications/repository/worker/__tests__/PhabricatorChangeParserTestCase.php index 54bbdd74c8..1f02ae8e7a 100644 --- a/src/applications/repository/worker/__tests__/PhabricatorChangeParserTestCase.php +++ b/src/applications/repository/worker/__tests__/PhabricatorChangeParserTestCase.php @@ -962,7 +962,7 @@ final class PhabricatorChangeParserTestCase id(new PhabricatorRepositoryDiscoveryEngine()) ->setRepository($repository) - ->discoverCommits($repository); + ->discoverCommits(); $viewer = PhabricatorUser::getOmnipotentUser(); @@ -1056,6 +1056,54 @@ final class PhabricatorChangeParserTestCase )); } + public function testSubversionValidRootParser() { + // First, automatically configure the root correctly. + $repository = $this->buildBareRepository('CHD'); + id(new PhabricatorRepositoryPullEngine()) + ->setRepository($repository) + ->pullRepository(); + + $caught = null; + try { + id(new PhabricatorRepositoryDiscoveryEngine()) + ->setRepository($repository) + ->discoverCommits(); + } catch (Exception $ex) { + $caught = $ex; + } + + $this->assertEqual( + false, + ($caught instanceof Exception), + pht('Natural SVN root should work properly.')); + + + // This time, artificially break the root. We expect this to fail. + $repository = $this->buildBareRepository('CHD'); + $repository->setDetail( + 'remote-uri', + $repository->getDetail('remote-uri').'trunk/'); + + id(new PhabricatorRepositoryPullEngine()) + ->setRepository($repository) + ->pullRepository(); + + $caught = null; + try { + id(new PhabricatorRepositoryDiscoveryEngine()) + ->setRepository($repository) + ->discoverCommits(); + } catch (Exception $ex) { + $caught = $ex; + } + + $this->assertEqual( + true, + ($caught instanceof Exception), + pht('Artificial SVN root should fail.')); + } + + private function expectChanges( PhabricatorRepository $repository, array $commits,