From 2cfc3acf323469ce0dff293ae2c55954c99e0cb8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 3 Jan 2014 12:24:28 -0800 Subject: [PATCH] Allow Herald pre-commit rules to act on repository projects Summary: Fixes T4264. Adds: - New "Repository's projects" field to Herald pre-commit rules, so you can write global rules which act based on projects. - Allows pre-ref/pre-content rules to bind to projects, and fire for all repositories in that project, so users with limited power can write rules which apply to many repositories. - The pre-ref and pre-content classes were starting to share a fair amount of code, so I made them both extend an abstract base class. Test Plan: Wrote new pre-ref and pre-content rules bound to projects, then pushed commits into repositories in those projects and not in those projects. The "repository projects" field populated, and the rules fired for repositories in the relevant projects. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4264 Differential Revision: https://secure.phabricator.com/D7883 --- scripts/repository/commit_hook.php | 1 + src/__phutil_library_map__.php | 6 +- .../herald/HeraldPreCommitAdapter.php | 113 +++++++++++++ .../herald/HeraldPreCommitContentAdapter.php | 157 +++--------------- .../herald/HeraldPreCommitRefAdapter.php | 111 +------------ .../herald/adapter/HeraldAdapter.php | 4 + .../herald/controller/HeraldNewController.php | 3 +- .../herald/storage/HeraldRule.php | 2 +- 8 files changed, 158 insertions(+), 239 deletions(-) create mode 100644 src/applications/diffusion/herald/HeraldPreCommitAdapter.php diff --git a/scripts/repository/commit_hook.php b/scripts/repository/commit_hook.php index 386075111d..6bf3c6e844 100755 --- a/scripts/repository/commit_hook.php +++ b/scripts/repository/commit_hook.php @@ -13,6 +13,7 @@ $engine = new DiffusionCommitHookEngine(); $repository = id(new PhabricatorRepositoryQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withCallsigns(array($argv[1])) + ->needProjectPHIDs(true) ->executeOne(); if (!$repository) { diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b31b9e6059..2547083c65 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -778,6 +778,7 @@ phutil_register_library_map(array( 'HeraldObjectTranscript' => 'applications/herald/storage/transcript/HeraldObjectTranscript.php', 'HeraldPHIDTypeRule' => 'applications/herald/phid/HeraldPHIDTypeRule.php', 'HeraldPholioMockAdapter' => 'applications/herald/adapter/HeraldPholioMockAdapter.php', + 'HeraldPreCommitAdapter' => 'applications/diffusion/herald/HeraldPreCommitAdapter.php', 'HeraldPreCommitContentAdapter' => 'applications/diffusion/herald/HeraldPreCommitContentAdapter.php', 'HeraldPreCommitRefAdapter' => 'applications/diffusion/herald/HeraldPreCommitRefAdapter.php', 'HeraldRecursiveConditionsException' => 'applications/herald/engine/exception/HeraldRecursiveConditionsException.php', @@ -3251,8 +3252,9 @@ phutil_register_library_map(array( 'HeraldNewController' => 'HeraldController', 'HeraldPHIDTypeRule' => 'PhabricatorPHIDType', 'HeraldPholioMockAdapter' => 'HeraldAdapter', - 'HeraldPreCommitContentAdapter' => 'HeraldAdapter', - 'HeraldPreCommitRefAdapter' => 'HeraldAdapter', + 'HeraldPreCommitAdapter' => 'HeraldAdapter', + 'HeraldPreCommitContentAdapter' => 'HeraldPreCommitAdapter', + 'HeraldPreCommitRefAdapter' => 'HeraldPreCommitAdapter', 'HeraldRecursiveConditionsException' => 'Exception', 'HeraldRemarkupRule' => 'PhabricatorRemarkupRuleObject', 'HeraldRule' => diff --git a/src/applications/diffusion/herald/HeraldPreCommitAdapter.php b/src/applications/diffusion/herald/HeraldPreCommitAdapter.php new file mode 100644 index 0000000000..7bb908373c --- /dev/null +++ b/src/applications/diffusion/herald/HeraldPreCommitAdapter.php @@ -0,0 +1,113 @@ +log = $log; + return $this; + } + + public function setHookEngine(DiffusionCommitHookEngine $engine) { + $this->hookEngine = $engine; + return $this; + } + + public function getHookEngine() { + return $this->hookEngine; + } + + public function getAdapterApplicationClass() { + return 'PhabricatorApplicationDiffusion'; + } + + public function getObject() { + return $this->log; + } + + public function supportsRuleType($rule_type) { + switch ($rule_type) { + case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: + case HeraldRuleTypeConfig::RULE_TYPE_OBJECT: + return true; + case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: + default: + return false; + } + } + + public function canTriggerOnObject($object) { + if ($object instanceof PhabricatorRepository) { + return true; + } + + if ($object instanceof PhabricatorProject) { + return true; + } + + return false; + } + + public function explainValidTriggerObjects() { + return pht( + 'This rule can trigger for **repositories** or **projects**.'); + } + + public function getTriggerObjectPHIDs() { + return array_merge( + array( + $this->hookEngine->getRepository()->getPHID(), + $this->getPHID(), + ), + $this->hookEngine->getRepository()->getProjectPHIDs()); + } + + public function getActions($rule_type) { + switch ($rule_type) { + case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: + case HeraldRuleTypeConfig::RULE_TYPE_OBJECT: + return array( + self::ACTION_BLOCK, + self::ACTION_NOTHING + ); + case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: + return array( + self::ACTION_NOTHING, + ); + } + } + + public function getPHID() { + return $this->getObject()->getPHID(); + } + + public function applyHeraldEffects(array $effects) { + assert_instances_of($effects, 'HeraldEffect'); + + $result = array(); + foreach ($effects as $effect) { + $action = $effect->getAction(); + switch ($action) { + case self::ACTION_NOTHING: + $result[] = new HeraldApplyTranscript( + $effect, + true, + pht('Did nothing.')); + break; + case self::ACTION_BLOCK: + $result[] = new HeraldApplyTranscript( + $effect, + true, + pht('Blocked push.')); + break; + default: + throw new Exception(pht('No rules to handle action "%s"!', $action)); + } + } + + return $result; + } + +} diff --git a/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php b/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php index d0472bdaa8..d1232b68bb 100644 --- a/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php +++ b/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php @@ -1,32 +1,12 @@ log = $log; - return $this; - } - - public function setHookEngine(DiffusionCommitHookEngine $engine) { - $this->hookEngine = $engine; - return $this; - } - - public function getAdapterApplicationClass() { - return 'PhabricatorApplicationDiffusion'; - } - - public function getObject() { - return $this->log; - } - public function getAdapterContentName() { return pht('Commit Hook: Commit Content'); } @@ -41,41 +21,6 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter { "Hook rules can block changes."); } - public function supportsRuleType($rule_type) { - switch ($rule_type) { - case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: - case HeraldRuleTypeConfig::RULE_TYPE_OBJECT: - return true; - case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: - default: - return false; - } - } - - public function canTriggerOnObject($object) { - if ($object instanceof PhabricatorRepository) { - return true; - } - return false; - } - - public function explainValidTriggerObjects() { - return pht( - 'This rule can trigger for **repositories**.'); - } - - public function getTriggerObjectPHIDs() { - return array( - $this->hookEngine->getRepository()->getPHID(), - $this->getPHID(), - ); - } - - public function getFieldNameMap() { - return array( - ) + parent::getFieldNameMap(); - } - public function getFields() { return array_merge( array( @@ -90,6 +35,7 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter { self::FIELD_DIFF_ADDED_CONTENT, self::FIELD_DIFF_REMOVED_CONTENT, self::FIELD_REPOSITORY, + self::FIELD_REPOSITORY_PROJECTS, self::FIELD_PUSHER, self::FIELD_PUSHER_PROJECTS, self::FIELD_DIFFERENTIAL_REVISION, @@ -102,37 +48,8 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter { parent::getFields()); } - public function getConditionsForField($field) { - switch ($field) { - } - return parent::getConditionsForField($field); - } - - public function getActions($rule_type) { - switch ($rule_type) { - case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: - case HeraldRuleTypeConfig::RULE_TYPE_OBJECT: - return array( - self::ACTION_BLOCK, - self::ACTION_NOTHING - ); - case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: - return array( - self::ACTION_NOTHING, - ); - } - } - - public function getValueTypeForFieldAndCondition($field, $condition) { - return parent::getValueTypeForFieldAndCondition($field, $condition); - } - - public function getPHID() { - return $this->getObject()->getPHID(); - } - public function getHeraldName() { - return pht('Push Log'); + return pht('Push Log (Content)'); } public function getHeraldField($field) { @@ -159,11 +76,13 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter { case self::FIELD_DIFF_REMOVED_CONTENT: return $this->getDiffContent('-'); case self::FIELD_REPOSITORY: - return $this->hookEngine->getRepository()->getPHID(); + return $this->getHookEngine()->getRepository()->getPHID(); + case self::FIELD_REPOSITORY_PROJECTS: + return $this->getHookEngine()->getRepository()->getProjectPHIDs(); case self::FIELD_PUSHER: - return $this->hookEngine->getViewer()->getPHID(); + return $this->getHookEngine()->getViewer()->getPHID(); case self::FIELD_PUSHER_PROJECTS: - return $this->hookEngine->loadViewerProjectPHIDsForHerald(); + return $this->getHookEngine()->loadViewerProjectPHIDsForHerald(); case self::FIELD_DIFFERENTIAL_REVISION: $revision = $this->getRevision(); if (!$revision) { @@ -199,38 +118,11 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter { return parent::getHeraldField($field); } - public function applyHeraldEffects(array $effects) { - assert_instances_of($effects, 'HeraldEffect'); - - $result = array(); - foreach ($effects as $effect) { - $action = $effect->getAction(); - switch ($action) { - case self::ACTION_NOTHING: - $result[] = new HeraldApplyTranscript( - $effect, - true, - pht('Did nothing.')); - break; - case self::ACTION_BLOCK: - $result[] = new HeraldApplyTranscript( - $effect, - true, - pht('Blocked push.')); - break; - default: - throw new Exception(pht('No rules to handle action "%s"!', $action)); - } - } - - return $result; - } - private function getDiffContent($type) { if ($this->changesets === null) { try { - $this->changesets = $this->hookEngine->loadChangesetsForCommit( - $this->log->getRefNew()); + $this->changesets = $this->getHookEngine()->loadChangesetsForCommit( + $this->getObject()->getRefNew()); } catch (Exception $ex) { $this->changesets = $ex; } @@ -277,14 +169,14 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter { private function getCommitRef() { if ($this->commitRef === null) { - $this->commitRef = $this->hookEngine->loadCommitRefForCommit( - $this->log->getRefNew()); + $this->commitRef = $this->getHookEngine()->loadCommitRefForCommit( + $this->getObject()->getRefNew()); } return $this->commitRef; } private function getAuthorPHID() { - $repository = $this->hookEngine->getRepository(); + $repository = $this->getHookEngine()->getRepository(); $vcs = $repository->getVersionControlSystem(); switch ($vcs) { case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: @@ -297,12 +189,12 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter { return $this->lookupUser($author); case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: // In Subversion, the pusher is always the author. - return $this->hookEngine->getViewer()->getPHID(); + return $this->getHookEngine()->getViewer()->getPHID(); } } private function getCommitterPHID() { - $repository = $this->hookEngine->getRepository(); + $repository = $this->getHookEngine()->getRepository(); $vcs = $repository->getVersionControlSystem(); switch ($vcs) { case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: @@ -321,12 +213,12 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter { return $phid; case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: // In Subversion, the pusher is always the committer. - return $this->hookEngine->getViewer()->getPHID(); + return $this->getHookEngine()->getViewer()->getPHID(); } } private function getAuthorRaw() { - $repository = $this->hookEngine->getRepository(); + $repository = $this->getHookEngine()->getRepository(); $vcs = $repository->getVersionControlSystem(); switch ($vcs) { case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: @@ -335,12 +227,12 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter { return $ref->getAuthor(); case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: // In Subversion, the pusher is always the author. - return $this->hookEngine->getViewer()->getUsername(); + return $this->getHookEngine()->getViewer()->getUsername(); } } private function getCommitterRaw() { - $repository = $this->hookEngine->getRepository(); + $repository = $this->getHookEngine()->getRepository(); $vcs = $repository->getVersionControlSystem(); switch ($vcs) { case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: @@ -355,7 +247,7 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter { return $ref->getAuthor(); case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: // In Subversion, the pusher is always the committer. - return $this->hookEngine->getViewer()->getUsername(); + return $this->getHookEngine()->getViewer()->getUsername(); } } @@ -368,7 +260,7 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter { private function getCommitFields() { if ($this->fields === null) { $this->fields = id(new DiffusionLowLevelCommitFieldsQuery()) - ->setRepository($this->hookEngine->getRepository()) + ->setRepository($this->getHookEngine()->getRepository()) ->withCommitRef($this->getCommitRef()) ->execute(); } @@ -394,14 +286,14 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter { } private function getIsMergeCommit() { - $repository = $this->hookEngine->getRepository(); + $repository = $this->getHookEngine()->getRepository(); $vcs = $repository->getVersionControlSystem(); switch ($vcs) { case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: $parents = id(new DiffusionLowLevelParentsQuery()) ->setRepository($repository) - ->withIdentifier($this->log->getRefNew()) + ->withIdentifier($this->getObject()->getRefNew()) ->execute(); return (count($parents) > 1); @@ -414,7 +306,8 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter { } private function getBranches() { - return $this->hookEngine->loadBranches($this->log->getRefNew()); + return $this->getHookEngine()->loadBranches( + $this->getObject()->getRefNew()); } } diff --git a/src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php b/src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php index dbbf6f6596..75ae07a5e9 100644 --- a/src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php +++ b/src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php @@ -1,9 +1,6 @@ log = $log; - return $this; - } - - public function setHookEngine(DiffusionCommitHookEngine $engine) { - $this->hookEngine = $engine; - return $this; - } - - public function getAdapterApplicationClass() { - return 'PhabricatorApplicationDiffusion'; - } - - public function getObject() { - return $this->log; - } - public function getAdapterContentName() { return pht('Commit Hook: Branches/Tags/Bookmarks'); } @@ -44,36 +23,6 @@ final class HeraldPreCommitRefAdapter extends HeraldAdapter { "Hook rules can block changes."); } - public function supportsRuleType($rule_type) { - switch ($rule_type) { - case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: - case HeraldRuleTypeConfig::RULE_TYPE_OBJECT: - return true; - case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: - default: - return false; - } - } - - public function canTriggerOnObject($object) { - if ($object instanceof PhabricatorRepository) { - return true; - } - return false; - } - - public function explainValidTriggerObjects() { - return pht( - 'This rule can trigger for **repositories**.'); - } - - public function getTriggerObjectPHIDs() { - return array( - $this->hookEngine->getRepository()->getPHID(), - $this->getPHID(), - ); - } - public function getFieldNameMap() { return array( self::FIELD_REF_TYPE => pht('Ref type'), @@ -89,6 +38,7 @@ final class HeraldPreCommitRefAdapter extends HeraldAdapter { self::FIELD_REF_NAME, self::FIELD_REF_CHANGE, self::FIELD_REPOSITORY, + self::FIELD_REPOSITORY_PROJECTS, self::FIELD_PUSHER, self::FIELD_PUSHER_PROJECTS, self::FIELD_RULE, @@ -119,21 +69,6 @@ final class HeraldPreCommitRefAdapter extends HeraldAdapter { return parent::getConditionsForField($field); } - public function getActions($rule_type) { - switch ($rule_type) { - case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: - case HeraldRuleTypeConfig::RULE_TYPE_OBJECT: - return array( - self::ACTION_BLOCK, - self::ACTION_NOTHING - ); - case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: - return array( - self::ACTION_NOTHING, - ); - } - } - public function getValueTypeForFieldAndCondition($field, $condition) { switch ($field) { case self::FIELD_REF_TYPE: @@ -145,12 +80,8 @@ final class HeraldPreCommitRefAdapter extends HeraldAdapter { return parent::getValueTypeForFieldAndCondition($field, $condition); } - public function getPHID() { - return $this->getObject()->getPHID(); - } - public function getHeraldName() { - return pht('Push Log'); + return pht('Push Log (Ref)'); } public function getHeraldField($field) { @@ -163,42 +94,16 @@ final class HeraldPreCommitRefAdapter extends HeraldAdapter { case self::FIELD_REF_CHANGE: return $log->getChangeFlags(); case self::FIELD_REPOSITORY: - return $this->hookEngine->getRepository()->getPHID(); + return $this->getHookEngine()->getRepository()->getPHID(); + case self::FIELD_REPOSITORY_PROJECTS: + return $this->getHookEngine()->getRepository()->getProjectPHIDs(); case self::FIELD_PUSHER: - return $this->hookEngine->getViewer()->getPHID(); + return $this->getHookEngine()->getViewer()->getPHID(); case self::FIELD_PUSHER_PROJECTS: - return $this->hookEngine->loadViewerProjectPHIDsForHerald(); + return $this->getHookEngine()->loadViewerProjectPHIDsForHerald(); } return parent::getHeraldField($field); } - - public function applyHeraldEffects(array $effects) { - assert_instances_of($effects, 'HeraldEffect'); - - $result = array(); - foreach ($effects as $effect) { - $action = $effect->getAction(); - switch ($action) { - case self::ACTION_NOTHING: - $result[] = new HeraldApplyTranscript( - $effect, - true, - pht('Did nothing.')); - break; - case self::ACTION_BLOCK: - $result[] = new HeraldApplyTranscript( - $effect, - true, - pht('Blocked push.')); - break; - default: - throw new Exception(pht('No rules to handle action "%s"!', $action)); - } - } - - return $result; - } - } diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 1a2fc9a378..4f753ea05a 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -19,6 +19,7 @@ abstract class HeraldAdapter { const FIELD_DIFF_ADDED_CONTENT = 'diff-added-content'; const FIELD_DIFF_REMOVED_CONTENT = 'diff-removed-content'; const FIELD_REPOSITORY = 'repository'; + const FIELD_REPOSITORY_PROJECTS = 'repository-projects'; const FIELD_RULE = 'rule'; const FIELD_AFFECTED_PACKAGE = 'affected-package'; const FIELD_AFFECTED_PACKAGE_OWNER = 'affected-package-owner'; @@ -193,6 +194,7 @@ abstract class HeraldAdapter { self::FIELD_DIFF_ADDED_CONTENT => pht('Any added file content'), self::FIELD_DIFF_REMOVED_CONTENT => pht('Any removed file content'), self::FIELD_REPOSITORY => pht('Repository'), + self::FIELD_REPOSITORY_PROJECTS => pht('Repository\'s projects'), self::FIELD_RULE => pht('Another Herald rule'), self::FIELD_AFFECTED_PACKAGE => pht('Any affected package'), self::FIELD_AFFECTED_PACKAGE_OWNER => @@ -283,6 +285,7 @@ abstract class HeraldAdapter { case self::FIELD_AFFECTED_PACKAGE: case self::FIELD_AFFECTED_PACKAGE_OWNER: case self::FIELD_PUSHER_PROJECTS: + case self::FIELD_REPOSITORY_PROJECTS: return array( self::CONDITION_INCLUDE_ALL, self::CONDITION_INCLUDE_ANY, @@ -713,6 +716,7 @@ abstract class HeraldAdapter { case self::FIELD_AUTHOR_PROJECTS: case self::FIELD_PUSHER_PROJECTS: case self::FIELD_PROJECTS: + case self::FIELD_REPOSITORY_PROJECTS: return self::VALUE_PROJECT; case self::FIELD_REVIEWERS: return self::VALUE_USER_OR_PROJECT; diff --git a/src/applications/herald/controller/HeraldNewController.php b/src/applications/herald/controller/HeraldNewController.php index bfc37d9edc..94d19d3765 100644 --- a/src/applications/herald/controller/HeraldNewController.php +++ b/src/applications/herald/controller/HeraldNewController.php @@ -172,7 +172,8 @@ final class HeraldNewController extends HeraldController { ->appendRemarkupInstructions( pht( 'Choose the object this rule will act on (for example, enter '. - '`rX` to act on the `rX` repository).')) + '`rX` to act on the `rX` repository, or `#project` to act on '. + 'a project).')) ->appendRemarkupInstructions( $adapter->explainValidTriggerObjects()) ->appendChild( diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php index 2eb442a8c7..9132ad9ce0 100644 --- a/src/applications/herald/storage/HeraldRule.php +++ b/src/applications/herald/storage/HeraldRule.php @@ -17,7 +17,7 @@ final class HeraldRule extends HeraldDAO protected $isDisabled = 0; protected $triggerObjectPHID; - protected $configVersion = 23; + protected $configVersion = 24; // phids for which this rule has been applied private $ruleApplied = self::ATTACHABLE;