From e4c4724afd6b8b1c261f429616b049c842a10ab5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 24 Aug 2016 05:24:07 -0700 Subject: [PATCH] Migrate the "badcommit" table to use the less-hacky "hint" mechanism Summary: Ref T11522. This migrates any "badcommit" data (which probably only exists at Facebook and on 1-2 other installs in the wild) to the new "hint" table. Test Plan: - Wrote some bad commit annotations to the badcommit table. - Viewed them in the web UI and used `bin/repository reparse --change ...` to reparse them. Saw "this is bad" messages. - Ran migration, verified that valid "badcommit" rows were successfully migrated to become "hint" rows. - Viewed the new web UI and re-parsed the change, saw "unreadable commit" messages. - Viewed a good commit; reparsed a good commit. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11522 Differential Revision: https://secure.phabricator.com/D16435 --- .../20160824.repohint.02.movebad.php | 39 +++++++++++++++++++ .../20160824.repohint.03.nukebad.sql | 1 + .../controller/DiffusionCommitController.php | 26 ++++++++----- .../query/DiffusionCommitHintQuery.php | 2 +- .../storage/PhabricatorRepository.php | 1 - .../PhabricatorRepositoryCommitHint.php | 5 +++ .../PhabricatorRepositorySchemaSpec.php | 14 ------- ...habricatorRepositoryCommitParserWorker.php | 16 ++++---- ...atorRepositoryCommitChangeParserWorker.php | 9 ++++- 9 files changed, 77 insertions(+), 36 deletions(-) create mode 100644 resources/sql/autopatches/20160824.repohint.02.movebad.php create mode 100644 resources/sql/autopatches/20160824.repohint.03.nukebad.sql diff --git a/resources/sql/autopatches/20160824.repohint.02.movebad.php b/resources/sql/autopatches/20160824.repohint.02.movebad.php new file mode 100644 index 0000000000..4127892f73 --- /dev/null +++ b/resources/sql/autopatches/20160824.repohint.02.movebad.php @@ -0,0 +1,39 @@ +establishConnection('w'); + +$rows = queryfx_all( + $conn, + 'SELECT fullCommitName FROM repository_badcommit'); + +$viewer = PhabricatorUser::getOmnipotentUser(); + +foreach ($rows as $row) { + $identifier = $row['fullCommitName']; + + $commit = id(new DiffusionCommitQuery()) + ->setViewer($viewer) + ->withIdentifiers(array($identifier)) + ->executeOne(); + + if (!$commit) { + echo tsprintf( + "%s\n", + pht( + 'Skipped hint for "%s", this is not a valid commit.', + $identifier)); + } else { + PhabricatorRepositoryCommitHint::updateHint( + $commit->getRepository()->getPHID(), + $commit->getCommitIdentifier(), + null, + PhabricatorRepositoryCommitHint::HINT_UNREADABLE); + + echo tsprintf( + "%s\n", + pht( + 'Updated commit hint for "%s".', + $identifier)); + } +} diff --git a/resources/sql/autopatches/20160824.repohint.03.nukebad.sql b/resources/sql/autopatches/20160824.repohint.03.nukebad.sql new file mode 100644 index 0000000000..88364aeef3 --- /dev/null +++ b/resources/sql/autopatches/20160824.repohint.03.nukebad.sql @@ -0,0 +1 @@ +DROP TABLE {$NAMESPACE}_repository.repository_badcommit; diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 1ee2ccedad..1346955f80 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -167,23 +167,29 @@ final class DiffusionCommitController extends DiffusionController { $count = count($changes); - $bad_commit = null; - if ($count == 0) { - $bad_commit = queryfx_one( - id(new PhabricatorRepository())->establishConnection('r'), - 'SELECT * FROM %T WHERE fullCommitName = %s', - PhabricatorRepository::TABLE_BADCOMMIT, - $commit->getMonogram()); + $is_unreadable = false; + if (!$count) { + $hint = id(new DiffusionCommitHintQuery()) + ->setViewer($viewer) + ->withRepositoryPHIDs(array($repository->getPHID())) + ->withOldCommitIdentifiers(array($commit->getCommitIdentifier())) + ->executeOne(); + if ($hint) { + $is_unreadable = $hint->isUnreadable(); + } } $show_changesets = false; $info_panel = null; $change_list = null; $change_table = null; - if ($bad_commit) { + if ($is_unreadable) { $info_panel = $this->renderStatusMessage( - pht('Bad Commit'), - $bad_commit['description']); + pht('Unreadable Commit'), + pht( + 'This commit has been marked as unreadable by an administrator. '. + 'It may have been corrupted or created improperly by an external '. + 'tool.')); } else if ($is_foreign) { // Don't render anything else. } else if (!$commit->isImported()) { diff --git a/src/applications/diffusion/query/DiffusionCommitHintQuery.php b/src/applications/diffusion/query/DiffusionCommitHintQuery.php index 34d7bb4e6c..28ae1ed709 100644 --- a/src/applications/diffusion/query/DiffusionCommitHintQuery.php +++ b/src/applications/diffusion/query/DiffusionCommitHintQuery.php @@ -43,7 +43,7 @@ final class DiffusionCommitHintQuery if ($this->repositoryPHIDs !== null) { $where[] = qsprintf( $conn, - 'reositoryPHID IN (%Ls)', + 'repositoryPHID IN (%Ls)', $this->repositoryPHIDs); } diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 654ea42c3e..3e59c7c53d 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -35,7 +35,6 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO const TABLE_PATHCHANGE = 'repository_pathchange'; const TABLE_FILESYSTEM = 'repository_filesystem'; const TABLE_SUMMARY = 'repository_summary'; - const TABLE_BADCOMMIT = 'repository_badcommit'; const TABLE_LINTMESSAGE = 'repository_lintmessage'; const TABLE_PARENTS = 'repository_parents'; const TABLE_COVERAGE = 'repository_coverage'; diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommitHint.php b/src/applications/repository/storage/PhabricatorRepositoryCommitHint.php index 4523617038..9ab8d2d056 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommitHint.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommitHint.php @@ -101,6 +101,11 @@ final class PhabricatorRepositoryCommitHint } + public function isUnreadable() { + return ($this->getHintType() == self::HINT_UNREADABLE); + } + + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/repository/storage/PhabricatorRepositorySchemaSpec.php b/src/applications/repository/storage/PhabricatorRepositorySchemaSpec.php index 8ff2ed0c9b..8f93e31c29 100644 --- a/src/applications/repository/storage/PhabricatorRepositorySchemaSpec.php +++ b/src/applications/repository/storage/PhabricatorRepositorySchemaSpec.php @@ -6,20 +6,6 @@ final class PhabricatorRepositorySchemaSpec public function buildSchemata() { $this->buildEdgeSchemata(new PhabricatorRepository()); - $this->buildRawSchema( - id(new PhabricatorRepository())->getApplicationName(), - PhabricatorRepository::TABLE_BADCOMMIT, - array( - 'fullCommitName' => 'text64', - 'description' => 'text', - ), - array( - 'PRIMARY' => array( - 'columns' => array('fullCommitName'), - 'unique' => true, - ), - )); - $this->buildRawSchema( id(new PhabricatorRepository())->getApplicationName(), PhabricatorRepository::TABLE_COVERAGE, diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php index b565d30c55..9d3b2793f8 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php @@ -95,16 +95,16 @@ abstract class PhabricatorRepositoryCommitParserWorker PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit); - protected function isBadCommit(PhabricatorRepositoryCommit $commit) { - $repository = new PhabricatorRepository(); + protected function loadCommitHint(PhabricatorRepositoryCommit $commit) { + $viewer = PhabricatorUser::getOmnipotentUser(); - $bad_commit = queryfx_one( - $repository->establishConnection('w'), - 'SELECT * FROM %T WHERE fullCommitName = %s', - PhabricatorRepository::TABLE_BADCOMMIT, - $commit->getMonogram()); + $repository = $commit->getRepository(); - return (bool)$bad_commit; + return id(new DiffusionCommitHintQuery()) + ->setViewer($viewer) + ->withRepositoryPHIDs(array($repository->getPHID())) + ->withOldCommitIdentifiers(array($commit->getCommitIdentifier())) + ->executeOne(); } public function renderForDisplay(PhabricatorUser $viewer) { diff --git a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php index f58856d614..6d79742a32 100644 --- a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php +++ b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php @@ -22,8 +22,13 @@ abstract class PhabricatorRepositoryCommitChangeParserWorker PhabricatorRepositoryCommit $commit) { $this->log("%s\n", pht('Parsing "%s"...', $commit->getMonogram())); - if ($this->isBadCommit($commit)) { - $this->log(pht('This commit is marked bad!')); + + $hint = $this->loadCommitHint($commit); + if ($hint && $hint->isUnreadable()) { + $this->log( + pht( + 'This commit is marked as unreadable, so changes will not be '. + 'parsed.')); return; }