From a78f141b3fa8d4d7434fe2f39255aaa15d9e0df2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 23 Apr 2019 10:57:51 -0700 Subject: [PATCH] Unify code for parsing "Reverts X" magic, and when something "reverts ", also revert associated revisions Summary: Depends on D20468. Ref T13276. See PHI1008. When a commit or revision "reverts ", 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 --- .../audit/editor/PhabricatorAuditEditor.php | 29 ++------ .../editor/DifferentialTransactionEditor.php | 18 ++--- .../query/DiffusionCommitRevisionQuery.php | 70 +++++++++++++++++++ 3 files changed, 81 insertions(+), 36 deletions(-) diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index 4a647bd886..d3bd7be7ae 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/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()) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index a250d5a2a0..0150500727 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/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 { diff --git a/src/applications/diffusion/query/DiffusionCommitRevisionQuery.php b/src/applications/diffusion/query/DiffusionCommitRevisionQuery.php index 68a3ff3beb..822ebfb014 100644 --- a/src/applications/diffusion/query/DiffusionCommitRevisionQuery.php +++ b/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; + } }