diff --git a/src/applications/diffusion/query/DiffusionCommitHintQuery.php b/src/applications/diffusion/query/DiffusionCommitHintQuery.php index 28ae1ed709..bfd8045131 100644 --- a/src/applications/diffusion/query/DiffusionCommitHintQuery.php +++ b/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'; } diff --git a/src/applications/diffusion/remarkup/DiffusionCommitRemarkupRule.php b/src/applications/diffusion/remarkup/DiffusionCommitRemarkupRule.php index 2722e9d819..4d2d130c32 100644 --- a/src/applications/diffusion/remarkup/DiffusionCommitRemarkupRule.php +++ b/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'); diff --git a/src/applications/repository/phid/PhabricatorRepositoryCommitPHIDType.php b/src/applications/repository/phid/PhabricatorRepositoryCommitPHIDType.php index 5d3ce3c961..df84f2dcfd 100644 --- a/src/applications/repository/phid/PhabricatorRepositoryCommitPHIDType.php +++ b/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); - } } } diff --git a/src/infrastructure/markup/rule/PhabricatorObjectRemarkupRule.php b/src/infrastructure/markup/rule/PhabricatorObjectRemarkupRule.php index d5addbc556..35c0ecfad0 100644 --- a/src/infrastructure/markup/rule/PhabricatorObjectRemarkupRule.php +++ b/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) {