From f7a1feea3867379261a75e4808900b98cb0f6209 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 20 Jan 2014 13:12:44 -0800 Subject: [PATCH] Begin making change parsers testable Summary: Ref T4327. There are a bunch of other probably-related tasks too, some linked there. We have some rare/unusual bugs in the change parsers, mostly in Subversion, but it's terrifying to touch them because they're complicated and fragile and have no test coverage. To fix this stuff, I want to make them more testable. In particular, they basically end with this big INSERT right now. Instead, I'm going to make them return objects representing the data to be inserted, then have the common infrastructure do the insert. This gives us two benefits: - Reduced code duplication on the insert; - we can stop before the insert and have unit tests examine the objects. This swaps the Git parser over, but doesn't swap the hg/svn parsers yet. I'll do those separately, the SVN one looks a bit tricky. Test Plan: - Used `scripts/repository/reparse.php` to reparse a Git commit, with `--trace`. Verified it looked the same as before and the SQL that was executed seemed reasonable. - Did the same for `hg` / `svn` commits, to make sure I didn't derp anything. These aren't expected to do anything differently. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4327 Differential Revision: https://secure.phabricator.com/D7980 --- src/__phutil_library_map__.php | 2 + .../PhabricatorRepositoryParsedChange.php | 77 +++++++++++++++++++ ...habricatorRepositoryCommitParserWorker.php | 3 +- ...atorRepositoryCommitChangeParserWorker.php | 59 ++++++++++++-- ...rRepositoryGitCommitChangeParserWorker.php | 53 +++++-------- ...itoryMercurialCommitChangeParserWorker.php | 2 + ...rRepositorySvnCommitChangeParserWorker.php | 2 + 7 files changed, 158 insertions(+), 40 deletions(-) create mode 100644 src/applications/repository/data/PhabricatorRepositoryParsedChange.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 202bb4a14e..e18b4da7cf 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1866,6 +1866,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryPHIDTypeMirror' => 'applications/repository/phid/PhabricatorRepositoryPHIDTypeMirror.php', 'PhabricatorRepositoryPHIDTypePushLog' => 'applications/repository/phid/PhabricatorRepositoryPHIDTypePushLog.php', 'PhabricatorRepositoryPHIDTypeRepository' => 'applications/repository/phid/PhabricatorRepositoryPHIDTypeRepository.php', + 'PhabricatorRepositoryParsedChange' => 'applications/repository/data/PhabricatorRepositoryParsedChange.php', 'PhabricatorRepositoryPullEngine' => 'applications/repository/engine/PhabricatorRepositoryPullEngine.php', 'PhabricatorRepositoryPullLocalDaemon' => 'applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php', 'PhabricatorRepositoryPushLog' => 'applications/repository/storage/PhabricatorRepositoryPushLog.php', @@ -4542,6 +4543,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryPHIDTypeMirror' => 'PhabricatorPHIDType', 'PhabricatorRepositoryPHIDTypePushLog' => 'PhabricatorPHIDType', 'PhabricatorRepositoryPHIDTypeRepository' => 'PhabricatorPHIDType', + 'PhabricatorRepositoryParsedChange' => 'Phobject', 'PhabricatorRepositoryPullEngine' => 'PhabricatorRepositoryEngine', 'PhabricatorRepositoryPullLocalDaemon' => 'PhabricatorDaemon', 'PhabricatorRepositoryPushLog' => diff --git a/src/applications/repository/data/PhabricatorRepositoryParsedChange.php b/src/applications/repository/data/PhabricatorRepositoryParsedChange.php new file mode 100644 index 0000000000..668a2f8c89 --- /dev/null +++ b/src/applications/repository/data/PhabricatorRepositoryParsedChange.php @@ -0,0 +1,77 @@ +pathID = $path_id; + return $this; + } + + public function getPathID() { + return $this->pathID; + } + + public function setCommitSequence($commit_sequence) { + $this->commitSequence = $commit_sequence; + return $this; + } + + public function getCommitSequence() { + return $this->commitSequence; + } + + public function setIsDirect($is_direct) { + $this->isDirect = $is_direct; + return $this; + } + + public function getIsDirect() { + return $this->isDirect; + } + + public function setFileType($file_type) { + $this->fileType = $file_type; + return $this; + } + + public function getFileType() { + return $this->fileType; + } + + public function setChangeType($change_type) { + $this->changeType = $change_type; + return $this; + } + + public function getChangeType() { + return $this->changeType; + } + + public function setTargetCommitID($target_commit_id) { + $this->targetCommitID = $target_commit_id; + return $this; + } + + public function getTargetCommitID() { + return $this->targetCommitID; + } + + + public function setTargetPathID($target_path_id) { + $this->targetPathID = $target_path_id; + return $this; + } + + public function getTargetPathID() { + return $this->targetPathID; + } + +} diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php index 3c1a6563d8..b93474e71c 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php @@ -40,8 +40,7 @@ abstract class PhabricatorRepositoryCommitParserWorker } $this->repository = $repository; - - return $this->parseCommit($repository, $this->commit); + $this->parseCommit($repository, $this->commit); } final protected function shouldQueueFollowupTasks() { diff --git a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php index bcd8f859f6..7a7cb6254d 100644 --- a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php +++ b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php @@ -24,14 +24,15 @@ abstract class PhabricatorRepositoryCommitChangeParserWorker $this->log("Parsing %s...\n", $full_name); if ($this->isBadCommit($full_name)) { $this->log("This commit is marked bad!"); - $result = null; - } else { - $result = $this->parseCommitChanges($repository, $commit); + return; + } + + $results = $this->parseCommitChanges($repository, $commit); + if ($results) { + $this->writeCommitChanges($repository, $commit, $results); } $this->finishParse(); - - return $result; } public static function lookupOrCreatePaths(array $paths) { @@ -99,4 +100,52 @@ abstract class PhabricatorRepositoryCommitChangeParserWorker } } + private function writeCommitChanges( + PhabricatorRepository $repository, + PhabricatorRepositoryCommit $commit, + array $changes) { + + $repository_id = (int)$repository->getID(); + $commit_id = (int)$commit->getID(); + + // NOTE: This SQL is being built manually instead of with qsprintf() + // because some SVN changes affect an enormous number of paths (millions) + // and this showed up as significantly slow on a profile at some point. + + $changes_sql = array(); + foreach ($changes as $change) { + $values = array( + $repository_id, + (int)$change->getPathID(), + $commit_id, + nonempty((int)$change->getTargetPathID(), 'null'), + nonempty((int)$change->getTargetCommitID(), 'null'), + (int)$change->getChangeType(), + (int)$change->getFileType(), + (int)$change->getIsDirect(), + (int)$change->getCommitSequence(), + ); + $changes_sql[] = '('.implode(', ', $values).')'; + } + + $conn_w = $repository->establishConnection('w'); + + queryfx( + $conn_w, + 'DELETE FROM %T WHERE commitID = %d', + PhabricatorRepository::TABLE_PATHCHANGE, + $commit_id); + + foreach (PhabricatorLiskDAO::chunkSQL($changes_sql) as $chunk) { + queryfx( + $conn_w, + 'INSERT INTO %T + (repositoryID, pathID, commitID, targetPathID, targetCommitID, + changeType, fileType, isDirect, commitSequence) + VALUES %Q', + PhabricatorRepository::TABLE_PATHCHANGE, + $chunk); + } + } + } diff --git a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryGitCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryGitCommitChangeParserWorker.php index da2cdbf18f..2242065729 100644 --- a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryGitCommitChangeParserWorker.php +++ b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryGitCommitChangeParserWorker.php @@ -228,43 +228,30 @@ final class PhabricatorRepositoryGitCommitChangeParserWorker } } - $conn_w = $repository->establishConnection('w'); - $changes_sql = array(); + $results = array(); foreach ($changes as $change) { - $values = array( - (int)$change['repositoryID'], - (int)$change['pathID'], - (int)$change['commitID'], - $change['targetPathID'] - ? (int)$change['targetPathID'] - : 'null', - $change['targetCommitID'] - ? (int)$change['targetCommitID'] - : 'null', - (int)$change['changeType'], - (int)$change['fileType'], - (int)$change['isDirect'], - (int)$change['commitSequence'], - ); - $changes_sql[] = '('.implode(', ', $values).')'; + $target_path_id = $change['targetPathID'] + ? (int)$change['targetPathID'] + : null; + + $target_commit_id = $change['targetCommitID'] + ? (int)$change['targetCommitID'] + : null; + + $result = id(new PhabricatorRepositoryParsedChange()) + ->setPathID((int)$change['pathID']) + ->setTargetPathID($target_path_id) + ->setTargetCommitID($target_commit_id) + ->setChangeType((int)$change['changeType']) + ->setFileType($change['fileType']) + ->setIsDirect((int)$change['isDirect']) + ->setCommitSequence((int)$change['commitSequence']); + + $results[] = $result; } - queryfx( - $conn_w, - 'DELETE FROM %T WHERE commitID = %d', - PhabricatorRepository::TABLE_PATHCHANGE, - $commit->getID()); - foreach (array_chunk($changes_sql, 256) as $sql_chunk) { - queryfx( - $conn_w, - 'INSERT INTO %T - (repositoryID, pathID, commitID, targetPathID, targetCommitID, - changeType, fileType, isDirect, commitSequence) - VALUES %Q', - PhabricatorRepository::TABLE_PATHCHANGE, - implode(', ', $sql_chunk)); - } + return $results; } } diff --git a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryMercurialCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryMercurialCommitChangeParserWorker.php index 14360b6f7f..cf17e65151 100644 --- a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryMercurialCommitChangeParserWorker.php +++ b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryMercurialCommitChangeParserWorker.php @@ -300,6 +300,8 @@ final class PhabricatorRepositoryMercurialCommitChangeParserWorker PhabricatorRepository::TABLE_PATHCHANGE, implode(', ', $sql_chunk)); } + + return array(); } private function mercurialPathExists( diff --git a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositorySvnCommitChangeParserWorker.php b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositorySvnCommitChangeParserWorker.php index 428780f2f4..b09eefdda6 100644 --- a/src/applications/repository/worker/commitchangeparser/PhabricatorRepositorySvnCommitChangeParserWorker.php +++ b/src/applications/repository/worker/commitchangeparser/PhabricatorRepositorySvnCommitChangeParserWorker.php @@ -359,6 +359,8 @@ final class PhabricatorRepositorySvnCommitChangeParserWorker $this->writeChanges($repository, $commit, $effects, $path_map, $commit_map); $this->writeBrowse($repository, $commit, $effects, $path_map); + + return array(); } private function writeChanges(