From a07f444f2a5ecee9569c565c0fbdc66370d4a3d8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 20 Nov 2013 10:41:56 -0800 Subject: [PATCH] Fix git "origin" remote in more circumstances Summary: Fixes T4041. We currently detect when "origin" is incorrect, but can do better: - When "origin" is missing, we can add it. This happens for Git 1.7.1 -- see T4041. - When "origin" is wrong, we can fix it automatically if we control the repository. We only need to fail when origin exists, is wrong, and we aren't in charge of the repository. Test Plan: Ran `bin/repository discover X` on a repository with a good origin, no origin, a bad-but-under-control origin, and a bad-out-of-control origin. Got the right behavior in all cases. Reviewers: btrahan Reviewed By: btrahan CC: aran, champo Maniphest Tasks: T4041 Differential Revision: https://secure.phabricator.com/D7614 --- .../PhabricatorRepositoryPullLocalDaemon.php | 109 ++++++++++++++---- .../PhabricatorRepositoryPullEngine.php | 14 +-- ...habricatorWorkingCopyDiscoveryTestCase.php | 15 +-- .../storage/PhabricatorRepository.php | 12 ++ 4 files changed, 105 insertions(+), 45 deletions(-) diff --git a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php index 52b805363b..88c708ac20 100644 --- a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php +++ b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php @@ -535,19 +535,7 @@ final class PhabricatorRepositoryPullLocalDaemon PhabricatorRepository $repository) { if (!$repository->isHosted()) { - list($remotes) = $repository->execxLocalCommand( - 'remote show -n origin'); - - $matches = null; - if (!preg_match('/^\s*Fetch URL:\s*(.*?)\s*$/m', $remotes, $matches)) { - throw new Exception( - "Expected 'Fetch URL' in 'git remote show -n origin'."); - } - - self::executeGitVerifySameOrigin( - $matches[1], - $repository->getRemoteURI(), - $repository->getLocalPath()); + $this->verifyOrigin($repository); } $refs = id(new DiffusionLowLevelGitRefQuery()) @@ -690,24 +678,101 @@ final class PhabricatorRepositoryPullLocalDaemon } + /** + * Verify that the "origin" remote exists, and points at the correct URI. + * + * This catches or corrects some types of misconfiguration, and also repairs + * an issue where Git 1.7.1 does not create an "origin" for `--bare` clones. + * See T4041. + * + * @param PhabricatorRepository Repository to verify. + * @return void + */ + private function verifyOrigin(PhabricatorRepository $repository) { + list($remotes) = $repository->execxLocalCommand( + 'remote show -n origin'); + + $matches = null; + if (!preg_match('/^\s*Fetch URL:\s*(.*?)\s*$/m', $remotes, $matches)) { + throw new Exception( + "Expected 'Fetch URL' in 'git remote show -n origin'."); + } + + $remote_uri = $matches[1]; + $expect_remote = $repository->getRemoteURI(); + + if ($remote_uri == "origin") { + // If a remote does not exist, git pretends it does and prints out a + // made up remote where the URI is the same as the remote name. This is + // definitely not correct. + + // Possibly, we should use `git remote --verbose` instead, which does not + // suffer from this problem (but is a little more complicated to parse). + $valid = false; + $exists = false; + } else { + $valid = self::isSameGitOrigin($remote_uri, $expect_remote); + $exists = true; + } + + if (!$valid) { + if (!$exists) { + // If there's no "origin" remote, just create it regardless of how + // strongly we own the working copy. There is almost no conceivable + // scenario in which this could do damage. + $this->log( + pht( + 'Remote "origin" does not exist. Creating "origin", with '. + 'URI "%s".', + $expect_remote)); + $repository->execxLocalCommand( + 'remote add origin %s', + $expect_remote); + + // NOTE: This doesn't fetch the origin (it just creates it), so we won't + // know about origin branches until the next "pull" happens. That's fine + // for our purposes, but might impact things in the future. + } else { + if ($repository->canDestroyWorkingCopy()) { + // Bad remote, but we can try to repair it. + $this->log( + pht( + 'Remote "origin" exists, but is pointed at the wrong URI, "%s". '. + 'Resetting origin URI to "%s.', + $remote_uri, + $expect_remote)); + $repository->execxLocalCommand( + 'remote set-url origin %s', + $expect_remote); + } else { + // Bad remote and we aren't comfortable repairing it. + $message = pht( + 'Working copy at "%s" has a mismatched origin URI, "%s". '. + 'The expected origin URI is "%s". Fix your configuration, or '. + 'set the remote URI correctly. To avoid breaking anything, '. + 'Phabricator will not automatically fix this.', + $repository->getLocalPath(), + $remote_uri, + $expect_remote); + throw new Exception($message); + } + } + } + } + + + /** * @task git */ - public static function executeGitVerifySameOrigin($remote, $expect, $where) { + 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); - if ($remote_match != $expect_match) { - throw new Exception( - "Working copy at '{$where}' has a mismatched origin URL. It has ". - "origin URL '{$remote}' (with remote path '{$remote_path}'), but the ". - "configured URL '{$expect}' (with remote path '{$expect_path}') is ". - "expected. Refusing to proceed because this may indicate that the ". - "working copy is actually some other repository."); - } + return ($remote_match == $expect_match); } private static function getPathFromGitURI($raw_uri) { diff --git a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php index 2d2fcb3e7e..a7ef8b7cd1 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php @@ -231,7 +231,7 @@ final class PhabricatorRepositoryPullEngine } } - if ($err && $this->canDestroyWorkingCopy($path)) { + if ($err && $repository->canDestroyWorkingCopy()) { phlog("Repository working copy at '{$path}' failed sanity check; ". "destroying and re-cloning. {$message}"); Filesystem::remove($path); @@ -256,7 +256,7 @@ final class PhabricatorRepositoryPullEngine $future->setCWD($path); list($err, $stdout, $stderr) = $future->resolve(); - if ($err && !$retry && $this->canDestroyWorkingCopy($path)) { + if ($err && !$retry && $repository->canDestroyWorkingCopy()) { $retry = true; // Fix remote origin url if it doesn't match our configuration $origin_url = $repository->execLocalCommand( @@ -357,14 +357,4 @@ final class PhabricatorRepositoryPullEngine execx('svnadmin create -- %s', $path); } - -/* -( Internals )---------------------------------------------------------- */ - - - private function canDestroyWorkingCopy($path) { - $default_path = PhabricatorEnv::getEnvConfig( - 'repository.default-local-path'); - return Filesystem::isDescendant($path, $default_path); - } - } diff --git a/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php b/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php index 03a4708699..b8274323f1 100644 --- a/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php +++ b/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php @@ -97,19 +97,12 @@ final class PhabricatorWorkingCopyDiscoveryTestCase foreach ($cases as $case) { list($remote, $config, $expect, $message) = $case; - $ex = null; - try { - PhabricatorRepositoryPullLocalDaemon::executeGitverifySameOrigin( - $remote, - $config, - '(a test case)'); - } catch (Exception $exception) { - $ex = $exception; - } - $this->assertEqual( $expect, - !$ex, + PhabricatorRepositoryPullLocalDaemon::isSameGitOrigin( + $remote, + $config, + '(a test case)'), "Verification that '{$remote}' and '{$config}' are the same origin ". "had a different outcome than expected: {$message}"); } diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 859c9eff7d..4597a36fe3 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -898,6 +898,18 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO } } + public function canDestroyWorkingCopy() { + if ($this->isHosted()) { + // Never destroy hosted working copies. + return false; + } + + $default_path = PhabricatorEnv::getEnvConfig( + 'repository.default-local-path'); + return Filesystem::isDescendant($this->getLocalPath(), $default_path); + } + + public function writeStatusMessage( $status_type, $status_code,