Use TransactionEditor when closing revisions in response to commits

Summary: Ref T2222. When we discover a commit associated with a revision, close it using modern transactions.

Test Plan: {F123848}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

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

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -4,6 +4,26 @@ final class DifferentialTransactionEditor
   extends PhabricatorApplicationTransactionEditor {
 
   private $heraldEmailPHIDs;
+  private $changedPriorToCommitURI;
+  private $isCloseByCommit;
+
+  public function setIsCloseByCommit($is_close_by_commit) {
+    $this->isCloseByCommit = $is_close_by_commit;
+    return $this;
+  }
+
+  public function getIsCloseByCommit() {
+    return $this->isCloseByCommit;
+  }
+
+  public function setChangedPriorToCommitURI($uri) {
+    $this->changedPriorToCommitURI = $uri;
+    return $this;
+  }
+
+  public function getChangedPriorToCommitURI() {
+    return $this->changedPriorToCommitURI;
+  }
 
   public function getTransactionTypes() {
     $types = parent::getTransactionTypes();
@@ -158,7 +178,9 @@ final class DifferentialTransactionEditor
       case PhabricatorTransactions::TYPE_EDGE:
         return;
       case DifferentialTransaction::TYPE_UPDATE:
-        $object->setStatus($status_review);
+        if (!$this->getIsCloseByCommit()) {
+          $object->setStatus($status_review);
+        }
         // TODO: Update the `diffPHID` once we add that.
         return;
       case DifferentialTransaction::TYPE_ACTION:
@@ -209,6 +231,12 @@ final class DifferentialTransactionEditor
     $results = parent::expandTransaction($object, $xaction);
     switch ($xaction->getTransactionType()) {
       case DifferentialTransaction::TYPE_UPDATE:
+        if ($this->getIsCloseByCommit()) {
+          // Don't bother with any of this if this update is a side effect of
+          // commit detection.
+          break;
+        }
+
         $new_accept = DifferentialReviewerStatus::STATUS_ACCEPTED;
         $new_reject = DifferentialReviewerStatus::STATUS_REJECTED;
         $old_accept = DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER;
@@ -784,19 +812,22 @@ final class DifferentialTransactionEditor
         break;
 
       case DifferentialAction::ACTION_CLOSE:
+        // We force revisions closed when we discover a corresponding commit.
+        // In this case, revisions are allowed to transition to closed from
+        // any state. This is an automated action taken by the daemons.
 
-        // TODO: Permit the daemons to take this action in all cases.
+        if (!$this->getIsCloseByCommit()) {
+          if (!$actor_is_author && !$always_allow_close) {
+            return pht(
+              "You can not close this revision because you do not own it. To ".
+              "close a revision, you must be its owner.");
+          }
 
-        if (!$actor_is_author && !$always_allow_close) {
+          if ($revision_status != $status_accepted) {
-          return pht(
+            return pht(
-            "You can not close this revision because you do not own it. To ".
+              "You can not close this revision because it has not been ".
-            "close a revision, you must be its owner.");
+              "accepted. You can only close accepted revisions.");
-        }
+          }
-
-        if ($revision_status != $status_accepted) {
-          return pht(
-            "You can not close this revision because it has not been ".
-            "accepted. You can only close accepted revisions.");
         }
         break;
     }
@@ -909,6 +940,13 @@ final class DifferentialTransactionEditor
       }
     }
 
+    $changed_uri = $this->getChangedPriorToCommitURI();
+    if ($changed_uri) {
+      $body->addTextSection(
+        pht('CHANGED PRIOR TO COMMIT'),
+        $changed_uri);
+    }
+
     if ($inlines) {
       $body->addTextSection(
         pht('INLINE COMMENTS'),
@@ -1099,7 +1137,9 @@ final class DifferentialTransactionEditor
     foreach ($xactions as $xaction) {
       switch ($xaction->getTransactionType()) {
         case DifferentialTransaction::TYPE_UPDATE:
-          return true;
+          if (!$this->getIsCloseByCommit()) {
+            return true;
+          }
       }
     }
 

src/applications/metamta/contentsource/PhabricatorContentSource.php

@@ -12,6 +12,7 @@ final class PhabricatorContentSource {
   const SOURCE_CONSOLE  = 'console';
   const SOURCE_HERALD   = 'herald';
   const SOURCE_LEGACY   = 'legacy';
+  const SOURCE_DAEMON   = 'daemon';
 
   private $source;
   private $params = array();
@@ -72,6 +73,7 @@ final class PhabricatorContentSource {
       self::SOURCE_CONSOLE  => pht('Console'),
       self::SOURCE_LEGACY   => pht('Legacy'),
       self::SOURCE_HERALD   => pht('Herald'),
+      self::SOURCE_DAEMON   => pht('Daemons'),
       self::SOURCE_UNKNOWN  => pht('Old World'),
     );
   }

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

@@ -78,16 +78,18 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
     $should_autoclose = $repository->shouldAutocloseCommit($commit, $data);
 
     if ($revision_id) {
-      $lock = PhabricatorGlobalLock::newLock(get_class($this).':'.$revision_id);
-      $lock->lock(5 * 60);
-
       // TODO: Check if a more restrictive viewer could be set here
-      $revision = id(new DifferentialRevisionQuery())
+      $revision_query = id(new DifferentialRevisionQuery())
         ->withIDs(array($revision_id))
         ->setViewer(PhabricatorUser::getOmnipotentUser())
-        ->needRelationships(true)
         ->needReviewerStatus(true)
-        ->executeOne();
+        ->needActiveDiffs(true);
+
+      // TODO: Remove this once we swap to new CustomFields. This is only
+      // required by the old FieldSpecifications, lower on in this worker.
+      $revision_query->needRelationships(true);
+
+      $revision = $revision_query->executeOne();
 
       if ($revision) {
         $commit_drev = PhabricatorEdgeConfig::TYPE_COMMIT_HAS_DREV;
@@ -112,29 +114,10 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
             $committer_phid,
             $author_phid,
             $revision->getAuthorPHID());
+
           $actor = id(new PhabricatorUser())
             ->loadOneWhere('phid = %s', $actor_phid);
 
-          $diff = $this->attachToRevision($revision, $actor_phid);
-
-          $editor = new DifferentialCommentEditor(
-            $revision,
-            DifferentialAction::ACTION_CLOSE);
-          $editor->setActor($actor);
-          $editor->setIsDaemonWorkflow(true);
-
-          $vs_diff = $this->loadChangedByCommit($diff);
-          if ($vs_diff) {
-            $data->setCommitDetail('vsDiff', $vs_diff->getID());
-
-            $changed_by_commit = PhabricatorEnv::getProductionURI(
-              '/D'.$revision->getID().
-              '?vs='.$vs_diff->getID().
-              '&id='.$diff->getID().
-              '#toc');
-            $editor->setChangedByCommit($changed_by_commit);
-          }
-
           $commit_name = $repository->formatCommitName(
             $commit->getCommitIdentifier());
 
@@ -148,24 +131,77 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
             $data->getAuthorName(),
             $actor);
 
-          $info = array();
-          $info[] = "authored by {$author_name}";
           if ($committer_name && ($committer_name != $author_name)) {
-            $info[] = "committed by {$committer_name}";
+            $message = pht(
+              'Closed by commit %s (authored by %s, committed by %s).',
+              $commit_name,
+              $author_name,
+              $committer_name);
+          } else {
+            $message = pht(
+              'Closed by commit %s (authored by %s).',
+              $commit_name,
+              $author_name);
           }
-          $info = implode(', ', $info);
 
-          $editor
+          $diff = $this->generateFinalDiff($revision, $actor_phid);
-            ->setMessage("Closed by commit {$commit_name} ({$info}).")
+
-            ->save();
+          $vs_diff = $this->loadChangedByCommit($revision, $diff);
+          $changed_uri = null;
+          if ($vs_diff) {
+            $data->setCommitDetail('vsDiff', $vs_diff->getID());
+
+            $changed_uri = PhabricatorEnv::getProductionURI(
+              '/D'.$revision->getID().
+              '?vs='.$vs_diff->getID().
+              '&id='.$diff->getID().
+              '#toc');
+          }
+
+          $xactions = array();
+
+          $xactions[] = id(new DifferentialTransaction())
+            ->setTransactionType(DifferentialTransaction::TYPE_ACTION)
+            ->setNewValue(DifferentialAction::ACTION_CLOSE);
+
+          $xactions[] = id(new DifferentialTransaction())
+            ->setTransactionType(DifferentialTransaction::TYPE_UPDATE)
+            ->setIgnoreOnNoEffect(true)
+            ->setNewValue($diff->getPHID());
+
+          $xactions[] = id(new DifferentialTransaction())
+            ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT)
+            ->setIgnoreOnNoEffect(true)
+            ->attachComment(
+              id(new DifferentialTransactionComment())
+                ->setContent($message));
+
+          $content_source = PhabricatorContentSource::newForSource(
+            PhabricatorContentSource::SOURCE_DAEMON,
+            array());
+
+          $editor = id(new DifferentialTransactionEditor())
+            ->setActor($actor)
+            ->setContinueOnMissingFields(true)
+            ->setContentSource($content_source)
+            ->setChangedPriorToCommitURI($changed_uri)
+            ->setIsCloseByCommit(true);
+
+          try {
+            $editor->applyTransactions($revision, $xactions);
+          } catch (PhabricatorApplicationTransactionNoEffectException $ex) {
+            // NOTE: We've marked transactions other than the CLOSE transaction
+            // as ignored when they don't have an effect, so this means that we
+            // lost a race to close the revision. That's perfectly fine, we can
+            // just continue normally.
+          }
         }
-
       }
-
-      $lock->unlock();
     }
 
     if ($should_autoclose) {
+      // TODO: When this is moved to CustomFields, remove the additional
+      // call above in query construction.
       $fields = DifferentialFieldSelector::newSelector()
         ->getFieldSpecifications();
       foreach ($fields as $key => $field) {
@@ -200,7 +236,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
     return '@'.$handle->getName();
   }
 
-  private function attachToRevision(
+  private function generateFinalDiff(
     DifferentialRevision $revision,
     $actor_phid) {
 
@@ -232,7 +268,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
     }
 
     $diff = DifferentialDiff::newFromRawChanges($changes)
-      ->setRevisionID($revision->getID())
+      ->setRepositoryPHID($this->repository->getPHID())
       ->setAuthorPHID($actor_phid)
       ->setCreationMethod('commit')
       ->setSourceControlSystem($this->repository->getVersionControlSystem())
@@ -265,18 +301,19 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
 
     // TODO: Attach binary files.
 
-    $revision->setLineCount($diff->getLineCount());
-
     return $diff->save();
   }
 
-  private function loadChangedByCommit(DifferentialDiff $diff) {
+  private function loadChangedByCommit(
+    DifferentialRevision $revision,
+    DifferentialDiff $diff) {
+
     $repository = $this->repository;
 
     $vs_changesets = array();
     $vs_diff = id(new DifferentialDiff())->loadOneWhere(
       'revisionID = %d AND creationMethod != %s ORDER BY id DESC LIMIT 1',
-      $diff->getRevisionID(),
+      $revision->getID(),
       'commit');
     foreach ($vs_diff->loadChangesets() as $changeset) {
       $path = $changeset->getAbsoluteRepositoryPath($repository, $vs_diff);