When commits have a "rewritten" hint, try to show that in handles in other applications

Summary:
Ref T11522. This tries to reduce the cost of rewriting a repository by making handles smarter about rewritten commits.

When a handle references an unreachable commit, try to load a rewrite hint for the commit. If we find one, change the handle name to "OldHash > NewHash" to provide a strong hint that the commit was rewritten and that copy/pasting the old hash (say, to the CLI) won't work.

I think this notation isn't totally self-evident, but users can click it to see the big error message on the page, and it's at least obvious that something weird is going on, which I think is the important part.

Some possible future work:

  - Not sure this ("Recycling Symbol") is the best symbol? Seems sort of reasonable but mabye there's a better one.
  - Putting this information directly on the hovercard could help explain what this means.

Test Plan: {F1780719}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11522

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

src/applications/diffusion/query/DiffusionCommitHintQuery.php

@@ -7,6 +7,9 @@ final class DiffusionCommitHintQuery
   private $repositoryPHIDs;
   private $oldCommitIdentifiers;
 
+  private $commits;
+  private $commitMap;
+
   public function withIDs(array $ids) {
     $this->ids = $ids;
     return $this;
@@ -22,10 +25,37 @@ final class DiffusionCommitHintQuery
     return $this;
   }
 
+  public function withCommits(array $commits) {
+    assert_instances_of($commits, 'PhabricatorRepositoryCommit');
+
+    $repository_phids = array();
+    foreach ($commits as $commit) {
+      $repository_phids[] = $commit->getRepository()->getPHID();
+    }
+
+    $this->repositoryPHIDs = $repository_phids;
+    $this->oldCommitIdentifiers = mpull($commits, 'getCommitIdentifier');
+    $this->commits = $commits;
+
+    return $this;
+  }
+
+  public function getCommitMap() {
+    if ($this->commitMap === null) {
+      throw new PhutilInvalidStateException('execute');
+    }
+
+    return $this->commitMap;
+  }
+
   public function newResultObject() {
     return new PhabricatorRepositoryCommitHint();
   }
 
+  protected function willExecute() {
+    $this->commitMap = array();
+  }
+
   protected function loadPage() {
     return $this->loadStandardPage($this->newResultObject());
   }
@@ -57,6 +87,28 @@ final class DiffusionCommitHintQuery
     return $where;
   }
 
+  protected function didFilterPage(array $hints) {
+    if ($this->commits) {
+      $map = array();
+      foreach ($this->commits as $commit) {
+        $repository_phid = $commit->getRepository()->getPHID();
+        $identifier = $commit->getCommitIdentifier();
+        $map[$repository_phid][$identifier] = $commit->getPHID();
+      }
+
+      foreach ($hints as $hint) {
+        $repository_phid = $hint->getRepositoryPHID();
+        $identifier = $hint->getOldCommitIdentifier();
+        if (isset($map[$repository_phid][$identifier])) {
+          $commit_phid = $map[$repository_phid][$identifier];
+          $this->commitMap[$commit_phid] = $hint;
+        }
+      }
+    }
+
+    return $hints;
+  }
+
   public function getQueryApplicationClass() {
     return 'PhabricatorDiffusionApplication';
   }

src/applications/diffusion/remarkup/DiffusionCommitRemarkupRule.php

@@ -14,6 +14,25 @@ final class DiffusionCommitRemarkupRule extends PhabricatorObjectRemarkupRule {
     return PhabricatorRepositoryCommitPHIDType::getCommitObjectNamePattern();
   }
 
+  protected function getObjectNameText(
+    $object,
+    PhabricatorObjectHandle $handle,
+    $id) {
+
+    // If this commit is unreachable, return the handle name instead of the
+    // normal text because it may be able to tell the user that the commit
+    // was rewritten and where to find the new one.
+
+    // By default, we try to preserve what the user actually typed as
+    // faithfully as possible, but if they're referencing a deleted commit
+    // it's more valuable to try to pick up any rewrite. See T11522.
+    if ($object->isUnreachable()) {
+      return $handle->getName();
+    }
+
+    return parent::getObjectNameText($object, $handle, $id);
+  }
+
   protected function loadObjects(array $ids) {
     $viewer = $this->getEngine()->getConfig('viewer');
 

src/applications/repository/phid/PhabricatorRepositoryCommitPHIDType.php

@@ -29,12 +29,47 @@ final class PhabricatorRepositoryCommitPHIDType extends PhabricatorPHIDType {
     array $handles,
     array $objects) {
 
+    $unreachable = array();
+    foreach ($handles as $phid => $handle) {
+      $commit = $objects[$phid];
+      if ($commit->isUnreachable()) {
+        $unreachable[$phid] = $commit;
+      }
+    }
+
+    if ($unreachable) {
+      $query = id(new DiffusionCommitHintQuery())
+        ->setViewer($query->getViewer())
+        ->withCommits($unreachable);
+
+      $query->execute();
+
+      $hints = $query->getCommitMap();
+    } else {
+      $hints = array();
+    }
+
     foreach ($handles as $phid => $handle) {
       $commit = $objects[$phid];
       $repository = $commit->getRepository();
       $commit_identifier = $commit->getCommitIdentifier();
 
       $name = $repository->formatCommitName($commit_identifier);
+
+      if ($commit->isUnreachable()) {
+        $handle->setStatus(PhabricatorObjectHandle::STATUS_CLOSED);
+
+        // If we have a hint about this commit being rewritten, add the
+        // rewrite target to the handle name. This reduces the chance users
+        // will be caught offguard by the rewrite.
+        $hint = idx($hints, $phid);
+        if ($hint && $hint->isRewritten()) {
+          $new_name = $hint->getNewCommitIdentifier();
+          $new_name = $repository->formatCommitName($new_name);
+          $name = pht("%s \xE2\x99\xBB %s",  $name, $new_name);
+        }
+      }
+
       $summary = $commit->getSummary();
       if (strlen($summary)) {
         $full_name = $name.': '.$summary;
@@ -46,10 +81,6 @@ final class PhabricatorRepositoryCommitPHIDType extends PhabricatorPHIDType {
       $handle->setFullName($full_name);
       $handle->setURI($commit->getURI());
       $handle->setTimestamp($commit->getEpoch());
-
-      if ($commit->isUnreachable()) {
-        $handle->setStatus(PhabricatorObjectHandle::STATUS_CLOSED);
-      }
     }
   }
 

src/infrastructure/markup/rule/PhabricatorObjectRemarkupRule.php

@@ -25,6 +25,13 @@ abstract class PhabricatorObjectRemarkupRule extends PhutilRemarkupRule {
     return true;
   }
 
+  protected function getObjectNameText(
+    $object,
+    PhabricatorObjectHandle $handle,
+    $id) {
+    return $this->getObjectNamePrefix().$id;
+  }
+
   protected function loadHandles(array $objects) {
     $phids = mpull($objects, 'getPHID');
 
@@ -60,7 +67,7 @@ abstract class PhabricatorObjectRemarkupRule extends PhutilRemarkupRule {
     $id) {
 
     $href = $this->getObjectHref($object, $handle, $id);
-    $text = $this->getObjectNamePrefix().$id;
+    $text = $this->getObjectNameText($object, $handle, $id);
 
     if ($anchor) {
       $href = $href.'#'.$anchor;
@@ -85,7 +92,7 @@ abstract class PhabricatorObjectRemarkupRule extends PhutilRemarkupRule {
     $id) {
 
     $href = $this->getObjectHref($object, $handle, $id);
-    $text = $this->getObjectNamePrefix().$id;
+    $text = $this->getObjectNameText($object, $handle, $id);
     $status_closed = PhabricatorObjectHandle::STATUS_CLOSED;
 
     if ($anchor) {