From d1c4b5081cd0908b4ff0edfc6aa241e8ade52d73 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 30 Oct 2013 13:06:28 -0700 Subject: [PATCH] Clean up Diffusion branch query a bit Summary: Ref T2716. - Serve from `DiffusionCommitQuery`, not `PhabricatorAuditCommitQuery` (which should probably die). - Fix logic for `limit`, which incorrectly failed to display the "Showing %d branches." text. - Clean up things a touch. - I didn't end up actually needing `needCommitData()`, but left it in there since I think it will be needed soon. - Removed a "TODO" because I don't remember what "etc etc" means. Test Plan: Looked at branches in several repositories. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T2716 Differential Revision: https://secure.phabricator.com/D7451 --- .../DiffusionRepositoryController.php | 105 +++++++++--------- .../diffusion/query/DiffusionCommitQuery.php | 38 ++++++- .../view/DiffusionBranchTableView.php | 6 +- .../storage/PhabricatorRepositoryCommit.php | 3 +- 4 files changed, 93 insertions(+), 59 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionRepositoryController.php b/src/applications/diffusion/controller/DiffusionRepositoryController.php index 6183c88136..c3d27ac196 100644 --- a/src/applications/diffusion/controller/DiffusionRepositoryController.php +++ b/src/applications/diffusion/controller/DiffusionRepositoryController.php @@ -198,61 +198,62 @@ final class DiffusionRepositoryController extends DiffusionController { } private function buildBranchListTable(DiffusionRequest $drequest) { - if ($drequest->getBranch() !== null) { - $limit = 15; + $viewer = $this->getRequest()->getUser(); - $branches = DiffusionBranchInformation::newFromConduit( - $this->callConduitWithDiffusionRequest( - 'diffusion.branchquery', - array( - 'limit' => $limit - ))); - if (!$branches) { - return null; - } - - $more_branches = (count($branches) > $limit); - $branches = array_slice($branches, 0, $limit); - - $commits = id(new PhabricatorAuditCommitQuery()) - ->withIdentifiers( - $drequest->getRepository()->getID(), - mpull($branches, 'getHeadCommitIdentifier')) - ->needCommitData(true) - ->execute(); - - $table = new DiffusionBranchTableView(); - $table->setDiffusionRequest($drequest); - $table->setBranches($branches); - $table->setCommits($commits); - $table->setUser($this->getRequest()->getUser()); - - $panel = new AphrontPanelView(); - $panel->setHeader(pht('Branches')); - $panel->setNoBackground(); - - if ($more_branches) { - $panel->setCaption(pht('Showing %d branches.', $limit)); - } - - $panel->addButton( - phutil_tag( - 'a', - array( - 'href' => $drequest->generateURI( - array( - 'action' => 'branches', - )), - 'class' => 'grey button', - ), - pht("Show All Branches \xC2\xBB"))); - - $panel->appendChild($table); - - return $panel; + if ($drequest->getBranch() === null) { + return null; } - return null; + $limit = 15; + + $branches = DiffusionBranchInformation::newFromConduit( + $this->callConduitWithDiffusionRequest( + 'diffusion.branchquery', + array( + 'limit' => $limit + 1, + ))); + if (!$branches) { + return null; + } + + $more_branches = (count($branches) > $limit); + $branches = array_slice($branches, 0, $limit); + + $commits = id(new DiffusionCommitQuery()) + ->setViewer($viewer) + ->withIdentifiers(mpull($branches, 'getHeadCommitIdentifier')) + ->withRepositoryIDs(array($drequest->getRepository()->getID())) + ->execute(); + + $table = new DiffusionBranchTableView(); + $table->setDiffusionRequest($drequest); + $table->setBranches($branches); + $table->setCommits($commits); + $table->setUser($this->getRequest()->getUser()); + + $panel = new AphrontPanelView(); + $panel->setHeader(pht('Branches')); + $panel->setNoBackground(); + + if ($more_branches) { + $panel->setCaption(pht('Showing %d branches.', $limit)); + } + + $panel->addButton( + phutil_tag( + 'a', + array( + 'href' => $drequest->generateURI( + array( + 'action' => 'branches', + )), + 'class' => 'grey button', + ), + pht("Show All Branches \xC2\xBB"))); + + $panel->appendChild($table); + + return $panel; } private function buildTagListTable(DiffusionRequest $drequest) { diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php index 3522b6e7fa..ba981ae9a7 100644 --- a/src/applications/diffusion/query/DiffusionCommitQuery.php +++ b/src/applications/diffusion/query/DiffusionCommitQuery.php @@ -8,6 +8,9 @@ final class DiffusionCommitQuery private $phids; private $defaultRepository; private $identifierMap; + private $repositoryIDs; + + private $needCommitData; /** * Load commits by partial or full identifiers, e.g. "rXab82393", "rX1234", @@ -34,6 +37,11 @@ final class DiffusionCommitQuery return $this; } + public function withRepositoryIDs(array $repository_ids) { + $this->repositoryIDs = $repository_ids; + return $this; + } + public function withIDs(array $ids) { $this->ids = $ids; return $this; @@ -44,6 +52,11 @@ final class DiffusionCommitQuery return $this; } + public function needCommitData($need) { + $this->needCommitData = $need; + return $this; + } + public function getIdentifierMap() { if ($this->identifierMap === null) { throw new Exception( @@ -71,7 +84,7 @@ final class DiffusionCommitQuery return $table->loadAllFromArray($data); } - public function willFilterPage(array $commits) { + protected function willFilterPage(array $commits) { $repository_ids = mpull($commits, 'getRepositoryID', 'getRepositoryID'); $repos = id(new PhabricatorRepositoryQuery()) ->setViewer($this->getViewer()) @@ -131,6 +144,22 @@ final class DiffusionCommitQuery return $commits; } + protected function didFilterPage(array $commits) { + + if ($this->needCommitData) { + $data = id(new PhabricatorRepositoryCommitData())->loadAllWhere( + 'commitID in (%Ld)', + mpull($commits, 'getID')); + $data = mpull($data, null, 'getCommitID'); + foreach ($commits as $commit) { + $commit_data = idx($data, $commit->getID()); + $commit->attachCommitData($commit_data); + } + } + + return $commits; + } + private function buildWhereClause(AphrontDatabaseConnection $conn_r) { $where = array(); @@ -237,6 +266,13 @@ final class DiffusionCommitQuery $this->phids); } + if ($this->repositoryIDs) { + $where[] = qsprintf( + $conn_r, + 'repositoryID IN (%Ld)', + $this->repositoryIDs); + } + return $this->formatWhereClause($where); } diff --git a/src/applications/diffusion/view/DiffusionBranchTableView.php b/src/applications/diffusion/view/DiffusionBranchTableView.php index 3da9c8c1f2..001af93685 100644 --- a/src/applications/diffusion/view/DiffusionBranchTableView.php +++ b/src/applications/diffusion/view/DiffusionBranchTableView.php @@ -25,10 +25,7 @@ final class DiffusionBranchTableView extends DiffusionView { foreach ($this->branches as $branch) { $commit = idx($this->commits, $branch->getHeadCommitIdentifier()); if ($commit) { - $details = $commit->getCommitData()->getCommitMessage(); - $details = idx(explode("\n", $details), 0); - $details = substr($details, 0, 80); - + $details = $commit->getSummary(); $datetime = phabricator_datetime($commit->getEpoch(), $this->user); } else { $datetime = null; @@ -61,7 +58,6 @@ final class DiffusionBranchTableView extends DiffusionView { $branch->getHeadCommitIdentifier()), $datetime, AphrontTableView::renderSingleDisplayLine($details), - // TODO: etc etc ); if ($branch->getName() == $current_branch) { $rowc[] = 'highlighted'; diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index a999726162..55ee28dac4 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -81,7 +81,8 @@ final class PhabricatorRepositoryCommit $this->getID()); } - public function attachCommitData(PhabricatorRepositoryCommitData $data) { + public function attachCommitData( + PhabricatorRepositoryCommitData $data = null) { $this->commitData = $data; return $this; }