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

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!");
     }

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;

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));
+    }
+  }
+
 }

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  )--------------------------------- */
 
 

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;
   }

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,