Fix several issues with application interactions while importing commits

Summary:
  - Fixes T5851. Currently, if a commit has `Fixes T123`, we generate an email with just that before generating the commit email. Don't send/publish transactions about a commit before it imports (this is a tiny bit hacky, but well-contained and I don't think it causes any problems).
  - Fixes T4864. Currently, we try to parse Differential information even if Differential is not installed. Instead, do this only if Differential is installed.
  - Fixes T5771. Currently, if we can't figure out who the committer/author of a commit is, we don't publish a `Fixes T123` transaction. Instead, fall back to acting as "Diffusion" if we can't find a better actor. Most of this diff expands the role of application actors. The existing application actors (Herald and Harbormaster) seem to be working well.

Test Plan:
  - Pushed a commit with `Fixes T123` and verified it did not generate email directly. (The task half of the transaction still does, correctly.)
  - Uninstalled Differential and pushed a commit, got a clean import instead of an exception.
  - Commented out author/committer PHIDs and pushed stuff, saw a "Diffusion" actor.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5771, T4864, T5851

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

src/applications/audit/editor/PhabricatorAuditEditor.php

@@ -153,7 +153,7 @@ final class PhabricatorAuditEditor
     $status_accepted = PhabricatorAuditStatusConstants::ACCEPTED;
     $status_concerned = PhabricatorAuditStatusConstants::CONCERNED;
 
-    $actor_phid = $this->requireActor()->getPHID();
+    $actor_phid = $this->getActingAsPHID();
     $actor_is_author = ($object->getAuthorPHID()) &&
       ($actor_phid == $object->getAuthorPHID());
 
@@ -317,7 +317,7 @@ final class PhabricatorAuditEditor
     $can_author_close = PhabricatorEnv::getEnvConfig($can_author_close_key);
 
     $actor_is_author = ($object->getAuthorPHID()) &&
-      ($object->getAuthorPHID() == $this->requireActor()->getPHID());
+      ($object->getAuthorPHID() == $this->getActingAsPHID());
 
     switch ($action) {
       case PhabricatorAuditActionConstants::CLOSE:
@@ -348,7 +348,7 @@ final class PhabricatorAuditEditor
   protected function shouldSendMail(
     PhabricatorLiskDAO $object,
     array $xactions) {
-    return true;
+    return $this->isCommitMostlyImported($object);
   }
 
   protected function buildReplyHandler(PhabricatorLiskDAO $object) {
@@ -467,7 +467,20 @@ final class PhabricatorAuditEditor
   protected function shouldPublishFeedStory(
     PhabricatorLiskDAO $object,
     array $xactions) {
-    return true;
+    return $this->isCommitMostlyImported($object);
+  }
+
+  private function isCommitMostlyImported(PhabricatorLiskDAO $object) {
+    $has_message = PhabricatorRepositoryCommit::IMPORTED_MESSAGE;
+    $has_changes = PhabricatorRepositoryCommit::IMPORTED_CHANGE;
+
+    // Don't publish feed stories or email about events which occur during
+    // import. In particular, this affects tasks being attached when they are
+    // closed by "Fixes Txxxx" in a commit message. See T5851.
+
+    $mask = ($has_message | $has_changes);
+
+    return $object->isPartiallyImported($mask);
   }
 
 }

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -98,6 +98,8 @@ final class DifferentialTransactionEditor
     PhabricatorLiskDAO $object,
     PhabricatorApplicationTransaction $xaction) {
 
+    $actor_phid = $this->getActingAsPHID();
+
     switch ($xaction->getTransactionType()) {
       case DifferentialTransaction::TYPE_INLINE:
         return $xaction->hasComment();
@@ -119,7 +121,6 @@ final class DifferentialTransactionEditor
             }
 
             $actor = $this->getActor();
-            $actor_phid = $actor->getPHID();
 
             // These transactions can cause effects in two ways: by altering the
             // status of an existing reviewer; or by adding the actor as a new
@@ -151,7 +152,6 @@ final class DifferentialTransactionEditor
           case DifferentialAction::ACTION_REQUEST:
             return ($object->getStatus() != $status_review);
           case DifferentialAction::ACTION_RESIGN:
-            $actor_phid = $this->getActor()->getPHID();
             foreach ($object->getReviewerStatus() as $reviewer) {
               if ($reviewer->getReviewerPHID() == $actor_phid) {
                 return true;
@@ -159,7 +159,6 @@ final class DifferentialTransactionEditor
             }
             return false;
           case DifferentialAction::ACTION_CLAIM:
-            $actor_phid = $this->getActor()->getPHID();
             return ($actor_phid != $object->getAuthorPHID());
         }
     }
@@ -231,7 +230,7 @@ final class DifferentialTransactionEditor
             $object->setStatus(ArcanistDifferentialRevisionStatus::CLOSED);
             return;
           case DifferentialAction::ACTION_CLAIM:
-            $object->setAuthorPHID($this->getActor()->getPHID());
+            $object->setAuthorPHID($this->getActingAsPHID());
             return;
         }
         break;
@@ -247,7 +246,7 @@ final class DifferentialTransactionEditor
     $results = parent::expandTransaction($object, $xaction);
 
     $actor = $this->getActor();
-    $actor_phid = $actor->getPHID();
+    $actor_phid = $this->getActingAsPHID();
     $type_edge = PhabricatorTransactions::TYPE_EDGE;
 
     $status_plan = ArcanistDifferentialRevisionStatus::CHANGES_PLANNED;
@@ -782,7 +781,7 @@ final class DifferentialTransactionEditor
     $action) {
 
     $author_phid = $revision->getAuthorPHID();
-    $actor_phid = $this->getActor()->getPHID();
+    $actor_phid = $this->getActingAsPHID();
     $actor_is_author = ($author_phid == $actor_phid);
 
     $config_abandon_key = 'differential.always-allow-abandon';

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -410,7 +410,7 @@ final class ManiphestTransactionEditor
   protected function getMailTo(PhabricatorLiskDAO $object) {
     return array(
       $object->getOwnerPHID(),
-      $this->requireActor()->getPHID(),
+      $this->getActingAsPHID(),
     );
   }
 

src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php

@@ -43,20 +43,24 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
         $author_phid);
     }
 
-    $field_values = id(new DiffusionLowLevelCommitFieldsQuery())
-      ->setRepository($repository)
-      ->withCommitRef($ref)
-      ->execute();
-    $revision_id = idx($field_values, 'revisionID');
+    $differential_app = 'PhabricatorDifferentialApplication';
+    $revision_id = null;
+    if (PhabricatorApplication::isClassInstalled($differential_app)) {
+      $field_values = id(new DiffusionLowLevelCommitFieldsQuery())
+        ->setRepository($repository)
+        ->withCommitRef($ref)
+        ->execute();
+      $revision_id = idx($field_values, 'revisionID');
 
-    if (!empty($field_values['reviewedByPHIDs'])) {
-      $data->setCommitDetail(
-        'reviewerPHID',
-        reset($field_values['reviewedByPHIDs']));
+      if (!empty($field_values['reviewedByPHIDs'])) {
+        $data->setCommitDetail(
+          'reviewerPHID',
+          reset($field_values['reviewedByPHIDs']));
+      }
+
+      $data->setCommitDetail('differential.revisionID', $revision_id);
     }
 
-    $data->setCommitDetail('differential.revisionID', $revision_id);
-
     if ($author_phid != $commit->getAuthorPHID()) {
       $commit->setAuthorPHID($author_phid);
     }
@@ -64,6 +68,17 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
     $commit->setSummary($data->getSummary());
     $commit->save();
 
+    // When updating related objects, we'll act under an omnipotent user to
+    // ensure we can see them, but take actions as either the committer or
+    // author (if we recognize their accounts) or the Diffusion application
+    // (if we do not).
+
+    $actor = PhabricatorUser::getOmnipotentUser();
+    $acting_as_phid = nonempty(
+      $committer_phid,
+      $author_phid,
+      id(new PhabricatorDiffusionApplication())->getPHID());
+
     $conn_w = id(new DifferentialRevision())->establishConnection('w');
 
     // NOTE: The `differential_commit` table has a unique ID on `commitPHID`,
@@ -78,10 +93,9 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
     $should_autoclose = $repository->shouldAutocloseCommit($commit, $data);
 
     if ($revision_id) {
-      // TODO: Check if a more restrictive viewer could be set here
       $revision_query = id(new DifferentialRevisionQuery())
         ->withIDs(array($revision_id))
-        ->setViewer(PhabricatorUser::getOmnipotentUser())
+        ->setViewer($actor)
         ->needReviewerStatus(true)
         ->needActiveDiffs(true);
 
@@ -105,14 +119,6 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
                         $should_autoclose;
 
         if ($should_close) {
-          $actor_phid = nonempty(
-            $committer_phid,
-            $author_phid,
-            $revision->getAuthorPHID());
-
-          $actor = id(new PhabricatorUser())
-            ->loadOneWhere('phid = %s', $actor_phid);
-
           $commit_name = $repository->formatCommitName(
             $commit->getCommitIdentifier());
 
@@ -139,7 +145,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
               $author_name);
           }
 
-          $diff = $this->generateFinalDiff($revision, $actor_phid);
+          $diff = $this->generateFinalDiff($revision, $acting_as_phid);
 
           $vs_diff = $this->loadChangedByCommit($revision, $diff);
           $changed_uri = null;
@@ -177,6 +183,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
 
           $editor = id(new DifferentialTransactionEditor())
             ->setActor($actor)
+            ->setActingAsPHID($acting_as_phid)
             ->setContinueOnMissingFields(true)
             ->setContentSource($content_source)
             ->setChangedPriorToCommitURI($changed_uri)
@@ -195,10 +202,12 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
     }
 
     if ($should_autoclose) {
-      // TODO: This isn't as general as it could be.
-      if ($user->getPHID()) {
-        $this->closeTasks($user, $repository, $commit, $message);
-      }
+      $this->closeTasks(
+        $actor,
+        $acting_as_phid,
+        $repository,
+        $commit,
+        $message);
     }
 
     $data->save();
@@ -405,6 +414,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
 
   private function closeTasks(
     PhabricatorUser $actor,
+    $acting_as,
     PhabricatorRepository $repository,
     PhabricatorRepositoryCommit $commit,
     $message) {
@@ -487,6 +497,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
 
       $editor = id(new ManiphestTransactionEditor())
         ->setActor($actor)
+        ->setActingAsPHID($acting_as)
         ->setContinueOnNoEffect(true)
         ->setContinueOnMissingFields(true)
         ->setContentSource($content_source);

src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php

@@ -4,6 +4,19 @@ final class PhabricatorApplicationTransactionCommentEditor
   extends PhabricatorEditor {
 
   private $contentSource;
+  private $actingAsPHID;
+
+  public function setActingAsPHID($acting_as_phid) {
+    $this->actingAsPHID = $acting_as_phid;
+    return $this;
+  }
+
+  public function getActingAsPHID() {
+    if ($this->actingAsPHID) {
+      return $this->actingAsPHID;
+    }
+    return $this->getActor()->getPHID();
+  }
 
   public function setContentSource(PhabricatorContentSource $content_source) {
     $this->contentSource = $content_source;
@@ -27,11 +40,11 @@ final class PhabricatorApplicationTransactionCommentEditor
     $actor = $this->requireActor();
 
     $comment->setContentSource($this->getContentSource());
-    $comment->setAuthorPHID($actor->getPHID());
+    $comment->setAuthorPHID($this->getActingAsPHID());
 
     // TODO: This needs to be more sophisticated once we have meta-policies.
     $comment->setViewPolicy(PhabricatorPolicies::POLICY_PUBLIC);
-    $comment->setEditPolicy($actor->getPHID());
+    $comment->setEditPolicy($this->getActingAsPHID());
 
     $xaction->openTransaction();
       $xaction->beginReadLocking();

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -479,7 +479,7 @@ abstract class PhabricatorApplicationTransactionEditor
     if ($actor->isOmnipotent()) {
       $xaction->setEditPolicy(PhabricatorPolicies::POLICY_NOONE);
     } else {
-      $xaction->setEditPolicy($actor->getPHID());
+      $xaction->setEditPolicy($this->getActingAsPHID());
     }
 
     $xaction->setAuthorPHID($this->getActingAsPHID());
@@ -644,6 +644,7 @@ abstract class PhabricatorApplicationTransactionEditor
 
     $comment_editor = id(new PhabricatorApplicationTransactionCommentEditor())
       ->setActor($actor)
+      ->setActingAsPHID($this->getActingAsPHID())
       ->setContentSource($this->getContentSource());
 
     if (!$transaction_open) {
@@ -1717,7 +1718,15 @@ abstract class PhabricatorApplicationTransactionEditor
       return $xactions;
     }
 
-    $actor_phid = $this->requireActor()->getPHID();
+    $actor_phid = $this->getActingAsPHID();
+
+    $type_user = PhabricatorPeopleUserPHIDType::TYPECONST;
+    if (phid_get_type($actor_phid) != $type_user) {
+      // Transactions by application actors like Herald, Harbormaster and
+      // Diffusion should not CC the applications.
+      return $xactions;
+    }
+
     if ($object->isAutomaticallySubscribed($actor_phid)) {
       // If they're auto-subscribed, don't CC them.
       return $xactions;
@@ -1827,7 +1836,7 @@ abstract class PhabricatorApplicationTransactionEditor
     }
 
     $template
-      ->setFrom($this->requireActor()->getPHID())
+      ->setFrom($this->getActingAsPHID())
       ->setSubjectPrefix($this->getMailSubjectPrefix())
       ->setVarySubjectPrefix('['.$action.']')
       ->setThreadID($this->getMailThreadID($object), $this->getIsNewObject())
@@ -2089,7 +2098,7 @@ abstract class PhabricatorApplicationTransactionEditor
 
     return array(
       $object->getPHID(),
-      $this->requireActor()->getPHID(),
+      $this->getActingAsPHID(),
     );
   }
 
@@ -2148,7 +2157,7 @@ abstract class PhabricatorApplicationTransactionEditor
       ->setStoryType($story_type)
       ->setStoryData($story_data)
       ->setStoryTime(time())
-      ->setStoryAuthorPHID($this->requireActor()->getPHID())
+      ->setStoryAuthorPHID($this->getActingAsPHID())
       ->setRelatedPHIDs($related_phids)
       ->setPrimaryObjectPHID($object->getPHID())
       ->setSubscribedPHIDs($subscribed_phids)
@@ -2394,6 +2403,7 @@ abstract class PhabricatorApplicationTransactionEditor
         ->setParentMessageID($this->getParentMessageID())
         ->setIsInverseEdgeEditor(true)
         ->setActor($this->requireActor())
+        ->setActingAsPHID($this->getActingAsPHID())
         ->setContentSource($this->getContentSource());
 
       $editor->applyTransactions($target, array($template));