From b32bfb6541633363d442131818c700924552b725 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 21 Feb 2013 15:09:35 -0800 Subject: [PATCH] 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 --- .../sql/patches/20130219.commitsummary.sql | 2 ++ .../sql/patches/20130219.commitsummarymig.php | 25 +++++++++++++++++++ .../view/PhabricatorAuditCommitListView.php | 6 +---- .../audit/view/PhabricatorAuditListView.php | 16 +++++------- .../controller/DiffusionCommitController.php | 4 +-- .../phid/PhabricatorObjectHandle.php | 1 - .../handle/PhabricatorObjectHandleData.php | 16 ++++++------ .../storage/PhabricatorRepositoryCommit.php | 1 + .../PhabricatorRepositoryCommitData.php | 10 +++++--- ...torRepositoryCommitMessageParserWorker.php | 4 ++- .../patch/PhabricatorBuiltinPatchList.php | 8 ++++++ 11 files changed, 64 insertions(+), 29 deletions(-) create mode 100644 resources/sql/patches/20130219.commitsummary.sql create mode 100644 resources/sql/patches/20130219.commitsummarymig.php diff --git a/resources/sql/patches/20130219.commitsummary.sql b/resources/sql/patches/20130219.commitsummary.sql new file mode 100644 index 0000000000..e20f35cb7f --- /dev/null +++ b/resources/sql/patches/20130219.commitsummary.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_repository.repository_commit + ADD summary VARCHAR(80) NOT NULL; diff --git a/resources/sql/patches/20130219.commitsummarymig.php b/resources/sql/patches/20130219.commitsummarymig.php new file mode 100644 index 0000000000..c5ed5ea596 --- /dev/null +++ b/resources/sql/patches/20130219.commitsummarymig.php @@ -0,0 +1,25 @@ +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"; diff --git a/src/applications/audit/view/PhabricatorAuditCommitListView.php b/src/applications/audit/view/PhabricatorAuditCommitListView.php index dc674cec8b..d7ab0cb671 100644 --- a/src/applications/audit/view/PhabricatorAuditCommitListView.php +++ b/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, )); diff --git a/src/applications/audit/view/PhabricatorAuditListView.php b/src/applications/audit/view/PhabricatorAuditListView.php index 5a7ef11a1f..b79634e888 100644 --- a/src/applications/audit/view/PhabricatorAuditListView.php +++ b/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, diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 64dc2fb1ba..2b3328686d 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/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); diff --git a/src/applications/phid/PhabricatorObjectHandle.php b/src/applications/phid/PhabricatorObjectHandle.php index c16059d505..03ccf8ca60 100644 --- a/src/applications/phid/PhabricatorObjectHandle.php +++ b/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: diff --git a/src/applications/phid/handle/PhabricatorObjectHandleData.php b/src/applications/phid/handle/PhabricatorObjectHandleData.php index 67eaaf8e63..5e05392ec5 100644 --- a/src/applications/phid/handle/PhabricatorObjectHandleData.php +++ b/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); } diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index 2cabed5735..2b24e835d6 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/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; diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommitData.php b/src/applications/repository/storage/PhabricatorRepositoryCommitData.php index dba8499bf6..aca44b5e47 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommitData.php +++ b/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; diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php index 16a218d05b..d457ec22f0 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/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`, diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index f678f7c4cb..c4d9940646 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/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'), -- 2.51.2