Diffusion - move merged commits query to happen over conduit.

Summary: Ref T2784. Also sneaks in a fix for branch query -- forgot to catch the unsupported VCS exception. One strange thing is I have test Phabricator repositories and the mercurial one is showing different data than git for the same commit. The data shown is consistent pre and post this diff though so its an existing issue. Also note the mercurial is an import of git so maybe its busted-ish?

Test Plan: viewed commits in mercurial, svn, and git that were merge commits. saw the right stuff in mercurial and git.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2784

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

src/__phutil_library_map__.php

@@ -158,6 +158,7 @@ phutil_register_library_map(array(
     'ConduitAPI_diffusion_getrecentcommitsbypath_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_getrecentcommitsbypath_Method.php',
     'ConduitAPI_diffusion_historyquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_historyquery_Method.php',
     'ConduitAPI_diffusion_lastmodifiedquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_lastmodifiedquery_Method.php',
+    'ConduitAPI_diffusion_mergedcommitsquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_mergedcommitsquery_Method.php',
     'ConduitAPI_diffusion_rawdiffquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_rawdiffquery_Method.php',
     'ConduitAPI_diffusion_refsquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_refsquery_Method.php',
     'ConduitAPI_diffusion_searchquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_searchquery_Method.php',
@@ -444,7 +445,6 @@ phutil_register_library_map(array(
     'DiffusionGitCommitParentsQuery' => 'applications/diffusion/query/parents/DiffusionGitCommitParentsQuery.php',
     'DiffusionGitExpandShortNameQuery' => 'applications/diffusion/query/expandshortname/DiffusionGitExpandShortNameQuery.php',
     'DiffusionGitFileContentQuery' => 'applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php',
-    'DiffusionGitMergedCommitsQuery' => 'applications/diffusion/query/mergedcommits/DiffusionGitMergedCommitsQuery.php',
     'DiffusionGitRawDiffQuery' => 'applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php',
     'DiffusionGitRequest' => 'applications/diffusion/request/DiffusionGitRequest.php',
     'DiffusionGitStableCommitNameQuery' => 'applications/diffusion/query/stablecommitname/DiffusionGitStableCommitNameQuery.php',
@@ -461,11 +461,9 @@ phutil_register_library_map(array(
     'DiffusionMercurialCommitParentsQuery' => 'applications/diffusion/query/parents/DiffusionMercurialCommitParentsQuery.php',
     'DiffusionMercurialExpandShortNameQuery' => 'applications/diffusion/query/expandshortname/DiffusionMercurialExpandShortNameQuery.php',
     'DiffusionMercurialFileContentQuery' => 'applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php',
-    'DiffusionMercurialMergedCommitsQuery' => 'applications/diffusion/query/mergedcommits/DiffusionMercurialMergedCommitsQuery.php',
     'DiffusionMercurialRawDiffQuery' => 'applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php',
     'DiffusionMercurialRequest' => 'applications/diffusion/request/DiffusionMercurialRequest.php',
     'DiffusionMercurialStableCommitNameQuery' => 'applications/diffusion/query/stablecommitname/DiffusionMercurialStableCommitNameQuery.php',
-    'DiffusionMergedCommitsQuery' => 'applications/diffusion/query/mergedcommits/DiffusionMergedCommitsQuery.php',
     'DiffusionPathChange' => 'applications/diffusion/data/DiffusionPathChange.php',
     'DiffusionPathChangeQuery' => 'applications/diffusion/query/pathchange/DiffusionPathChangeQuery.php',
     'DiffusionPathCompleteController' => 'applications/diffusion/controller/DiffusionPathCompleteController.php',
@@ -486,7 +484,6 @@ phutil_register_library_map(array(
     'DiffusionStableCommitNameQuery' => 'applications/diffusion/query/stablecommitname/DiffusionStableCommitNameQuery.php',
     'DiffusionSvnCommitParentsQuery' => 'applications/diffusion/query/parents/DiffusionSvnCommitParentsQuery.php',
     'DiffusionSvnFileContentQuery' => 'applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php',
-    'DiffusionSvnMergedCommitsQuery' => 'applications/diffusion/query/mergedcommits/DiffusionSvnMergedCommitsQuery.php',
     'DiffusionSvnRawDiffQuery' => 'applications/diffusion/query/rawdiff/DiffusionSvnRawDiffQuery.php',
     'DiffusionSvnRequest' => 'applications/diffusion/request/DiffusionSvnRequest.php',
     'DiffusionSvnStableCommitNameQuery' => 'applications/diffusion/query/stablecommitname/DiffusionSvnStableCommitNameQuery.php',
@@ -1968,6 +1965,7 @@ phutil_register_library_map(array(
     'ConduitAPI_diffusion_getrecentcommitsbypath_Method' => 'ConduitAPI_diffusion_Method',
     'ConduitAPI_diffusion_historyquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',
     'ConduitAPI_diffusion_lastmodifiedquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',
+    'ConduitAPI_diffusion_mergedcommitsquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',
     'ConduitAPI_diffusion_rawdiffquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',
     'ConduitAPI_diffusion_refsquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',
     'ConduitAPI_diffusion_searchquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',
@@ -2242,7 +2240,6 @@ phutil_register_library_map(array(
     'DiffusionGitCommitParentsQuery' => 'DiffusionCommitParentsQuery',
     'DiffusionGitExpandShortNameQuery' => 'DiffusionExpandShortNameQuery',
     'DiffusionGitFileContentQuery' => 'DiffusionFileContentQuery',
-    'DiffusionGitMergedCommitsQuery' => 'DiffusionMergedCommitsQuery',
     'DiffusionGitRawDiffQuery' => 'DiffusionRawDiffQuery',
     'DiffusionGitRequest' => 'DiffusionRequest',
     'DiffusionGitStableCommitNameQuery' => 'DiffusionStableCommitNameQuery',
@@ -2258,11 +2255,9 @@ phutil_register_library_map(array(
     'DiffusionMercurialCommitParentsQuery' => 'DiffusionCommitParentsQuery',
     'DiffusionMercurialExpandShortNameQuery' => 'DiffusionExpandShortNameQuery',
     'DiffusionMercurialFileContentQuery' => 'DiffusionFileContentQuery',
-    'DiffusionMercurialMergedCommitsQuery' => 'DiffusionMergedCommitsQuery',
     'DiffusionMercurialRawDiffQuery' => 'DiffusionRawDiffQuery',
     'DiffusionMercurialRequest' => 'DiffusionRequest',
     'DiffusionMercurialStableCommitNameQuery' => 'DiffusionStableCommitNameQuery',
-    'DiffusionMergedCommitsQuery' => 'DiffusionQuery',
     'DiffusionPathCompleteController' => 'DiffusionController',
     'DiffusionPathQueryTestCase' => 'PhabricatorTestCase',
     'DiffusionPathValidateController' => 'DiffusionController',
@@ -2275,7 +2270,6 @@ phutil_register_library_map(array(
     'DiffusionStableCommitNameQuery' => 'DiffusionQuery',
     'DiffusionSvnCommitParentsQuery' => 'DiffusionCommitParentsQuery',
     'DiffusionSvnFileContentQuery' => 'DiffusionFileContentQuery',
-    'DiffusionSvnMergedCommitsQuery' => 'DiffusionMergedCommitsQuery',
     'DiffusionSvnRawDiffQuery' => 'DiffusionRawDiffQuery',
     'DiffusionSvnRequest' => 'DiffusionRequest',
     'DiffusionSvnStableCommitNameQuery' => 'DiffusionStableCommitNameQuery',

src/applications/diffusion/conduit/ConduitAPI_diffusion_mergedcommitsquery_Method.php

@@ -0,0 +1,107 @@
+<?php
+
+/**
+ * @group conduit
+ */
+final class ConduitAPI_diffusion_mergedcommitsquery_Method
+extends ConduitAPI_diffusion_abstractquery_Method {
+
+  public function getMethodDescription() {
+    return
+      'Merged commit information for a specific commit in a repository.';
+  }
+
+  public function defineReturnType() {
+    return 'array';
+  }
+
+  protected function defineCustomParamTypes() {
+    return array(
+      'commit' => 'required string',
+      'limit' => 'optional int',
+    );
+  }
+
+  private function getLimit(ConduitAPIRequest $request) {
+    return $request->getValue('limit', PHP_INT_MAX);
+  }
+
+  protected function getGitResult(ConduitAPIRequest $request) {
+    $drequest = $this->getDiffusionRequest();
+    $repository = $drequest->getRepository();
+    $commit = $request->getValue('commit');
+    $limit = $this->getLimit($request);
+
+    list($parents) = $repository->execxLocalCommand(
+      'log -n 1 --format=%s %s',
+      '%P',
+      $commit);
+
+    $parents = preg_split('/\s+/', trim($parents));
+    if (count($parents) < 2) {
+      // This is not a merge commit, so it doesn't merge anything.
+      return array();
+    }
+
+    // Get all of the commits which are not reachable from the first parent.
+    // These are the commits this change merges.
+
+    $first_parent = head($parents);
+    list($logs) = $repository->execxLocalCommand(
+      'log -n %d --format=%s %s %s --',
+      // NOTE: "+ 1" accounts for the merge commit itself.
+      $limit + 1,
+      '%H',
+      $commit,
+      '^'.$first_parent);
+
+    $hashes = explode("\n", trim($logs));
+
+    // Remove the merge commit.
+    $hashes = array_diff($hashes, array($commit));
+
+    return DiffusionQuery::loadHistoryForCommitIdentifiers(
+      $hashes,
+      $drequest);
+   }
+
+  protected function getMercurialResult(ConduitAPIRequest $request) {
+    $drequest = $this->getDiffusionRequest();
+    $repository = $drequest->getRepository();
+    $commit = $request->getValue('commit');
+    $limit = $this->getLimit($request);
+
+    list($parents) = $repository->execxLocalCommand(
+      'parents --template=%s --rev %s',
+      '{node}\\n',
+      $commit);
+    $parents = explode("\n", trim($parents));
+
+    if (count($parents) < 2) {
+      // Not a merge commit.
+      return array();
+    }
+
+    // NOTE: In Git, the first parent is the "mainline". In Mercurial, the
+    // second parent is the "mainline" (the way 'git merge' and 'hg merge'
+    // work is also reversed).
+
+    $last_parent = last($parents);
+    list($logs) = $repository->execxLocalCommand(
+      'log --template=%s --follow --limit %d --rev %s:0 --prune %s --',
+      '{node}\\n',
+      $limit + 1,
+      $commit,
+      $last_parent);
+
+    $hashes = explode("\n", trim($logs));
+
+    // Remove the merge commit.
+    $hashes = array_diff($hashes, array($commit));
+
+    return DiffusionQuery::loadHistoryForCommitIdentifiers(
+      $hashes,
+      $drequest);
+  }
+
+}

src/applications/diffusion/controller/DiffusionCommitBranchesController.php

@@ -10,9 +10,16 @@ final class DiffusionCommitBranchesController extends DiffusionController {
   public function processRequest() {
     $request = $this->getDiffusionRequest();
 
-    $branches = $this->callConduitWithDiffusionRequest(
-      'diffusion.commitbranchesquery',
-      array('commit' => $request->getCommit()));
+    $branches = array();
+    try {
+      $branches = $this->callConduitWithDiffusionRequest(
+        'diffusion.commitbranchesquery',
+        array('commit' => $request->getCommit()));
+    } catch (ConduitException $ex) {
+      if ($ex->getMessage() != 'ERR-UNSUPPORTED-VCS') {
+        throw $ex;
+      }
+    }
 
     $branch_links = array();
     foreach ($branches as $branch => $commit) {

src/applications/diffusion/controller/DiffusionCommitController.php

@@ -803,13 +803,20 @@ final class DiffusionCommitController extends DiffusionController {
 
   private function buildMergesTable(PhabricatorRepositoryCommit $commit) {
     $drequest = $this->getDiffusionRequest();
-
     $limit = 50;
 
-    $merge_query = DiffusionMergedCommitsQuery::newFromDiffusionRequest(
-      $drequest);
-    $merge_query->setLimit($limit + 1);
-    $merges = $merge_query->loadMergedCommits();
+    $merges = array();
+    try {
+      $merges = $this->callConduitWithDiffusionRequest(
+        'diffusion.mergedcommitsquery',
+        array(
+          'commit' => $drequest->getCommit(),
+          'limit' => $limit + 1));
+    } catch (ConduitException $ex) {
+      if ($ex->getMessage() != 'ERR-UNSUPPORTED-VCS') {
+        throw $ex;
+      }
+    }
 
     if (!$merges) {
       return null;

src/applications/diffusion/query/mergedcommits/DiffusionGitMergedCommitsQuery.php

@@ -1,41 +0,0 @@
-<?php
-
-final class DiffusionGitMergedCommitsQuery extends DiffusionMergedCommitsQuery {
-
-  protected function executeQuery() {
-    $request = $this->getRequest();
-    $repository = $request->getRepository();
-
-    list($parents) = $repository->execxLocalCommand(
-      'log -n 1 --format=%s %s',
-      '%P',
-      $request->getCommit());
-    $parents = preg_split('/\s+/', trim($parents));
-    if (count($parents) < 2) {
-      // This is not a merge commit, so it doesn't merge anything.
-      return array();
-    }
-
-    // Get all of the commits which are not reachable from the first parent.
-    // These are the commits this change merges.
-
-    $first_parent = head($parents);
-    list($logs) = $repository->execxLocalCommand(
-      'log -n %d --format=%s %s %s --',
-      // NOTE: "+ 1" accounts for the merge commit itself.
-      $this->getLimit() + 1,
-      '%H',
-      $request->getCommit(),
-      '^'.$first_parent);
-
-    $hashes = explode("\n", trim($logs));
-
-    // Remove the merge commit.
-    $hashes = array_diff($hashes, array($request->getCommit()));
-
-    return DiffusionQuery::loadHistoryForCommitIdentifiers(
-      $hashes,
-      $request);
-  }
-
-}

src/applications/diffusion/query/mergedcommits/DiffusionMercurialMergedCommitsQuery.php

@@ -1,43 +0,0 @@
-<?php
-
-final class DiffusionMercurialMergedCommitsQuery
-  extends DiffusionMergedCommitsQuery {
-
-  protected function executeQuery() {
-    $request = $this->getRequest();
-    $repository = $request->getRepository();
-
-    list($parents) = $repository->execxLocalCommand(
-      'parents --template=%s --rev %s',
-      '{node}\\n',
-      $request->getCommit());
-    $parents = explode("\n", trim($parents));
-
-    if (count($parents) < 2) {
-      // Not a merge commit.
-      return array();
-    }
-
-    // NOTE: In Git, the first parent is the "mainline". In Mercurial, the
-    // second parent is the "mainline" (the way 'git merge' and 'hg merge'
-    // work is also reversed).
-
-    $last_parent = last($parents);
-    list($logs) = $repository->execxLocalCommand(
-      'log --template=%s --follow --limit %d --rev %s:0 --prune %s --',
-      '{node}\\n',
-      $this->getLimit() + 1,
-      $request->getCommit(),
-      $last_parent);
-
-    $hashes = explode("\n", trim($logs));
-
-    // Remove the merge commit.
-    $hashes = array_diff($hashes, array($request->getCommit()));
-
-    return DiffusionQuery::loadHistoryForCommitIdentifiers(
-      $hashes,
-      $request);
-  }
-
-}

src/applications/diffusion/query/mergedcommits/DiffusionMergedCommitsQuery.php

@@ -1,26 +0,0 @@
-<?php
-
-abstract class DiffusionMergedCommitsQuery extends DiffusionQuery {
-
-  private $limit = PHP_INT_MAX;
-
-  final public static function newFromDiffusionRequest(
-    DiffusionRequest $request) {
-
-    return self::newQueryObject(__CLASS__, $request);
-  }
-
-  final public function loadMergedCommits() {
-    return $this->executeQuery();
-  }
-
-  final public function setLimit($limit) {
-    $this->limit = $limit;
-    return $this;
-  }
-
-  final public function getLimit() {
-    return $this->limit;
-  }
-
-}

src/applications/diffusion/query/mergedcommits/DiffusionSvnMergedCommitsQuery.php

@@ -1,11 +0,0 @@
-<?php
-
-final class DiffusionSvnMergedCommitsQuery extends DiffusionMergedCommitsQuery {
-
-  protected function executeQuery() {
-    // TODO: It might be possible to do something reasonable in recent versions
-    // of SVN.
-    return array();
-  }
-
-}