From 1cda1402c7c29170167e28e598bc9bf350e96ac5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 14 Apr 2019 11:13:42 -0700 Subject: [PATCH] Do not publish/notify about commits which are not reachable from any "Autoclose" ref Summary: Depends on D20418. Ref T13277. Fixes T11314. Currently, when you push commits to some arbitrary ref or tag (like `refs/pull/123` on GitHub, `refs/tags/phabricator/diff/123` on Phabricator, or `refs/changes/whatever` on Gerrit), we do not "autoclose" related objects. This means that we don't process `Ref T123` to create links to tasks, and don't process `Differential Revision: xyz` to close revisions. However, we //do// still publish these commits. "Publish" means: trigger audits, publish feed stories, and run Herald rules. - Stop publishing these commits. - In the UI, show these commits as "Not Permanent" with a note that they are "Not [on any permanent branch]." These commits will publish and autoclose if they ever become reachable from an "autoclose" ref (most commonly, if they are later merged to "master"). Test Plan: - Pushed a commit to `refs/tags/quack`. - Before: got a feed story. - After: no feed story, UI shows commit as "Not Permanent". Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13277, T11314 Differential Revision: https://secure.phabricator.com/D20419 --- .../audit/editor/PhabricatorAuditEditor.php | 4 ++-- .../controller/DiffusionCommitController.php | 13 +++++++++++++ .../repository/storage/PhabricatorRepository.php | 15 +++++++++++++-- .../storage/PhabricatorRepositoryCommit.php | 4 ++++ .../PhabricatorRepositoryCommitOwnersWorker.php | 2 +- 5 files changed, 33 insertions(+), 5 deletions(-) diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index d4fa1c32ff..d23bc8ba45 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -718,7 +718,7 @@ final class PhabricatorAuditEditor switch ($xaction->getTransactionType()) { case PhabricatorAuditTransaction::TYPE_COMMIT: $repository = $object->getRepository(); - if (!$repository->shouldPublish()) { + if (!$repository->shouldPublishCommit($object)) { return false; } return true; @@ -779,7 +779,7 @@ final class PhabricatorAuditEditor // TODO: They should, and then we should simplify this. $repository = $object->getRepository($assert_attached = false); if ($repository != PhabricatorLiskDAO::ATTACHABLE) { - if (!$repository->shouldPublish()) { + if (!$repository->shouldPublishCommit($object)) { return false; } } diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index fae215f381..d49584f6b4 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -240,6 +240,19 @@ final class DiffusionCommitController extends DiffusionController { 'reachable from any branch, tag, or ref.'); } } + if (!$commit->isPermanentCommit()) { + $nonpermanent_tag = id(new PHUITagView()) + ->setType(PHUITagView::TYPE_SHADE) + ->setName(pht('Not Permanent')) + ->setColor(PHUITagView::COLOR_ORANGE); + + $header->addTag($nonpermanent_tag); + + $this->commitErrors[] = pht( + 'This commit is not reachable from any permanent branch, tag, '. + 'or ref.'); + } + if ($this->getCommitErrors()) { $error_panel = id(new PHUIInfoView()) diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index d5ad83b6d6..cd5aa1d479 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -1055,6 +1055,18 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO return true; } + public function shouldPublishCommit(PhabricatorRepositoryCommit $commit) { + if (!$this->shouldPublish()) { + return false; + } + + if (!$commit->isPermanentCommit()) { + return false; + } + + return true; + } + /* -( Autoclose )---------------------------------------------------------- */ @@ -1152,8 +1164,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO throw new Exception(pht('Unrecognized version control system.')); } - $closeable_flag = PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE; - if (!$commit->isPartiallyImported($closeable_flag)) { + if (!$commit->isPermanentCommit()) { return self::BECAUSE_NOT_ON_AUTOCLOSE_BRANCH; } diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index fecb1762bd..cc4745a754 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -496,6 +496,10 @@ final class PhabricatorRepositoryCommit return $this->getAuditStatusObject()->isAudited(); } + public function isPermanentCommit() { + return (bool)$this->isPartiallyImported(self::IMPORTED_CLOSEABLE); + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ public function getCapabilities() { diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php index d5054a7f18..fc5ad874db 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php @@ -30,7 +30,7 @@ final class PhabricatorRepositoryCommitOwnersWorker PhabricatorRepositoryCommit $commit) { $viewer = PhabricatorUser::getOmnipotentUser(); - if (!$repository->shouldPublish()) { + if (!$repository->shouldPublishCommit($commit)) { return; }