diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a6515e9733..1f8cf4585b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1881,6 +1881,8 @@ phutil_register_library_map(array( '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 @@ phutil_register_library_map(array( 'PhabricatorRepositoryTestCase' => 'PhabricatorTestCase', 'PhabricatorRepositoryTransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorRepositoryTransactionQuery' => 'PhabricatorApplicationTransactionQuery', + 'PhabricatorRepositoryURINormalizer' => 'Phobject', + 'PhabricatorRepositoryURINormalizerTestCase' => 'PhabricatorTestCase', 'PhabricatorRepositoryURITestCase' => 'PhabricatorTestCase', 'PhabricatorRepositoryVCSPassword' => 'PhabricatorRepositoryDAO', 'PhabricatorS3FileStorageEngine' => 'PhabricatorFileStorageEngine', diff --git a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php index 9dc67a0bd0..70bd4f10e3 100644 --- a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php +++ b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php @@ -719,7 +719,17 @@ final class PhabricatorRepositoryPullLocalDaemon $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 @@ final class PhabricatorRepositoryPullLocalDaemon } } - - - /** - * @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; diff --git a/src/applications/repository/data/PhabricatorRepositoryURINormalizer.php b/src/applications/repository/data/PhabricatorRepositoryURINormalizer.php new file mode 100644 index 0000000000..aa7c8d6520 --- /dev/null +++ b/src/applications/repository/data/PhabricatorRepositoryURINormalizer.php @@ -0,0 +1,98 @@ +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; + } + +} diff --git a/src/applications/repository/data/__tests__/PhabricatorRepositoryURINormalizerTestCase.php b/src/applications/repository/data/__tests__/PhabricatorRepositoryURINormalizerTestCase.php new file mode 100644 index 0000000000..32429ce244 --- /dev/null +++ b/src/applications/repository/data/__tests__/PhabricatorRepositoryURINormalizerTestCase.php @@ -0,0 +1,31 @@ + '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)); + } + } +} diff --git a/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php b/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php index b8274323f1..9f4f48d156 100644 --- a/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php +++ b/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php @@ -24,88 +24,4 @@ final class PhabricatorWorkingCopyDiscoveryTestCase $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}"); - } - } - }