Index: src/__phutil_library_map__.php =================================================================== --- src/__phutil_library_map__.php +++ src/__phutil_library_map__.php @@ -1881,6 +1881,8 @@ 'PhabricatorRepositoryTransaction' => 'applications/repository/storage/PhabricatorRepositoryTransaction.php', 'PhabricatorRepositoryTransactionQuery' => 'applications/repository/query/PhabricatorRepositoryTransactionQuery.php', 'PhabricatorRepositoryType' => 'applications/repository/constants/PhabricatorRepositoryType.php', + 'PhabricatorRepositoryURINormalizer' => 'applications/repository/data/PhabricatorRepositoryURINormalizer.php', + 'PhabricatorRepositoryURINormalizerTestCase' => 'applications/repository/data/__tests__/PhabricatorRepositoryURINormalizerTestCase.php', 'PhabricatorRepositoryURITestCase' => 'applications/repository/storage/__tests__/PhabricatorRepositoryURITestCase.php', 'PhabricatorRepositoryVCSPassword' => 'applications/repository/storage/PhabricatorRepositoryVCSPassword.php', 'PhabricatorS3FileStorageEngine' => 'applications/files/engine/PhabricatorS3FileStorageEngine.php', @@ -4551,6 +4553,8 @@ 'PhabricatorRepositoryTestCase' => 'PhabricatorTestCase', 'PhabricatorRepositoryTransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorRepositoryTransactionQuery' => 'PhabricatorApplicationTransactionQuery', + 'PhabricatorRepositoryURINormalizer' => 'Phobject', + 'PhabricatorRepositoryURINormalizerTestCase' => 'PhabricatorTestCase', 'PhabricatorRepositoryURITestCase' => 'PhabricatorTestCase', 'PhabricatorRepositoryVCSPassword' => 'PhabricatorRepositoryDAO', 'PhabricatorS3FileStorageEngine' => 'PhabricatorFileStorageEngine', Index: src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php =================================================================== --- src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php +++ src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php @@ -719,7 +719,17 @@ $valid = false; $exists = false; } else { - $valid = self::isSameGitOrigin($remote_uri, $expect_remote); + $normal_type_git = PhabricatorRepositoryURINormalizer::TYPE_GIT; + + $remote_normal = id(new PhabricatorRepositoryURINormalizer( + $normal_type_git, + $remote_uri))->getNormalizedPath(); + + $expect_normal = id(new PhabricatorRepositoryURINormalizer( + $normal_type_git, + $expect_remote))->getNormalizedPath(); + + $valid = ($remote_normal == $expect_normal); $exists = true; } @@ -768,48 +778,6 @@ } } - - - /** - * @task git - */ - public static function isSameGitOrigin($remote, $expect) { - $remote_path = self::getPathFromGitURI($remote); - $expect_path = self::getPathFromGitURI($expect); - - $remote_match = self::executeGitNormalizePath($remote_path); - $expect_match = self::executeGitNormalizePath($expect_path); - - return ($remote_match == $expect_match); - } - - private static function getPathFromGitURI($raw_uri) { - $uri = new PhutilURI($raw_uri); - if ($uri->getProtocol()) { - return $uri->getPath(); - } - - $uri = new PhutilGitURI($raw_uri); - if ($uri->getDomain()) { - return $uri->getPath(); - } - - return $raw_uri; - } - - - /** - * @task git - */ - private static function executeGitNormalizePath($path) { - // Strip away "/" and ".git", so similar paths correctly match. - - $path = trim($path, '/'); - $path = preg_replace('/\.git$/', '', $path); - return $path; - } - - private function pushToMirrors(PhabricatorRepository $repository) { if (!$repository->canMirror()) { return; Index: src/applications/repository/data/PhabricatorRepositoryURINormalizer.php =================================================================== --- /dev/null +++ src/applications/repository/data/PhabricatorRepositoryURINormalizer.php @@ -0,0 +1,98 @@ +<?php + +/** + * Normalize repository URIs. For example, these URIs are generally equivalent + * and all point at the same repository: + * + * ssh://user@host/repo + * ssh://user@host/repo/ + * ssh://user@host:22/repo + * user@host:/repo + * ssh://user@host/repo.git + * + * This class can be used to normalize URIs like this, in order to detect + * alternate spellings of the same repository URI. In particular, the + * @{method:getNormalizedPath} method will return: + * + * repo + * + * ...for all of these URIs. Generally, usage looks like this: + * + * $norm_a = new PhabricatorRepositoryURINormalizer($type, $uri_a); + * $norm_b = new PhabricatorRepositoryURINormalizer($type, $uri_b); + * + * if ($norm_a->getNormalizedPath() == $norm_b->getNormalizedPath()) { + * // URIs appear to point at the same repository. + * } else { + * // URIs are very unlikely to be the same repository. + * } + * + * Because a repository can be hosted at arbitrarly many arbitrary URIs, there + * is no way to completely prevent false negatives by only examining URIs + * (that is, repositories with totally different URIs could really be the same). + * However, normalization is relatively agressive and false negatives should + * be rare: if normalization says two URIs are different repositories, they + * probably are. + * + * @task normal Normalizing URIs + */ +final class PhabricatorRepositoryURINormalizer extends Phobject { + + const TYPE_GIT = 'git'; + private $type; + private $uri; + + public function __construct($type, $uri) { + switch ($type) { + case self::TYPE_GIT: + break; + default: + throw new Exception(pht('Unknown URI type "%s"!')); + } + + $this->type = $type; + $this->uri = $uri; + } + + +/* -( Normalizing URIs )--------------------------------------------------- */ + + + /** + * @task normal + */ + public function getPath() { + switch ($this->type) { + case self::TYPE_GIT: + $uri = new PhutilURI($this->uri); + if ($uri->getProtocol()) { + return $uri->getPath(); + } + + $uri = new PhutilGitURI($this->uri); + if ($uri->getDomain()) { + return $uri->getPath(); + } + + return $this->uri; + } + } + + + /** + * @task normal + */ + public function getNormalizedPath() { + $path = $this->getPath(); + $path = trim($path, '/'); + + switch ($this->type) { + case self::TYPE_GIT: + $path = preg_replace('/\.git$/', '', $path); + break; + } + + return $path; + } + +} Index: src/applications/repository/data/__tests__/PhabricatorRepositoryURINormalizerTestCase.php =================================================================== --- /dev/null +++ src/applications/repository/data/__tests__/PhabricatorRepositoryURINormalizerTestCase.php @@ -0,0 +1,31 @@ +<?php + +final class PhabricatorRepositoryURINormalizerTestCase + extends PhabricatorTestCase { + + public function testGitURINormalizer() { + $cases = array( + 'ssh://user@domain.com/path.git' => 'path', + 'https://user@domain.com/path.git' => 'path', + 'git@domain.com:path.git' => 'path', + 'ssh://user@gitserv002.com/path.git' => 'path', + 'ssh://htaft@domain.com/path.git' => 'path', + 'ssh://user@domain.com/bananas.git' => 'bananas', + 'git@domain.com:bananas.git' => 'bananas', + 'user@domain.com:path/repo' => 'path/repo', + 'user@domain.com:path/repo/' => 'path/repo', + 'file:///path/to/local/repo.git' => 'path/to/local/repo', + '/path/to/local/repo.git' => 'path/to/local/repo', + ); + + $type_git = PhabricatorRepositoryURINormalizer::TYPE_GIT; + + foreach ($cases as $input => $expect) { + $normal = new PhabricatorRepositoryURINormalizer($type_git, $input); + $this->assertEqual( + $expect, + $normal->getNormalizedPath(), + pht('Normalized path for "%s".', $input)); + } + } +} Index: src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php =================================================================== --- src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php +++ src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php @@ -24,88 +24,4 @@ $this->assertEqual(array(), $refs); } - public function testExecuteGitVerifySameOrigin() { - $cases = array( - array( - 'ssh://user@domain.com/path.git', - 'ssh://user@domain.com/path.git', - true, - 'Identical paths should pass.', - ), - array( - 'ssh://user@domain.com/path.git', - 'https://user@domain.com/path.git', - true, - 'Protocol changes should pass.', - ), - array( - 'ssh://user@domain.com/path.git', - 'git@domain.com:path.git', - true, - 'Git implicit SSH should pass.', - ), - array( - 'ssh://user@gitserv001.com/path.git', - 'ssh://user@gitserv002.com/path.git', - true, - 'Domain changes should pass.', - ), - array( - 'ssh://alincoln@domain.com/path.git', - 'ssh://htaft@domain.com/path.git', - true, - 'User/auth changes should pass.', - ), - array( - 'ssh://user@domain.com/apples.git', - 'ssh://user@domain.com/bananas.git', - false, - 'Path changes should fail.', - ), - array( - 'ssh://user@domain.com/apples.git', - 'git@domain.com:bananas.git', - false, - 'Git implicit SSH path changes should fail.', - ), - array( - 'user@domain.com:path/repo.git', - 'user@domain.com:path/repo', - true, - 'Optional .git extension should not prevent matches.', - ), - array( - 'user@domain.com:path/repo/', - 'user@domain.com:path/repo', - true, - 'Optional trailing slash should not prevent matches.', - ), - array( - 'file:///path/to/local/repo.git', - 'file:///path/to/local/repo.git', - true, - 'file:// protocol should be supported.', - ), - array( - '/path/to/local/repo.git', - 'file:///path/to/local/repo.git', - true, - 'Implicit file:// protocol should be recognized.', - ), - ); - - foreach ($cases as $case) { - list($remote, $config, $expect, $message) = $case; - - $this->assertEqual( - $expect, - PhabricatorRepositoryPullLocalDaemon::isSameGitOrigin( - $remote, - $config, - '(a test case)'), - "Verification that '{$remote}' and '{$config}' are the same origin ". - "had a different outcome than expected: {$message}"); - } - } - }