Fixes T6637, "closing by commit" should update task status and specify responsible commit, but drop the artificial comment.

Summary: When updating the status of a task via commit, transaction should show responsible commit and status update if it was changed.

Test Plan: Push a commit "Fixes Txx", transaction should include status update and commit number.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T6637

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

src/applications/maniphest/storage/ManiphestTransaction.php

@@ -114,6 +114,12 @@ final class ManiphestTransaction
           $phids[] = $phid;
         }
         break;
+      case self::TYPE_STATUS:
+        $commit_phid = $this->getMetadataValue('commitPHID');
+        if ($commit_phid) {
+          $phids[] = $commit_phid;
+        }
+        break;
     }
 
     return $phids;
@@ -121,6 +127,16 @@ final class ManiphestTransaction
 
   public function shouldHide() {
     switch ($this->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_EDGE:
+        $commit_phid = $this->getMetadataValue('commitPHID');
+        $edge_type = $this->getMetadataValue('edge:type');
+
+        if ($edge_type == ManiphestTaskHasCommitEdgeType::EDGECONST) {
+          if ($commit_phid) {
+            return true;
+          }
+        }
+        break;
       case self::TYPE_DESCRIPTION:
       case self::TYPE_PRIORITY:
       case self::TYPE_STATUS:
@@ -385,22 +401,55 @@ final class ManiphestTransaction
         $old_name = ManiphestTaskStatus::getTaskStatusName($old);
         $new_name = ManiphestTaskStatus::getTaskStatusName($new);
 
+        $commit_phid = $this->getMetadataValue('commitPHID');
+
         if ($new_closed && !$old_closed) {
           if ($new == ManiphestTaskStatus::getDuplicateStatus()) {
+            if ($commit_phid) {
+              return pht(
+                '%s closed this task as a duplicate by committing %s.',
+                $this->renderHandleLink($author_phid),
+                $this->renderHandleLink($commit_phid));
+            } else {
               return pht(
                 '%s closed this task as a duplicate.',
                 $this->renderHandleLink($author_phid));
+            }
+          } else {
+            if ($commit_phid) {
+              return pht(
+                '%s closed this task as "%s" by committing %s.',
+                $this->renderHandleLink($author_phid),
+                $new_name,
+                $this->renderHandleLink($commit_phid));
             } else {
               return pht(
                 '%s closed this task as "%s".',
                 $this->renderHandleLink($author_phid),
                 $new_name);
             }
+          }
         } else if (!$new_closed && $old_closed) {
+          if ($commit_phid) {
+            return pht(
+              '%s reopened this task as "%s" by committing %s.',
+              $this->renderHandleLink($author_phid),
+              $new_name,
+              $this->renderHandleLink($commit_phid));
+          } else {
             return pht(
               '%s reopened this task as "%s".',
               $this->renderHandleLink($author_phid),
               $new_name);
+          }
+        } else {
+          if ($commit_phid) {
+            return pht(
+              '%s changed the task status from "%s" to "%s" by committing %s.',
+              $this->renderHandleLink($author_phid),
+              $old_name,
+              $new_name,
+              $this->renderHandleLink($commit_phid));
           } else {
             return pht(
               '%s changed the task status from "%s" to "%s".',
@@ -408,6 +457,7 @@ final class ManiphestTransaction
               $old_name,
               $new_name);
           }
+        }
 
       case self::TYPE_UNBLOCK:
         $blocker_phid = key($new);
@@ -590,12 +640,30 @@ final class ManiphestTransaction
         $old_name = ManiphestTaskStatus::getTaskStatusName($old);
         $new_name = ManiphestTaskStatus::getTaskStatusName($new);
 
+        $commit_phid = $this->getMetadataValue('commitPHID');
+
         if ($new_closed && !$old_closed) {
           if ($new == ManiphestTaskStatus::getDuplicateStatus()) {
+            if ($commit_phid) {
+              return pht(
+                '%s closed %s as a duplicate by committing %s.',
+                $this->renderHandleLink($author_phid),
+                $this->renderHandleLink($object_phid),
+                $this->renderHandleLink($commit_phid));
+            } else {
               return pht(
                 '%s closed %s as a duplicate.',
                 $this->renderHandleLink($author_phid),
                 $this->renderHandleLink($object_phid));
+            }
+          } else {
+            if ($commit_phid) {
+              return pht(
+                '%s closed %s as "%s" by committing %s.',
+                $this->renderHandleLink($author_phid),
+                $this->renderHandleLink($object_phid),
+                $new_name,
+                $this->renderHandleLink($commit_phid));
             } else {
               return pht(
                 '%s closed %s as "%s".',
@@ -603,12 +671,31 @@ final class ManiphestTransaction
                 $this->renderHandleLink($object_phid),
                 $new_name);
             }
+          }
         } else if (!$new_closed && $old_closed) {
+          if ($commit_phid) {
+            return pht(
+              '%s reopened %s as "%s" by committing %s.',
+              $this->renderHandleLink($author_phid),
+              $this->renderHandleLink($object_phid),
+              $new_name,
+              $this->renderHandleLink($commit_phid));
+          } else {
             return pht(
               '%s reopened %s as "%s".',
               $this->renderHandleLink($author_phid),
               $this->renderHandleLink($object_phid),
               $new_name);
+          }
+        } else {
+          if ($commit_phid) {
+            return pht(
+              '%s changed the status of %s from "%s" to "%s" by committing %s.',
+              $this->renderHandleLink($author_phid),
+              $this->renderHandleLink($object_phid),
+              $old_name,
+              $new_name,
+              $this->renderHandleLink($commit_phid));
           } else {
             return pht(
               '%s changed the status of %s from "%s" to "%s".',
@@ -617,6 +704,7 @@ final class ManiphestTransaction
               $old_name,
               $new_name);
           }
+        }
 
       case self::TYPE_UNBLOCK:
         $blocker_phid = key($new);

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

@@ -466,7 +466,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
       $xactions = array();
 
       $edge_type = ManiphestTaskHasCommitEdgeType::EDGECONST;
-      $xactions[] = id(new ManiphestTransaction())
+      $edge_xaction = id(new ManiphestTransaction())
         ->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
         ->setMetadataValue('edge:type', $edge_type)
         ->setNewValue(
@@ -481,23 +481,15 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
         if ($task->getStatus() != $status) {
           $xactions[] = id(new ManiphestTransaction())
             ->setTransactionType(ManiphestTransaction::TYPE_STATUS)
+            ->setMetadataValue('commitPHID', $commit->getPHID())
             ->setNewValue($status);
 
-          $commit_name = $repository->formatCommitName(
+          $edge_xaction->setMetadataValue('commitPHID', $commit->getPHID());
-            $commit->getCommitIdentifier());
-
-          $status_message = pht(
-            'Closed by commit %s.',
-            $commit_name);
-
-          $xactions[] = id(new ManiphestTransaction())
-            ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT)
-            ->attachComment(
-              id(new ManiphestTransactionComment())
-                ->setContent($status_message));
         }
       }
 
+      $xactions[] = $edge_xaction;
+
       $content_source = PhabricatorContentSource::newForSource(
         PhabricatorContentSource::SOURCE_DAEMON,
         array());