Add "is merge commit" Herald field for pre-commit rules

Summary:
Ref T4195. This allows you to write rules which disallow merge commits.

Also make the reject message a little more useful.

Test Plan:
  remote: This push was rejected by Herald push rule H27.
  remote: Change: commit/daed0d448404
  remote:   Rule: No Merges
  remote: Reason: No merge commits allowed. If you must push a merge, include "@force-merge" in the commit message.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7809

src/applications/diffusion/engine/DiffusionCommitHookEngine.php

@@ -259,6 +259,7 @@ final class DiffusionCommitHookEngine extends Phobject {
     $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 @@ final class DiffusionCommitHookEngine extends Phobject {
         foreach ($effects as $effect) {
           if ($effect->getAction() == HeraldAdapter::ACTION_BLOCK) {
             $blocking_effect = $effect;
+            $blocked_update = $update;
             break;
           }
         }
@@ -300,12 +302,19 @@ final class DiffusionCommitHookEngine extends Phobject {
         $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".
+          "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));
     }

src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php

@@ -53,6 +53,7 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter {
         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 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter {
           return array();
         }
         return $revision->getCCPHIDs();
+      case self::FIELD_IS_MERGE_COMMIT:
+        return $this->getIsMergeCommit();
     }
 
     return parent::getHeraldField($field);
@@ -306,4 +309,24 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter {
     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;
+    }
+  }
+
 }

src/applications/herald/adapter/HeraldAdapter.php

@@ -31,6 +31,7 @@ abstract class HeraldAdapter {
   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 @@ abstract class HeraldAdapter {
   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 @@ abstract class HeraldAdapter {
       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 @@ abstract class HeraldAdapter {
       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 @@ abstract class HeraldAdapter {
           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 @@ abstract class HeraldAdapter {
           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 @@ abstract class HeraldAdapter {
       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 @@ abstract class HeraldAdapter {
       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: