Unify code for parsing "Reverts X" magic, and when something "reverts <hash>", also revert associated revisions

Summary:
Depends on D20468. Ref T13276. See PHI1008.

When a commit or revision "reverts <hash>", and that's the hash of a commit which has a revision, also write a "reverts" edge to the revision.

Test Plan:
Created "reverts X" objects for:

  - a revision;
  - a commit;
  - a commit with a revision (both got marked properly).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

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

src/applications/audit/editor/PhabricatorAuditEditor.php

@@ -390,30 +390,13 @@ final class PhabricatorAuditEditor
       ->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_objects = DiffusionCommitRevisionQuery::loadRevertedObjects(
+        $actor,
+        $object,
+        $reverts,
+        $object->getRepository());
 
-      $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()]);
+      $reverted_phids = mpull($reverted_objects, 'getPHID', 'getPHID');
 
       $reverts_edge = DiffusionCommitRevertsCommitEdgeType::EDGECONST;
       $result[] = id(new PhabricatorAuditTransaction())

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -832,22 +832,14 @@ final class DifferentialTransactionEditor
     }
 
     if ($revert_monograms) {
-      $revert_objects = id(new PhabricatorObjectQuery())
-        ->setViewer($this->getActor())
-        ->withNames($revert_monograms)
-        ->withTypes(
-          array(
-            DifferentialRevisionPHIDType::TYPECONST,
-            PhabricatorRepositoryCommitPHIDType::TYPECONST,
-          ))
-        ->execute();
+      $revert_objects = DiffusionCommitRevisionQuery::loadRevertedObjects(
+        $this->getActor(),
+        $object,
+        $revert_monograms,
+        null);
 
       $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 {

src/applications/diffusion/query/DiffusionCommitRevisionQuery.php

@@ -64,5 +64,75 @@ final class DiffusionCommitRevisionQuery
       ->executeOne();
   }
 
+  public static function loadRevertedObjects(
+    PhabricatorUser $viewer,
+    $source_object,
+    array $object_names,
+    PhabricatorRepository $repository_scope = null) {
+
+    // Fetch commits first, since we need to load data on commits in order
+    // to identify associated revisions later on.
+    $commit_query = id(new DiffusionCommitQuery())
+      ->setViewer($viewer)
+      ->withIdentifiers($object_names)
+      ->needCommitData(true);
+
+    // If we're acting in a specific repository, only allow commits in that
+    // repository to be affected: when commit X reverts commit Y by hash, we
+    // only want to revert commit Y in the same repository, even if other
+    // repositories have a commit with the same hash.
+    if ($repository_scope) {
+      $commit_query->withRepository($repository_scope);
+    }
+
+    $objects = $commit_query->execute();
+
+    $more_objects = id(new PhabricatorObjectQuery())
+      ->setViewer($viewer)
+      ->withNames($object_names)
+      ->withTypes(
+        array(
+          DifferentialRevisionPHIDType::TYPECONST,
+        ))
+      ->execute();
+    foreach ($more_objects as $object) {
+      $objects[] = $object;
+    }
+
+    // See PHI1008 and T13276. If something reverts commit X, we also revert
+    // any associated revision.
+
+    // For now, we don't try to find associated commits if something reverts
+    // revision Y. This is less common, although we could make more of an
+    // effort in the future.
+
+    foreach ($objects as $object) {
+      if (!($object instanceof PhabricatorRepositoryCommit)) {
+        continue;
+      }
+
+      // NOTE: If our object "reverts X", where "X" is a commit hash, it is
+      // possible that "X" will not have parsed yet, so we'll fail to find
+      // a revision even though one exists.
+
+      // For now, do nothing. It's rare to push a commit which reverts some
+      // commit "X" before "X" has parsed, so we expect this to be unusual.
+
+      $revision = self::loadRevisionForCommit(
+        $viewer,
+        $object);
+      if ($revision) {
+        $objects[] = $revision;
+      }
+    }
+
+    $objects = mpull($objects, null, 'getPHID');
+
+    // Prevent an object from reverting itself, although this would be very
+    // clever in Git or Mercurial.
+    unset($objects[$source_object->getPHID()]);
+
+    return $objects;
+  }
 
 }