Improve clarity of commit and symbol handling in DiffusionRequest

Summary:
Ref T2683. Currently, DiffusionRequest has four different "commitey" things:

  - `commit`
  - `rawCommit`
  - `symbolicCommit`
  - `stableCommit`

Of these, only two are actually distinct, useful values: `symbolicCommit` (which holds the value the request originally contained, if one existed) and `stableCommit` (which resolves that value, or the value implied by its omission, into a stable, permanent commit identifier).

  - `rawCommit` is equivalent to `symbolicCommit` and can be simply removed.
  - `commit` has some sketchy magic around it that needs to be pulled out before it can be jettisoned.

Test Plan: Viewed SVN, Git, and Mercurial repositories. Viewed brwose/history/change/tag/branch/etc views.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2683

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

src/applications/diffusion/conduit/ConduitAPI_diffusion_tagsquery_Method.php

@@ -24,7 +24,7 @@ final class ConduitAPI_diffusion_tagsquery_Method
   protected function getGitResult(ConduitAPIRequest $request) {
     $drequest = $this->getDiffusionRequest();
     $repository = $drequest->getRepository();
-    $commit = $drequest->getRawCommit();
+    $commit = $drequest->getSymbolicCommit();
 
     $commit_filter = null;
     if ($commit) {

src/applications/diffusion/controller/DiffusionBrowseController.php

@@ -96,7 +96,7 @@ abstract class DiffusionBrowseController extends DiffusionController {
         ->setHref($history_uri)
         ->setIcon('fa-list'));
 
-    $behind_head = $drequest->getRawCommit();
+    $behind_head = $drequest->getSymbolicCommit();
     $head_uri = $drequest->generateURI(
       array(
         'commit' => '',

src/applications/diffusion/controller/DiffusionChangeController.php

@@ -31,7 +31,7 @@ final class DiffusionChangeController extends DiffusionController {
 
     $repository = $drequest->getRepository();
     $callsign = $repository->getCallsign();
-    $commit = $drequest->getRawCommit();
+    $commit = $drequest->getSymbolicCommit();
     $changesets = array(
       0 => $changeset,
     );
@@ -53,7 +53,7 @@ final class DiffusionChangeController extends DiffusionController {
     );
 
     $right_uri = $drequest->generateURI($raw_params);
-    $raw_params['params']['before'] = $drequest->getRawCommit();
+    $raw_params['params']['before'] = $drequest->getStableCommit();
     $left_uri = $drequest->generateURI($raw_params);
     $changeset_view->setRawFileURIs($left_uri, $right_uri);
 

src/applications/diffusion/controller/DiffusionCommitController.php

@@ -21,6 +21,7 @@ final class DiffusionCommitController extends DiffusionController {
 
   public function processRequest() {
     $drequest = $this->getDiffusionRequest();
+
     $request = $this->getRequest();
     $user = $request->getUser();
 
@@ -32,7 +33,6 @@ final class DiffusionCommitController extends DiffusionController {
     $callsign = $repository->getCallsign();
 
     $content = array();
-
     $commit = id(new DiffusionCommitQuery())
       ->setViewer($request->getUser())
       ->withRepository($repository)

src/applications/diffusion/controller/DiffusionController.php

@@ -98,17 +98,17 @@ abstract class DiffusionController extends PhabricatorController {
         )));
     $crumb_list[] = $crumb;
 
-    $raw_commit = $drequest->getRawCommit();
+    $stable_commit = $drequest->getStableCommit();
 
     if ($spec['tags']) {
       $crumb = new PhabricatorCrumbView();
       if ($spec['commit']) {
         $crumb->setName(
-          pht("Tags for %s", 'r'.$callsign.$raw_commit));
+          pht("Tags for %s", 'r'.$callsign.$stable_commit));
         $crumb->setHref($drequest->generateURI(
           array(
             'action' => 'commit',
-            'commit' => $raw_commit,
+            'commit' => $drequest->getStableCommit(),
           )));
       } else {
         $crumb->setName(pht('Tags'));
@@ -126,8 +126,8 @@ abstract class DiffusionController extends PhabricatorController {
 
     if ($spec['commit']) {
       $crumb = id(new PhabricatorCrumbView())
-        ->setName("r{$callsign}{$raw_commit}")
-        ->setHref("r{$callsign}{$raw_commit}");
+        ->setName("r{$callsign}{$stable_commit}")
+        ->setHref("r{$callsign}{$stable_commit}");
       $crumb_list[] = $crumb;
       return $crumb_list;
     }

src/applications/diffusion/controller/DiffusionHistoryController.php

@@ -152,7 +152,7 @@ final class DiffusionHistoryController extends DiffusionController {
       ->setUser($viewer)
       ->setActionList($actions);
 
-    $stable_commit = $drequest->getStableCommit();
+    $stable_commit = $drequest->getStableCommitName();
     $callsign = $drequest->getRepository()->getCallsign();
 
     $view->addProperty(

src/applications/diffusion/controller/DiffusionTagListController.php

@@ -20,9 +20,10 @@ final class DiffusionTagListController extends DiffusionController {
     $params = array(
       'limit' => $pager->getPageSize() + 1,
       'offset' => $pager->getOffset());
-    if ($drequest->getRawCommit()) {
+
+    if ($drequest->getSymbolicCommit()) {
       $is_commit = true;
-      $params['commit'] = $drequest->getRawCommit();
+      $params['commit'] = $drequest->getSymbolicCommit();
     } else {
       $is_commit = false;
     }
@@ -76,7 +77,7 @@ final class DiffusionTagListController extends DiffusionController {
     $crumbs = $this->buildCrumbs(
       array(
         'tags' => true,
-        'commit' => $drequest->getRawCommit(),
+        'commit' => $drequest->getSymbolicCommit(),
       ));
 
     return $this->buildApplicationPage(

src/applications/diffusion/request/DiffusionGitRequest.php

@@ -1,20 +1,13 @@
 <?php
 
-/**
- * @group diffusion
- */
 final class DiffusionGitRequest extends DiffusionRequest {
 
   protected function getSupportsBranches() {
     return true;
   }
 
-  protected function didInitialize() {
-    if (!$this->commit) {
-      return;
-    }
-
-    $this->expandCommitName();
+  protected function isStableCommit($symbol) {
+    return preg_match('/^[a-f0-9]{40}\z/', $symbol);
   }
 
   public function getBranch() {
@@ -27,14 +20,6 @@ final class DiffusionGitRequest extends DiffusionRequest {
     throw new Exception("Unable to determine branch!");
   }
 
-  public function getCommit() {
-    if ($this->commit) {
-      return $this->commit;
-    }
-
-    return $this->getResolvableBranchName($this->getBranch());
-  }
-
   protected function getResolvableBranchName($branch) {
     if ($this->repository->isWorkingCopyBare()) {
       return $branch;

src/applications/diffusion/request/DiffusionMercurialRequest.php

@@ -1,27 +1,14 @@
 <?php
 
-/**
- * @group diffusion
- */
 final class DiffusionMercurialRequest extends DiffusionRequest {
 
   protected function getSupportsBranches() {
     return true;
   }
 
-  protected function didInitialize() {
-    // Expand abbreviated hashes to full hashes so "/rXnnnn" (i.e., fewer than
-    // 40 characters) works correctly.
-    if (!$this->commit) {
-      return;
-    }
-
-    if (strlen($this->commit) == 40) {
-      return;
-    }
-
-    $this->expandCommitName();
- }
+  protected function isStableCommit($symbol) {
+    return preg_match('/^[a-f0-9]{40}\z/', $symbol);
+  }
 
   public function getBranch() {
     if ($this->branch) {
@@ -35,11 +22,4 @@ final class DiffusionMercurialRequest extends DiffusionRequest {
     throw new Exception("Unable to determine branch!");
   }
 
-  public function getCommit() {
-    if ($this->commit) {
-      return $this->commit;
-    }
-    return $this->getBranch();
-  }
-
 }

src/applications/diffusion/request/DiffusionRequest.php

@@ -29,7 +29,11 @@ abstract class DiffusionRequest {
   private $user;
 
   abstract protected function getSupportsBranches();
-  abstract protected function didInitialize();
+  abstract protected function isStableCommit($symbol);
+
+  protected function didInitialize() {
+    return null;
+  }
 
 
 /* -(  Creating Requests  )-------------------------------------------------- */
@@ -130,6 +134,7 @@ abstract class DiffusionRequest {
       ->setViewer($viewer)
       ->withCallsigns(array($callsign))
       ->executeOne();
+
     if (!$repository) {
       throw new Exception("No such repository '{$callsign}'.");
     }
@@ -179,11 +184,10 @@ abstract class DiffusionRequest {
    */
   final private function initializeFromDictionary(array $data) {
     $this->path            = idx($data, 'path');
-    $this->symbolicCommit  = idx($data, 'commit');
-    $this->commit          = idx($data, 'commit');
     $this->line            = idx($data, 'line');
     $this->initFromConduit = idx($data, 'initFromConduit', true);
 
+    $this->symbolicCommit = idx($data, 'commit');
     if ($this->getSupportsBranches()) {
       $this->branch = idx($data, 'branch');
     }
@@ -234,17 +238,85 @@ abstract class DiffusionRequest {
   }
 
   public function getCommit() {
-    return $this->commit;
+
+    // TODO: Probably remove all of this.
+
+    // Required for sketchy sins that `diffusion.diffquery` commits.
+    if ($this->commit) {
+      return $this->commit;
+    }
+
+    if ($this->getSymbolicCommit() !== null) {
+      return $this->getSymbolicCommit();
+    }
+
+    return $this->getStableCommit();
   }
 
+  /**
+   * Get the symbolic commit associated with this request.
+   *
+   * A symbolic commit may be a commit hash, an abbreviated commit hash, a
+   * branch name, a tag name, or an expression like "HEAD^^^". The symbolic
+   * commit may also be absent.
+   *
+   * This method always returns the symbol present in the original request,
+   * in unmodified form.
+   *
+   * See also @{method:getStableCommit}.
+   *
+   * @return string|null  Symbolic commit, if one was present in the request.
+   */
   public function getSymbolicCommit() {
     return $this->symbolicCommit;
   }
 
+
+  /**
+   * Get the ref type (`commit` or `tag`) of the location associated with this
+   * request.
+   *
+   * If a symbolic commit is present in the request, this method identifies
+   * the type of the symbol. Otherwise, it identifies the type of symbol of
+   * the location the request is implicitly associated with. This will probably
+   * always be `commit`.
+   *
+   * @return string   Symbolic commit type (`commit` or `tag`).
+   */
   public function getSymbolicType() {
+    if ($this->symbolicType === null) {
+      // As a side effect, this resolves the symbolic type.
+      $this->getStableCommit();
+    }
     return $this->symbolicType;
   }
 
+
+  /**
+   * Retrieve the stable, permanent commit name identifying the repository
+   * location associated with this request.
+   *
+   * This returns a non-symbolic identifier for the current commit: in Git and
+   * Mercurial, a 40-character SHA1; in SVN, a revision number.
+   *
+   * See also @{method:getSymbolicCommit}.
+   *
+   * @return string Stable commit name, like a git hash or SVN revision. Not
+   *                a symbolic commit reference.
+   */
+  public function getStableCommit() {
+    if (!$this->stableCommit) {
+      if ($this->isStableCommit($this->symbolicCommit)) {
+        $this->stableCommit = $this->symbolicCommit;
+        $this->symbolicType = 'commit';
+      } else {
+        $this->queryStableCommit();
+      }
+    }
+    return $this->stableCommit;
+  }
+
+
   public function getBranch() {
     return $this->branch;
   }
@@ -308,26 +380,6 @@ abstract class DiffusionRequest {
     return $this->repositoryCommitData;
   }
 
-  /**
-   * Retrieve a stable, permanent commit name. This returns a non-symbolic
-   * identifier for the current commit: e.g., a specific commit hash in git
-   * (NOT a symbolic name like "origin/master") or a specific revision number
-   * in SVN (NOT a symbolic name like "HEAD").
-   *
-   * @return string Stable commit name, like a git hash or SVN revision. Not
-   *                a symbolic commit reference.
-   */
-  public function getStableCommit() {
-    if (!$this->stableCommit) {
-      $this->queryStableCommit();
-    }
-    return $this->stableCommit;
-  }
-
-  final public function getRawCommit() {
-    return $this->commit;
-  }
-
   public function setCommit($commit) {
     $this->commit = $commit;
     return $this;
@@ -347,7 +399,7 @@ abstract class DiffusionRequest {
    */
   public function generateURI(array $params) {
     if (empty($params['stable'])) {
-      $default_commit = $this->getRawCommit();
+      $default_commit = $this->getSymbolicCommit();
     } else {
       $default_commit = $this->getStableCommit();
     }
@@ -620,30 +672,15 @@ abstract class DiffusionRequest {
       "Guide' in the documentation for help setting up repositories.");
   }
 
-  final protected function expandCommitName() {
-    $results = $this->resolveRefs(array($this->commit));
-    $matches = idx($results, $this->commit, array());
-    if (count($results) !== 1) {
-      throw new Exception(
-        pht('Ref "%s" is ambiguous or does not exist.', $this->commit));
-    }
-
-    $match = head($matches);
-
-    $this->commit = $match['identifier'];
-    $this->symbolicType = $match['type'];
-  }
-
   private function queryStableCommit() {
-    if ($this->commit) {
-      $this->stableCommit = $this->commit;
-      return $this->stableCommit;
-    }
-
-    if ($this->getSupportsBranches()) {
-      $ref = $this->getResolvableBranchName($this->getBranch());
+    if ($this->symbolicCommit) {
+      $ref = $this->symbolicCommit;
     } else {
-      $ref = 'HEAD';
+      if ($this->getSupportsBranches()) {
+        $ref = $this->getResolvableBranchName($this->getBranch());
+      } else {
+        $ref = 'HEAD';
+      }
     }
 
     $results = $this->resolveRefs(array($ref));
@@ -655,8 +692,10 @@ abstract class DiffusionRequest {
         ->setRef($ref);
     }
 
-    $this->stableCommit = idx(head($matches), 'identifier');
-    return $this->stableCommit;
+    $match = head($matches);
+
+    $this->stableCommit = $match['identifier'];
+    $this->symbolicType = $match['type'];
   }
 
   protected function getResolvableBranchName($branch) {

src/applications/diffusion/request/DiffusionSvnRequest.php

@@ -1,14 +1,15 @@
 <?php
 
-/**
- * @group diffusion
- */
 final class DiffusionSvnRequest extends DiffusionRequest {
 
   protected function getSupportsBranches() {
     return false;
   }
 
+  protected function isStableCommit($symbol) {
+    return preg_match('/^[1-9]\d*\z/', $symbol);
+  }
+
   protected function didInitialize() {
     if ($this->path === null) {
       $subpath = $this->repository->getDetail('svn-subpath');
@@ -22,12 +23,4 @@ final class DiffusionSvnRequest extends DiffusionRequest {
     return 'svn';
   }
 
-  public function getCommit() {
-    if ($this->commit) {
-      return $this->commit;
-    }
-
-    return $this->getStableCommit();
-  }
-
 }