DiffusionBranchQuery => Conduit

Summary: ref T2784. This one had a few fun spots where I had to move data around. Also, is there some common object (or should I add it?) that can do this toDictionary newFromConduit stuff? Also, this assumes D5803 is largely correct at the time of this diff.

Test Plan: browsed mercurial and git repository page. saw the branches i expected.

Reviewers: epriestley

Reviewed By: epriestley

CC: chad, aran, Korvin

Maniphest Tasks: T2784

Differential Revision: https://secure.phabricator.com/D5810

src/__phutil_library_map__.php

@@ -145,6 +145,7 @@ phutil_register_library_map(array(
     'ConduitAPI_differential_updateunitresults_Method' => 'applications/differential/conduit/ConduitAPI_differential_updateunitresults_Method.php',
     'ConduitAPI_diffusion_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_Method.php',
     'ConduitAPI_diffusion_abstractquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_abstractquery_Method.php',
+    'ConduitAPI_diffusion_branchquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_branchquery_Method.php',
     'ConduitAPI_diffusion_existsquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_existsquery_Method.php',
     'ConduitAPI_diffusion_findsymbols_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_findsymbols_Method.php',
     'ConduitAPI_diffusion_getcommits_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_getcommits_Method.php',
@@ -397,7 +398,6 @@ phutil_register_library_map(array(
     'DifferentialUnitStatus' => 'applications/differential/constants/DifferentialUnitStatus.php',
     'DifferentialUnitTestResult' => 'applications/differential/constants/DifferentialUnitTestResult.php',
     'DiffusionBranchInformation' => 'applications/diffusion/data/DiffusionBranchInformation.php',
-    'DiffusionBranchQuery' => 'applications/diffusion/query/branch/DiffusionBranchQuery.php',
     'DiffusionBranchTableController' => 'applications/diffusion/controller/DiffusionBranchTableController.php',
     'DiffusionBranchTableView' => 'applications/diffusion/view/DiffusionBranchTableView.php',
     'DiffusionBrowseController' => 'applications/diffusion/controller/DiffusionBrowseController.php',
@@ -423,8 +423,8 @@ phutil_register_library_map(array(
     'DiffusionExternalController' => 'applications/diffusion/controller/DiffusionExternalController.php',
     'DiffusionFileContent' => 'applications/diffusion/data/DiffusionFileContent.php',
     'DiffusionFileContentQuery' => 'applications/diffusion/query/filecontent/DiffusionFileContentQuery.php',
-    'DiffusionGitBranchQuery' => 'applications/diffusion/query/branch/DiffusionGitBranchQuery.php',
-    'DiffusionGitBranchQueryTestCase' => 'applications/diffusion/query/branch/__tests__/DiffusionGitBranchQueryTestCase.php',
+    'DiffusionGitBranch' => 'applications/diffusion/data/DiffusionGitBranch.php',
+    'DiffusionGitBranchTestCase' => 'applications/diffusion/data/__tests__/DiffusionGitBranchTestCase.php',
     'DiffusionGitBrowseQuery' => 'applications/diffusion/query/browse/DiffusionGitBrowseQuery.php',
     'DiffusionGitCommitParentsQuery' => 'applications/diffusion/query/parents/DiffusionGitCommitParentsQuery.php',
     'DiffusionGitCommitTagsQuery' => 'applications/diffusion/query/committags/DiffusionGitCommitTagsQuery.php',
@@ -449,7 +449,6 @@ phutil_register_library_map(array(
     'DiffusionLintController' => 'applications/diffusion/controller/DiffusionLintController.php',
     'DiffusionLintDetailsController' => 'applications/diffusion/controller/DiffusionLintDetailsController.php',
     'DiffusionLintSaveRunner' => 'applications/diffusion/DiffusionLintSaveRunner.php',
-    'DiffusionMercurialBranchQuery' => 'applications/diffusion/query/branch/DiffusionMercurialBranchQuery.php',
     'DiffusionMercurialBrowseQuery' => 'applications/diffusion/query/browse/DiffusionMercurialBrowseQuery.php',
     'DiffusionMercurialCommitParentsQuery' => 'applications/diffusion/query/parents/DiffusionMercurialCommitParentsQuery.php',
     'DiffusionMercurialCommitTagsQuery' => 'applications/diffusion/query/committags/DiffusionMercurialCommitTagsQuery.php',
@@ -477,7 +476,7 @@ phutil_register_library_map(array(
     'DiffusionRenameHistoryQuery' => 'applications/diffusion/query/DiffusionRenameHistoryQuery.php',
     'DiffusionRepositoryController' => 'applications/diffusion/controller/DiffusionRepositoryController.php',
     'DiffusionRepositoryPath' => 'applications/diffusion/data/DiffusionRepositoryPath.php',
-    'DiffusionRepositoryTag' => 'applications/diffusion/DiffusionRepositoryTag.php',
+    'DiffusionRepositoryTag' => 'applications/diffusion/data/DiffusionRepositoryTag.php',
     'DiffusionRequest' => 'applications/diffusion/request/DiffusionRequest.php',
     'DiffusionSetupException' => 'applications/diffusion/exception/DiffusionSetupException.php',
     'DiffusionSvnBrowseQuery' => 'applications/diffusion/query/browse/DiffusionSvnBrowseQuery.php',
@@ -1912,6 +1911,7 @@ phutil_register_library_map(array(
     'ConduitAPI_differential_updateunitresults_Method' => 'ConduitAPIMethod',
     'ConduitAPI_diffusion_Method' => 'ConduitAPIMethod',
     'ConduitAPI_diffusion_abstractquery_Method' => 'ConduitAPI_diffusion_Method',
+    'ConduitAPI_diffusion_branchquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',
     'ConduitAPI_diffusion_existsquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',
     'ConduitAPI_diffusion_findsymbols_Method' => 'ConduitAPI_diffusion_Method',
     'ConduitAPI_diffusion_getcommits_Method' => 'ConduitAPI_diffusion_Method',
@@ -2178,8 +2178,7 @@ phutil_register_library_map(array(
     'DiffusionEmptyResultView' => 'DiffusionView',
     'DiffusionExternalController' => 'DiffusionController',
     'DiffusionFileContentQuery' => 'DiffusionQuery',
-    'DiffusionGitBranchQuery' => 'DiffusionBranchQuery',
-    'DiffusionGitBranchQueryTestCase' => 'PhabricatorTestCase',
+    'DiffusionGitBranchTestCase' => 'PhabricatorTestCase',
     'DiffusionGitBrowseQuery' => 'DiffusionBrowseQuery',
     'DiffusionGitCommitParentsQuery' => 'DiffusionCommitParentsQuery',
     'DiffusionGitCommitTagsQuery' => 'DiffusionCommitTagsQuery',
@@ -2203,7 +2202,6 @@ phutil_register_library_map(array(
     'DiffusionLastModifiedQuery' => 'DiffusionQuery',
     'DiffusionLintController' => 'DiffusionController',
     'DiffusionLintDetailsController' => 'DiffusionController',
-    'DiffusionMercurialBranchQuery' => 'DiffusionBranchQuery',
     'DiffusionMercurialBrowseQuery' => 'DiffusionBrowseQuery',
     'DiffusionMercurialCommitParentsQuery' => 'DiffusionCommitParentsQuery',
     'DiffusionMercurialCommitTagsQuery' => 'DiffusionCommitTagsQuery',

src/applications/diffusion/conduit/ConduitAPI_diffusion_branchquery_Method.php

@@ -0,0 +1,95 @@
+<?php
+
+/**
+ * @group conduit
+ */
+final class ConduitAPI_diffusion_branchquery_Method
+  extends ConduitAPI_diffusion_abstractquery_Method {
+
+  public function getMethodDescription() {
+    return 'Determine what branches exist for a repository.';
+  }
+
+  public function defineReturnType() {
+    return 'array';
+  }
+
+  protected function defineCustomParamTypes() {
+    return array(
+      'limit' => 'optional int',
+      'offset' => 'optional int'
+    );
+  }
+
+  protected function getGitResult(ConduitAPIRequest $request) {
+    $drequest = $this->getDiffusionRequest();
+    $repository = $drequest->getRepository();
+    $limit = $request->getValue('limit');
+    $offset = $request->getValue('offset');
+
+    // We need to add 1 in case we pick up HEAD.
+    $count = $offset + $limit + 1;
+
+    list($stdout) = $repository->execxLocalCommand(
+      'for-each-ref %C --sort=-creatordate --format=%s refs/remotes',
+      $count ? '--count='.(int)$count : null,
+      '%(refname:short) %(objectname)');
+
+    $branch_list = DiffusionGitBranch::parseRemoteBranchOutput(
+      $stdout,
+      $only_this_remote = DiffusionBranchInformation::DEFAULT_GIT_REMOTE);
+
+    $branches = array();
+    foreach ($branch_list as $name => $head) {
+      if (!$repository->shouldTrackBranch($name)) {
+        continue;
+      }
+
+      $branch = new DiffusionBranchInformation();
+      $branch->setName($name);
+      $branch->setHeadCommitIdentifier($head);
+      $branches[] = $branch->toDictionary();
+    }
+
+    if ($offset) {
+      $branches = array_slice($branches, $offset);
+    }
+
+    // We might have too many even after offset slicing, if there was no HEAD
+    // for some reason.
+    if ($limit) {
+      $branches = array_slice($branches, 0, $limit);
+    }
+
+    return $branches;
+  }
+
+  protected function getMercurialResult(ConduitAPIRequest $request) {
+    $drequest = $this->getDiffusionRequest();
+    $repository = $drequest->getRepository();
+    $offset = $request->getValue('offset');
+    $limit = $request->getValue('limit');
+
+    list($stdout) = $repository->execxLocalCommand(
+      '--debug branches');
+    $branch_info = ArcanistMercurialParser::parseMercurialBranches($stdout);
+
+    $branches = array();
+    foreach ($branch_info as $name => $info) {
+      $branch = new DiffusionBranchInformation();
+      $branch->setName($name);
+      $branch->setHeadCommitIdentifier($info['rev']);
+      $branches[] = $branch->toDictionary();
+    }
+
+    if ($offset) {
+      $branches = array_slice($branches, $offset);
+    }
+
+    if ($limit) {
+      $branches = array_slice($branches, 0, $limit);
+    }
+
+    return $branches;
+  }
+}

src/applications/diffusion/controller/DiffusionBranchTableController.php

@@ -14,11 +14,13 @@ final class DiffusionBranchTableController extends DiffusionController {
     $pager->setOffset($request->getInt('offset'));
 
     // TODO: Add support for branches that contain commit
-    $query = DiffusionBranchQuery::newFromDiffusionRequest($drequest);
-    $query->setOffset($pager->getOffset());
-    $query->setLimit($pager->getPageSize() + 1);
-    $branches = $query->loadBranches();
-
+    $branches = DiffusionBranchInformation::newFromConduit(
+      $this->callConduitWithDiffusionRequest(
+        'diffusion.branchquery',
+        array(
+          'offset' => $pager->getOffset(),
+          'limit' => $pager->getPageSize() + 1
+        )));
     $branches = $pager->sliceResults($branches);
 
     $content = null;

src/applications/diffusion/controller/DiffusionRepositoryController.php

@@ -154,12 +154,14 @@ final class DiffusionRepositoryController extends DiffusionController {
     if ($drequest->getBranch() !== null) {
       $limit = 15;
 
-      $branch_query = DiffusionBranchQuery::newFromDiffusionRequest($drequest);
-      $branch_query->setLimit($limit + 1);
-      $branches = $branch_query->loadBranches();
-
+      $branches = DiffusionBranchInformation::newFromConduit(
+        $this->callConduitWithDiffusionRequest(
+          'diffusion.branchquery',
+          array(
+            'limit' => $limit
+          )));
       if (!$branches) {
-          return null;
+        return null;
       }
 
       $more_branches = (count($branches) > $limit);

src/applications/diffusion/data/DiffusionBranchInformation.php

@@ -25,4 +25,21 @@ final class DiffusionBranchInformation {
     return $this->headCommitIdentifier;
   }
 
+  public static function newFromConduit(array $dicts) {
+    $branches = array();
+    foreach ($dicts as $dict) {
+      $branches[] = id(new DiffusionBranchInformation())
+        ->setName($dict['name'])
+        ->setHeadCommitIdentifier($dict['head_commit_identifier']);
+    }
+    return $branches;
+  }
+
+  public function toDictionary() {
+    return array(
+      'name' => $this->getName(),
+      'head_commit_identifier' => $this->getHeadCommitIdentifier()
+    );
+  }
+
 }

src/applications/diffusion/data/DiffusionGitBranch.php

@@ -1,51 +1,6 @@
 <?php
 
-final class DiffusionGitBranchQuery extends DiffusionBranchQuery {
-
-  protected function executeQuery() {
-    $drequest = $this->getRequest();
-    $repository = $drequest->getRepository();
-
-    // We need to add 1 in case we pick up HEAD.
-
-    $count = $this->getOffset() + $this->getLimit() + 1;
-
-    list($stdout) = $repository->execxLocalCommand(
-      'for-each-ref %C --sort=-creatordate --format=%s refs/remotes',
-      $count ? '--count='.(int)$count : null,
-      '%(refname:short) %(objectname)');
-
-    $branch_list = self::parseGitRemoteBranchOutput(
-      $stdout,
-      $only_this_remote = DiffusionBranchInformation::DEFAULT_GIT_REMOTE);
-
-    $branches = array();
-    foreach ($branch_list as $name => $head) {
-      if (!$repository->shouldTrackBranch($name)) {
-        continue;
-      }
-
-      $branch = new DiffusionBranchInformation();
-      $branch->setName($name);
-      $branch->setHeadCommitIdentifier($head);
-      $branches[] = $branch;
-    }
-
-    $offset = $this->getOffset();
-    if ($offset) {
-      $branches = array_slice($branches, $offset);
-    }
-
-    // We might have too many even after offset slicing, if there was no HEAD
-    // for some reason.
-    $limit = $this->getLimit();
-    if ($limit) {
-      $branches = array_slice($branches, 0, $limit);
-    }
-
-    return $branches;
-  }
-
+final class DiffusionGitBranch {
 
   /**
    * Parse the output of 'git branch -r --verbose --no-abbrev' or similar into
@@ -66,7 +21,7 @@ final class DiffusionGitBranchQuery extends DiffusionBranchQuery {
    * @param string Filter branches to those on a specific remote.
    * @return map Map of 'branch' or 'remote/branch' to hash at HEAD.
    */
-  public static function parseGitRemoteBranchOutput(
+  public static function parseRemoteBranchOutput(
     $stdout,
     $only_this_remote = null) {
     $map = array();
@@ -115,5 +70,4 @@ final class DiffusionGitBranchQuery extends DiffusionBranchQuery {
 
     return $map;
   }
-
 }

src/applications/diffusion/data/__tests__/DiffusionGitBranchTestCase.php

@@ -1,6 +1,6 @@
 <?php
 
-final class DiffusionGitBranchQueryTestCase
+final class DiffusionGitBranchTestCase
   extends PhabricatorTestCase {
 
   public function testRemoteBranchParser() {
@@ -26,7 +26,7 @@ EOTXT;
         'origin/weekend-refactoring' => '6e947ab0498b82075ca6195ac168385a11326c4b',
         'alternate/release-1.0.0'    => '9ddd5d67962dd89fa167f9989954468b6c517b87',
       ),
-      DiffusionGitBranchQuery::parseGitRemoteBranchOutput($output));
+      DiffusionGitBranch::parseRemoteBranchOutput($output));
 
     $this->assertEqual(
       array(
@@ -35,7 +35,7 @@ EOTXT;
         'master'              => '713f1fc54f9cfc830acbf6bbdb46a2883f772896',
         'weekend-refactoring' => '6e947ab0498b82075ca6195ac168385a11326c4b',
       ),
-      DiffusionGitBranchQuery::parseGitRemoteBranchOutput($output, 'origin'));
+      DiffusionGitBranch::parseRemoteBranchOutput($output, 'origin'));
   }
 
 }

src/applications/diffusion/query/branch/DiffusionBranchQuery.php

@@ -1,61 +0,0 @@
-<?php
-
-abstract class DiffusionBranchQuery {
-
-  private $request;
-  private $limit;
-  private $offset;
-
-  public function setOffset($offset) {
-    $this->offset = $offset;
-    return $this;
-  }
-
-  public function getOffset() {
-    return $this->offset;
-  }
-
-  public function setLimit($limit) {
-    $this->limit = $limit;
-    return $this;
-  }
-
-  protected function getLimit() {
-    return $this->limit;
-  }
-
-  final private function __construct() {
-    // <private>
-  }
-
-  final public static function newFromDiffusionRequest(
-    DiffusionRequest $request) {
-
-    $repository = $request->getRepository();
-
-    switch ($repository->getVersionControlSystem()) {
-      case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
-        $query = new DiffusionGitBranchQuery();
-        break;
-      case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
-        $query = new DiffusionMercurialBranchQuery();
-        break;
-      default:
-        throw new Exception("Unsupported VCS!");
-    }
-
-    $query->request = $request;
-
-    return $query;
-  }
-
-  final protected function getRequest() {
-    return $this->request;
-  }
-
-  final public function loadBranches() {
-    return $this->executeQuery();
-  }
-
-  abstract protected function executeQuery();
-}

src/applications/diffusion/query/branch/DiffusionMercurialBranchQuery.php

@@ -1,32 +0,0 @@
-<?php
-
-final class DiffusionMercurialBranchQuery extends DiffusionBranchQuery {
-
-  protected function executeQuery() {
-    $drequest = $this->getRequest();
-    $repository = $drequest->getRepository();
-
-    list($stdout) = $repository->execxLocalCommand(
-      '--debug branches');
-    $branch_info = ArcanistMercurialParser::parseMercurialBranches($stdout);
-
-    $branches = array();
-    foreach ($branch_info as $name => $info) {
-      $branch = new DiffusionBranchInformation();
-      $branch->setName($name);
-      $branch->setHeadCommitIdentifier($info['rev']);
-      $branches[] = $branch;
-    }
-
-    if ($this->getOffset()) {
-      $branches = array_slice($branches, $this->getOffset());
-    }
-
-    if ($this->getLimit()) {
-      $branches = array_slice($branches, 0, $this->getLimit());
-    }
-
-    return $branches;
-  }
-
-}

src/applications/diffusion/query/contains/DiffusionGitContainsQuery.php

@@ -10,7 +10,7 @@ final class DiffusionGitContainsQuery extends DiffusionContainsQuery {
       'branch -r --verbose --no-abbrev --contains %s',
       $request->getCommit());
 
-    return DiffusionGitBranchQuery::parseGitRemoteBranchOutput(
+    return DiffusionGitBranch::parseRemoteBranchOutput(
       $contains,
       DiffusionBranchInformation::DEFAULT_GIT_REMOTE);
   }

src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php

@@ -606,7 +606,7 @@ final class PhabricatorRepositoryPullLocalDaemon
     list($stdout) = $repository->execxLocalCommand(
       'branch -r --verbose --no-abbrev');
 
-    $branches = DiffusionGitBranchQuery::parseGitRemoteBranchOutput(
+    $branches = DiffusionGitBranch::parseRemoteBranchOutput(
       $stdout,
       $only_this_remote = DiffusionBranchInformation::DEFAULT_GIT_REMOTE);