Merge diffusion.commitbranchesquery into diffusion.branchquery

Summary:
Ref T4327. This is general cleanup since I was in this area of the code. Primarily, the Mercurial implementation here was completely broken and wrong:

  - It returned only one branch, but a commit can be present on many branches.
  - It did not account for multiple branch heads.
  - It returned a result implying the branch head pointed at the queried commit, which is no consistent or accurate.

Simplify the amount of API we're dealing with by collapsing this method into the very similar `diffusion.branchquery` method.

Test Plan: Looked at mercurial and git repositories and commits, branch information seemed correct.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4327

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

src/__phutil_library_map__.php

@@ -155,7 +155,6 @@ phutil_register_library_map(array(
     '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_browsequery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_browsequery_Method.php',
-    'ConduitAPI_diffusion_commitbranchesquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_commitbranchesquery_Method.php',
     'ConduitAPI_diffusion_commitparentsquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_commitparentsquery_Method.php',
     'ConduitAPI_diffusion_createcomment_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_createcomment_Method.php',
     'ConduitAPI_diffusion_diffquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_diffquery_Method.php',
@@ -2615,7 +2614,6 @@ phutil_register_library_map(array(
     'ConduitAPI_diffusion_abstractquery_Method' => 'ConduitAPI_diffusion_Method',
     'ConduitAPI_diffusion_branchquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',
     'ConduitAPI_diffusion_browsequery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',
-    'ConduitAPI_diffusion_commitbranchesquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',
     'ConduitAPI_diffusion_commitparentsquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',
     'ConduitAPI_diffusion_createcomment_Method' => 'ConduitAPI_diffusion_Method',
     'ConduitAPI_diffusion_diffquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',

src/applications/diffusion/conduit/ConduitAPI_diffusion_branchquery_Method.php

@@ -1,35 +1,59 @@
 <?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.';
+    return pht('Determine what branches exist for a repository.');
   }
 
   public function defineReturnType() {
-    return 'array';
+    return 'list<dict>';
   }
 
   protected function defineCustomParamTypes() {
     return array(
       'limit' => 'optional int',
-      'offset' => 'optional int'
+      'offset' => 'optional int',
+      'contains' => 'optional string',
     );
   }
 
-
   protected function getGitResult(ConduitAPIRequest $request) {
     $drequest = $this->getDiffusionRequest();
     $repository = $drequest->getRepository();
 
-    $refs = id(new DiffusionLowLevelGitRefQuery())
-      ->setRepository($repository)
-      ->withIsOriginBranch(true)
-      ->execute();
+    $contains = $request->getValue('contains');
+    if (strlen($contains)) {
+      // NOTE: We can't use DiffusionLowLevelGitRefQuery here because
+      // `git for-each-ref` does not support `--contains`.
+      if ($repository->isWorkingCopyBare()) {
+        list($stdout) = $repository->execxLocalCommand(
+          'branch --verbose --no-abbrev --contains %s --',
+          $contains);
+        $ref_map = DiffusionGitBranch::parseLocalBranchOutput(
+          $stdout);
+      } else {
+        list($stdout) = $repository->execxLocalCommand(
+          'branch -r --verbose --no-abbrev --contains %s --',
+          $contains);
+        $ref_map = DiffusionGitBranch::parseRemoteBranchOutput(
+          $stdout,
+          DiffusionGitBranch::DEFAULT_GIT_REMOTE);
+      }
+
+      $refs = array();
+      foreach ($ref_map as $ref => $commit) {
+        $refs[] = id(new DiffusionRepositoryRef())
+          ->setShortName($ref)
+          ->setCommitIdentifier($commit);
+      }
+    } else {
+      $refs = id(new DiffusionLowLevelGitRefQuery())
+        ->setRepository($repository)
+        ->withIsOriginBranch(true)
+        ->execute();
+    }
 
     return $this->processBranchRefs($request, $refs);
   }
@@ -42,6 +66,37 @@ final class ConduitAPI_diffusion_branchquery_Method
       ->setRepository($repository)
       ->execute();
 
+    // If we have a 'contains' query, filter these branches down to just the
+    // ones which contain the commit.
+    $contains = $request->getValue('contains');
+    if (strlen($contains)) {
+      list($branches_raw) = $repository->execxLocalCommand(
+        'log --template %s --limit 1 --rev %s --',
+        '{branches}',
+        hgsprintf('%s', $contains));
+
+      $branches_raw = trim($branches_raw);
+      if (!strlen($branches_raw)) {
+        $containing_branches = array('default');
+      } else {
+        $containing_branches = explode(' ', $branches_raw);
+      }
+
+      $containing_branches = array_fuse($containing_branches);
+
+      // NOTE: We get this very slightly wrong: a branch may have multiple
+      // heads and we'll keep all of the heads of the branch, even if the
+      // commit is only on some of the heads. This should be rare, is probably
+      // more clear to users as is, and would potentially be expensive to get
+      // right since we'd have to do additional checks.
+
+      foreach ($refs as $key => $ref) {
+        if (empty($containing_branches[$ref->getShortName()])) {
+          unset($refs[$key]);
+        }
+      }
+    }
+
     return $this->processBranchRefs($request, $refs);
   }
 

src/applications/diffusion/conduit/ConduitAPI_diffusion_commitbranchesquery_Method.php

@@ -1,66 +0,0 @@
-<?php
-
-/**
- * @group conduit
- */
-final class ConduitAPI_diffusion_commitbranchesquery_Method
-  extends ConduitAPI_diffusion_abstractquery_Method {
-
-  public function getMethodDescription() {
-    return 'Determine what branches contain a commit in a repository.';
-  }
-
-  public function defineReturnType() {
-    return 'array';
-  }
-
-  protected function defineCustomParamTypes() {
-    return array(
-      'commit' => 'required string',
-    );
-  }
-
-  protected function getGitResult(ConduitAPIRequest $request) {
-    $drequest = $this->getDiffusionRequest();
-    $repository = $drequest->getRepository();
-    $commit = $request->getValue('commit');
-
-    // NOTE: We can't use DiffusionLowLevelGitRefQuery here because
-    // `git for-each-ref` does not support `--contains`.
-    if ($repository->isWorkingCopyBare()) {
-      list($contains) = $repository->execxLocalCommand(
-        'branch --verbose --no-abbrev --contains %s',
-        $commit);
-      return DiffusionGitBranch::parseLocalBranchOutput(
-        $contains);
-    } else {
-      list($contains) = $repository->execxLocalCommand(
-        'branch -r --verbose --no-abbrev --contains %s',
-        $commit);
-      return DiffusionGitBranch::parseRemoteBranchOutput(
-        $contains,
-        DiffusionGitBranch::DEFAULT_GIT_REMOTE);
-    }
-  }
-
-  protected function getMercurialResult(ConduitAPIRequest $request) {
-    $drequest = $this->getDiffusionRequest();
-    $repository = $drequest->getRepository();
-    $commit = $request->getValue('commit');
-
-    // TODO: This should use `branches`. Also, this entire method's API needs
-    // to be fixed to support multiple branch heads in Mercurial. We should
-    // probably return `DiffusionRepositoryRefs` and probably merge this into
-    // `diffusion.branchesquery`.
-
-    list($contains) = $repository->execxLocalCommand(
-      'log --template %s --limit 1 --rev %s --',
-      '{branch}',
-      $commit);
-
-    return array(
-      trim($contains) => $commit,
-    );
-
-  }
-}

src/applications/diffusion/controller/DiffusionCommitBranchesController.php

@@ -17,26 +17,30 @@ final class DiffusionCommitBranchesController extends DiffusionController {
     $branches = array();
     try {
       $branches = $this->callConduitWithDiffusionRequest(
-        'diffusion.commitbranchesquery',
-        array('commit' => $request->getCommit()));
+        'diffusion.branchquery',
+        array(
+          'contains' => $request->getCommit(),
+        ));
     } catch (ConduitException $ex) {
       if ($ex->getMessage() != 'ERR-UNSUPPORTED-VCS') {
         throw $ex;
       }
     }
 
+    $branches = DiffusionRepositoryRef::loadAllFromDictionaries($branches);
+
     $branch_links = array();
-    foreach ($branches as $branch => $commit) {
+    foreach ($branches as $branch) {
       $branch_links[] = phutil_tag(
         'a',
         array(
           'href' => $request->generateURI(
             array(
               'action'  => 'browse',
-              'branch'  => $branch,
+              'branch'  => $branch->getShortName(),
             )),
         ),
-        $branch);
+        $branch->getShortName());
     }
 
     return id(new AphrontAjaxResponse())