Page MenuHomePhabricator
Files F15421522

D20469.diff
No OneTemporary
Actions

D20469.diff
View Options

diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php
--- a/src/applications/audit/editor/PhabricatorAuditEditor.php
+++ b/src/applications/audit/editor/PhabricatorAuditEditor.php
@@ -390,30 +390,13 @@
->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');
+ $reverted_objects = DiffusionCommitRevisionQuery::loadRevertedObjects(
+ $actor,
+ $object,
+ $reverts,
+ $object->getRepository());
- // 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
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -832,22 +832,14 @@
}
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
--- a/src/applications/diffusion/query/DiffusionCommitRevisionQuery.php
+++ b/src/applications/diffusion/query/DiffusionCommitRevisionQuery.php
@@ -64,5 +64,75 @@
->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;
+ }
}

File Metadata

Mime Type
text/plain
Expires
Sun, Mar 23, 12:32 AM (2 w, 5 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7717621
Default Alt Text
D20469.diff (5 KB)

Event Timeline

Phabricator · Built by Phacility