Modernize Diffusion commitparentsquery

Summary: Ref T4195. Ref T2783. We have an old-school implementation of this; move it into a LowLevel query and make callers all run through Conduit. I need the LowLevel query for hooks, to implement an "is merge commit" Herald rule.

Test Plan:
  - Ran query via Conduit for SVN, Mercurial, Git.
  - Parsed a commit which closed a revision, attach/closed worked correctly.
  - Browsed Diffusion.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195, T2783

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

src/__phutil_library_map__.php

@@ -481,7 +481,6 @@ phutil_register_library_map(array(
     'DiffusionCommitHash' => 'applications/diffusion/data/DiffusionCommitHash.php',
     'DiffusionCommitHookEngine' => 'applications/diffusion/engine/DiffusionCommitHookEngine.php',
     'DiffusionCommitHookRejectException' => 'applications/diffusion/exception/DiffusionCommitHookRejectException.php',
-    'DiffusionCommitParentsQuery' => 'applications/diffusion/query/parents/DiffusionCommitParentsQuery.php',
     'DiffusionCommitQuery' => 'applications/diffusion/query/DiffusionCommitQuery.php',
     'DiffusionCommitRef' => 'applications/diffusion/data/DiffusionCommitRef.php',
     'DiffusionCommitTagsController' => 'applications/diffusion/controller/DiffusionCommitTagsController.php',
@@ -494,7 +493,6 @@ phutil_register_library_map(array(
     'DiffusionFileContentQuery' => 'applications/diffusion/query/filecontent/DiffusionFileContentQuery.php',
     'DiffusionGitBranch' => 'applications/diffusion/data/DiffusionGitBranch.php',
     'DiffusionGitBranchTestCase' => 'applications/diffusion/data/__tests__/DiffusionGitBranchTestCase.php',
-    'DiffusionGitCommitParentsQuery' => 'applications/diffusion/query/parents/DiffusionGitCommitParentsQuery.php',
     'DiffusionGitFileContentQuery' => 'applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php',
     'DiffusionGitRawDiffQuery' => 'applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php',
     'DiffusionGitRequest' => 'applications/diffusion/request/DiffusionGitRequest.php',
@@ -512,9 +510,9 @@ phutil_register_library_map(array(
     'DiffusionLowLevelCommitQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelCommitQuery.php',
     'DiffusionLowLevelGitRefQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelGitRefQuery.php',
     'DiffusionLowLevelMercurialBranchesQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialBranchesQuery.php',
+    'DiffusionLowLevelParentsQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php',
     'DiffusionLowLevelQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelQuery.php',
     'DiffusionLowLevelResolveRefsQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php',
-    'DiffusionMercurialCommitParentsQuery' => 'applications/diffusion/query/parents/DiffusionMercurialCommitParentsQuery.php',
     'DiffusionMercurialFileContentQuery' => 'applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php',
     'DiffusionMercurialRawDiffQuery' => 'applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php',
     'DiffusionMercurialRequest' => 'applications/diffusion/request/DiffusionMercurialRequest.php',
@@ -572,7 +570,6 @@ phutil_register_library_map(array(
     'DiffusionSetupException' => 'applications/diffusion/exception/DiffusionSetupException.php',
     'DiffusionSubversionWireProtocol' => 'applications/diffusion/protocol/DiffusionSubversionWireProtocol.php',
     'DiffusionSubversionWireProtocolTestCase' => 'applications/diffusion/protocol/__tests__/DiffusionSubversionWireProtocolTestCase.php',
-    'DiffusionSvnCommitParentsQuery' => 'applications/diffusion/query/parents/DiffusionSvnCommitParentsQuery.php',
     'DiffusionSvnFileContentQuery' => 'applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php',
     'DiffusionSvnRawDiffQuery' => 'applications/diffusion/query/rawdiff/DiffusionSvnRawDiffQuery.php',
     'DiffusionSvnRequest' => 'applications/diffusion/request/DiffusionSvnRequest.php',
@@ -2871,7 +2868,6 @@ phutil_register_library_map(array(
     'DiffusionCommitHash' => 'Phobject',
     'DiffusionCommitHookEngine' => 'Phobject',
     'DiffusionCommitHookRejectException' => 'Exception',
-    'DiffusionCommitParentsQuery' => 'DiffusionQuery',
     'DiffusionCommitQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'DiffusionCommitRef' => 'Phobject',
     'DiffusionCommitTagsController' => 'DiffusionController',
@@ -2882,7 +2878,6 @@ phutil_register_library_map(array(
     'DiffusionExternalController' => 'DiffusionController',
     'DiffusionFileContentQuery' => 'DiffusionQuery',
     'DiffusionGitBranchTestCase' => 'PhabricatorTestCase',
-    'DiffusionGitCommitParentsQuery' => 'DiffusionCommitParentsQuery',
     'DiffusionGitFileContentQuery' => 'DiffusionFileContentQuery',
     'DiffusionGitRawDiffQuery' => 'DiffusionRawDiffQuery',
     'DiffusionGitRequest' => 'DiffusionRequest',
@@ -2899,9 +2894,9 @@ phutil_register_library_map(array(
     'DiffusionLowLevelCommitQuery' => 'DiffusionLowLevelQuery',
     'DiffusionLowLevelGitRefQuery' => 'DiffusionLowLevelQuery',
     'DiffusionLowLevelMercurialBranchesQuery' => 'DiffusionLowLevelQuery',
+    'DiffusionLowLevelParentsQuery' => 'DiffusionLowLevelQuery',
     'DiffusionLowLevelQuery' => 'Phobject',
     'DiffusionLowLevelResolveRefsQuery' => 'DiffusionLowLevelQuery',
-    'DiffusionMercurialCommitParentsQuery' => 'DiffusionCommitParentsQuery',
     'DiffusionMercurialFileContentQuery' => 'DiffusionFileContentQuery',
     'DiffusionMercurialRawDiffQuery' => 'DiffusionRawDiffQuery',
     'DiffusionMercurialRequest' => 'DiffusionRequest',
@@ -2957,7 +2952,6 @@ phutil_register_library_map(array(
     'DiffusionSetupException' => 'AphrontUsageException',
     'DiffusionSubversionWireProtocol' => 'Phobject',
     'DiffusionSubversionWireProtocolTestCase' => 'PhabricatorTestCase',
-    'DiffusionSvnCommitParentsQuery' => 'DiffusionCommitParentsQuery',
     'DiffusionSvnFileContentQuery' => 'DiffusionFileContentQuery',
     'DiffusionSvnRawDiffQuery' => 'DiffusionRawDiffQuery',
     'DiffusionSvnRequest' => 'DiffusionRequest',

src/applications/diffusion/conduit/ConduitAPI_diffusion_commitparentsquery_Method.php

@@ -7,12 +7,12 @@ final class ConduitAPI_diffusion_commitparentsquery_Method
   extends ConduitAPI_diffusion_abstractquery_Method {
 
   public function getMethodDescription() {
-    return
+    return pht(
-      'Commit parent(s) information for a commit in a repository.';
+      "Get the commit identifiers for a commit's parent or parents.");
   }
 
   public function defineReturnType() {
-    return 'array';
+    return 'list<string>';
   }
 
   protected function defineCustomParamTypes() {
@@ -22,10 +22,12 @@ final class ConduitAPI_diffusion_commitparentsquery_Method
   }
 
   protected function getResult(ConduitAPIRequest $request) {
-    $drequest = $this->getDiffusionRequest();
+    $repository = $this->getRepository($request);
 
-    $query = DiffusionCommitParentsQuery::newFromDiffusionRequest($drequest);
+    return id(new DiffusionLowLevelParentsQuery())
-    $parents = $query->loadParents();
+      ->setRepository($repository)
-    return $parents;
+      ->withIdentifier($request->getValue('commit'))
+      ->execute();
   }
+
 }

src/applications/diffusion/controller/DiffusionBrowseFileController.php

@@ -857,17 +857,16 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
     // NOTE: We need to get the grandparent so we can capture filename changes
     // in the parent.
 
-    $parent = $this->loadParentRevisionOf($before);
+    $parent = $this->loadParentCommitOf($before);
     $old_filename = null;
     $was_created = false;
     if ($parent) {
-      $grandparent = $this->loadParentRevisionOf(
+      $grandparent = $this->loadParentCommitOf($parent);
-        $parent->getCommitIdentifier());
 
       if ($grandparent) {
         $rename_query = new DiffusionRenameHistoryQuery();
         $rename_query->setRequest($drequest);
-        $rename_query->setOldCommit($grandparent->getCommitIdentifier());
+        $rename_query->setOldCommit($grandparent);
         $rename_query->setViewer($request->getUser());
         $old_filename = $rename_query->loadOldFilename();
         $was_created = $rename_query->getWasCreated();
@@ -955,7 +954,7 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
     return $line;
   }
 
-  private function loadParentRevisionOf($commit) {
+  private function loadParentCommitOf($commit) {
     $drequest = $this->getDiffusionRequest();
     $user = $this->getRequest()->getUser();
 
@@ -963,7 +962,7 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
       array(
         'user' => $user,
         'repository' => $drequest->getRepository(),
-        'commit'     => $commit,
+        'commit' => $commit,
       ));
 
     $parents = DiffusionQuery::callConduitWithDiffusionRequest(
@@ -971,7 +970,8 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController {
       $before_req,
       'diffusion.commitparentsquery',
       array(
-        'commit' => $commit));
+        'commit' => $commit,
+      ));
 
     return head($parents);
   }

src/applications/diffusion/controller/DiffusionCommitController.php

@@ -102,6 +102,14 @@ final class DiffusionCommitController extends DiffusionController {
         'diffusion.commitparentsquery',
         array('commit' => $drequest->getCommit()));
 
+      if ($parents) {
+        $parents = id(new DiffusionCommitQuery())
+          ->setViewer($user)
+          ->withRepository($repository)
+          ->withIdentifiers($parents)
+          ->execute();
+      }
+
       $headsup_view = id(new PHUIHeaderView())
         ->setHeader(nonempty($commit->getSummary(), pht('Commit Detail')));
 

src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php

@@ -0,0 +1,84 @@
+<?php
+
+final class DiffusionLowLevelParentsQuery
+  extends DiffusionLowLevelQuery {
+
+  private $identifier;
+
+  public function withIdentifier($identifier) {
+    $this->identifier = $identifier;
+    return $this;
+  }
+
+  public function executeQuery() {
+    if (!strlen($this->identifier)) {
+      throw new Exception(
+        pht('You must provide an identifier with withIdentifier()!'));
+    }
+
+    $type = $this->getRepository()->getVersionControlSystem();
+    switch ($type) {
+      case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
+        $result = $this->loadGitParents();
+        break;
+      case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
+        $result = $this->loadMercurialParents();
+        break;
+      case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
+        $result = $this->loadSubversionParents();
+        break;
+      default:
+        throw new Exception(pht('Unsupported repository type "%s"!', $type));
+    }
+
+    return $result;
+  }
+
+  private function loadGitParents() {
+    $repository = $this->getRepository();
+
+    list($stdout) = $repository->execxLocalCommand(
+      'log -n 1 --format=%s %s',
+      '%P',
+      $this->identifier);
+
+    return preg_split('/\s+/', trim($stdout));
+  }
+
+  private function loadMercurialParents() {
+    $repository = $this->getRepository();
+
+    list($stdout) = $repository->execxLocalCommand(
+      'log --debug --limit 1 --template={parents} --rev %s',
+      $this->identifier);
+    $stdout = PhabricatorRepository::filterMercurialDebugOutput($stdout);
+
+    $hashes = preg_split('/\s+/', trim($stdout));
+    foreach ($hashes as $key => $value) {
+      // Mercurial parents look like "23:ad9f769d6f786fad9f76d9a" -- we want
+      // to strip out the local rev part.
+      list($local, $global) = explode(':', $value);
+      $hashes[$key] = $global;
+
+      // With --debug we get 40-character hashes but also get the "000000..."
+      // hash for missing parents; ignore it.
+      if (preg_match('/^0+$/', $global)) {
+        unset($hashes[$key]);
+      }
+    }
+
+    return $hashes;
+  }
+
+  private function loadSubversionParents() {
+    $n = (int)$this->identifier;
+    if ($n > 1) {
+      $ids = array($n - 1);
+    } else {
+      $ids = array();
+    }
+
+    return $ids;
+  }
+
+}

src/applications/diffusion/query/parents/DiffusionCommitParentsQuery.php

@@ -1,14 +0,0 @@
-<?php
-
-abstract class DiffusionCommitParentsQuery extends DiffusionQuery {
-
-  final public static function newFromDiffusionRequest(
-    DiffusionRequest $request) {
-    return self::newQueryObject(__CLASS__, $request);
-  }
-
-  final public function loadParents() {
-    return $this->executeQuery();
-  }
-
-}

src/applications/diffusion/query/parents/DiffusionGitCommitParentsQuery.php

@@ -1,19 +0,0 @@
-<?php
-
-final class DiffusionGitCommitParentsQuery
-  extends DiffusionCommitParentsQuery {
-
-  protected function executeQuery() {
-    $drequest = $this->getRequest();
-    $repository = $drequest->getRepository();
-
-    list($stdout) = $repository->execxLocalCommand(
-      'log -n 1 --format=%s %s',
-      '%P',
-      $drequest->getStableCommitName());
-
-    $hashes = preg_split('/\s+/', trim($stdout));
-
-    return self::loadCommitsByIdentifiers($hashes, $drequest);
-  }
-}

src/applications/diffusion/query/parents/DiffusionMercurialCommitParentsQuery.php

@@ -1,31 +0,0 @@
-<?php
-
-final class DiffusionMercurialCommitParentsQuery
-  extends DiffusionCommitParentsQuery {
-
-  protected function executeQuery() {
-    $drequest = $this->getRequest();
-    $repository = $drequest->getRepository();
-
-    list($stdout) = $repository->execxLocalCommand(
-      'log --debug --limit 1 --template={parents} --rev %s',
-      $drequest->getStableCommitName());
-    $stdout = PhabricatorRepository::filterMercurialDebugOutput($stdout);
-
-    $hashes = preg_split('/\s+/', trim($stdout));
-    foreach ($hashes as $key => $value) {
-      // Mercurial parents look like "23:ad9f769d6f786fad9f76d9a" -- we want
-      // to strip out the local rev part.
-      list($local, $global) = explode(':', $value);
-      $hashes[$key] = $global;
-
-      // With --debug we get 40-character hashes but also get the "000000..."
-      // hash for missing parents; ignore it.
-      if (preg_match('/^0+$/', $global)) {
-        unset($hashes[$key]);
-      }
-    }
-
-    return self::loadCommitsByIdentifiers($hashes, $drequest);
-  }
-}

src/applications/diffusion/query/parents/DiffusionSvnCommitParentsQuery.php

@@ -1,22 +0,0 @@
-<?php
-
-final class DiffusionSvnCommitParentsQuery
-  extends DiffusionCommitParentsQuery {
-
-  protected function executeQuery() {
-    $drequest = $this->getRequest();
-    $repository = $drequest->getRepository();
-
-    // TODO: With merge properties in recent versions of SVN, can we do
-    // a better job of this?
-
-    $n = $drequest->getStableCommitName();
-    if ($n > 1) {
-      $ids = array($n - 1);
-    } else {
-      $ids = array();
-    }
-
-    return self::loadCommitsByIdentifiers($ids, $drequest);
-  }
-}

src/applications/harbormaster/step/WaitForPreviousBuildStepImplementation.php

@@ -82,15 +82,10 @@ final class WaitForPreviousBuildStepImplementation
     $call->setUser(PhabricatorUser::getOmnipotentUser());
     $parents = $call->execute();
 
-    $hashes = array();
-    foreach ($parents as $parent => $obj) {
-      $hashes[] = $parent;
-    }
-
     $parents = id(new DiffusionCommitQuery())
       ->setViewer(PhabricatorUser::getOmnipotentUser())
       ->withRepository($commit->getRepository())
-      ->withIdentifiers($hashes)
+      ->withIdentifiers($parents)
       ->execute();
 
     $blockers = array();

src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php

@@ -205,15 +205,20 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
     DifferentialRevision $revision,
     $actor_phid) {
 
+    $viewer = PhabricatorUser::getOmnipotentUser();
+
     $drequest = DiffusionRequest::newFromDictionary(array(
-      'user' => PhabricatorUser::getOmnipotentUser(),
+      'user' => $viewer,
-      'initFromConduit' => false,
       'repository' => $this->repository,
-      'commit' => $this->commit->getCommitIdentifier(),
     ));
 
-    $raw_diff = DiffusionRawDiffQuery::newFromDiffusionRequest($drequest)
+    $raw_diff = DiffusionQuery::callConduitWithDiffusionRequest(
-      ->loadRawDiff();
+      $viewer,
+      $drequest,
+      'diffusion.rawdiffquery',
+      array(
+        'commit' => $this->commit->getCommitIdentifier(),
+      ));
 
     // TODO: Support adds, deletes and moves under SVN.
     if (strlen($raw_diff)) {
@@ -248,10 +253,15 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
       $diff->setArcanistProjectPHID($arcanist_project->getPHID());
     }
 
-    $parents = DiffusionCommitParentsQuery::newFromDiffusionRequest($drequest)
+    $parents = DiffusionQuery::callConduitWithDiffusionRequest(
-      ->loadParents();
+      $viewer,
+      $drequest,
+      'diffusion.commitparentsquery',
+      array(
+        'commit' => $this->commit->getCommitIdentifier(),
+      ));
     if ($parents) {
-      $diff->setSourceControlBaseRevision(head_key($parents));
+      $diff->setSourceControlBaseRevision(head($parents));
     }
 
     // TODO: Attach binary files.