Improve Diffusion behavior for no-longer-existing commits

Summary:
Ref T9028. When users push a commit, then later delete it (e.g., by deleting the branch which contained it) we currently explode when trying to view it.

Instead, degrade gradually if some information is not available.

Test Plan:
  - Looked at valid commits with parents, refs, branches and merges.
  - Looked at invalid commits.
  - Looked at a previously valid, now-deleted + gc'd commit:

{F859273}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9028

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

Author: epriestley

src/applications/diffusion/controller/DiffusionCommitController.php

@@ -7,6 +7,12 @@ final class DiffusionCommitController extends DiffusionController {
   private $auditAuthorityPHIDs;
   private $highlightedAudits;
 
+  private $commitParents;
+  private $commitRefs;
+  private $commitMerges;
+  private $commitErrors;
+  private $commitExists;
+
   public function shouldAllowPublic() {
     return true;
   }
@@ -17,6 +23,7 @@ protected function shouldLoadDiffusionRequest() {
 
   protected function processDiffusionRequest(AphrontRequest $request) {
     $user = $request->getUser();
+
     // This controller doesn't use blob/path stuff, just pass the dictionary
     // in directly instead of using the AphrontRequest parsing mechanism.
     $data = $request->getURIMap();
@@ -45,10 +52,7 @@ protected function processDiffusionRequest(AphrontRequest $request) {
     ));
 
     if (!$commit) {
-      $exists = $this->callConduitWithDiffusionRequest(
-        'diffusion.existsquery',
-        array('commit' => $drequest->getCommit()));
-      if (!$exists) {
+      if (!$this->getCommitExists()) {
         return new Aphront404Response();
       }
 
@@ -95,18 +99,6 @@ protected function processDiffusionRequest(AphrontRequest $request) {
 
       require_celerity_resource('phabricator-remarkup-css');
 
-      $parents = $this->callConduitWithDiffusionRequest(
-        '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')));
 
@@ -115,7 +107,6 @@ protected function processDiffusionRequest(AphrontRequest $request) {
       $commit_properties = $this->loadCommitProperties(
         $commit,
         $commit_data,
-        $parents,
         $audit_requests);
       $property_list = id(new PHUIPropertyListView())
         ->setHasKeyboardShortcuts(true)
@@ -148,8 +139,10 @@ protected function processDiffusionRequest(AphrontRequest $request) {
           $message));
 
       $headsup_view->setTall(true);
+
       $object_box = id(new PHUIObjectBoxView())
         ->setHeader($headsup_view)
+        ->setFormErrors($this->getCommitErrors())
         ->addPropertyList($property_list)
         ->addPropertyList($detail_list);
 
@@ -216,6 +209,10 @@ protected function processDiffusionRequest(AphrontRequest $request) {
           'This commit is enormous, and affects more than %d files. '.
           'Changes are not shown.',
           $hard_limit));
+    } else if (!$this->getCommitExists()) {
+      $content[] = $this->renderStatusMessage(
+        pht('Commit No Longer Exists'),
+        pht('This commit no longer exists in the repository.'));
     } else {
       $show_changesets = true;
 
@@ -382,10 +379,8 @@ protected function processDiffusionRequest(AphrontRequest $request) {
   private function loadCommitProperties(
     PhabricatorRepositoryCommit $commit,
     PhabricatorRepositoryCommitData $data,
-    array $parents,
     array $audit_requests) {
 
-    assert_instances_of($parents, 'PhabricatorRepositoryCommit');
     $viewer = $this->getRequest()->getUser();
     $commit_phid = $commit->getPHID();
     $drequest = $this->getDiffusionRequest();
@@ -423,11 +418,6 @@ private function loadCommitProperties(
     if ($data->getCommitDetail('committerPHID')) {
       $phids[] = $data->getCommitDetail('committerPHID');
     }
-    if ($parents) {
-      foreach ($parents as $parent) {
-        $phids[] = $parent->getPHID();
-      }
-    }
 
     // NOTE: We should never normally have more than a single push log, but
     // it can occur naturally if a commit is pushed, then the branch it was
@@ -564,39 +554,47 @@ private function loadCommitProperties(
       $props['Differential Revision'] = $handles[$revision_phid]->renderLink();
     }
 
+    $parents = $this->getCommitParents();
     if ($parents) {
-      $parent_links = array();
-      foreach ($parents as $parent) {
-        $parent_links[] = $handles[$parent->getPHID()]->renderLink();
-      }
-      $props['Parents'] = phutil_implode_html(" \xC2\xB7 ", $parent_links);
+      $props['Parents'] = $viewer->renderHandleList(mpull($parents, 'getPHID'));
     }
 
-    $props['Branches'] = phutil_tag(
-      'span',
-      array(
-        'id' => 'commit-branches',
-      ),
-      pht('Unknown'));
-    $props['Tags'] = phutil_tag(
-      'span',
-      array(
-        'id' => 'commit-tags',
-      ),
-      pht('Unknown'));
+    if ($this->getCommitExists()) {
+      $props['Branches'] = phutil_tag(
+        'span',
+        array(
+          'id' => 'commit-branches',
+        ),
+        pht('Unknown'));
+      $props['Tags'] = phutil_tag(
+        'span',
+        array(
+          'id' => 'commit-tags',
+        ),
+        pht('Unknown'));
 
-    $callsign = $repository->getCallsign();
-    $root = '/diffusion/'.$callsign.'/commit/'.$commit->getCommitIdentifier();
-    Javelin::initBehavior(
-      'diffusion-commit-branches',
-      array(
-        $root.'/branches/' => 'commit-branches',
-        $root.'/tags/' => 'commit-tags',
-      ));
+      $callsign = $repository->getCallsign();
+      $root = '/diffusion/'.$callsign.'/commit/'.$commit->getCommitIdentifier();
+      Javelin::initBehavior(
+        'diffusion-commit-branches',
+        array(
+          $root.'/branches/' => 'commit-branches',
+          $root.'/tags/' => 'commit-tags',
+        ));
+    }
 
-    $refs = $this->buildRefs($drequest);
+    $refs = $this->getCommitRefs();
     if ($refs) {
-      $props['References'] = $refs;
+      $ref_links = array();
+      foreach ($refs as $ref_data) {
+        $ref_links[] = phutil_tag(
+          'a',
+          array(
+            'href' => $ref_data['href'],
+          ),
+          $ref_data['ref']);
+      }
+      $props['References'] = phutil_implode_html(', ', $ref_links);
     }
 
     if ($reverts_phids) {
@@ -860,26 +858,12 @@ private function buildMergesTable(PhabricatorRepositoryCommit $commit) {
     $drequest = $this->getDiffusionRequest();
     $repository = $drequest->getRepository();
 
-    $vcs = $repository->getVersionControlSystem();
-    switch ($vcs) {
-      case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
-        // These aren't supported under SVN.
-        return null;
-    }
-
-    $limit = 50;
-
-    $merges = $this->callConduitWithDiffusionRequest(
-      'diffusion.mergedcommitsquery',
-      array(
-        'commit' => $drequest->getCommit(),
-        'limit' => $limit + 1,
-      ));
-
+    $merges = $this->getCommitMerges();
     if (!$merges) {
       return null;
     }
-    $merges = DiffusionPathChange::newFromConduit($merges);
+
+    $limit = $this->getMergeDisplayLimit();
 
     $caption = null;
     if (count($merges) > $limit) {
@@ -961,27 +945,6 @@ private function renderHeadsupActionList(
     return $actions;
   }
 
-  private function buildRefs(DiffusionRequest $request) {
-    // this is git-only, so save a conduit round trip and just get out of
-    // here if the repository isn't git
-    $type_git = PhabricatorRepositoryType::REPOSITORY_TYPE_GIT;
-    $repository = $request->getRepository();
-    if ($repository->getVersionControlSystem() != $type_git) {
-      return null;
-    }
-
-    $results = $this->callConduitWithDiffusionRequest(
-      'diffusion.refsquery',
-      array('commit' => $request->getCommit()));
-    $ref_links = array();
-    foreach ($results as $ref_data) {
-      $ref_links[] = phutil_tag('a',
-        array('href' => $ref_data['href']),
-        $ref_data['ref']);
-    }
-    return phutil_implode_html(', ', $ref_links);
-  }
-
   private function buildRawDiffResponse(DiffusionRequest $drequest) {
     $raw_diff = $this->callConduitWithDiffusionRequest(
       'diffusion.rawdiffquery',
@@ -1137,4 +1100,140 @@ private function buildTableOfContents(array $changesets) {
     return $toc_view;
   }
 
+  private function loadCommitState() {
+    $viewer = $this->getViewer();
+    $drequest = $this->getDiffusionRequest();
+    $repository = $drequest->getRepository();
+    $commit = $drequest->getCommit();
+
+    // TODO: We could use futures here and resolve these calls in parallel.
+
+    $exceptions = array();
+
+    try {
+      $parent_refs = $this->callConduitWithDiffusionRequest(
+        'diffusion.commitparentsquery',
+        array(
+          'commit' => $commit,
+        ));
+
+      if ($parent_refs) {
+        $parents = id(new DiffusionCommitQuery())
+          ->setViewer($viewer)
+          ->withRepository($repository)
+          ->withIdentifiers($parent_refs)
+          ->execute();
+      } else {
+        $parents = array();
+      }
+
+      $this->commitParents = $parents;
+    } catch (Exception $ex) {
+      $this->commitParents = false;
+      $exceptions[] = $ex;
+    }
+
+    $merge_limit = $this->getMergeDisplayLimit();
+
+    try {
+      $merges = $this->callConduitWithDiffusionRequest(
+        'diffusion.mergedcommitsquery',
+        array(
+          'commit' => $commit,
+          'limit' => $merge_limit + 1,
+        ));
+
+      $this->commitMerges = DiffusionPathChange::newFromConduit($merges);
+    } catch (Exception $ex) {
+      $this->commitMerges = false;
+      $exceptions[] = $ex;
+    }
+
+
+    try {
+      if ($repository->isGit()) {
+        $refs = $this->callConduitWithDiffusionRequest(
+          'diffusion.refsquery',
+          array(
+            'commit' => $commit,
+          ));
+      } else {
+        $refs = array();
+      }
+
+      $this->commitRefs = $refs;
+    } catch (Exception $ex) {
+      $this->commitRefs = false;
+      $exceptions[] = $ex;
+    }
+
+    if ($exceptions) {
+      $exists = $this->callConduitWithDiffusionRequest(
+        'diffusion.existsquery',
+        array(
+          'commit' => $commit,
+        ));
+
+      if ($exists) {
+        $this->commitExists = true;
+        foreach ($exceptions as $exception) {
+          $this->commitErrors[] = $exception->getMessage();
+        }
+      } else {
+        $this->commitExists = false;
+        $this->commitErrors[] = pht(
+          'This commit no longer exists in the repository. It may have '.
+          'been part of a branch which was deleted.');
+      }
+    } else {
+      $this->commitExists = true;
+      $this->commitErrors = array();
+    }
+  }
+
+  private function getMergeDisplayLimit() {
+    return 50;
+  }
+
+  private function getCommitExists() {
+    if ($this->commitExists === null) {
+      $this->loadCommitState();
+    }
+
+    return $this->commitExists;
+  }
+
+  private function getCommitParents() {
+    if ($this->commitParents === null) {
+      $this->loadCommitState();
+    }
+
+    return $this->commitParents;
+  }
+
+  private function getCommitRefs() {
+    if ($this->commitRefs === null) {
+      $this->loadCommitState();
+    }
+
+    return $this->commitRefs;
+  }
+
+  private function getCommitMerges() {
+    if ($this->commitMerges === null) {
+      $this->loadCommitState();
+    }
+
+    return $this->commitMerges;
+  }
+
+  private function getCommitErrors() {
+    if ($this->commitErrors === null) {
+      $this->loadCommitState();
+    }
+
+    return $this->commitErrors;
+  }
+
+
 }