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

src/applications/diffusion/controller/DiffusionRepositoryController.php

@@ -198,61 +198,62 @@ final class DiffusionRepositoryController extends DiffusionController {
   }
 
   private function buildBranchListTable(DiffusionRequest $drequest) {
-    if ($drequest->getBranch() !== null) {
+    $viewer = $this->getRequest()->getUser();
-      $limit = 15;
 
-      $branches = DiffusionBranchInformation::newFromConduit(
+    if ($drequest->getBranch() === null) {
-        $this->callConduitWithDiffusionRequest(
+      return null;
-          '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;
     }
 
-    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) {

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);
   }
 

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 = $commit->getSummary();
-        $details = idx(explode("\n", $details), 0);
-        $details = substr($details, 0, 80);
-
         $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';

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;
   }