Index: src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php =================================================================== --- src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php +++ src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php @@ -535,19 +535,7 @@ 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()) @@ -691,23 +679,100 @@ /** + * 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) { Index: src/applications/repository/engine/PhabricatorRepositoryPullEngine.php =================================================================== --- src/applications/repository/engine/PhabricatorRepositoryPullEngine.php +++ src/applications/repository/engine/PhabricatorRepositoryPullEngine.php @@ -231,7 +231,7 @@ } } - 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 @@ $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 @@ execx('svnadmin create -- %s', $path); } - -/* -( Internals )---------------------------------------------------------- */ - - - private function canDestroyWorkingCopy($path) { - $default_path = PhabricatorEnv::getEnvConfig( - 'repository.default-local-path'); - return Filesystem::isDescendant($path, $default_path); - } - } Index: src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php =================================================================== --- src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php +++ src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php @@ -97,19 +97,12 @@ 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}"); } Index: src/applications/repository/storage/PhabricatorRepository.php =================================================================== --- src/applications/repository/storage/PhabricatorRepository.php +++ src/applications/repository/storage/PhabricatorRepository.php @@ -898,6 +898,18 @@ } } + 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,