Make Subversion URI construction more consistent

Summary:
Ref T2230. SVN has some weird rules about path construction. Particularly, if you're missing a "/" in the remote URI right now, the change parsing step doesn't build the right paths.

Instead, build the right paths more intelligently.

Test Plan: Added and executed unit tests. Imported an SVN repo.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, jpeffer

Maniphest Tasks: T2230

Differential Revision: https://secure.phabricator.com/D7590

src/applications/repository/storage/PhabricatorRepository.php

@@ -156,6 +156,14 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
   }
 
   public function getSubversionBaseURI() {
+    $subpath = $this->getDetail('svn-subpath');
+    if (!strlen($subpath)) {
+      $subpath = null;
+    }
+    return $this->getSubversionPathURI($subpath);
+  }
+
+  public function getSubversionPathURI($path = null, $commit = null) {
     $vcs = $this->getVersionControlSystem();
     if ($vcs != PhabricatorRepositoryType::REPOSITORY_TYPE_SVN) {
       throw new Exception("Not a subversion repository!");
@@ -167,12 +175,23 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
       $uri = $this->getDetail('remote-uri');
     }
 
-    $subpath = $this->getDetail('svn-subpath');
-    if ($subpath) {
-      $subpath = '/'.ltrim($subpath, '/');
+    $uri = rtrim($uri, '/');
+
+    if (strlen($path)) {
+      $path = rawurlencode($path);
+      $path = str_replace('%2F', '/', $path);
+      $uri = $uri.'/'.ltrim($path, '/');
     }
 
-    return $uri.$subpath;
+    if ($path !== null || $commit !== null) {
+      $uri .= '@';
+    }
+
+    if ($commit !== null) {
+      $uri .= $commit;
+    }
+
+    return $uri;
   }
 
   public function execRemoteCommand($pattern /* , $arg, ... */) {

src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php

@@ -55,4 +55,46 @@ final class PhabricatorRepositoryTestCase
       'Do not track unlisted branches.');
   }
 
+  public function testSubversionPathInfo() {
+    $svn = PhabricatorRepositoryType::REPOSITORY_TYPE_SVN;
+
+    $repo = new PhabricatorRepository();
+    $repo->setVersionControlSystem($svn);
+
+    $repo->setDetail('remote-uri', 'http://svn.example.com/repo');
+    $this->assertEqual(
+      'http://svn.example.com/repo',
+      $repo->getSubversionPathURI());
+
+    $repo->setDetail('remote-uri', 'http://svn.example.com/repo/');
+    $this->assertEqual(
+      'http://svn.example.com/repo',
+      $repo->getSubversionPathURI());
+
+    $repo->setDetail('hosting-enabled', true);
+
+    $repo->setDetail('local-path', '/var/repo/SVN');
+    $this->assertEqual(
+      'file:///var/repo/SVN',
+      $repo->getSubversionPathURI());
+
+    $repo->setDetail('local-path', '/var/repo/SVN/');
+    $this->assertEqual(
+      'file:///var/repo/SVN',
+      $repo->getSubversionPathURI());
+
+    $this->assertEqual(
+      'file:///var/repo/SVN/a@',
+      $repo->getSubversionPathURI('a'));
+
+    $this->assertEqual(
+      'file:///var/repo/SVN/a@1',
+      $repo->getSubversionPathURI('a', 1));
+
+    $this->assertEqual(
+      'file:///var/repo/SVN/%3F@22',
+      $repo->getSubversionPathURI('?', 22));
+  }
+
+
 }

src/applications/repository/worker/commitchangeparser/PhabricatorRepositorySvnCommitChangeParserWorker.php

@@ -24,7 +24,7 @@ final class PhabricatorRepositorySvnCommitChangeParserWorker
     // recursive paths were affected if it was moved or copied. This is very
     // complicated and has many special cases.
 
-    $uri = $repository->getDetail('remote-uri');
+    $uri = $repository->getSubversionPathURI();
     $svn_commit = $commit->getCommitIdentifier();
 
     // Pull the top-level path changes out of "svn log". This is pretty
@@ -561,7 +561,7 @@ final class PhabricatorRepositorySvnCommitChangeParserWorker
     array $paths) {
 
     $result_map = array();
-    $repository_uri = $repository->getDetail('remote-uri');
+    $repository_uri = $repository->getSubversionPathURI();
 
     if (isset($paths['/'])) {
       $result_map['/'] = DifferentialChangeType::FILE_DIRECTORY;
@@ -572,9 +572,10 @@ final class PhabricatorRepositorySvnCommitChangeParserWorker
     $path_mapping = array();
     foreach ($paths as $path => $lookup) {
       $parent = dirname($lookup['rawPath']);
-      $parent = ltrim($parent, '/');
-      $parent = $this->encodeSVNPath($parent);
-      $parent = $repository_uri.$parent.'@'.$lookup['rawCommit'];
+      $parent = $repository->getSubversionPathURI(
+        $parent,
+        $lookup['rawCommit']);
+
       $parent = escapeshellarg($parent);
       $parents[$parent] = true;
       $path_mapping[$parent][] = dirname($path);
@@ -626,12 +627,6 @@ final class PhabricatorRepositorySvnCommitChangeParserWorker
     return $result_map;
   }
 
-  private function encodeSVNPath($path) {
-    $path = rawurlencode($path);
-    $path = str_replace('%2F', '/', $path);
-    return $path;
-  }
-
   private function getFileTypeFromSVNKind($kind) {
     $kind = (string)$kind;
     switch ($kind) {
@@ -648,9 +643,9 @@ final class PhabricatorRepositorySvnCommitChangeParserWorker
 
     $path = $info['rawPath'];
     $rev  = $info['rawCommit'];
-    $path = $this->encodeSVNPath($path);
 
-    $hashkey = md5($repository->getDetail('remote-uri').$path.'@'.$rev);
+    $path_uri = $repository->getSubversionPathURI($path, $rev);
+    $hashkey = md5($path_uri);
 
     // This method is quite horrible. The underlying challenge is that some
     // commits in the Facebook repository are enormous, taking multiple hours
@@ -664,10 +659,8 @@ final class PhabricatorRepositorySvnCommitChangeParserWorker
     if (!Filesystem::pathExists($cache_loc)) {
       $tmp = new TempFile();
       $repository->execxRemoteCommand(
-        '--xml ls -R %s%s@%d > %s',
-        $repository->getDetail('remote-uri'),
-        $path,
-        $rev,
+        '--xml ls -R %s > %s',
+        $path_uri,
         $tmp);
       execx(
         'mv %s %s',