Page MenuHomePhabricator
Files F84161

D7614.diff
All Users
Actions

D7614.diff
View Options

diff --git a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php b/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php
--- a/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php
+++ b/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) {
diff --git a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php
--- a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php
+++ b/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);
- }
-
}
diff --git a/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php b/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php
--- a/src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php
+++ b/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}");
}
diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php
--- a/src/applications/repository/storage/PhabricatorRepository.php
+++ b/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,

File Metadata

Mime Type
text/x-diff
Storage Engine
amazon-s3
Storage Format
Raw Data
Storage Handle
phabricator/bn/ex/xj7xgylblomwwwlu
Default Alt Text
D7614.diff (8 KB)

Event Timeline

Phabricator · Built by Phacility