From 07f4772d0be42bccb8438bc8f4174c0645864eb6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 28 Sep 2011 08:15:35 -0700 Subject: [PATCH] Make all parsers use credentials Summary: We need to issue all commands as $repository->junk() so we can pick up credentials. Some of this stuff predates that change landing. (I removed the "https" vs "svn+ssh" fallback code since it's specific to Facebook, affected a tiny number of commits, is basically an SVN bug with UTF-8 handling and HTTP support, and doesn't make sense in the general case. The user has the tools they need to force it via "reparse.php" if it's really an issue.) Test Plan: Created new authenticated-remote mercurial and git repositories and pulled/discovered them with credentials. Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran Reviewed By: Makinde CC: aran, Makinde Differential Revision: 970 --- ...atorRepositoryGitCommitDiscoveryDaemon.php | 17 +++++-------- .../daemon/commitdiscovery/git/__init__.php | 2 -- ...positoryMercurialCommitDiscoveryDaemon.php | 1 - ...atorRepositorySvnCommitDiscoveryDaemon.php | 4 ++-- .../PhabricatorRepositoryGitFetchDaemon.php | 18 ++++++++++---- .../repository/daemon/gitfetch/__init__.php | 2 -- ...abricatorRepositoryMercurialPullDaemon.php | 16 +++++++++---- .../daemon/mercurialpull/__init__.php | 2 -- .../PhabricatorRepositoryPullLocalDaemon.php | 16 ++++++------- ...habricatorRepositoryCommitParserWorker.php | 24 ++++--------------- .../repository/worker/base/__init__.php | 2 -- ...rRepositoryGitCommitChangeParserWorker.php | 11 ++++----- .../commitchangeparser/git/__init__.php | 2 -- 13 files changed, 51 insertions(+), 66 deletions(-) diff --git a/src/applications/repository/daemon/commitdiscovery/git/PhabricatorRepositoryGitCommitDiscoveryDaemon.php b/src/applications/repository/daemon/commitdiscovery/git/PhabricatorRepositoryGitCommitDiscoveryDaemon.php index 9ad82fdb28..cfb9685dd8 100644 --- a/src/applications/repository/daemon/commitdiscovery/git/PhabricatorRepositoryGitCommitDiscoveryDaemon.php +++ b/src/applications/repository/daemon/commitdiscovery/git/PhabricatorRepositoryGitCommitDiscoveryDaemon.php @@ -32,10 +32,8 @@ class PhabricatorRepositoryGitCommitDiscoveryDaemon $repository_phid = $repository->getPHID(); - $repo_base = $repository->getDetail('local-path'); - list($stdout) = execx( - '(cd %s && git branch -r --verbose --no-abbrev)', - $repo_base); + list($stdout) = $repository->execxLocalCommand( + 'branch -r --verbose --no-abbrev'); $branches = DiffusionGitBranchQuery::parseGitRemoteBranchOutput($stdout); @@ -57,7 +55,6 @@ class PhabricatorRepositoryGitCommitDiscoveryDaemon $insert = array(); $repository = $this->getRepository(); - $repo_base = $repository->getDetail('local-path'); $discover[] = $commit; $insert[] = $commit; @@ -66,9 +63,8 @@ class PhabricatorRepositoryGitCommitDiscoveryDaemon while (true) { $target = array_pop($discover); - list($parents) = execx( - '(cd %s && git log -n1 --pretty="%%P" %s)', - $repo_base, + list($parents) = $repository->execxLocalCommand( + 'log -n1 --pretty="%%P" %s', $target); $parents = array_filter(explode(' ', trim($parents))); foreach ($parents as $parent) { @@ -93,9 +89,8 @@ class PhabricatorRepositoryGitCommitDiscoveryDaemon while (true) { $target = array_pop($insert); - list($epoch) = execx( - '(cd %s && git log -n1 --pretty="%%at" %s)', - $repo_base, + list($epoch) = $repository->execxLocalCommand( + 'log -n1 --pretty="%%at" %s', $target); $epoch = trim($epoch); diff --git a/src/applications/repository/daemon/commitdiscovery/git/__init__.php b/src/applications/repository/daemon/commitdiscovery/git/__init__.php index 8fc9566ada..685253a440 100644 --- a/src/applications/repository/daemon/commitdiscovery/git/__init__.php +++ b/src/applications/repository/daemon/commitdiscovery/git/__init__.php @@ -10,7 +10,5 @@ phutil_require_module('phabricator', 'applications/diffusion/query/branch/git'); phutil_require_module('phabricator', 'applications/repository/constants/repositorytype'); phutil_require_module('phabricator', 'applications/repository/daemon/commitdiscovery/base'); -phutil_require_module('phutil', 'future/exec'); - phutil_require_source('PhabricatorRepositoryGitCommitDiscoveryDaemon.php'); diff --git a/src/applications/repository/daemon/commitdiscovery/mercurial/PhabricatorRepositoryMercurialCommitDiscoveryDaemon.php b/src/applications/repository/daemon/commitdiscovery/mercurial/PhabricatorRepositoryMercurialCommitDiscoveryDaemon.php index 01de317162..66dd5c2e2d 100644 --- a/src/applications/repository/daemon/commitdiscovery/mercurial/PhabricatorRepositoryMercurialCommitDiscoveryDaemon.php +++ b/src/applications/repository/daemon/commitdiscovery/mercurial/PhabricatorRepositoryMercurialCommitDiscoveryDaemon.php @@ -29,7 +29,6 @@ class PhabricatorRepositoryMercurialCommitDiscoveryDaemon $repository_phid = $repository->getPHID(); - $repo_base = $repository->getDetail('local-path'); list($stdout) = $repository->execxLocalCommand('branches'); $branches = ArcanistMercurialParser::parseMercurialBranches($stdout); diff --git a/src/applications/repository/daemon/commitdiscovery/svn/PhabricatorRepositorySvnCommitDiscoveryDaemon.php b/src/applications/repository/daemon/commitdiscovery/svn/PhabricatorRepositorySvnCommitDiscoveryDaemon.php index fc68dad836..4020c887b5 100644 --- a/src/applications/repository/daemon/commitdiscovery/svn/PhabricatorRepositorySvnCommitDiscoveryDaemon.php +++ b/src/applications/repository/daemon/commitdiscovery/svn/PhabricatorRepositorySvnCommitDiscoveryDaemon.php @@ -29,8 +29,8 @@ class PhabricatorRepositorySvnCommitDiscoveryDaemon $uri = $this->getBaseSVNLogURI(); list($xml) = $repository->execxRemoteCommand( - ' log --xml --quiet --limit 1 %s@HEAD', - $uri); + 'log --xml --quiet --limit 1 %s@HEAD', + $uri); $results = $this->parseSVNLogXML($xml); $commit = key($results); diff --git a/src/applications/repository/daemon/gitfetch/PhabricatorRepositoryGitFetchDaemon.php b/src/applications/repository/daemon/gitfetch/PhabricatorRepositoryGitFetchDaemon.php index aab67da028..7b67006c4e 100644 --- a/src/applications/repository/daemon/gitfetch/PhabricatorRepositoryGitFetchDaemon.php +++ b/src/applications/repository/daemon/gitfetch/PhabricatorRepositoryGitFetchDaemon.php @@ -23,12 +23,22 @@ final class PhabricatorRepositoryGitFetchDaemon return PhabricatorRepositoryType::REPOSITORY_TYPE_GIT; } - protected function executeCreate($remote_uri, $local_path) { - execx('git clone %s %s', $remote_uri, rtrim($local_path, '/')); + protected function executeCreate( + PhabricatorRepository $repository, + $local_path) { + + $repository->execxRemoteCommand( + 'clone %s %s', + $repository->getRemoteURI(), + rtrim($local_path, '/')); } - protected function executeUpdate($remote_uri, $local_path) { - execx('(cd %s && git fetch --all)', $local_path); + protected function executeUpdate( + PhabricatorRepository $repository, + $local_path) { + + $repository->execxLocalCommand( + 'fetch --all'); } } diff --git a/src/applications/repository/daemon/gitfetch/__init__.php b/src/applications/repository/daemon/gitfetch/__init__.php index 61b3f4bad3..d59679cb9a 100644 --- a/src/applications/repository/daemon/gitfetch/__init__.php +++ b/src/applications/repository/daemon/gitfetch/__init__.php @@ -9,7 +9,5 @@ phutil_require_module('phabricator', 'applications/repository/constants/repositorytype'); phutil_require_module('phabricator', 'applications/repository/daemon/pulllocal'); -phutil_require_module('phutil', 'future/exec'); - phutil_require_source('PhabricatorRepositoryGitFetchDaemon.php'); diff --git a/src/applications/repository/daemon/mercurialpull/PhabricatorRepositoryMercurialPullDaemon.php b/src/applications/repository/daemon/mercurialpull/PhabricatorRepositoryMercurialPullDaemon.php index 079e932116..9bee564e41 100644 --- a/src/applications/repository/daemon/mercurialpull/PhabricatorRepositoryMercurialPullDaemon.php +++ b/src/applications/repository/daemon/mercurialpull/PhabricatorRepositoryMercurialPullDaemon.php @@ -23,12 +23,20 @@ final class PhabricatorRepositoryMercurialPullDaemon return PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL; } - protected function executeCreate($remote_uri, $local_path) { - execx('hg clone %s %s', $remote_uri, rtrim($local_path, '/')); + protected function executeCreate( + PhabricatorRepository $repository, + $local_path) { + $repository->execxRemoteCommand( + 'clone %s %s', + $repository->getRemoteURI(), + rtrim($local_path, '/')); } - protected function executeUpdate($remote_uri, $local_path) { - execx('(cd %s && hg pull -u)', $local_path); + protected function executeUpdate( + PhabricatorRepository $repository, + $local_path) { + $repository->execxLocalCommand( + 'pull -u'); } } diff --git a/src/applications/repository/daemon/mercurialpull/__init__.php b/src/applications/repository/daemon/mercurialpull/__init__.php index c2ca3a6c33..e2b6f2382d 100644 --- a/src/applications/repository/daemon/mercurialpull/__init__.php +++ b/src/applications/repository/daemon/mercurialpull/__init__.php @@ -9,7 +9,5 @@ phutil_require_module('phabricator', 'applications/repository/constants/repositorytype'); phutil_require_module('phabricator', 'applications/repository/daemon/pulllocal'); -phutil_require_module('phutil', 'future/exec'); - phutil_require_source('PhabricatorRepositoryMercurialPullDaemon.php'); diff --git a/src/applications/repository/daemon/pulllocal/PhabricatorRepositoryPullLocalDaemon.php b/src/applications/repository/daemon/pulllocal/PhabricatorRepositoryPullLocalDaemon.php index dd02d3ee8c..f7f00ed4a3 100644 --- a/src/applications/repository/daemon/pulllocal/PhabricatorRepositoryPullLocalDaemon.php +++ b/src/applications/repository/daemon/pulllocal/PhabricatorRepositoryPullLocalDaemon.php @@ -20,8 +20,12 @@ abstract class PhabricatorRepositoryPullLocalDaemon extends PhabricatorRepositoryDaemon { abstract protected function getSupportedRepositoryType(); - abstract protected function executeCreate($remote_uri, $local_path); - abstract protected function executeUpdate($remote_uri, $local_path); + abstract protected function executeCreate( + PhabricatorRepository $repository, + $local_path); + abstract protected function executeUpdate( + PhabricatorRepository $repository, + $local_path); final public function run() { $repository = $this->loadRepository(); @@ -45,7 +49,6 @@ abstract class PhabricatorRepositoryPullLocalDaemon } $local_path = $repository->getDetail('local-path'); - $remote_uri = $repository->getDetail('remote-uri'); if (!$local_path) { throw new Exception("No local path is available for this repository."); @@ -53,13 +56,10 @@ abstract class PhabricatorRepositoryPullLocalDaemon while (true) { if (!Filesystem::pathExists($local_path)) { - if (!$remote_uri) { - throw new Exception("No remote URI is available."); - } execx('mkdir -p %s', dirname($local_path)); - $this->executeCreate($remote_uri, $local_path); + $this->executeCreate($repository, $local_path); } else { - $this->executeUpdate($remote_uri, $local_path); + $this->executeUpdate($repository, $local_path); } $this->sleep($repository->getDetail('pull-frequency', 15)); } diff --git a/src/applications/repository/worker/base/PhabricatorRepositoryCommitParserWorker.php b/src/applications/repository/worker/base/PhabricatorRepositoryCommitParserWorker.php index 938c9eb199..8641cd8c7d 100644 --- a/src/applications/repository/worker/base/PhabricatorRepositoryCommitParserWorker.php +++ b/src/applications/repository/worker/base/PhabricatorRepositoryCommitParserWorker.php @@ -67,26 +67,10 @@ abstract class PhabricatorRepositoryCommitParserWorker $verbose = '--verbose'; } - try { - list($xml) = $this->repository->execxRemoteCommand( - "log --xml {$verbose} --limit 1 %s@%d", - $uri, - $revision); - } catch (CommandException $ex) { - // HTTPS is generally faster and more reliable than svn+ssh, but some - // commit messages with non-UTF8 text can't be retrieved over HTTPS, see - // Facebook rE197184 for one example. Make an attempt to fall back to - // svn+ssh if we've failed outright to retrieve the message. - $fallback_uri = new PhutilURI($uri); - if ($fallback_uri->getProtocol() != 'https') { - throw $ex; - } - $fallback_uri->setProtocol('svn+ssh'); - list($xml) = execx( - "svn log --xml {$verbose} --limit 1 --non-interactive %s@%d", - $fallback_uri, - $revision); - } + list($xml) = $this->repository->execxRemoteCommand( + "log --xml {$verbose} --limit 1 %s@%d", + $uri, + $revision); // Subversion may send us back commit messages which won't parse because // they have non UTF-8 garbage in them. Slam them into valid UTF-8. diff --git a/src/applications/repository/worker/base/__init__.php b/src/applications/repository/worker/base/__init__.php index d78aaa2839..564fece8ea 100644 --- a/src/applications/repository/worker/base/__init__.php +++ b/src/applications/repository/worker/base/__init__.php @@ -11,8 +11,6 @@ phutil_require_module('phabricator', 'applications/repository/storage/repository phutil_require_module('phabricator', 'infrastructure/daemon/workers/worker'); phutil_require_module('phabricator', 'storage/queryfx'); -phutil_require_module('phutil', 'future/exec'); -phutil_require_module('phutil', 'parser/uri'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/repository/worker/commitchangeparser/git/PhabricatorRepositoryGitCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/git/PhabricatorRepositoryGitCommitChangeParserWorker.php index 27ca9c6bbc..19c96506ce 100644 --- a/src/applications/repository/worker/commitchangeparser/git/PhabricatorRepositoryGitCommitChangeParserWorker.php +++ b/src/applications/repository/worker/commitchangeparser/git/PhabricatorRepositoryGitCommitChangeParserWorker.php @@ -30,12 +30,11 @@ class PhabricatorRepositoryGitCommitChangeParserWorker return; } - $local_path = $repository->getDetail('local-path'); - - list($raw) = execx( - '(cd %s && git log -n1 -M -C -B --find-copies-harder --raw -t '. - '--abbrev=40 --pretty=format: %s)', - $local_path, + // NOTE: "--pretty=format: " is to disable log output, we only want the + // part we get from "--raw". + list($raw) = $repository->execLocalCommand( + 'log -n1 -M -C -B --find-copies-harder --raw -t '. + '--abbrev=40 --pretty=format: %s', $commit->getCommitIdentifier()); $changes = array(); diff --git a/src/applications/repository/worker/commitchangeparser/git/__init__.php b/src/applications/repository/worker/commitchangeparser/git/__init__.php index 6bc6d8d55e..012d74a45c 100644 --- a/src/applications/repository/worker/commitchangeparser/git/__init__.php +++ b/src/applications/repository/worker/commitchangeparser/git/__init__.php @@ -11,7 +11,5 @@ phutil_require_module('phabricator', 'applications/repository/storage/repository phutil_require_module('phabricator', 'applications/repository/worker/commitchangeparser/base'); phutil_require_module('phabricator', 'storage/queryfx'); -phutil_require_module('phutil', 'future/exec'); - phutil_require_source('PhabricatorRepositoryGitCommitChangeParserWorker.php');