From 7150aa8e192ea2ad4ca7536f00722b1671e27303 Mon Sep 17 00:00:00 2001 From: June Rhodes Date: Fri, 8 Apr 2016 13:08:56 -0700 Subject: [PATCH] Use Conduit in PhabricatorRepositoryGitCommitChangeParserWorker Summary: Ref T2783. This allows this worker to run on a machine different to the one that stores the repository, by routing the execution of Git over Conduit calls. This API method is super gross, but fixing it isn't straightforward and it runs into other complicated considerations. We can fix it later; for now, just define it as "internal" to limit how much mess this creates. "Internal" methods do not appear on the console. Test Plan: Ran `bin/repository reparse --change --trace` on several commits, saw daemons make a Conduit call instead of running a `git` command. Reviewers: hach-que, chad Reviewed By: chad Subscribers: joshuaspence, Korvin, epriestley Maniphest Tasks: T2783 Differential Revision: https://secure.phabricator.com/D11874 --- src/__phutil_library_map__.php | 2 + .../conduit/method/ConduitAPIMethod.php | 3 + .../query/PhabricatorConduitMethodQuery.php | 14 ++++ .../query/PhabricatorConduitSearchEngine.php | 1 + ...nternalGitRawDiffQueryConduitAPIMethod.php | 74 +++++++++++++++++++ ...rRepositoryGitCommitChangeParserWorker.php | 44 +++-------- 6 files changed, 106 insertions(+), 32 deletions(-) create mode 100644 src/applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a1588abcae..1b86548f5c 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -650,6 +650,7 @@ phutil_register_library_map(array( 'DiffusionHovercardEngineExtension' => 'applications/diffusion/engineextension/DiffusionHovercardEngineExtension.php', 'DiffusionInlineCommentController' => 'applications/diffusion/controller/DiffusionInlineCommentController.php', 'DiffusionInlineCommentPreviewController' => 'applications/diffusion/controller/DiffusionInlineCommentPreviewController.php', + 'DiffusionInternalGitRawDiffQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php', 'DiffusionLastModifiedController' => 'applications/diffusion/controller/DiffusionLastModifiedController.php', 'DiffusionLastModifiedQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionLastModifiedQueryConduitAPIMethod.php', 'DiffusionLintController' => 'applications/diffusion/controller/DiffusionLintController.php', @@ -4836,6 +4837,7 @@ phutil_register_library_map(array( 'DiffusionHovercardEngineExtension' => 'PhabricatorHovercardEngineExtension', 'DiffusionInlineCommentController' => 'PhabricatorInlineCommentController', 'DiffusionInlineCommentPreviewController' => 'PhabricatorInlineCommentPreviewController', + 'DiffusionInternalGitRawDiffQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionLastModifiedController' => 'DiffusionController', 'DiffusionLastModifiedQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionLintController' => 'DiffusionController', diff --git a/src/applications/conduit/method/ConduitAPIMethod.php b/src/applications/conduit/method/ConduitAPIMethod.php index 5b6c16bb93..7992518919 100644 --- a/src/applications/conduit/method/ConduitAPIMethod.php +++ b/src/applications/conduit/method/ConduitAPIMethod.php @@ -52,6 +52,9 @@ abstract class ConduitAPIMethod abstract protected function execute(ConduitAPIRequest $request); + public function isInternalAPI() { + return false; + } public function getParamTypes() { $types = $this->defineParamTypes(); diff --git a/src/applications/conduit/query/PhabricatorConduitMethodQuery.php b/src/applications/conduit/query/PhabricatorConduitMethodQuery.php index cb30bf5e55..512dc70e6f 100644 --- a/src/applications/conduit/query/PhabricatorConduitMethodQuery.php +++ b/src/applications/conduit/query/PhabricatorConduitMethodQuery.php @@ -9,6 +9,7 @@ final class PhabricatorConduitMethodQuery private $applicationNames; private $nameContains; private $methods; + private $isInternal; public function withMethods(array $methods) { $this->methods = $methods; @@ -40,6 +41,11 @@ final class PhabricatorConduitMethodQuery return $this; } + public function withIsInternal($is_internal) { + $this->isInternal = $is_internal; + return $this; + } + protected function loadPage() { $methods = $this->getAllMethods(); $methods = $this->filterMethods($methods); @@ -112,6 +118,14 @@ final class PhabricatorConduitMethodQuery } } + if ($this->isInternal !== null) { + foreach ($methods as $key => $method) { + if ($method->isInternalAPI() !== $this->isInternal) { + unset($methods[$key]); + } + } + } + return $methods; } diff --git a/src/applications/conduit/query/PhabricatorConduitSearchEngine.php b/src/applications/conduit/query/PhabricatorConduitSearchEngine.php index 6c067a8ff4..22e4e19e53 100644 --- a/src/applications/conduit/query/PhabricatorConduitSearchEngine.php +++ b/src/applications/conduit/query/PhabricatorConduitSearchEngine.php @@ -37,6 +37,7 @@ final class PhabricatorConduitSearchEngine $query->withIsStable($saved->getParameter('isStable')); $query->withIsUnstable($saved->getParameter('isUnstable')); $query->withIsDeprecated($saved->getParameter('isDeprecated')); + $query->withIsInternal(false); $names = $saved->getParameter('applicationNames', array()); if ($names) { diff --git a/src/applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php new file mode 100644 index 0000000000..1e4c906322 --- /dev/null +++ b/src/applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php @@ -0,0 +1,74 @@ + 'required string', + ); + } + + protected function getResult(ConduitAPIRequest $request) { + $drequest = $this->getDiffusionRequest(); + $repository = $drequest->getRepository(); + + if (!$repository->isGit()) { + throw new Exception( + pht( + 'This API method can only be called on Git repositories.')); + } + + // Check if the commit has parents. We're testing to see whether it is the + // first commit in history (in which case we must use "git log") or some + // other commit (in which case we can use "git diff"). We'd rather use + // "git diff" because it has the right behavior for merge commits, but + // it requires the commit to have a parent that we can diff against. The + // first commit doesn't, so "commit^" is not a valid ref. + list($parents) = $repository->execxLocalCommand( + 'log -n1 --format=%s %s', + '%P', + $request->getValue('commit')); + + $use_log = !strlen(trim($parents)); + if ($use_log) { + // This is the first commit so we need to use "log". We know it's not a + // merge commit because it couldn't be merging anything, so this is safe. + + // NOTE: "--pretty=format: " is to disable diff output, we only want the + // part we get from "--raw". + list($raw) = $repository->execxLocalCommand( + 'log -n1 -M -C -B --find-copies-harder --raw -t '. + '--pretty=format: --abbrev=40 %s', + $request->getValue('commit')); + } else { + // Otherwise, we can use "diff", which will give us output for merges. + // We diff against the first parent, as this is generally the expectation + // and results in sensible behavior. + list($raw) = $repository->execxLocalCommand( + 'diff -n1 -M -C -B --find-copies-harder --raw -t '. + '--abbrev=40 %s^1 %s', + $request->getValue('commit'), + $request->getValue('commit')); + } + + return $raw; + } + +} diff --git a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryGitCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryGitCommitChangeParserWorker.php index 4ab6f03f32..e421da228a 100644 --- a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryGitCommitChangeParserWorker.php +++ b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryGitCommitChangeParserWorker.php @@ -7,38 +7,18 @@ final class PhabricatorRepositoryGitCommitChangeParserWorker PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit) { - // Check if the commit has parents. We're testing to see whether it is the - // first commit in history (in which case we must use "git log") or some - // other commit (in which case we can use "git diff"). We'd rather use - // "git diff" because it has the right behavior for merge commits, but - // it requires the commit to have a parent that we can diff against. The - // first commit doesn't, so "commit^" is not a valid ref. - list($parents) = $repository->execxLocalCommand( - 'log -n1 --format=%s %s', - '%P', - $commit->getCommitIdentifier()); - - $use_log = !strlen(trim($parents)); - if ($use_log) { - // This is the first commit so we need to use "log". We know it's not a - // merge commit because it couldn't be merging anything, so this is safe. - - // NOTE: "--pretty=format: " is to disable diff output, we only want the - // part we get from "--raw". - list($raw) = $repository->execxLocalCommand( - 'log -n1 -M -C -B --find-copies-harder --raw -t '. - '--pretty=format: --abbrev=40 %s', - $commit->getCommitIdentifier()); - } else { - // Otherwise, we can use "diff", which will give us output for merges. - // We diff against the first parent, as this is generally the expectation - // and results in sensible behavior. - list($raw) = $repository->execxLocalCommand( - 'diff -n1 -M -C -B --find-copies-harder --raw -t '. - '--abbrev=40 %s^1 %s', - $commit->getCommitIdentifier(), - $commit->getCommitIdentifier()); - } + $viewer = PhabricatorUser::getOmnipotentUser(); + $raw = DiffusionQuery::callConduitWithDiffusionRequest( + $viewer, + DiffusionRequest::newFromDictionary( + array( + 'repository' => $repository, + 'user' => $viewer, + )), + 'diffusion.internal.gitrawdiffquery', + array( + 'commit' => $commit->getCommitIdentifier(), + )); $changes = array(); $move_away = array();