Allow revisions to revert commits and one another, and commits to revert revisions

Summary: Ref T13057. This makes "reverts" syntax more visible and useful. In particular, you can now `Reverts Dxx` in a revision or commit, and `Reverts <hash>` from a revision. When you do, the corresponding object will get a more-visible cross-reference marker in its timeline: {F5405517} From here, we can look at surfacing revert information more heavily, since we can now query it on revision/commit pages via edges. Test Plan: Used "reverts <hash>" and "reverts <revision>" in Differential and Diffusion, got sensible results in the timeline. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13057 Differential Revision: https://secure.phabricator.com/D18978
2024-11-22 14:52:41 +01:00 · 2018-01-31 12:19:19 -08:00 · 2018-01-31 12:19:19 -08:00 · 032f5b2294
commit 032f5b2294
parent f535981c0d
7 changed files with 149 additions and 74 deletions
--- a/src/applications/audit/editor/PhabricatorAuditEditor.php
+++ b/src/applications/audit/editor/PhabricatorAuditEditor.php
@ -358,9 +358,17 @@ final class PhabricatorAuditEditor
    array $changes,
    PhutilMarkupEngine $engine) {

-    // we are only really trying to find unmentionable phids here...
-    // don't bother with this outside initial commit (i.e. create)
-    // transaction
+    $actor = $this->getActor();
+    $result = array();
+
+    // Some interactions (like "Fixes Txxx" interacting with Maniphest) have
+    // already been processed, so we're only re-parsing them here to avoid
+    // generating an extra redundant mention. Other interactions are being
+    // processed for the first time.
+
+    // We're only recognizing magic in the commit message itself, not in
+    // audit comments.
+
    $is_commit = false;
    foreach ($xactions as $xaction) {
      switch ($xaction->getTransactionType()) {
@ -370,8 +378,6 @@ final class PhabricatorAuditEditor
      }
    }

-    // "result" is always an array....
-    $result = array();
    if (!$is_commit) {
      return $result;
    }
@ -403,6 +409,46 @@ final class PhabricatorAuditEditor
      ->withNames($monograms)
      ->execute();
    $phid_map[] = mpull($objects, 'getPHID', 'getPHID');
+
+
+    $reverts_refs = id(new DifferentialCustomFieldRevertsParser())
+      ->parseCorpus($huge_block);
+    $reverts = array_mergev(ipull($reverts_refs, 'monograms'));
+    if ($reverts) {
+      // Only allow commits to revert other commits in the same repository.
+      $reverted_commits = id(new DiffusionCommitQuery())
+        ->setViewer($actor)
+        ->withRepository($object->getRepository())
+        ->withIdentifiers($reverts)
+        ->execute();
+
+      $reverted_revisions = id(new PhabricatorObjectQuery())
+        ->setViewer($actor)
+        ->withNames($reverts)
+        ->withTypes(
+          array(
+            DifferentialRevisionPHIDType::TYPECONST,
+          ))
+        ->execute();
+
+      $reverted_phids =
+        mpull($reverted_commits, 'getPHID', 'getPHID') +
+        mpull($reverted_revisions, 'getPHID', 'getPHID');
+
+      // NOTE: Skip any write attempts if a user cleverly implies a commit
+      // reverts itself, although this would be exceptionally clever in Git
+      // or Mercurial.
+      unset($reverted_phids[$object->getPHID()]);
+
+      $reverts_edge = DiffusionCommitRevertsCommitEdgeType::EDGECONST;
+      $result[] = id(new PhabricatorAuditTransaction())
+        ->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
+        ->setMetadataValue('edge:type', $reverts_edge)
+        ->setNewValue(array('+' => $reverted_phids));
+
+      $phid_map[] = $reverted_phids;
+    }
+
    $phid_map = array_mergev($phid_map);
    $this->setUnmentionablePHIDMap($phid_map);

--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@ -919,7 +919,44 @@ final class DifferentialTransactionEditor
      }
    }

-    $this->setUnmentionablePHIDMap(array_merge($task_phids, $rev_phids));
+    $revert_refs = id(new DifferentialCustomFieldRevertsParser())
+      ->parseCorpus($content_block);
+
+    $revert_monograms = array();
+    foreach ($revert_refs as $match) {
+      foreach ($match['monograms'] as $monogram) {
+        $revert_monograms[] = $monogram;
+      }
+    }
+
+    if ($revert_monograms) {
+      $revert_objects = id(new PhabricatorObjectQuery())
+        ->setViewer($this->getActor())
+        ->withNames($revert_monograms)
+        ->withTypes(
+          array(
+            DifferentialRevisionPHIDType::TYPECONST,
+            PhabricatorRepositoryCommitPHIDType::TYPECONST,
+          ))
+        ->execute();
+
+      $revert_phids = mpull($revert_objects, 'getPHID', 'getPHID');
+
+      // Don't let an object revert itself, although other silly stuff like
+      // cycles of objects reverting each other is not prevented.
+      unset($revert_phids[$object->getPHID()]);
+
+      $revert_type = DiffusionCommitRevertsCommitEdgeType::EDGECONST;
+      $edges[$revert_type] = $revert_phids;
+    } else {
+      $revert_phids = array();
+    }
+
+    $this->setUnmentionablePHIDMap(
+      array_merge(
+        $task_phids,
+        $rev_phids,
+        $revert_phids));

    $result = array();
    foreach ($edges as $type => $specs) {
--- a/src/applications/diffusion/edge/DiffusionCommitRevertedByCommitEdgeType.php
+++ b/src/applications/diffusion/edge/DiffusionCommitRevertedByCommitEdgeType.php
@ -19,7 +19,7 @@ final class DiffusionCommitRevertedByCommitEdgeType
    $add_edges) {

    return pht(
-      '%s added %s reverting commit(s): %s.',
+      '%s added %s reverting change(s): %s.',
      $actor,
      $add_count,
      $add_edges);
@ -31,7 +31,7 @@ final class DiffusionCommitRevertedByCommitEdgeType
    $rem_edges) {

    return pht(
-      '%s removed %s reverting commit(s): %s.',
+      '%s removed %s reverting change(s): %s.',
      $actor,
      $rem_count,
      $rem_edges);
@ -46,7 +46,7 @@ final class DiffusionCommitRevertedByCommitEdgeType
    $rem_edges) {

    return pht(
-      '%s edited reverting commit(s), added %s: %s; removed %s: %s.',
+      '%s edited reverting change(s), added %s: %s; removed %s: %s.',
      $actor,
      $add_count,
      $add_edges,
@ -61,7 +61,7 @@ final class DiffusionCommitRevertedByCommitEdgeType
    $add_edges) {

    return pht(
-      '%s added %s reverting commit(s) for %s: %s.',
+      '%s added %s reverting change(s) for %s: %s.',
      $actor,
      $add_count,
      $object,
@ -75,7 +75,7 @@ final class DiffusionCommitRevertedByCommitEdgeType
    $rem_edges) {

    return pht(
-      '%s removed %s reverting commit(s) for %s: %s.',
+      '%s removed %s reverting change(s) for %s: %s.',
      $actor,
      $rem_count,
      $object,
@ -92,7 +92,7 @@ final class DiffusionCommitRevertedByCommitEdgeType
    $rem_edges) {

    return pht(
-      '%s edited reverting commit(s) for %s, added %s: %s; removed %s: %s.',
+      '%s edited reverting change(s) for %s, added %s: %s; removed %s: %s.',
      $actor,
      $object,
      $add_count,
--- a/src/applications/diffusion/edge/DiffusionCommitRevertsCommitEdgeType.php
+++ b/src/applications/diffusion/edge/DiffusionCommitRevertsCommitEdgeType.php
@ -22,7 +22,7 @@ final class DiffusionCommitRevertsCommitEdgeType extends PhabricatorEdgeType {
    $add_edges) {

    return pht(
-      '%s added %s reverted commit(s): %s.',
+      '%s added %s reverted change(s): %s.',
      $actor,
      $add_count,
      $add_edges);
@ -34,7 +34,7 @@ final class DiffusionCommitRevertsCommitEdgeType extends PhabricatorEdgeType {
    $rem_edges) {

    return pht(
-      '%s removed %s reverted commit(s): %s.',
+      '%s removed %s reverted change(s): %s.',
      $actor,
      $rem_count,
      $rem_edges);
@ -49,7 +49,7 @@ final class DiffusionCommitRevertsCommitEdgeType extends PhabricatorEdgeType {
    $rem_edges) {

    return pht(
-      '%s edited reverted commit(s), added %s: %s; removed %s: %s.',
+      '%s edited reverted change(s), added %s: %s; removed %s: %s.',
      $actor,
      $add_count,
      $add_edges,
@ -64,7 +64,7 @@ final class DiffusionCommitRevertsCommitEdgeType extends PhabricatorEdgeType {
    $add_edges) {

    return pht(
-      '%s added %s reverted commit(s) for %s: %s.',
+      '%s added %s reverted change(s) for %s: %s.',
      $actor,
      $add_count,
      $object,
@ -78,7 +78,7 @@ final class DiffusionCommitRevertsCommitEdgeType extends PhabricatorEdgeType {
    $rem_edges) {

    return pht(
-      '%s removed %s reverted commit(s) for %s: %s.',
+      '%s removed %s reverted change(s) for %s: %s.',
      $actor,
      $rem_count,
      $object,
@ -95,7 +95,7 @@ final class DiffusionCommitRevertsCommitEdgeType extends PhabricatorEdgeType {
    $rem_edges) {

    return pht(
-      '%s edited reverted commit(s) for %s, added %s: %s; removed %s: %s.',
+      '%s edited reverted change(s) for %s, added %s: %s; removed %s: %s.',
      $actor,
      $object,
      $add_count,
--- a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php
+++ b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php
@ -15,6 +15,7 @@ final class PhabricatorRepositoryCommitHeraldWorker
  protected function parseCommit(
    PhabricatorRepository $repository,
    PhabricatorRepositoryCommit $commit) {
+    $viewer = PhabricatorUser::getOmnipotentUser();

    if ($this->shouldSkipImportStep()) {
      // This worker has no followup tasks, so we can just bail out
@ -50,7 +51,7 @@ final class PhabricatorRepositoryCommitHeraldWorker
      id(new PhabricatorDiffusionApplication())->getPHID());

    $editor = id(new PhabricatorAuditEditor())
-      ->setActor(PhabricatorUser::getOmnipotentUser())
+      ->setActor($viewer)
      ->setActingAsPHID($acting_as_phid)
      ->setContinueOnMissingFields(true)
      ->setContinueOnNoEffect(true)
@ -69,29 +70,6 @@ final class PhabricatorRepositoryCommitHeraldWorker
        'committerPHID' => $data->getCommitDetail('committerPHID'),
      ));

-    $reverts_refs = id(new DifferentialCustomFieldRevertsParser())
-      ->parseCorpus($data->getCommitMessage());
-    $reverts = array_mergev(ipull($reverts_refs, 'monograms'));
-
-    if ($reverts) {
-      $reverted_commits = id(new DiffusionCommitQuery())
-        ->setViewer(PhabricatorUser::getOmnipotentUser())
-        ->withRepository($repository)
-        ->withIdentifiers($reverts)
-        ->execute();
-      $reverted_commit_phids = mpull($reverted_commits, 'getPHID', 'getPHID');
-
-      // NOTE: Skip any write attempts if a user cleverly implies a commit
-      // reverts itself.
-      unset($reverted_commit_phids[$commit->getPHID()]);
-
-      $reverts_edge = DiffusionCommitRevertsCommitEdgeType::EDGECONST;
-      $xactions[] = id(new PhabricatorAuditTransaction())
-        ->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
-        ->setMetadataValue('edge:type', $reverts_edge)
-        ->setNewValue(array('+' => array_fuse($reverted_commit_phids)));
-    }
-
    try {
      $raw_patch = $this->loadRawPatchText($repository, $commit);
    } catch (Exception $ex) {
--- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@ -458,6 +458,12 @@ abstract class PhabricatorApplicationTransaction
      case PhabricatorTransactions::TYPE_JOIN_POLICY:
        return 'fa-lock';
      case PhabricatorTransactions::TYPE_EDGE:
+        switch ($this->getMetadataValue('edge:type')) {
+          case DiffusionCommitRevertedByCommitEdgeType::EDGECONST:
+            return 'fa-undo';
+          case DiffusionCommitRevertsCommitEdgeType::EDGECONST:
+            return 'fa-ambulance';
+        }
        return 'fa-link';
      case PhabricatorTransactions::TYPE_BUILDABLE:
        return 'fa-wrench';
@ -496,6 +502,14 @@ abstract class PhabricatorApplicationTransaction
          return 'black';
        }
        break;
+      case PhabricatorTransactions::TYPE_EDGE:
+        switch ($this->getMetadataValue('edge:type')) {
+          case DiffusionCommitRevertedByCommitEdgeType::EDGECONST:
+            return 'pink';
+          case DiffusionCommitRevertsCommitEdgeType::EDGECONST:
+            return 'sky';
+        }
+        break;
      case PhabricatorTransactions::TYPE_BUILDABLE:
        switch ($this->getNewValue()) {
          case HarbormasterBuildable::STATUS_PASSED:
--- a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php
+++ b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php
@ -676,73 +676,73 @@ final class PhabricatorUSEnglishTranslation
      '%s edited commit(s), added %s: %s; removed %s: %s.' =>
        '%s edited commits, added %3$s; removed %5$s.',

-      '%s added %s reverted commit(s): %s.' => array(
+      '%s added %s reverted change(s): %s.' => array(
        array(
-          '%s added a reverted commit: %3$s.',
-          '%s added reverted commits: %3$s.',
+          '%s added a reverted change: %3$s.',
+          '%s added reverted changes: %3$s.',
        ),
      ),

-      '%s removed %s reverted commit(s): %s.' => array(
+      '%s removed %s reverted change(s): %s.' => array(
        array(
-          '%s removed a reverted commit: %3$s.',
-          '%s removed reverted commits: %3$s.',
+          '%s removed a reverted change: %3$s.',
+          '%s removed reverted changes: %3$s.',
        ),
      ),

-      '%s edited reverted commit(s), added %s: %s; removed %s: %s.' =>
-        '%s edited reverted commits, added %3$s; removed %5$s.',
+      '%s edited reverted change(s), added %s: %s; removed %s: %s.' =>
+        '%s edited reverted changes, added %3$s; removed %5$s.',

-      '%s added %s reverted commit(s) for %s: %s.' => array(
+      '%s added %s reverted change(s) for %s: %s.' => array(
        array(
-          '%s added a reverted commit for %3$s: %4$s.',
-          '%s added reverted commits for %3$s: %4$s.',
+          '%s added a reverted change for %3$s: %4$s.',
+          '%s added reverted changes for %3$s: %4$s.',
        ),
      ),

-      '%s removed %s reverted commit(s) for %s: %s.' => array(
+      '%s removed %s reverted change(s) for %s: %s.' => array(
        array(
-          '%s removed a reverted commit for %3$s: %4$s.',
-          '%s removed reverted commits for %3$s: %4$s.',
+          '%s removed a reverted change for %3$s: %4$s.',
+          '%s removed reverted changes for %3$s: %4$s.',
        ),
      ),

-      '%s edited reverted commit(s) for %s, added %s: %s; removed %s: %s.' =>
-        '%s edited reverted commits for %2$s, added %4$s; removed %6$s.',
+      '%s edited reverted change(s) for %s, added %s: %s; removed %s: %s.' =>
+        '%s edited reverted changes for %2$s, added %4$s; removed %6$s.',

-      '%s added %s reverting commit(s): %s.' => array(
+      '%s added %s reverting change(s): %s.' => array(
        array(
-          '%s added a reverting commit: %3$s.',
-          '%s added reverting commits: %3$s.',
+          '%s added a reverting change: %3$s.',
+          '%s added reverting changes: %3$s.',
        ),
      ),

-      '%s removed %s reverting commit(s): %s.' => array(
+      '%s removed %s reverting change(s): %s.' => array(
        array(
-          '%s removed a reverting commit: %3$s.',
-          '%s removed reverting commits: %3$s.',
+          '%s removed a reverting change: %3$s.',
+          '%s removed reverting changes: %3$s.',
        ),
      ),

-      '%s edited reverting commit(s), added %s: %s; removed %s: %s.' =>
-        '%s edited reverting commits, added %3$s; removed %5$s.',
+      '%s edited reverting change(s), added %s: %s; removed %s: %s.' =>
+        '%s edited reverting changes, added %3$s; removed %5$s.',

-      '%s added %s reverting commit(s) for %s: %s.' => array(
+      '%s added %s reverting change(s) for %s: %s.' => array(
        array(
-          '%s added a reverting commit for %3$s: %4$s.',
-          '%s added reverting commits for %3$s: %4$s.',
+          '%s added a reverting change for %3$s: %4$s.',
+          '%s added reverting changes for %3$s: %4$s.',
        ),
      ),

-      '%s removed %s reverting commit(s) for %s: %s.' => array(
+      '%s removed %s reverting change(s) for %s: %s.' => array(
        array(
-          '%s removed a reverting commit for %3$s: %4$s.',
-          '%s removed reverting commits for %3$s: %4$s.',
+          '%s removed a reverting change for %3$s: %4$s.',
+          '%s removed reverting changes for %3$s: %4$s.',
        ),
      ),

-      '%s edited reverting commit(s) for %s, added %s: %s; removed %s: %s.' =>
-        '%s edited reverting commits for %s, added %4$s; removed %6$s.',
+      '%s edited reverting change(s) for %s, added %s: %s; removed %s: %s.' =>
+        '%s edited reverting changes for %s, added %4$s; removed %6$s.',

      '%s changed project member(s), added %d: %s; removed %d: %s.' =>
        '%s changed project members, added %3$s; removed %5$s.',