Index: src/applications/diffusion/engine/DiffusionCommitHookEngine.php =================================================================== --- src/applications/diffusion/engine/DiffusionCommitHookEngine.php +++ src/applications/diffusion/engine/DiffusionCommitHookEngine.php @@ -259,6 +259,7 @@ $engine = new HeraldEngine(); $rules = null; $blocking_effect = null; + $blocked_update = null; foreach ($updates as $update) { $adapter = id(clone $adapter_template) ->setPushLog($update); @@ -275,6 +276,7 @@ foreach ($effects as $effect) { if ($effect->getAction() == HeraldAdapter::ACTION_BLOCK) { $blocking_effect = $effect; + $blocked_update = $update; break; } } @@ -300,12 +302,19 @@ $rule_name = pht('Unnamed Herald Rule'); } + $blocked_ref_name = coalesce( + $blocked_update->getRefName(), + $blocked_update->getRefNewShort()); + $blocked_name = $blocked_update->getRefType().'/'.$blocked_ref_name; + throw new DiffusionCommitHookRejectException( pht( - "This commit was rejected by Herald pre-commit rule %s.\n". - "Rule: %s\n". + "This push was rejected by Herald push rule %s.\n". + "Change: %s\n". + " Rule: %s\n". "Reason: %s", 'H'.$blocking_effect->getRuleID(), + $blocked_name, $rule_name, $message)); } Index: src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php =================================================================== --- src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php +++ src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php @@ -53,6 +53,7 @@ self::FIELD_DIFFERENTIAL_ACCEPTED, self::FIELD_DIFFERENTIAL_REVIEWERS, self::FIELD_DIFFERENTIAL_CCS, + self::FIELD_IS_MERGE_COMMIT, self::FIELD_RULE, ), parent::getFields()); @@ -141,6 +142,8 @@ return array(); } return $revision->getCCPHIDs(); + case self::FIELD_IS_MERGE_COMMIT: + return $this->getIsMergeCommit(); } return parent::getHeraldField($field); @@ -306,4 +309,24 @@ return $this->revision; } + private function getIsMergeCommit() { + $repository = $this->hookEngine->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()) + ->execute(); + + return (count($parents) > 1); + case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: + // NOTE: For now, we ignore "svn:mergeinfo" at all levels. We might + // change this some day, but it's not nearly as clear a signal as + // ancestry is in Git/Mercurial. + return false; + } + } + } Index: src/applications/herald/adapter/HeraldAdapter.php =================================================================== --- src/applications/herald/adapter/HeraldAdapter.php +++ src/applications/herald/adapter/HeraldAdapter.php @@ -31,6 +31,7 @@ const FIELD_DIFFERENTIAL_REVIEWERS = 'differential-reviewers'; const FIELD_DIFFERENTIAL_CCS = 'differential-ccs'; const FIELD_DIFFERENTIAL_ACCEPTED = 'differential-accepted'; + const FIELD_IS_MERGE_COMMIT = 'is-merge-commit'; const CONDITION_CONTAINS = 'contains'; const CONDITION_NOT_CONTAINS = '!contains'; @@ -52,6 +53,8 @@ const CONDITION_REGEXP_PAIR = 'regexp-pair'; const CONDITION_HAS_BIT = 'bit'; const CONDITION_NOT_BIT = '!bit'; + const CONDITION_IS_TRUE = 'true'; + const CONDITION_IS_FALSE = 'false'; const ACTION_ADD_CC = 'addcc'; const ACTION_REMOVE_CC = 'remcc'; @@ -172,6 +175,7 @@ self::FIELD_DIFFERENTIAL_CCS => pht('Differential CCs'), self::FIELD_DIFFERENTIAL_ACCEPTED => pht('Accepted Differential revision'), + self::FIELD_IS_MERGE_COMMIT => pht('Commit is a merge'), ); } @@ -186,6 +190,8 @@ self::CONDITION_IS => pht('is'), self::CONDITION_IS_NOT => pht('is not'), self::CONDITION_IS_ANY => pht('is any of'), + self::CONDITION_IS_TRUE => pht('is true'), + self::CONDITION_IS_FALSE => pht('is false'), self::CONDITION_IS_NOT_ANY => pht('is not any of'), self::CONDITION_INCLUDE_ALL => pht('include all of'), self::CONDITION_INCLUDE_ANY => pht('include any of'), @@ -292,6 +298,11 @@ self::CONDITION_EXISTS, self::CONDITION_NOT_EXISTS, ); + case self::FIELD_IS_MERGE_COMMIT: + return array( + self::CONDITION_IS_TRUE, + self::CONDITION_IS_FALSE, + ); default: throw new Exception( "This adapter does not define conditions for field '{$field}'!"); @@ -362,8 +373,10 @@ array_fuse($field_value), $condition_value); case self::CONDITION_EXISTS: + case self::CONDITION_IS_TRUE: return (bool)$field_value; case self::CONDITION_NOT_EXISTS: + case self::CONDITION_IS_FALSE: return !$field_value; case self::CONDITION_UNCONDITIONALLY: return (bool)$field_value; @@ -515,6 +528,8 @@ case self::CONDITION_UNCONDITIONALLY: case self::CONDITION_HAS_BIT: case self::CONDITION_NOT_BIT: + case self::CONDITION_IS_TRUE: + case self::CONDITION_IS_FALSE: // No explicit validation for these types, although there probably // should be in some cases. break; @@ -669,6 +684,8 @@ case self::CONDITION_EXISTS: case self::CONDITION_NOT_EXISTS: case self::CONDITION_UNCONDITIONALLY: + case self::CONDITION_IS_TRUE: + case self::CONDITION_IS_FALSE: return self::VALUE_NONE; case self::CONDITION_RULE: case self::CONDITION_NOT_RULE: