From 839f3df9c262612cc07326fd6a7310175dd4e600 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 23 Jul 2012 12:05:03 -0700 Subject: [PATCH] Stop trasforming "scp-style" URIs into normal URIs in Git Summary: See D3033, T1529. We currently transform "scp-style" `user@host:path` URIs into normal `ssh://user@host/path` URIs. This is undesirable for two reasons: - The paths aren't always equivalent. They are for GitHub, which is why I missed this originally, but in the general case the ":path" is resolved relatively and the "/path" is resolved absolutely. So this transformation can break things. - It confuses users, who do not think of "git@host:path" URIs as SSH URIs even though the SSH protocol is implied. So stop using them, and just use the "git@host:path" URIs instead. This is a bit messy since we have some validation built up on top of URIs. Hopefully we can get rid of more of this in the future as we simplify repository management. Test Plan: Unit tests cover this stuff pretty well. Made a new git repository with a "git@host:path" style URI and did pull/discover on it, verified the right URI was used. Reviewers: btrahan, vrana Reviewed By: btrahan CC: aran Maniphest Tasks: T1529 Differential Revision: https://secure.phabricator.com/D3036 --- .../PhabricatorRepositoryPullLocalDaemon.php | 25 +++++-- .../storage/PhabricatorRepository.php | 67 ++++++++----------- .../PhabricatorRepositoryTestCase.php | 35 ---------- 3 files changed, 46 insertions(+), 81 deletions(-) diff --git a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php index a25372ce93..982df11317 100644 --- a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php +++ b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php @@ -724,11 +724,8 @@ final class PhabricatorRepositoryPullLocalDaemon * @task git */ public static function executeGitVerifySameOrigin($remote, $expect, $where) { - $remote_uri = PhabricatorRepository::newPhutilURIFromGitURI($remote); - $expect_uri = PhabricatorRepository::newPhutilURIFromGitURI($expect); - - $remote_path = $remote_uri->getPath(); - $expect_path = $expect_uri->getPath(); + $remote_path = self::getPathFromGitURI($remote); + $expect_path = self::getPathFromGitURI($expect); $remote_match = self::executeGitNormalizePath($remote_path); $expect_match = self::executeGitNormalizePath($expect_path); @@ -743,14 +740,28 @@ final class PhabricatorRepositoryPullLocalDaemon } } + 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 trailing "/" and ".git", so similar paths correctly match. + // Strip away "/" and ".git", so similar paths correctly match. - $path = rtrim($path, '/'); + $path = trim($path, '/'); $path = preg_replace('/\.git$/', '', $path); return $path; } diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index e5599f642f..2257777b61 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -70,99 +70,88 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO { )); } - public static function newPhutilURIFromGitURI($raw_uri) { - $uri = new PhutilURI($raw_uri); - if (!$uri->getProtocol()) { - if (strpos($raw_uri, '/') === 0) { - // If the URI starts with a '/', it's an implicit file:// URI on the - // local disk. - $uri = new PhutilURI('file://'.$raw_uri); - } else if (strpos($raw_uri, ':') !== false) { - // If there's no protocol (git implicit SSH) but the URI has a colon, - // it's a git implicit SSH URI. Reformat the URI to be a normal URI. - // These git URIs look like "user@domain.com:path" instead of - // "ssh://user@domain/path". - list($domain, $path) = explode(':', $raw_uri, 2); - $uri = new PhutilURI('ssh://'.$domain.'/'.$path); - } else { - throw new Exception("The Git URI '{$raw_uri}' could not be parsed."); - } - } - - return $uri; - } - public function getRemoteURI() { $raw_uri = $this->getDetail('remote-uri'); if (!$raw_uri) { return null; } - $vcs = $this->getVersionControlSystem(); - $is_git = ($vcs == PhabricatorRepositoryType::REPOSITORY_TYPE_GIT); - - if ($is_git) { - $uri = self::newPhutilURIFromGitURI($raw_uri); - } else { - $uri = new PhutilURI($raw_uri); + if (strpos($raw_uri, '/') === 0) { + // If the URI starts with a '/', it's an implicit file:// URI on the + // local disk. + $uri = new PhutilURI('file://'.$raw_uri); + return (string)$uri; } - if ($this->isSSHProtocol($uri->getProtocol())) { + $uri = new PhutilURI($raw_uri); + if ($uri->getProtocol()) { + if ($this->isSSHProtocol($uri->getProtocol())) { + if ($this->getSSHLogin()) { + $uri->setUser($this->getSSHLogin()); + } + } + return (string)$uri; + } + + $uri = new PhutilGitURI($raw_uri); + if ($uri->getDomain()) { if ($this->getSSHLogin()) { $uri->setUser($this->getSSHLogin()); } + return (string)$uri; } - return (string)$uri; + throw new Exception( + "Repository remote URI '{$raw_uri}' could not be parsed!"); } public function getLocalPath() { return $this->getDetail('local-path'); } - public function execRemoteCommand($pattern /*, $arg, ... */) { + public function execRemoteCommand($pattern /* , $arg, ... */) { $args = func_get_args(); $args = $this->formatRemoteCommand($args); return call_user_func_array('exec_manual', $args); } - public function execxRemoteCommand($pattern /*, $arg, ... */) { + public function execxRemoteCommand($pattern /* , $arg, ... */) { $args = func_get_args(); $args = $this->formatRemoteCommand($args); return call_user_func_array('execx', $args); } - public function getRemoteCommandFuture($pattern /*, $arg, ... */) { + public function getRemoteCommandFuture($pattern /* , $arg, ... */) { $args = func_get_args(); $args = $this->formatRemoteCommand($args); return newv('ExecFuture', $args); } - public function passthruRemoteCommand($pattern /*, $arg, ... */) { + public function passthruRemoteCommand($pattern /* , $arg, ... */) { $args = func_get_args(); $args = $this->formatRemoteCommand($args); return call_user_func_array('phutil_passthru', $args); } - public function execLocalCommand($pattern /*, $arg, ... */) { + public function execLocalCommand($pattern /* , $arg, ... */) { $args = func_get_args(); $args = $this->formatLocalCommand($args); return call_user_func_array('exec_manual', $args); } - public function execxLocalCommand($pattern /*, $arg, ... */) { + public function execxLocalCommand($pattern /* , $arg, ... */) { $args = func_get_args(); $args = $this->formatLocalCommand($args); return call_user_func_array('execx', $args); } - public function getLocalCommandFuture($pattern /*, $arg, ... */) { + public function getLocalCommandFuture($pattern /* , $arg, ... */) { $args = func_get_args(); $args = $this->formatLocalCommand($args); return newv('ExecFuture', $args); } - public function passthruLocalCommand($pattern /*, $arg, ... */) { + public function passthruLocalCommand($pattern /* , $arg, ... */) { $args = func_get_args(); $args = $this->formatLocalCommand($args); return call_user_func_array('phutil_passthru', $args); diff --git a/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php b/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php index eb5d178d26..6b8d3672b0 100644 --- a/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php +++ b/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php @@ -19,41 +19,6 @@ final class PhabricatorRepositoryTestCase extends PhabricatorTestCase { - public function testParseGitURI() { - static $map = array( - 'ssh://user@domain.com/path.git' => 'ssh://user@domain.com/path.git', - 'user@domain.com:path.git' => 'ssh://user@domain.com/path.git', - '/path/to/local/repo.git' => 'file:///path/to/local/repo.git', - ); - - foreach ($map as $raw => $expect) { - $uri = PhabricatorRepository::newPhutilURIFromGitURI($raw); - $this->assertEqual( - $expect, - (string)$uri, - "Normalized Git URI '{$raw}'"); - } - } - - public function testParseBadGitURI() { - $junk = array( - 'herp derp moon balloon', - ); - - foreach ($junk as $garbage) { - $ex = null; - try { - $uri = PhabricatorRepository::newPhutilURIFromGitURI($garbage); - } catch (Exception $caught) { - $ex = $caught; - } - $this->assertEqual( - true, - (bool)$ex, - 'Expect exception when parsing garbage.'); - } - } - public function testBranchFilter() { $git = PhabricatorRepositoryType::REPOSITORY_TYPE_GIT;