From 9125095587231886ecf0d1c42a1adae14953f79b Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 26 Oct 2013 19:16:10 -0700 Subject: [PATCH] Distinguish between empty and unparsed commits in Diffusion Summary: Fixes T3416. Fixes T1733. - Adds a flag to the commit table showing whether or not we have parsed it. - The flag is set to `0` initially when the commit is discovered. - The flag is set to `1` when the changes are parsed. - The UI can now use the flag to distinguish between "empty commit" and "commit which we haven't imported changes for yet". - Simplify rendering code a little bit. - Fix an issue with the Message parser for empty commits. - There's a key on the flag so we can do `SELECT * FROM repository_commit WHERE repositoryID = %d AND importStatus = 0 LIMIT 1` soon, to determine if a repository is fully imported or not. This will let us improve the UI (Ref T776, Ref T3217). Test Plan: - Ran `bin/storage upgrade -f`. - Created an empty commit. - Without the daemons running, ran `bin/repository pull GTEST` and `bin/repository discover GTEST`. - Viewed web UI to get the first screenshot ("Still Importing..."). - Ran the message and change steps with `scripts/repository/reparse.php`. - Viewed web UI to get the second screenshot ("Empty Commit"). Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T776, T1733, T3416, T3217 Differential Revision: https://secure.phabricator.com/D7428 --- .../sql/patches/20131026.commitstatus.sql | 8 +++ .../controller/DiffusionCommitController.php | 59 ++++++++++--------- .../storage/PhabricatorRepositoryCommit.php | 21 +++++++ ...habricatorRepositoryCommitHeraldWorker.php | 12 ++++ ...habricatorRepositoryCommitOwnersWorker.php | 3 + ...atorRepositoryCommitChangeParserWorker.php | 3 + ...torRepositoryCommitMessageParserWorker.php | 15 ++++- .../patch/PhabricatorBuiltinPatchList.php | 4 ++ 8 files changed, 96 insertions(+), 29 deletions(-) create mode 100644 resources/sql/patches/20131026.commitstatus.sql diff --git a/resources/sql/patches/20131026.commitstatus.sql b/resources/sql/patches/20131026.commitstatus.sql new file mode 100644 index 0000000000..373fbddf3c --- /dev/null +++ b/resources/sql/patches/20131026.commitstatus.sql @@ -0,0 +1,8 @@ +ALTER TABLE {$NAMESPACE}_repository.repository_commit + ADD COLUMN importStatus INT UNSIGNED NOT NULL COLLATE utf8_bin; + +UPDATE {$NAMESPACE}_repository.repository_commit + SET importStatus = 15; + +ALTER TABLE {$NAMESPACE}_repository.repository_commit + ADD KEY (repositoryID, importStatus); diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index a79ef123c0..9535d31b78 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -76,7 +76,6 @@ final class DiffusionCommitController extends DiffusionController { $this->auditAuthorityPHIDs = PhabricatorAuditCommentEditor::loadAuditPHIDsForUser($user); - $is_foreign = $commit_data->getCommitDetail('foreign-svn-stub'); $changesets = null; if ($is_foreign) { @@ -154,10 +153,14 @@ final class DiffusionCommitController extends DiffusionController { $hard_limit = 1000; - $change_query = DiffusionPathChangeQuery::newFromDiffusionRequest( - $drequest); - $change_query->setLimit($hard_limit + 1); - $changes = $change_query->loadChanges(); + if ($commit->isImported()) { + $change_query = DiffusionPathChangeQuery::newFromDiffusionRequest( + $drequest); + $change_query->setLimit($hard_limit + 1); + $changes = $change_query->loadChanges(); + } else { + $changes = array(); + } $was_limited = (count($changes) > $hard_limit); if ($was_limited) { @@ -206,37 +209,29 @@ final class DiffusionCommitController extends DiffusionController { } if ($bad_commit) { - $error_panel = new AphrontErrorView(); - $error_panel->setTitle(pht('Bad Commit')); - $error_panel->appendChild($bad_commit['description']); - - $content[] = $error_panel; + $content[] = $this->renderStatusMessage( + pht('Bad Commit'), + $bad_commit['description']); } else if ($is_foreign) { // Don't render anything else. + } else if (!$commit->isImported()) { + $content[] = $this->renderStatusMessage( + pht('Still Importing...'), + pht( + 'This commit is still importing. Changes will be visible once '. + 'the import finishes.')); } else if (!count($changes)) { - $no_changes = new AphrontErrorView(); - $no_changes->setSeverity(AphrontErrorView::SEVERITY_WARNING); - $no_changes->setTitle(pht('Not Yet Parsed')); - // TODO: This can also happen with weird SVN changes that don't do - // anything (or only alter properties?), although the real no-changes case - // is extremely rare and might be impossible to produce organically. We - // should probably write some kind of "Nothing Happened!" change into the - // DB once we parse these changes so we can distinguish between - // "not parsed yet" and "no changes". - $no_changes->appendChild( - pht("This commit hasn't been fully parsed yet (or doesn't affect any ". - "paths).")); - $content[] = $no_changes; + $content[] = $this->renderStatusMessage( + pht('Empty Commit'), + pht( + 'This commit is empty and does not affect any paths.')); } else if ($was_limited) { - $huge_commit = new AphrontErrorView(); - $huge_commit->setSeverity(AphrontErrorView::SEVERITY_WARNING); - $huge_commit->setTitle(pht('Enormous Commit')); - $huge_commit->appendChild( + $content[] = $this->renderStatusMessage( + pht('Enormous Commit'), pht( 'This commit is enormous, and affects more than %d files. '. 'Changes are not shown.', $hard_limit)); - $content[] = $huge_commit; } else { // The user has clicked "Show All Changes", and we should show all the // changes inline even if there are more than the soft limit. @@ -253,6 +248,7 @@ final class DiffusionCommitController extends DiffusionController { 'href' => '?show_all=true', ), pht('Show All Changes')); + $warning_view = id(new AphrontErrorView()) ->setSeverity(AphrontErrorView::SEVERITY_WARNING) ->setTitle('Very Large Commit') @@ -1084,4 +1080,11 @@ final class DiffusionCommitController extends DiffusionController { return $parser->processCorpus($corpus); } + private function renderStatusMessage($title, $body) { + return id(new AphrontErrorView()) + ->setSeverity(AphrontErrorView::SEVERITY_WARNING) + ->setTitle($title) + ->appendChild($body); + } + } diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index 66f0fd1026..a999726162 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -15,6 +15,13 @@ final class PhabricatorRepositoryCommit protected $authorPHID; protected $auditStatus = PhabricatorAuditCommitStatusConstants::NONE; protected $summary = ''; + protected $importStatus = 0; + + const IMPORTED_MESSAGE = 1; + const IMPORTED_CHANGE = 2; + const IMPORTED_OWNERS = 4; + const IMPORTED_HERALD = 8; + const IMPORTED_ALL = 15; private $commitData = self::ATTACHABLE; private $audits; @@ -39,6 +46,20 @@ final class PhabricatorRepositoryCommit return $this->isUnparsed; } + public function isImported() { + return ($this->getImportStatus() == self::IMPORTED_ALL); + } + + public function writeImportStatusFlag($flag) { + queryfx( + $this->establishConnection('w'), + 'UPDATE %T SET importStatus = (importStatus | %d) WHERE id = %d', + $this->getTableName(), + $flag, + $this->getID()); + return $this; + } + public function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php index e896536ef4..3ec5b69057 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php @@ -12,6 +12,18 @@ final class PhabricatorRepositoryCommitHeraldWorker PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit) { + $result = $this->applyHeraldRules($repository, $commit); + + $commit->writeImportStatusFlag( + PhabricatorRepositoryCommit::IMPORTED_HERALD); + + return $result; + } + + private function applyHeraldRules( + PhabricatorRepository $repository, + PhabricatorRepositoryCommit $commit) { + $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere( 'commitID = %d', $commit->getID()); diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php index 78d062d223..e02632ad9f 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php @@ -58,6 +58,9 @@ final class PhabricatorRepositoryCommitOwnersWorker $commit->save(); } + $commit->writeImportStatusFlag( + PhabricatorRepositoryCommit::IMPORTED_OWNERS); + if ($this->shouldQueueFollowupTasks()) { PhabricatorWorker::scheduleTask( 'PhabricatorRepositoryCommitHeraldWorker', diff --git a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php index 10b99993cc..9ff7f89234 100644 --- a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php +++ b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php @@ -58,6 +58,9 @@ abstract class PhabricatorRepositoryCommitChangeParserWorker protected function finishParse() { $commit = $this->commit; + $commit->writeImportStatusFlag( + PhabricatorRepositoryCommit::IMPORTED_CHANGE); + id(new PhabricatorSearchIndexer()) ->indexDocumentByPHID($commit->getPHID()); diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php index 71f88f1a1e..cced97b33d 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -220,6 +220,9 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker } $data->save(); + + $commit->writeImportStatusFlag( + PhabricatorRepositoryCommit::IMPORTED_MESSAGE); } private function loadUserName($user_phid, $default, PhabricatorUser $actor) { @@ -249,7 +252,17 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker ->loadRawDiff(); // TODO: Support adds, deletes and moves under SVN. - $changes = id(new ArcanistDiffParser())->parseDiff($raw_diff); + if (strlen($raw_diff)) { + $changes = id(new ArcanistDiffParser())->parseDiff($raw_diff); + } else { + // This is an empty diff, maybe made with `git commit --allow-empty`. + // NOTE: These diffs have the same tree hash as their ancestors, so + // they may attach to revisions in an unexpected way. Just let this + // happen for now, although it might make sense to special case it + // eventually. + $changes = array(); + } + $diff = DifferentialDiff::newFromRawChanges($changes) ->setRevisionID($revision->getID()) ->setAuthorPHID($actor_phid) diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index b005e7e764..5b21dcfab8 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1708,6 +1708,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('20131025.repopush.sql'), ), + '20131026.commitstatus.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('20131026.commitstatus.sql'), + ), ); } }