Index: src/applications/repository/storage/PhabricatorRepository.php =================================================================== --- src/applications/repository/storage/PhabricatorRepository.php +++ src/applications/repository/storage/PhabricatorRepository.php @@ -156,6 +156,14 @@ } 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 @@ $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, '/'); + } + + if ($path !== null || $commit !== null) { + $uri .= '@'; + } + + if ($commit !== null) { + $uri .= $commit; } - return $uri.$subpath; + return $uri; } public function execRemoteCommand($pattern /* , $arg, ... */) { Index: src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php =================================================================== --- src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php +++ src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php @@ -55,4 +55,46 @@ '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)); + } + + } Index: src/applications/repository/worker/commitchangeparser/PhabricatorRepositorySvnCommitChangeParserWorker.php =================================================================== --- src/applications/repository/worker/commitchangeparser/PhabricatorRepositorySvnCommitChangeParserWorker.php +++ src/applications/repository/worker/commitchangeparser/PhabricatorRepositorySvnCommitChangeParserWorker.php @@ -24,7 +24,7 @@ // 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 @@ 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 @@ $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 @@ 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 @@ $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 @@ 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',