From 7fa0d066bc459e47e7381476c6fd708355cb9104 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 31 Oct 2017 10:29:33 -0700
Subject: [PATCH] Don't run Herald build rules when Differential revisions are
 updated automatically

Summary: Ref T2543. After D18731, Herald build rules run more often, but now incorrectly try to run builds when Diffusion closes a revision because a commit landed.

Test Plan: Made some mundane updates locally; this is tricky to test comprehensively locally so I'm mostly planning to just push it to `secure`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18745
---
 .../editor/DifferentialTransactionEditor.php  | 24 +++++++++++++++----
 .../herald/DifferentialHeraldStateReasons.php |  4 ++++
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
index edc8fd0ad3..70c3272a83 100644
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -1204,16 +1204,32 @@ final class DifferentialTransactionEditor
     // edited the title or changed subscribers), prevent "Run build plan"
     // and other similar rules from acting yet, since the build results will
     // not (or, at least, should not) change unless the actual source changes.
+    // We also don't run Differential builds if the update was caused by
+    // discovering a commit, as the expectation is that Diffusion builds take
+    // over once things land.
     $has_update = false;
+    $has_commit = false;
+
     $type_update = DifferentialTransaction::TYPE_UPDATE;
     foreach ($xactions as $xaction) {
-      if ($xaction->getTransactionType() == $type_update) {
-        $has_update = true;
-        break;
+      if ($xaction->getTransactionType() != $type_update) {
+        continue;
       }
+
+      if ($xaction->getMetadataValue('isCommitUpdate')) {
+        $has_commit = true;
+      } else {
+        $has_update = true;
+      }
+
+      break;
     }
 
-    if (!$has_update) {
+    if ($has_commit) {
+      $adapter->setForbiddenAction(
+        HeraldBuildableState::STATECONST,
+        DifferentialHeraldStateReasons::REASON_LANDED);
+    } else if (!$has_update) {
       $adapter->setForbiddenAction(
         HeraldBuildableState::STATECONST,
         DifferentialHeraldStateReasons::REASON_UNCHANGED);
diff --git a/src/applications/differential/herald/DifferentialHeraldStateReasons.php b/src/applications/differential/herald/DifferentialHeraldStateReasons.php
index d3259560d5..92d2ca3067 100644
--- a/src/applications/differential/herald/DifferentialHeraldStateReasons.php
+++ b/src/applications/differential/herald/DifferentialHeraldStateReasons.php
@@ -5,6 +5,7 @@ final class DifferentialHeraldStateReasons
 
   const REASON_DRAFT = 'differential.draft';
   const REASON_UNCHANGED = 'differential.unchanged';
+  const REASON_LANDED = 'differential.landed';
 
   public function explainReason($reason) {
     $reasons = array(
@@ -14,6 +15,9 @@ final class DifferentialHeraldStateReasons
       self::REASON_UNCHANGED => pht(
         'The update which triggered Herald did not update the diff for '.
         'this revision, so builds will not run.'),
+      self::REASON_LANDED => pht(
+        'The update which triggered Herald was an automatic update in '.
+        'response to discovering a commit, so builds will not run.'),
     );
 
     return idx($reasons, $reason);