Render commit summaries when rendering handles

Summary:
Fixes T2563. Instead of rendering "rPnnnnnn", render "rPnnnnnn: add feature X". Tweak Audit tables to accommodate.

@vrana / @nh, this migration might take a while. You could safely skip it when deploying and then run it after deployment.

I think I fixed all the other places where these render, but might have missed something.

Test Plan:
  - Ran first schema migration, clicked around to make sure nothing broke.
  - Ran `scripts/repository/reparse.php --message rXyyyyy`, verified summary populated.
  - Ran second migration.
  - Checked task/diffusion/audit/differential for weird rendering.

Reviewers: vrana

Reviewed By: vrana

CC: nh, aran, chrisbolt, allixsenos

Maniphest Tasks: T2563

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

resources/sql/patches/20130219.commitsummary.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_repository.repository_commit
+  ADD summary VARCHAR(80) NOT NULL;

resources/sql/patches/20130219.commitsummarymig.php

@@ -0,0 +1,25 @@
+<?php
+
+echo "Backfilling commit summaries...\n";
+
+$commits = new LiskMigrationIterator(new PhabricatorRepositoryCommit());
+foreach ($commits as $commit) {
+  echo 'Filling Commit #'.$commit->getID()."\n";
+
+  if (strlen($commit->getSummary())) {
+    continue;
+  }
+
+  $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere(
+    'commitID = %d',
+    $commit->getID());
+
+  if (!$data) {
+    continue;
+  }
+
+  $commit->setSummary($data->getSummary());
+  $commit->save();
+}
+
+echo "Done.\n";

src/applications/audit/view/PhabricatorAuditCommitListView.php

@@ -70,7 +70,6 @@ final class PhabricatorAuditCommitListView extends AphrontView {
       $rows[] = array(
         $commit_name,
         $author_name,
-        $commit->getCommitData()->getSummary(),
         PhabricatorAuditCommitStatusConstants::getStatusName(
           $commit->getAuditStatus()),
         phutil_implode_html(', ', $auditors),
@@ -83,19 +82,17 @@ final class PhabricatorAuditCommitListView extends AphrontView {
       array(
         'Commit',
         'Author',
-        'Summary',
         'Audit Status',
         'Auditors',
         'Date',
       ));
     $table->setColumnClasses(
       array(
-        'n',
-        '',
         'wide',
         '',
         '',
         '',
+        '',
       ));
 
     if ($this->commits && reset($this->commits)->getAudits() === null) {
@@ -104,7 +101,6 @@ final class PhabricatorAuditCommitListView extends AphrontView {
           true,
           true,
           true,
-          true,
           false,
           true,
         ));

src/applications/audit/view/PhabricatorAuditListView.php

@@ -7,7 +7,7 @@ final class PhabricatorAuditListView extends AphrontView {
   private $authorityPHIDs = array();
   private $noDataString;
   private $commits;
-  private $showDescriptions = true;
+  private $showCommits = true;
 
   private $highlightedAudits;
 
@@ -43,8 +43,8 @@ final class PhabricatorAuditListView extends AphrontView {
     return $this;
   }
 
-  public function setShowDescriptions($show_descriptions) {
-    $this->showDescriptions = $show_descriptions;
+  public function setShowCommits($show_commits) {
+    $this->showCommits = $show_commits;
     return $this;
   }
 
@@ -137,7 +137,6 @@ final class PhabricatorAuditListView extends AphrontView {
       $auditor_handle = $this->getHandle($audit->getAuditorPHID());
       $rows[] = array(
         $commit_name,
-        $commit_desc,
         $committed,
         $auditor_handle->renderLink(),
         $status,
@@ -155,7 +154,6 @@ final class PhabricatorAuditListView extends AphrontView {
     $table->setHeaders(
       array(
         'Commit',
-        'Description',
         'Committed',
         'Auditor',
         'Status',
@@ -164,18 +162,16 @@ final class PhabricatorAuditListView extends AphrontView {
     $table->setColumnClasses(
       array(
         'pri',
-        ($this->showDescriptions ? 'wide' : ''),
         '',
         '',
         '',
-        ($this->showDescriptions ? '' : 'wide'),
+        ($this->showCommits ? '' : 'wide'),
       ));
     $table->setRowClasses($rowc);
     $table->setColumnVisibility(
       array(
-        $this->showDescriptions,
-        $this->showDescriptions,
-        $this->showDescriptions,
+        $this->showCommits,
+        $this->showCommits,
         true,
         true,
         true,

src/applications/diffusion/controller/DiffusionCommitController.php

@@ -75,7 +75,7 @@ final class DiffusionCommitController extends DiffusionController {
         $drequest);
 
       $headsup_view = id(new PhabricatorHeaderView())
-        ->setHeader('Commit Detail');
+        ->setHeader(nonempty($commit->getSummary(), pht('Commit Detail')));
 
       $headsup_actions = $this->renderHeadsupActionList($commit, $repository);
 
@@ -504,7 +504,7 @@ final class DiffusionCommitController extends DiffusionController {
     $view->setAudits($audits);
     $view->setCommits(array($commit));
     $view->setUser($user);
-    $view->setShowDescriptions(false);
+    $view->setShowCommits(false);
 
     $phids = $view->getRequiredHandlePHIDs();
     $handles = $this->loadViewerHandles($phids);

src/applications/phid/PhabricatorObjectHandle.php

@@ -203,7 +203,6 @@ final class PhabricatorObjectHandle {
   public function getLinkName() {
     switch ($this->getType()) {
       case PhabricatorPHIDConstants::PHID_TYPE_USER:
-      case PhabricatorPHIDConstants::PHID_TYPE_CMIT:
         $name = $this->getName();
         break;
       default:

src/applications/phid/handle/PhabricatorObjectHandleData.php

@@ -328,6 +328,7 @@ final class PhabricatorObjectHandleData {
             $handle = new PhabricatorObjectHandle();
             $handle->setPHID($phid);
             $handle->setType($type);
+
             $repository = null;
             if (!empty($objects[$phid])) {
               $repository = $objects[$phid]->loadOneRelative(
@@ -335,6 +336,7 @@ final class PhabricatorObjectHandleData {
                 'id',
                 'getRepositoryID');
             }
+
             if (!$repository) {
               $handle->setName('Unknown Commit');
             } else {
@@ -342,17 +344,17 @@ final class PhabricatorObjectHandleData {
               $callsign = $repository->getCallsign();
               $commit_identifier = $commit->getCommitIdentifier();
 
-              // In case where the repository for the commit was deleted,
-              // we don't have info about the repository anymore.
-              if ($repository) {
-                $name = $repository->formatCommitName($commit_identifier);
-                $handle->setName($name);
+              $name = $repository->formatCommitName($commit_identifier);
+              $handle->setName($name);
+
+              $summary = $commit->getSummary();
+              if (strlen($summary)) {
+                $handle->setFullName($name.': '.$summary);
               } else {
-                $handle->setName('Commit '.'r'.$callsign.$commit_identifier);
+                $handle->setFullName($name);
               }
 
               $handle->setURI('/r'.$callsign.$commit_identifier);
-              $handle->setFullName('r'.$callsign.$commit_identifier);
               $handle->setTimestamp($commit->getEpoch());
               $handle->setComplete(true);
             }

src/applications/repository/storage/PhabricatorRepositoryCommit.php

@@ -9,6 +9,7 @@ final class PhabricatorRepositoryCommit extends PhabricatorRepositoryDAO {
   protected $mailKey;
   protected $authorPHID;
   protected $auditStatus = PhabricatorAuditCommitStatusConstants::NONE;
+  protected $summary = '';
 
   private $commitData;
   private $audits;

src/applications/repository/storage/PhabricatorRepositoryCommitData.php

@@ -2,7 +2,11 @@
 
 final class PhabricatorRepositoryCommitData extends PhabricatorRepositoryDAO {
 
-  const SUMMARY_MAX_LENGTH = 100;
+  /**
+   * NOTE: We denormalize this into the commit table; make sure the sizes
+   * match up.
+   */
+  const SUMMARY_MAX_LENGTH = 80;
 
   protected $commitID;
   protected $authorName    = '';
@@ -20,9 +24,9 @@ final class PhabricatorRepositoryCommitData extends PhabricatorRepositoryDAO {
 
   public function getSummary() {
     $message = $this->getCommitMessage();
-    $lines = explode("\n", $message);
-    $summary = head($lines);
 
+    $summary = phutil_split_lines($message);
+    $summary = head($summary);
     $summary = phutil_utf8_shorten($summary, self::SUMMARY_MAX_LENGTH);
 
     return $summary;

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

@@ -77,9 +77,11 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
 
     if ($author_phid != $commit->getAuthorPHID()) {
       $commit->setAuthorPHID($author_phid);
-      $commit->save();
     }
 
+    $commit->setSummary($data->getSummary());
+    $commit->save();
+
     $conn_w = id(new DifferentialRevision())->establishConnection('w');
 
     // NOTE: The `differential_commit` table has a unique ID on `commitPHID`,

src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php

@@ -1137,6 +1137,14 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList {
         'type'    => 'sql',
         'name'    => $this->getPatchPath('20130218.longdaemon.sql'),
       ),
+      '20130219.commitsummary.sql' => array(
+        'type'    => 'sql',
+        'name'    => $this->getPatchPath('20130219.commitsummary.sql'),
+      ),
+      '20130219.commitsummarymig.php' => array(
+        'type'    => 'php',
+        'name'    => $this->getPatchPath('20130219.commitsummarymig.php'),
+      ),
       '20130215.phabricatorfileaddttl.sql' => array(
         'type'    => 'sql',
         'name'    => $this->getPatchPath('20130215.phabricatorfileaddttl.sql'),