From 7e8dc0742bdca62edfcd4aa699fad096a5974093 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 22 Apr 2019 11:54:42 -0700 Subject: [PATCH] Remove all callers to "DifferentialRevision->loadIDsByCommitPHIDs()" Summary: Depends on D20457. Ref T13276. Kill all remaining callers to this method and delete it. Test Plan: - Grepped for `loadIDsByCommitPHIDs`. - Viewed blame again to make sure I didn't break it. - Viewed "History" view for commits with revisions. - Viewed "Graph" view for commits with revisions. - Viewed "Merged Commits" table for commits with revisions. - Viewed "Compare" table for commits with revisions. - Viewed "Repository" main page history table for commits with revisions. - Grepped for `linkRevision`. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13276 Differential Revision: https://secure.phabricator.com/D20458 --- src/__phutil_library_map__.php | 2 + .../storage/DifferentialRevision.php | 12 ----- .../controller/DiffusionBlameController.php | 45 ++--------------- .../controller/DiffusionBrowseController.php | 6 +-- .../controller/DiffusionCommitController.php | 2 - .../controller/DiffusionCompareController.php | 6 +-- .../controller/DiffusionGraphController.php | 1 - .../controller/DiffusionHistoryController.php | 1 - .../DiffusionRepositoryController.php | 8 +-- .../query/DiffusionCommitRevisionQuery.php | 49 +++++++++++++++++++ .../view/DiffusionHistoryListView.php | 11 +++-- .../view/DiffusionHistoryTableView.php | 18 +++++-- .../diffusion/view/DiffusionHistoryView.php | 45 ++++++++++------- .../diffusion/view/DiffusionView.php | 13 ----- 14 files changed, 106 insertions(+), 113 deletions(-) create mode 100644 src/applications/diffusion/query/DiffusionCommitRevisionQuery.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 2e22fc3301..0e3a89ab89 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -765,6 +765,7 @@ phutil_register_library_map(array( 'DiffusionCommitRevisionAcceptedHeraldField' => 'applications/diffusion/herald/DiffusionCommitRevisionAcceptedHeraldField.php', 'DiffusionCommitRevisionAcceptingReviewersHeraldField' => 'applications/diffusion/herald/DiffusionCommitRevisionAcceptingReviewersHeraldField.php', 'DiffusionCommitRevisionHeraldField' => 'applications/diffusion/herald/DiffusionCommitRevisionHeraldField.php', + 'DiffusionCommitRevisionQuery' => 'applications/diffusion/query/DiffusionCommitRevisionQuery.php', 'DiffusionCommitRevisionReviewersHeraldField' => 'applications/diffusion/herald/DiffusionCommitRevisionReviewersHeraldField.php', 'DiffusionCommitRevisionSubscribersHeraldField' => 'applications/diffusion/herald/DiffusionCommitRevisionSubscribersHeraldField.php', 'DiffusionCommitSearchConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionCommitSearchConduitAPIMethod.php', @@ -6427,6 +6428,7 @@ phutil_register_library_map(array( 'DiffusionCommitRevisionAcceptedHeraldField' => 'DiffusionCommitHeraldField', 'DiffusionCommitRevisionAcceptingReviewersHeraldField' => 'DiffusionCommitHeraldField', 'DiffusionCommitRevisionHeraldField' => 'DiffusionCommitHeraldField', + 'DiffusionCommitRevisionQuery' => 'Phobject', 'DiffusionCommitRevisionReviewersHeraldField' => 'DiffusionCommitHeraldField', 'DiffusionCommitRevisionSubscribersHeraldField' => 'DiffusionCommitHeraldField', 'DiffusionCommitSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod', diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index a2a058568b..971c6d92a7 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -158,18 +158,6 @@ final class DifferentialRevision extends DifferentialDAO return '/'.$this->getMonogram(); } - public function loadIDsByCommitPHIDs($phids) { - if (!$phids) { - return array(); - } - $revision_ids = queryfx_all( - $this->establishConnection('r'), - 'SELECT * FROM %T WHERE commitPHID IN (%Ls)', - self::TABLE_COMMIT, - $phids); - return ipull($revision_ids, 'revisionID', 'commitPHID'); - } - public function loadCommitPHIDs() { if (!$this->getID()) { return ($this->commits = array()); diff --git a/src/applications/diffusion/controller/DiffusionBlameController.php b/src/applications/diffusion/controller/DiffusionBlameController.php index 775b61c124..5de464b335 100644 --- a/src/applications/diffusion/controller/DiffusionBlameController.php +++ b/src/applications/diffusion/controller/DiffusionBlameController.php @@ -36,7 +36,9 @@ final class DiffusionBlameController extends DiffusionController { $commit_map = mpull($commits, 'getCommitIdentifier', 'getPHID'); - $revision_map = $this->loadRevisionsForCommits($commits); + $revision_map = DiffusionCommitRevisionQuery::loadRevisionMapForCommits( + $viewer, + $commits); $base_href = (string)$drequest->generateURI( array( @@ -267,45 +269,4 @@ final class DiffusionBlameController extends DiffusionController { } } - private function loadRevisionsForCommits(array $commits) { - if (!$commits) { - return array(); - } - - $commit_phids = mpull($commits, 'getPHID'); - - $edge_query = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs($commit_phids) - ->withEdgeTypes( - array( - DiffusionCommitHasRevisionEdgeType::EDGECONST, - )); - $edge_query->execute(); - - $revision_phids = $edge_query->getDestinationPHIDs(); - if (!$revision_phids) { - return array(); - } - - $viewer = $this->getViewer(); - - $revisions = id(new DifferentialRevisionQuery()) - ->setViewer($viewer) - ->withPHIDs($revision_phids) - ->execute(); - $revisions = mpull($revisions, null, 'getPHID'); - - $map = array(); - foreach ($commit_phids as $commit_phid) { - $revision_phids = $edge_query->getDestinationPHIDs( - array( - $commit_phid, - )); - - $map[$commit_phid] = array_select_keys($revisions, $revision_phids); - } - - return $map; - } - } diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index 1493d658e6..60df3f35ac 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -1106,11 +1106,7 @@ final class DiffusionBrowseController extends DiffusionController { $history_table = id(new DiffusionHistoryTableView()) ->setViewer($viewer) ->setDiffusionRequest($drequest) - ->setHistory($history); - - $history_table->loadRevisions(); - - $history_table + ->setHistory($history) ->setParents($results['parents']) ->setFilterParents(true) ->setIsHead(true) diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index d49584f6b4..64a6297f21 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -807,8 +807,6 @@ final class DiffusionCommitController extends DiffusionController { ->setDiffusionRequest($drequest) ->setHistory($merges); - $history_table->loadRevisions(); - $panel = id(new PHUIObjectBoxView()) ->setHeaderText(pht('Merged Changes')) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) diff --git a/src/applications/diffusion/controller/DiffusionCompareController.php b/src/applications/diffusion/controller/DiffusionCompareController.php index 2361c066dd..7dc3fb14c5 100644 --- a/src/applications/diffusion/controller/DiffusionCompareController.php +++ b/src/applications/diffusion/controller/DiffusionCompareController.php @@ -299,11 +299,7 @@ final class DiffusionCompareController extends DiffusionController { $history_table = id(new DiffusionHistoryTableView()) ->setUser($viewer) ->setDiffusionRequest($drequest) - ->setHistory($history); - - $history_table->loadRevisions(); - - $history_table + ->setHistory($history) ->setParents($results['parents']) ->setFilterParents(true) ->setIsHead(!$pager->getOffset()) diff --git a/src/applications/diffusion/controller/DiffusionGraphController.php b/src/applications/diffusion/controller/DiffusionGraphController.php index 096204ac6d..4536e29eaf 100644 --- a/src/applications/diffusion/controller/DiffusionGraphController.php +++ b/src/applications/diffusion/controller/DiffusionGraphController.php @@ -40,7 +40,6 @@ final class DiffusionGraphController extends DiffusionController { ->setDiffusionRequest($drequest) ->setHistory($history); - $graph->loadRevisions(); $show_graph = !strlen($drequest->getPath()); if ($show_graph) { $graph->setParents($history_results['parents']); diff --git a/src/applications/diffusion/controller/DiffusionHistoryController.php b/src/applications/diffusion/controller/DiffusionHistoryController.php index f50e73295f..f0fec0dd2d 100644 --- a/src/applications/diffusion/controller/DiffusionHistoryController.php +++ b/src/applications/diffusion/controller/DiffusionHistoryController.php @@ -40,7 +40,6 @@ final class DiffusionHistoryController extends DiffusionController { ->setDiffusionRequest($drequest) ->setHistory($history); - $history_list->loadRevisions(); $header = $this->buildHeader($drequest); $crumbs = $this->buildCrumbs( diff --git a/src/applications/diffusion/controller/DiffusionRepositoryController.php b/src/applications/diffusion/controller/DiffusionRepositoryController.php index bec4f1e71a..12aaefe12a 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryController.php @@ -440,17 +440,13 @@ final class DiffusionRepositoryController extends DiffusionController { $history_table = id(new DiffusionHistoryTableView()) ->setUser($viewer) ->setDiffusionRequest($drequest) - ->setHistory($history); - - // TODO: Super sketchy. - $history_table->loadRevisions(); + ->setHistory($history) + ->setIsHead(true); if ($history_results) { $history_table->setParents($history_results['parents']); } - $history_table->setIsHead(true); - $panel = id(new PHUIObjectBoxView()) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) ->addClass('diffusion-mobile-view'); diff --git a/src/applications/diffusion/query/DiffusionCommitRevisionQuery.php b/src/applications/diffusion/query/DiffusionCommitRevisionQuery.php new file mode 100644 index 0000000000..a73db18a59 --- /dev/null +++ b/src/applications/diffusion/query/DiffusionCommitRevisionQuery.php @@ -0,0 +1,49 @@ +withSourcePHIDs($commit_phids) + ->withEdgeTypes( + array( + DiffusionCommitHasRevisionEdgeType::EDGECONST, + )); + $edge_query->execute(); + + $revision_phids = $edge_query->getDestinationPHIDs(); + if (!$revision_phids) { + return array(); + } + + $revisions = id(new DifferentialRevisionQuery()) + ->setViewer($viewer) + ->withPHIDs($revision_phids) + ->execute(); + $revisions = mpull($revisions, null, 'getPHID'); + + $map = array(); + foreach ($commit_phids as $commit_phid) { + $revision_phids = $edge_query->getDestinationPHIDs( + array( + $commit_phid, + )); + + $map[$commit_phid] = array_select_keys($revisions, $revision_phids); + } + + return $map; + } + +} diff --git a/src/applications/diffusion/view/DiffusionHistoryListView.php b/src/applications/diffusion/view/DiffusionHistoryListView.php index 62ba7b70c2..cc7bade925 100644 --- a/src/applications/diffusion/view/DiffusionHistoryListView.php +++ b/src/applications/diffusion/view/DiffusionHistoryListView.php @@ -104,16 +104,17 @@ final class DiffusionHistoryListView extends DiffusionHistoryView { $diff_tag = null; if ($show_revisions && $commit) { - $d_id = idx($this->getRevisions(), $commit->getPHID()); - if ($d_id) { + $revisions = $this->getRevisionsForCommit($commit); + if ($revisions) { + $revision = head($revisions); $diff_tag = id(new PHUITagView()) - ->setName('D'.$d_id) + ->setName($revision->getMonogram()) ->setType(PHUITagView::TYPE_SHADE) ->setColor(PHUITagView::COLOR_BLUE) - ->setHref('/D'.$d_id) + ->setHref($revision->getURI()) ->setBorder(PHUITagView::BORDER_NONE) ->setSlimShady(true); - } + } } $build_view = null; diff --git a/src/applications/diffusion/view/DiffusionHistoryTableView.php b/src/applications/diffusion/view/DiffusionHistoryTableView.php index cfd7019679..bd4fd5490a 100644 --- a/src/applications/diffusion/view/DiffusionHistoryTableView.php +++ b/src/applications/diffusion/view/DiffusionHistoryTableView.php @@ -127,6 +127,20 @@ final class DiffusionHistoryTableView extends DiffusionHistoryView { 'tip' => $name, )); + $revision_link = null; + if ($commit) { + $revisions = $this->getRevisionsForCommit($commit); + if ($revisions) { + $revision = head($revisions); + $revision_link = phutil_tag( + 'a', + array( + 'href' => $revision->getURI(), + ), + $revision->getMonogram()); + } + } + $rows[] = array( $graph ? $graph[$ii++] : null, $browse, @@ -135,9 +149,7 @@ final class DiffusionHistoryTableView extends DiffusionHistoryView { $history->getCommitIdentifier()), $build, $audit_view, - ($commit ? - self::linkRevision(idx($this->getRevisions(), $commit->getPHID())) : - null), + $revision_link, $author, $summary, $committed, diff --git a/src/applications/diffusion/view/DiffusionHistoryView.php b/src/applications/diffusion/view/DiffusionHistoryView.php index 56a0f3b75f..d7ae662ab6 100644 --- a/src/applications/diffusion/view/DiffusionHistoryView.php +++ b/src/applications/diffusion/view/DiffusionHistoryView.php @@ -9,6 +9,7 @@ abstract class DiffusionHistoryView extends DiffusionView { private $isTail; private $parents; private $filterParents; + private $revisionMap; public function setHistory(array $history) { assert_instances_of($history, 'DiffusionPathChange'); @@ -20,24 +21,6 @@ abstract class DiffusionHistoryView extends DiffusionView { return $this->history; } - public function loadRevisions() { - $commit_phids = array(); - foreach ($this->history as $item) { - if ($item->getCommit()) { - $commit_phids[] = $item->getCommit()->getPHID(); - } - } - - // TODO: Get rid of this. - $this->revisions = id(new DifferentialRevision()) - ->loadIDsByCommitPHIDs($commit_phids); - return $this; - } - - public function getRevisions() { - return $this->revisions; - } - public function setHandles(array $handles) { assert_instances_of($handles, 'PhabricatorObjectHandle'); $this->handles = $handles; @@ -98,4 +81,30 @@ abstract class DiffusionHistoryView extends DiffusionView { public function render() {} + final protected function getRevisionsForCommit( + PhabricatorRepositoryCommit $commit) { + + if ($this->revisionMap === null) { + $this->revisionMap = $this->newRevisionMap(); + } + + return idx($this->revisionMap, $commit->getPHID(), array()); + } + + private function newRevisionMap() { + $history = $this->history; + + $commits = array(); + foreach ($history as $item) { + $commit = $item->getCommit(); + if ($commit) { + $commits[] = $commit; + } + } + + return DiffusionCommitRevisionQuery::loadRevisionMapForCommits( + $this->getViewer(), + $commits); + } + } diff --git a/src/applications/diffusion/view/DiffusionView.php b/src/applications/diffusion/view/DiffusionView.php index 123d22af5e..795b47c773 100644 --- a/src/applications/diffusion/view/DiffusionView.php +++ b/src/applications/diffusion/view/DiffusionView.php @@ -169,19 +169,6 @@ abstract class DiffusionView extends AphrontView { $detail); } - final public static function linkRevision($id) { - if (!$id) { - return null; - } - - return phutil_tag( - 'a', - array( - 'href' => "/D{$id}", - ), - "D{$id}"); - } - final public static function renderName($name) { $email = new PhutilEmailAddress($name); if ($email->getDisplayName() && $email->getDomainName()) {