diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index 0cf6339239..049733f777 100644 --- 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); diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index ecd1e6c95c..3a1537b01d 100644 --- 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) { diff --git a/src/applications/diffusion/edge/DiffusionCommitRevertedByCommitEdgeType.php b/src/applications/diffusion/edge/DiffusionCommitRevertedByCommitEdgeType.php index 53c39e54c9..ae59a3b1e6 100644 --- 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, diff --git a/src/applications/diffusion/edge/DiffusionCommitRevertsCommitEdgeType.php b/src/applications/diffusion/edge/DiffusionCommitRevertsCommitEdgeType.php index 8f32797173..ee0223c966 100644 --- 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, diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php index 9fb6667924..818d1e8781 100644 --- 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) { diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 97b57009ec..d5c28b83bf 100644 --- 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: diff --git a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php index 40cedacf06..eb74f8debf 100644 --- 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.',