From 1eff4fdca332c25c3ff081a815622048bcb635f8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 22 May 2019 14:04:15 -0700 Subject: [PATCH] Prevent "Differential Revision: ..." from counting as a mention in commit messages Summary: Ref T13290. Ref T13291. Now that a full URI is a "mention", the full URI in "Differential Revision: ..." also triggers a mention. Stop it from doing that, since these mentions are silly/redundant/unintended. The API here is also slightly odd; simplify it a little bit to get rid of doing "append" with "get + append + set". Test Plan: Used `bin/repository reparse --publish` to republish commits with "Differential Revision: ..." and verified that the revision PHID was properly dropped from the mention list. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13291, T13290 Differential Revision: https://secure.phabricator.com/D20544 --- .../audit/editor/PhabricatorAuditEditor.php | 6 ++---- .../editor/DifferentialTransactionEditor.php | 10 +++------- .../DiffusionUpdateObjectAfterCommitWorker.php | 5 +---- .../PhabricatorRepositoryCommitPublishWorker.php | 15 ++++++++++++++- .../PhabricatorApplicationTransactionEditor.php | 12 ++++++++---- 5 files changed, 28 insertions(+), 20 deletions(-) diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index d3bd7be7ae..7995e2a36d 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -239,7 +239,7 @@ final class PhabricatorAuditEditor $object); if ($request) { $xactions[] = $request; - $this->setUnmentionablePHIDMap($request->getNewValue()); + $this->addUnmentionablePHIDs($request->getNewValue()); } break; default: @@ -360,7 +360,6 @@ final class PhabricatorAuditEditor $flat_blocks = mpull($changes, 'getNewValue'); $huge_block = implode("\n\n", $flat_blocks); $phid_map = array(); - $phid_map[] = $this->getUnmentionablePHIDMap(); $monograms = array(); $task_refs = id(new ManiphestCustomFieldStatusParser()) @@ -385,7 +384,6 @@ final class PhabricatorAuditEditor ->execute(); $phid_map[] = mpull($objects, 'getPHID', 'getPHID'); - $reverts_refs = id(new DifferentialCustomFieldRevertsParser()) ->parseCorpus($huge_block); $reverts = array_mergev(ipull($reverts_refs, 'monograms')); @@ -408,7 +406,7 @@ final class PhabricatorAuditEditor } $phid_map = array_mergev($phid_map); - $this->setUnmentionablePHIDMap($phid_map); + $this->addUnmentionablePHIDs($phid_map); return $result; } diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 0150500727..eba641771e 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -846,13 +846,9 @@ final class DifferentialTransactionEditor $revert_phids = array(); } - // See PHI574. Respect any unmentionable PHIDs which were set on the - // Editor by the caller. - $unmentionable_map = $this->getUnmentionablePHIDMap(); - $unmentionable_map += $task_phids; - $unmentionable_map += $rev_phids; - $unmentionable_map += $revert_phids; - $this->setUnmentionablePHIDMap($unmentionable_map); + $this->addUnmentionablePHIDs($task_phids); + $this->addUnmentionablePHIDs($rev_phids); + $this->addUnmentionablePHIDs($revert_phids); $result = array(); foreach ($edges as $type => $specs) { diff --git a/src/applications/diffusion/worker/DiffusionUpdateObjectAfterCommitWorker.php b/src/applications/diffusion/worker/DiffusionUpdateObjectAfterCommitWorker.php index a0a18c0662..b652ae86d6 100644 --- a/src/applications/diffusion/worker/DiffusionUpdateObjectAfterCommitWorker.php +++ b/src/applications/diffusion/worker/DiffusionUpdateObjectAfterCommitWorker.php @@ -139,10 +139,7 @@ final class DiffusionUpdateObjectAfterCommitWorker ->setContentSource($content_source) ->setContinueOnNoEffect(true) ->setContinueOnMissingFields(true) - ->setUnmentionablePHIDMap( - array( - $commit_phid => $commit_phid, - )); + ->addUnmentionablePHIDs(array($commit_phid)); $editor->applyTransactions($task, $xactions); } diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php index 9394ba6b71..17074f8cb4 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php @@ -62,12 +62,25 @@ final class PhabricatorRepositoryCommitPublishWorker $acting_phid = $this->getPublishAsPHID($commit); $content_source = $this->newContentSource(); + $revision = DiffusionCommitRevisionQuery::loadRevisionForCommit( + $viewer, + $commit); + + // Prevent the commit from generating a mention of the associated + // revision, if one exists, so we don't double up because of the URI + // in the commit message. + $unmentionable_phids = array(); + if ($revision) { + $unmentionable_phids[] = $revision->getPHID(); + } + $editor = $commit->getApplicationTransactionEditor() ->setActor($viewer) ->setActingAsPHID($acting_phid) ->setContinueOnNoEffect(true) ->setContinueOnMissingFields(true) - ->setContentSource($content_source); + ->setContentSource($content_source) + ->addUnmentionablePHIDs($unmentionable_phids); try { $raw_patch = $this->loadRawPatchText($repository, $commit); diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 0c1d5f06e2..a114eb21b3 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -260,12 +260,14 @@ abstract class PhabricatorApplicationTransactionEditor return $this->isHeraldEditor; } - public function setUnmentionablePHIDMap(array $map) { - $this->unmentionablePHIDMap = $map; + public function addUnmentionablePHIDs(array $phids) { + foreach ($phids as $phid) { + $this->unmentionablePHIDMap[$phid] = true; + } return $this; } - public function getUnmentionablePHIDMap() { + private function getUnmentionablePHIDMap() { return $this->unmentionablePHIDMap; } @@ -2090,12 +2092,14 @@ abstract class PhabricatorApplicationTransactionEditor ->withPHIDs($mentioned_phids) ->execute(); + $unmentionable_map = $this->getUnmentionablePHIDMap(); + $mentionable_phids = array(); if ($this->shouldEnableMentions($object, $xactions)) { foreach ($mentioned_objects as $mentioned_object) { if ($mentioned_object instanceof PhabricatorMentionableInterface) { $mentioned_phid = $mentioned_object->getPHID(); - if (idx($this->getUnmentionablePHIDMap(), $mentioned_phid)) { + if (isset($unmentionable_map[$mentioned_phid])) { continue; } // don't let objects mention themselves