Diffusion - move querying for stable commit name to occur over conduit

Summary: Ref T2784. This is a lower-level one from drequest so it gets the conditional initialization treatment. Consolidated SVN as well even though SVN is issuing database queries; I felt better about the code de-duplication despite the small performance hit when we could just query the DB directly in the SVN case.

Test Plan: browsed around my Phabricator repositories in Mercurial, Git, and SVN flavors. Looked good.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T2784

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

src/__phutil_library_map__.php

@@ -158,6 +158,7 @@ phutil_register_library_map(array(
     'ConduitAPI_diffusion_lastmodifiedquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_lastmodifiedquery_Method.php',
     'ConduitAPI_diffusion_rawdiffquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_rawdiffquery_Method.php',
     'ConduitAPI_diffusion_searchquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_searchquery_Method.php',
+    'ConduitAPI_diffusion_stablecommitnamequery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_stablecommitnamequery_Method.php',
     'ConduitAPI_diffusion_tagsquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_tagsquery_Method.php',
     'ConduitAPI_feed_Method' => 'applications/feed/conduit/ConduitAPI_feed_Method.php',
     'ConduitAPI_feed_publish_Method' => 'applications/feed/conduit/ConduitAPI_feed_publish_Method.php',
@@ -446,6 +447,7 @@ phutil_register_library_map(array(
     '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',
     'DiffusionHistoryController' => 'applications/diffusion/controller/DiffusionHistoryController.php',
     'DiffusionHistoryQuery' => 'applications/diffusion/query/history/DiffusionHistoryQuery.php',
     'DiffusionHistoryTableView' => 'applications/diffusion/view/DiffusionHistoryTableView.php',
@@ -465,6 +467,7 @@ phutil_register_library_map(array(
     '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',
@@ -483,6 +486,7 @@ phutil_register_library_map(array(
     'DiffusionRepositoryTag' => 'applications/diffusion/data/DiffusionRepositoryTag.php',
     'DiffusionRequest' => 'applications/diffusion/request/DiffusionRequest.php',
     'DiffusionSetupException' => 'applications/diffusion/exception/DiffusionSetupException.php',
+    'DiffusionStableCommitNameQuery' => 'applications/diffusion/query/stablecommitname/DiffusionStableCommitNameQuery.php',
     'DiffusionSvnCommitParentsQuery' => 'applications/diffusion/query/parents/DiffusionSvnCommitParentsQuery.php',
     'DiffusionSvnContainsQuery' => 'applications/diffusion/query/contains/DiffusionSvnContainsQuery.php',
     'DiffusionSvnFileContentQuery' => 'applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php',
@@ -490,6 +494,7 @@ phutil_register_library_map(array(
     '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',
     'DiffusionSymbolController' => 'applications/diffusion/controller/DiffusionSymbolController.php',
     'DiffusionSymbolQuery' => 'applications/diffusion/query/DiffusionSymbolQuery.php',
     'DiffusionTagListController' => 'applications/diffusion/controller/DiffusionTagListController.php',
@@ -1957,6 +1962,7 @@ phutil_register_library_map(array(
     'ConduitAPI_diffusion_lastmodifiedquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',
     'ConduitAPI_diffusion_rawdiffquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',
     'ConduitAPI_diffusion_searchquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',
+    'ConduitAPI_diffusion_stablecommitnamequery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',
     'ConduitAPI_diffusion_tagsquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method',
     'ConduitAPI_feed_Method' => 'ConduitAPIMethod',
     'ConduitAPI_feed_publish_Method' => 'ConduitAPI_feed_Method',
@@ -2233,6 +2239,7 @@ phutil_register_library_map(array(
     'DiffusionGitMergedCommitsQuery' => 'DiffusionMergedCommitsQuery',
     'DiffusionGitRawDiffQuery' => 'DiffusionRawDiffQuery',
     'DiffusionGitRequest' => 'DiffusionRequest',
+    'DiffusionGitStableCommitNameQuery' => 'DiffusionStableCommitNameQuery',
     'DiffusionHistoryController' => 'DiffusionController',
     'DiffusionHistoryQuery' => 'DiffusionQuery',
     'DiffusionHistoryTableView' => 'DiffusionView',
@@ -2251,6 +2258,7 @@ phutil_register_library_map(array(
     'DiffusionMercurialMergedCommitsQuery' => 'DiffusionMergedCommitsQuery',
     'DiffusionMercurialRawDiffQuery' => 'DiffusionRawDiffQuery',
     'DiffusionMercurialRequest' => 'DiffusionRequest',
+    'DiffusionMercurialStableCommitNameQuery' => 'DiffusionStableCommitNameQuery',
     'DiffusionMergedCommitsQuery' => 'DiffusionQuery',
     'DiffusionPathCompleteController' => 'DiffusionController',
     'DiffusionPathQueryTestCase' => 'PhabricatorTestCase',
@@ -2261,6 +2269,7 @@ phutil_register_library_map(array(
     'DiffusionRemarkupRule' => 'PhabricatorRemarkupRuleObject',
     'DiffusionRepositoryController' => 'DiffusionController',
     'DiffusionSetupException' => 'AphrontUsageException',
+    'DiffusionStableCommitNameQuery' => 'DiffusionQuery',
     'DiffusionSvnCommitParentsQuery' => 'DiffusionCommitParentsQuery',
     'DiffusionSvnContainsQuery' => 'DiffusionContainsQuery',
     'DiffusionSvnFileContentQuery' => 'DiffusionFileContentQuery',
@@ -2268,6 +2277,7 @@ phutil_register_library_map(array(
     'DiffusionSvnMergedCommitsQuery' => 'DiffusionMergedCommitsQuery',
     'DiffusionSvnRawDiffQuery' => 'DiffusionRawDiffQuery',
     'DiffusionSvnRequest' => 'DiffusionRequest',
+    'DiffusionSvnStableCommitNameQuery' => 'DiffusionStableCommitNameQuery',
     'DiffusionSymbolController' => 'DiffusionController',
     'DiffusionSymbolQuery' => 'PhabricatorOffsetPagedQuery',
     'DiffusionTagListController' => 'DiffusionController',

src/applications/diffusion/conduit/ConduitAPI_diffusion_stablecommitnamequery_Method.php

@@ -0,0 +1,35 @@
+<?php
+
+/**
+ * @group conduit
+ */
+final class ConduitAPI_diffusion_stablecommitnamequery_Method
+  extends ConduitAPI_diffusion_abstractquery_Method {
+
+  public function __construct() {
+    $this->setShouldCreateDiffusionRequest(false);
+  }
+
+  public function getMethodDescription() {
+    return
+      'Identifies the latest commit in a repository. Repositories with '.
+      'branch support must specify which branch to look at.';
+  }
+
+  public function defineReturnType() {
+    return 'string';
+  }
+
+  protected function defineCustomParamTypes() {
+    return array(
+      'branch' => 'required string',
+    );
+  }
+
+  protected function getResult(ConduitAPIRequest $request) {
+    $repository = $this->getRepository($request);
+    $query = DiffusionStableCommitNameQuery::newFromRepository($repository);
+    $query->setBranch($request->getValue('branch'));
+    return $query->load();
+  }
+}

src/applications/diffusion/query/stablecommitname/DiffusionGitStableCommitNameQuery.php

@@ -0,0 +1,17 @@
+<?php
+
+final class DiffusionGitStableCommitNameQuery
+extends DiffusionStableCommitNameQuery {
+
+  protected function executeQuery() {
+    $repository = $this->getRepository();
+    $branch = $this->getBranch();
+    list($stdout) = $repository->execxLocalCommand(
+      'rev-parse --verify %s/%s',
+      DiffusionBranchInformation::DEFAULT_GIT_REMOTE,
+      $branch);
+
+    $commit = trim($stdout);
+    return substr($commit, 0, 16);
+  }
+}

src/applications/diffusion/query/stablecommitname/DiffusionMercurialStableCommitNameQuery.php

@@ -0,0 +1,30 @@
+<?php
+
+final class DiffusionMercurialStableCommitNameQuery
+extends DiffusionStableCommitNameQuery {
+
+  protected function executeQuery() {
+    $repository = $this->getRepository();
+
+    // NOTE: For branches with spaces in their name like "a b", this
+    // does not work properly:
+    //
+    //   $ hg log --rev 'a b'
+    //
+    // We can use revsets instead:
+    //
+    //   $ hg log --rev branch('a b')
+    //
+    // ...but they require a somewhat newer version of Mercurial. Instead,
+    // use "-b" flag with limit 1 for greatest compatibility across
+    // versions.
+
+    list($stable_commit_name) = $repository->execxLocalCommand(
+      'log --template=%s -b %s --limit 1',
+      '{node}',
+      $this->getBranch());
+
+    return $stable_commit_name;
+  }
+
+}

src/applications/diffusion/query/stablecommitname/DiffusionStableCommitNameQuery.php

@@ -0,0 +1,35 @@
+<?php
+
+abstract class DiffusionStableCommitNameQuery extends DiffusionQuery {
+
+  private $branch;
+  private $repository;
+
+  public function setBranch($branch) {
+    $this->branch = $branch;
+    return $this;
+  }
+  public function getBranch() {
+    return $this->branch;
+  }
+
+  public function setRepository(PhabricatorRepository $repository) {
+    $this->repository = $repository;
+    return $this;
+  }
+  public function getRepository() {
+    return $this->repository;
+  }
+
+  final public static function newFromRepository(
+    PhabricatorRepository $repository) {
+
+    $obj = parent::initQueryObject(__CLASS__, $repository);
+    $obj->setRepository($repository);
+    return $obj;
+  }
+
+  final public function load() {
+    return $this->executeQuery();
+  }
+}

src/applications/diffusion/query/stablecommitname/DiffusionSvnStableCommitNameQuery.php

@@ -0,0 +1,23 @@
+<?php
+
+final class DiffusionSvnStableCommitNameQuery
+extends DiffusionStableCommitNameQuery {
+
+  protected function executeQuery() {
+    $repository = $this->getRepository();
+
+    $commit = id(new PhabricatorRepositoryCommit())
+      ->loadOneWhere(
+        'repositoryID = %d ORDER BY epoch DESC LIMIT 1',
+        $repository->getID());
+    if ($commit) {
+      $stable_commit_name = $commit->getCommitIdentifier();
+    } else {
+      // For new repositories, we may not have parsed any commits yet. Call
+      // the stable commit "1" and avoid fataling.
+      $stable_commit_name = 1;
+    }
+
+    return $stable_commit_name;
+  }
+}

src/applications/diffusion/request/DiffusionGitRequest.php

@@ -35,20 +35,4 @@ final class DiffusionGitRequest extends DiffusionRequest {
     return $remote.'/'.$this->getBranch();
   }
 
-  public function getStableCommitName() {
-    if (!$this->stableCommitName) {
-      if ($this->commit) {
-        $this->stableCommitName = $this->commit;
-      } else {
-        $branch = $this->getBranch();
-        list($stdout) = $this->getRepository()->execxLocalCommand(
-          'rev-parse --verify %s/%s',
-          DiffusionBranchInformation::DEFAULT_GIT_REMOTE,
-          $branch);
-        $this->stableCommitName = trim($stdout);
-      }
-    }
-    return substr($this->stableCommitName, 0, 16);
-  }
-
 }

src/applications/diffusion/request/DiffusionMercurialRequest.php

@@ -42,32 +42,4 @@ final class DiffusionMercurialRequest extends DiffusionRequest {
     return $this->getBranch();
   }
 
-  public function getStableCommitName() {
-    if (!$this->stableCommitName) {
-      if ($this->commit) {
-        $this->stableCommitName = $this->commit;
-      } else {
-
-        // NOTE: For branches with spaces in their name like "a b", this
-        // does not work properly:
-        //
-        //   $ hg log --rev 'a b'
-        //
-        // We can use revsets instead:
-        //
-        //   $ hg log --rev branch('a b')
-        //
-        // ...but they require a somewhat newer version of Mercurial. Instead,
-        // use "-b" flag with limit 1 for greatest compatibility across
-        // versions.
-
-        list($this->stableCommitName) = $this->repository->execxLocalCommand(
-          'log --template=%s -b %s --limit 1',
-          '{node}',
-          $this->getBranch());
-      }
-    }
-    return $this->stableCommitName;
-  }
-
 }

src/applications/diffusion/request/DiffusionRequest.php

@@ -306,6 +306,9 @@ abstract class DiffusionRequest {
    *                a symbolic commit reference.
    */
   public function getStableCommitName() {
+    if (!$this->stableCommitName) {
+      $this->queryStableCommitName();
+    }
     return $this->stableCommitName;
   }
 
@@ -628,4 +631,23 @@ abstract class DiffusionRequest {
     $this->tagContent = $commit_data['tagContent'];
   }
 
+  private function queryStableCommitName() {
+    if ($this->commit) {
+      $this->stableCommitName = $this->commit;
+    } else if ($this->shouldInitFromConduit()) {
+      $this->stableCommitName = DiffusionQuery::callConduitWithDiffusionRequest(
+        $this->getUser(),
+        $this,
+        'diffusion.stablecommitnamequery',
+        array(
+          'branch' => $this->getBranch()
+        ));
+    } else {
+      $query = DiffusionStableCommitNameQuery::newFromRepository(
+        $this->getRepository());
+      $query->setBranch($this->getBranch());
+      $this->stableCommitName = $query->load();
+    }
+    return $this->stableCommitName;
+  }
 }

src/applications/diffusion/request/DiffusionSvnRequest.php

@@ -22,28 +22,6 @@ final class DiffusionSvnRequest extends DiffusionRequest {
     return 'svn';
   }
 
-  public function getStableCommitName() {
-    if ($this->commit) {
-      return $this->commit;
-    }
-
-    if ($this->stableCommitName === null) {
-      $commit = id(new PhabricatorRepositoryCommit())
-        ->loadOneWhere(
-          'repositoryID = %d ORDER BY epoch DESC LIMIT 1',
-          $this->getRepository()->getID());
-      if ($commit) {
-        $this->stableCommitName = $commit->getCommitIdentifier();
-      } else {
-        // For new repositories, we may not have parsed any commits yet. Call
-        // the stable commit "1" and avoid fataling.
-        $this->stableCommitName = 1;
-      }
-    }
-
-    return $this->stableCommitName;
-  }
-
   public function getCommit() {
     if ($this->commit) {
       return $this->commit;