From 5b640a434cc4d5d18beae015295deca70b224d95 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 7 May 2018 13:38:11 -0700 Subject: [PATCH] Support an "Ancestors Of: ..." constraint in commit queries Summary: Ref T13137. See PHI609. An install would like to filter audit requests on a particular branch, e.g. "master". This is difficult in the general case because we can not apply this constraint efficiently under every conceivable data shape, but we can do a reasonable job in most practical cases. See T13137#238822 for more detailed discussion on the approach here. This is a bit rough, but should do the job for now. Test Plan: - Filtered commits by various branches, e.g. "master"; "lfs". Saw correct-seeming results. - Stubbed out the "just list everything" path to hit the `diffusion.internal.ancestors` path, saw the same correct-seeming results. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13137 Differential Revision: https://secure.phabricator.com/D19431 --- src/__phutil_library_map__.php | 2 + .../query/PhabricatorCommitSearchEngine.php | 11 ++ ...usionInternalAncestorsConduitAPIMethod.php | 51 ++++++ .../diffusion/query/DiffusionCommitQuery.php | 148 +++++++++++++++++- 4 files changed, 211 insertions(+), 1 deletion(-) create mode 100644 src/applications/diffusion/conduit/DiffusionInternalAncestorsConduitAPIMethod.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 6c91c0f0a4..e1b13dbecb 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -816,6 +816,7 @@ phutil_register_library_map(array( 'DiffusionHovercardEngineExtension' => 'applications/diffusion/engineextension/DiffusionHovercardEngineExtension.php', 'DiffusionInlineCommentController' => 'applications/diffusion/controller/DiffusionInlineCommentController.php', 'DiffusionInlineCommentPreviewController' => 'applications/diffusion/controller/DiffusionInlineCommentPreviewController.php', + 'DiffusionInternalAncestorsConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionInternalAncestorsConduitAPIMethod.php', 'DiffusionInternalGitRawDiffQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php', 'DiffusionLastModifiedController' => 'applications/diffusion/controller/DiffusionLastModifiedController.php', 'DiffusionLastModifiedQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionLastModifiedQueryConduitAPIMethod.php', @@ -6149,6 +6150,7 @@ phutil_register_library_map(array( 'DiffusionHovercardEngineExtension' => 'PhabricatorHovercardEngineExtension', 'DiffusionInlineCommentController' => 'PhabricatorInlineCommentController', 'DiffusionInlineCommentPreviewController' => 'PhabricatorInlineCommentPreviewController', + 'DiffusionInternalAncestorsConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionInternalGitRawDiffQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionLastModifiedController' => 'DiffusionController', 'DiffusionLastModifiedQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', diff --git a/src/applications/audit/query/PhabricatorCommitSearchEngine.php b/src/applications/audit/query/PhabricatorCommitSearchEngine.php index 6811427c93..993626e1e3 100644 --- a/src/applications/audit/query/PhabricatorCommitSearchEngine.php +++ b/src/applications/audit/query/PhabricatorCommitSearchEngine.php @@ -53,6 +53,10 @@ final class PhabricatorCommitSearchEngine $query->withUnreachable($map['unreachable']); } + if ($map['ancestorsOf']) { + $query->withAncestorsOf($map['ancestorsOf']); + } + return $query; } @@ -103,6 +107,13 @@ final class PhabricatorCommitSearchEngine pht( 'Find or exclude unreachable commits which are not ancestors of '. 'any branch, tag, or ref.')), + id(new PhabricatorSearchStringListField()) + ->setLabel(pht('Ancestors Of')) + ->setKey('ancestorsOf') + ->setDescription( + pht( + 'Find commits which are ancestors of a particular ref, '. + 'like "master".')), ); } diff --git a/src/applications/diffusion/conduit/DiffusionInternalAncestorsConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionInternalAncestorsConduitAPIMethod.php new file mode 100644 index 0000000000..01808bb4eb --- /dev/null +++ b/src/applications/diffusion/conduit/DiffusionInternalAncestorsConduitAPIMethod.php @@ -0,0 +1,51 @@ +'; + } + + protected function defineCustomParamTypes() { + return array( + 'ref' => 'required string', + 'commits' => 'required list', + ); + } + + protected function getResult(ConduitAPIRequest $request) { + $drequest = $this->getDiffusionRequest(); + $repository = $drequest->getRepository(); + + $commits = $request->getValue('commits'); + $ref = $request->getValue('ref'); + + $graph = new PhabricatorGitGraphStream($repository, $ref); + + $keep = array(); + foreach ($commits as $identifier) { + try { + $graph->getCommitDate($identifier); + $keep[] = $identifier; + } catch (Exception $ex) { + // Not an ancestor. + } + } + + return $keep; + } + +} diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php index 8996bab628..53e8d51bf9 100644 --- a/src/applications/diffusion/query/DiffusionCommitQuery.php +++ b/src/applications/diffusion/query/DiffusionCommitQuery.php @@ -22,10 +22,14 @@ final class DiffusionCommitQuery private $epochMin; private $epochMax; private $importing; + private $ancestorsOf; private $needCommitData; private $needDrafts; + private $mustFilterRefs = false; + private $refRepository; + public function withIDs(array $ids) { $this->ids = $ids; return $this; @@ -92,7 +96,7 @@ final class DiffusionCommitQuery } public function withRepositoryIDs(array $repository_ids) { - $this->repositoryIDs = $repository_ids; + $this->repositoryIDs = array_unique($repository_ids); return $this; } @@ -152,6 +156,11 @@ final class DiffusionCommitQuery return $this; } + public function withAncestorsOf(array $refs) { + $this->ancestorsOf = $refs; + return $this; + } + public function getIdentifierMap() { if ($this->identifierMap === null) { throw new Exception( @@ -307,6 +316,54 @@ final class DiffusionCommitQuery protected function didFilterPage(array $commits) { $viewer = $this->getViewer(); + if ($this->mustFilterRefs) { + // If this flag is set, the query has an "Ancestors Of" constraint and + // at least one of the constraining refs had too many ancestors for us + // to apply the constraint with a big "commitIdentifier IN (%Ls)" clause. + // We're going to filter each page and hope we get a full result set + // before the query overheats. + + $ancestor_list = mpull($commits, 'getCommitIdentifier'); + $ancestor_list = array_values($ancestor_list); + + foreach ($this->ancestorsOf as $ref) { + try { + $ancestor_list = DiffusionQuery::callConduitWithDiffusionRequest( + $viewer, + DiffusionRequest::newFromDictionary( + array( + 'repository' => $this->refRepository, + 'user' => $viewer, + )), + 'diffusion.internal.ancestors', + array( + 'ref' => $ref, + 'commits' => $ancestor_list, + )); + } catch (ConduitClientException $ex) { + throw new PhabricatorSearchConstraintException( + $ex->getMessage()); + } + + if (!$ancestor_list) { + break; + } + } + + $ancestor_list = array_fuse($ancestor_list); + foreach ($commits as $key => $commit) { + $identifier = $commit->getCommitIdentifier(); + if (!isset($ancestor_list[$identifier])) { + $this->didRejectResult($commit); + unset($commits[$key]); + } + } + + if (!$commits) { + return $commits; + } + } + if ($this->needCommitData) { $data = id(new PhabricatorRepositoryCommitData())->loadAllWhere( 'commitID in (%Ld)', @@ -364,6 +421,95 @@ final class DiffusionCommitQuery $this->withRepositoryIDs($repository_ids); } + if ($this->ancestorsOf !== null) { + if (count($this->repositoryIDs) !== 1) { + throw new PhabricatorSearchConstraintException( + pht( + 'To search for commits which are ancestors of particular refs, '. + 'you must constrain the search to exactly one repository.')); + } + + $repository_id = head($this->repositoryIDs); + $history_limit = $this->getRawResultLimit() * 32; + $viewer = $this->getViewer(); + + $repository = id(new PhabricatorRepositoryQuery()) + ->setViewer($viewer) + ->withIDs(array($repository_id)) + ->executeOne(); + + if (!$repository) { + throw new PhabricatorEmptyQueryException(); + } + + if ($repository->isSVN()) { + throw new PhabricatorSearchConstraintException( + pht( + 'Subversion does not support searching for ancestors of '. + 'a particular ref. This operation is not meaningful in '. + 'Subversion.')); + } + + if ($repository->isHg()) { + throw new PhabricatorSearchConstraintException( + pht( + 'Mercurial does not currently support searching for ancestors of '. + 'a particular ref.')); + } + + $can_constrain = true; + $history_identifiers = array(); + foreach ($this->ancestorsOf as $key => $ref) { + try { + $raw_history = DiffusionQuery::callConduitWithDiffusionRequest( + $viewer, + DiffusionRequest::newFromDictionary( + array( + 'repository' => $repository, + 'user' => $viewer, + )), + 'diffusion.historyquery', + array( + 'commit' => $ref, + 'limit' => $history_limit, + )); + } catch (ConduitClientException $ex) { + throw new PhabricatorSearchConstraintException( + $ex->getMessage()); + } + + $ref_identifiers = array(); + foreach ($raw_history['pathChanges'] as $change) { + $ref_identifiers[] = $change['commitIdentifier']; + } + + // If this ref had fewer total commits than the limit, we're safe to + // apply the constraint as a large `IN (...)` query for a list of + // commit identifiers. This is efficient. + if ($history_limit) { + if (count($ref_identifiers) >= $history_limit) { + $can_constrain = false; + break; + } + } + + $history_identifiers += array_fuse($ref_identifiers); + } + + // If all refs had a small number of ancestors, we can just put the + // constraint into the query here and we're done. Otherwise, we need + // to filter each page after it comes out of the MySQL layer. + if ($can_constrain) { + $where[] = qsprintf( + $conn, + 'commit.commitIdentifier IN (%Ls)', + $history_identifiers); + } else { + $this->mustFilterRefs = true; + $this->refRepository = $repository; + } + } + if ($this->ids !== null) { $where[] = qsprintf( $conn,