Differential - change "closed by commit" comment to real transaction

Summary:
Implements a new transaction - still TYPE_ACTION - but using a new DifferentialAction::ACTION_COMMIT_CLOSE. Augment rendering as necessary to display this new transaction. Saves enough information so T3686 is possible but stops short of implementing a popup to display this information. Fixes T5875. Ref T3686.

One small display oddity - this new transaction now renders at the top of the transaction group whereas when it was a comment it was on the bottom. I think this is basically okay but if not how fix? (Playing with the "strength" of these actions will mess up the email too?)

Test Plan: made a diff X that fixed task Y. committed. checked diff X, task Y, and the commit pages for proper transactions and all looked good.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T3686, T5875

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

src/applications/differential/constants/DifferentialAction.php

@@ -22,83 +22,83 @@ final class DifferentialAction {
 
   public static function getBasicStoryText($action, $author_name) {
     switch ($action) {
-        case DifferentialAction::ACTION_COMMENT:
-          $title = pht('%s commented on this revision.',
-            $author_name);
+      case DifferentialAction::ACTION_COMMENT:
+        $title = pht('%s commented on this revision.',
+          $author_name);
         break;
-        case DifferentialAction::ACTION_ACCEPT:
-          $title = pht('%s accepted this revision.',
-            $author_name);
+      case DifferentialAction::ACTION_ACCEPT:
+        $title = pht('%s accepted this revision.',
+          $author_name);
         break;
-        case DifferentialAction::ACTION_REJECT:
-          $title = pht('%s requested changes to this revision.',
-            $author_name);
+      case DifferentialAction::ACTION_REJECT:
+        $title = pht('%s requested changes to this revision.',
+          $author_name);
         break;
-        case DifferentialAction::ACTION_RETHINK:
-          $title = pht('%s planned changes to this revision.',
-            $author_name);
+      case DifferentialAction::ACTION_RETHINK:
+        $title = pht('%s planned changes to this revision.',
+          $author_name);
         break;
-        case DifferentialAction::ACTION_ABANDON:
-          $title = pht('%s abandoned this revision.',
-            $author_name);
+      case DifferentialAction::ACTION_ABANDON:
+        $title = pht('%s abandoned this revision.',
+          $author_name);
         break;
-        case DifferentialAction::ACTION_CLOSE:
-          $title = pht('%s closed this revision.',
-            $author_name);
+      case DifferentialAction::ACTION_CLOSE:
+        $title = pht('%s closed this revision.',
+          $author_name);
         break;
-        case DifferentialAction::ACTION_REQUEST:
-          $title = pht('%s requested a review of this revision.',
-            $author_name);
+      case DifferentialAction::ACTION_REQUEST:
+        $title = pht('%s requested a review of this revision.',
+          $author_name);
         break;
-        case DifferentialAction::ACTION_RECLAIM:
-          $title = pht('%s reclaimed this revision.',
-            $author_name);
+      case DifferentialAction::ACTION_RECLAIM:
+        $title = pht('%s reclaimed this revision.',
+          $author_name);
         break;
-        case DifferentialAction::ACTION_UPDATE:
-          $title = pht('%s updated this revision.',
-            $author_name);
+      case DifferentialAction::ACTION_UPDATE:
+        $title = pht('%s updated this revision.',
+          $author_name);
         break;
-        case DifferentialAction::ACTION_RESIGN:
-          $title = pht('%s resigned from this revision.',
-            $author_name);
+      case DifferentialAction::ACTION_RESIGN:
+        $title = pht('%s resigned from this revision.',
+          $author_name);
         break;
-        case DifferentialAction::ACTION_SUMMARIZE:
-          $title = pht('%s summarized this revision.',
-            $author_name);
+      case DifferentialAction::ACTION_SUMMARIZE:
+        $title = pht('%s summarized this revision.',
+          $author_name);
         break;
-        case DifferentialAction::ACTION_TESTPLAN:
-          $title = pht('%s explained the test plan for this revision.',
-            $author_name);
+      case DifferentialAction::ACTION_TESTPLAN:
+        $title = pht('%s explained the test plan for this revision.',
+          $author_name);
         break;
-        case DifferentialAction::ACTION_CREATE:
-          $title = pht('%s created this revision.',
-            $author_name);
+      case DifferentialAction::ACTION_CREATE:
+        $title = pht('%s created this revision.',
+          $author_name);
         break;
-        case DifferentialAction::ACTION_ADDREVIEWERS:
-          $title = pht('%s added reviewers to this revision.',
-            $author_name);
+      case DifferentialAction::ACTION_ADDREVIEWERS:
+        $title = pht('%s added reviewers to this revision.',
+          $author_name);
         break;
-        case DifferentialAction::ACTION_ADDCCS:
-          $title = pht('%s added CCs to this revision.',
-            $author_name);
+      case DifferentialAction::ACTION_ADDCCS:
+        $title = pht('%s added CCs to this revision.',
+          $author_name);
         break;
-        case DifferentialAction::ACTION_CLAIM:
-          $title = pht('%s commandeered this revision.',
-            $author_name);
+      case DifferentialAction::ACTION_CLAIM:
+        $title = pht('%s commandeered this revision.',
+          $author_name);
         break;
-        case DifferentialAction::ACTION_REOPEN:
-          $title = pht('%s reopened this revision.',
-            $author_name);
+      case DifferentialAction::ACTION_REOPEN:
+        $title = pht('%s reopened this revision.',
+          $author_name);
         break;
-        case DifferentialTransaction::TYPE_INLINE:
-          $title = pht(
-            '%s added an inline comment.',
-            $author_name);
-          break;
-        default:
-          $title = pht('Ghosts happened to this revision.');
+      case DifferentialTransaction::TYPE_INLINE:
+        $title = pht(
+          '%s added an inline comment.',
+          $author_name);
         break;
-      }
+      default:
+        $title = pht('Ghosts happened to this revision.');
+        break;
+    }
     return $title;
   }
 

src/applications/differential/storage/DifferentialTransaction.php

@@ -102,6 +102,18 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
     $new = $this->getNewValue();
 
     switch ($this->getTransactionType()) {
+      case self::TYPE_ACTION:
+        if ($new == DifferentialAction::ACTION_CLOSE &&
+            $this->getMetadataValue('isCommitClose')) {
+          $phids[] = $this->getMetadataValue('commitPHID');
+          if ($this->getMetadataValue('committerPHID')) {
+            $phids[] = $this->getMetadataValue('committerPHID');
+          }
+          if ($this->getMetadataValue('authorPHID')) {
+            $phids[] = $this->getMetadataValue('authorPHID');
+          }
+        }
+        break;
       case self::TYPE_UPDATE:
         if ($new) {
           $phids[] = $new;
@@ -216,7 +228,11 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
           '%s added inline comments.',
           $author_handle);
       case self::TYPE_UPDATE:
-        if ($new) {
+        if ($this->getMetadataValue('isCommitUpdate')) {
+          return pht(
+            'This revision was automatically updated to reflect the '.
+            'committed changes.');
+        } else if ($new) {
           // TODO: Migrate to PHIDs and use handles here?
           if (phid_get_type($new) == DifferentialDiffPHIDType::TYPECONST) {
             return pht(
@@ -234,7 +250,45 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
             $author_handle);
         }
       case self::TYPE_ACTION:
-        return DifferentialAction::getBasicStoryText($new, $author_handle);
+        switch ($new) {
+          case DifferentialAction::ACTION_CLOSE:
+            if (!$this->getMetadataValue('isCommitClose')) {
+              return DifferentialAction::getBasicStoryText(
+                $new,
+                $author_handle);
+            }
+            $commit_name = $this->renderHandleLink(
+              $this->getMetadataValue('commitPHID'));
+            $committer_phid = $this->getMetadataValue('committerPHID');
+            $author_phid = $this->getMetadataValue('authorPHID');
+            if ($this->getHandleIfExists($committer_phid)) {
+              $committer_name = $this->renderHandleLink($committer_phid);
+            } else {
+              $committer_name = $this->getMetadataValue('committerName');
+            }
+            if ($this->getHandleIfExists($author_phid)) {
+              $author_name = $this->renderHandleLink($author_phid);
+            } else {
+              $author_name = $this->getMetadataValue('authorName');
+            }
+
+            if ($committer_name && ($committer_name != $author_name)) {
+              return pht(
+                'Closed by commit %s (authored by %s, committed by %s).',
+                $commit_name,
+                $author_name,
+                $committer_name);
+            } else {
+              return pht(
+                'Closed by commit %s (authored by %s).',
+                $commit_name,
+                $author_name);
+            }
+            break;
+          default:
+            return DifferentialAction::getBasicStoryText($new, $author_handle);
+        }
+        break;
       case self::TYPE_STATUS:
         switch ($this->getNewValue()) {
           case ArcanistDifferentialRevisionStatus::ACCEPTED:
@@ -296,10 +350,44 @@ final class DifferentialTransaction extends PhabricatorApplicationTransaction {
               $author_link,
               $object_link);
           case DifferentialAction::ACTION_CLOSE:
-            return pht(
-              '%s closed %s.',
-              $author_link,
-              $object_link);
+            if (!$this->getMetadataValue('isCommitClose')) {
+              return pht(
+                '%s closed %s.',
+                $author_link,
+                $object_link);
+            } else {
+              $commit_name = $this->renderHandleLink(
+                $this->getMetadataValue('commitPHID'));
+              $committer_phid = $this->getMetadataValue('committerPHID');
+              $author_phid = $this->getMetadataValue('authorPHID');
+              if ($this->getHandleIfExists($committer_phid)) {
+                $committer_name = $this->renderHandleLink($committer_phid);
+              } else {
+                $committer_name = $this->getMetadataValue('committerName');
+              }
+              if ($this->getHandleIfExists($author_phid)) {
+                $author_name = $this->renderHandleLink($author_phid);
+              } else {
+                $author_name = $this->getMetadataValue('authorName');
+              }
+
+              if ($committer_name && ($committer_name != $author_name)) {
+                return pht(
+                  '%s closed %s by commit %s (authored by %s).',
+                  $author_link,
+                  $object_link,
+                  $commit_name,
+                  $author_name);
+              } else {
+                return pht(
+                  '%s closed %s by commit %s.',
+                  $author_link,
+                  $object_link,
+                  $commit_name);
+              }
+            }
+            break;
+
           case DifferentialAction::ACTION_REQUEST:
             return pht(
               '%s requested review of %s.',

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

@@ -140,31 +140,29 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
                         $should_autoclose;
 
         if ($should_close) {
-          $commit_name = $repository->formatCommitName(
-            $commit->getCommitIdentifier());
+           $commit_close_xaction = id(new DifferentialTransaction())
+            ->setTransactionType(DifferentialTransaction::TYPE_ACTION)
+            ->setNewValue(DifferentialAction::ACTION_CLOSE)
+            ->setMetadataValue('isCommitClose', true);
 
-          $committer_name = $this->loadUserName(
-            $committer_phid,
-            $data->getCommitDetail('committer'),
-            $actor);
-
-          $author_name = $this->loadUserName(
-            $author_phid,
-            $data->getAuthorName(),
-            $actor);
-
-          if ($committer_name && ($committer_name != $author_name)) {
-            $revision_update_comment = pht(
-              'Closed by commit %s (authored by %s, committed by %s).',
-              $commit_name,
-              $author_name,
-              $committer_name);
-          } else {
-            $revision_update_comment = pht(
-              'Closed by commit %s (authored by %s).',
-              $commit_name,
-              $author_name);
-          }
+          $commit_close_xaction->setMetadataValue(
+            'commitPHID',
+            $commit->getPHID());
+          $commit_close_xaction->setMetadataValue(
+            'committerPHID',
+            $committer_phid);
+          $commit_close_xaction->setMetadataValue(
+            'committerName',
+            $data->getCommitDetail('committer'));
+          $commit_close_xaction->setMetadataValue(
+            'authorPHID',
+            $author_phid);
+          $commit_close_xaction->setMetadataValue(
+            'authorName',
+            $data->getAuthorName());
+          $commit_close_xaction->setMetadataValue(
+            'commitHashes',
+            $hashes);
 
           $diff = $this->generateFinalDiff($revision, $acting_as_phid);
 
@@ -181,22 +179,12 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
           }
 
           $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($revision_update_comment));
+            ->setNewValue($diff->getPHID())
+            ->setMetadataValue('isCommitUpdate', true);
+          $xactions[] = $commit_close_xaction;
 
           $content_source = PhabricatorContentSource::newForSource(
             PhabricatorContentSource::SOURCE_DAEMON,
@@ -237,18 +225,6 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
       PhabricatorRepositoryCommit::IMPORTED_MESSAGE);
   }
 
-  private function loadUserName($user_phid, $default, PhabricatorUser $actor) {
-    if (!$user_phid) {
-      return $default;
-    }
-    $handle = id(new PhabricatorHandleQuery())
-      ->setViewer($actor)
-      ->withPHIDs(array($user_phid))
-      ->executeOne();
-
-    return '@'.$handle->getName();
-  }
-
   private function generateFinalDiff(
     DifferentialRevision $revision,
     $actor_phid) {
@@ -521,6 +497,8 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
         ->setActingAsPHID($acting_as)
         ->setContinueOnNoEffect(true)
         ->setContinueOnMissingFields(true)
+        ->setUnmentionablePHIDMap(
+          array($commit->getPHID() => $commit->getPHID()))
         ->setContentSource($content_source);
 
       $editor->applyTransactions($task, $xactions);