diff --git a/resources/sql/patches/20130304.lintauthor.sql b/resources/sql/patches/20130304.lintauthor.sql new file mode 100644 index 0000000000..ffcb484c26 --- /dev/null +++ b/resources/sql/patches/20130304.lintauthor.sql @@ -0,0 +1,3 @@ +ALTER TABLE `{$NAMESPACE}_repository`.`repository_lintmessage` + ADD authorPHID varchar(64) COLLATE utf8_bin AFTER line, + ADD INDEX key_author (authorPHID); diff --git a/scripts/repository/save_lint.php b/scripts/repository/save_lint.php index 2777d3cd5b..0a23adb49c 100755 --- a/scripts/repository/save_lint.php +++ b/scripts/repository/save_lint.php @@ -39,6 +39,10 @@ $args = id(new PhutilArgumentParser($argv)) 'default' => 256, 'help' => "Number of paths passed to `arc` at once.", ), + array( + 'name' => 'blame', + 'help' => "Assign lint errors to authors who last modified the line.", + ), )); echo "Saving lint errors to database...\n"; @@ -48,6 +52,7 @@ $count = id(new DiffusionLintSaveRunner()) ->setArc($args->getArg('arc')) ->setSeverity($args->getArg('severity')) ->setChunkSize($args->getArg('chunk-size')) + ->setNeedsBlame($args->getArg('blame')) ->run('.'); echo "\nProcessed {$count} files.\n"; diff --git a/src/applications/diffusion/DiffusionLintSaveRunner.php b/src/applications/diffusion/DiffusionLintSaveRunner.php index 90069b1660..6d2f0b3b4e 100644 --- a/src/applications/diffusion/DiffusionLintSaveRunner.php +++ b/src/applications/diffusion/DiffusionLintSaveRunner.php @@ -5,6 +5,7 @@ final class DiffusionLintSaveRunner { private $severity = ArcanistLintSeverity::SEVERITY_ADVICE; private $all = false; private $chunkSize = 256; + private $needsBlame = false; private $svnRoot; private $lintCommit; @@ -12,6 +13,7 @@ final class DiffusionLintSaveRunner { private $conn; private $deletes = array(); private $inserts = array(); + private $blame = array(); public function setArc($path) { @@ -34,6 +36,11 @@ final class DiffusionLintSaveRunner { return $this; } + public function setNeedsBlame($boolean) { + $this->needsBlame = $boolean; + return $this; + } + public function run($dir) { $working_copy = ArcanistWorkingCopyIdentity::newFromPath($dir); @@ -44,7 +51,7 @@ final class DiffusionLintSaveRunner { $svn_fetch = $api->getGitConfig('svn-remote.svn.fetch'); list($this->svnRoot) = explode(':', $svn_fetch); if ($this->svnRoot != '') { - $this->svnRoot = '/' . $this->svnRoot; + $this->svnRoot = '/'.$this->svnRoot; } } @@ -98,8 +105,6 @@ final class DiffusionLintSaveRunner { $all_files = $api->getAllFiles(); } - $this->deletes = array(); - $this->inserts = array(); $count = 0; $files = array(); @@ -123,9 +128,16 @@ final class DiffusionLintSaveRunner { $this->runArcLint($files); $this->saveLintMessages(); - $this->branch->setLintCommit($api->getUnderlyingWorkingCopyRevision()); + + $this->lintCommit = $api->getUnderlyingWorkingCopyRevision(); + $this->branch->setLintCommit($this->lintCommit); $this->branch->save(); + if ($this->blame) { + $this->blameAuthors(); + $this->blame = array(); + } + return $count; } @@ -156,22 +168,26 @@ final class DiffusionLintSaveRunner { } foreach ($messages as $message) { + $line = idx($message, 'line', 0); + $this->inserts[] = qsprintf( $this->conn, '(%d, %s, %d, %s, %s, %s, %s)', $this->branch->getID(), $this->svnRoot.'/'.$path, - idx($message, 'line', 0), + $line, idx($message, 'code', ''), idx($message, 'severity', ''), idx($message, 'name', ''), idx($message, 'description', '')); + + if ($line && $this->needsBlame) { + $this->blame[$path][$line] = true; + } } if (count($this->deletes) >= 1024 || count($this->inserts) >= 256) { - $this->saveLintMessages($this->branch); - $this->deletes = array(); - $this->inserts = array(); + $this->saveLintMessages(); } } } @@ -205,6 +221,67 @@ final class DiffusionLintSaveRunner { } $this->conn->saveTransaction(); + + $this->deletes = array(); + $this->inserts = array(); + } + + + private function blameAuthors() { + $repository = id(new PhabricatorRepository())->load( + $this->branch->getRepositoryID()); + + $queries = array(); + $futures = array(); + foreach ($this->blame as $path => $lines) { + $drequest = DiffusionRequest::newFromDictionary(array( + 'repository' => $repository, + 'branch' => $this->branch->getName(), + 'path' => $path, + 'commit' => $this->lintCommit, + )); + $query = DiffusionFileContentQuery::newFromDiffusionRequest($drequest) + ->setNeedsBlame(true); + $queries[$path] = $query; + $futures[$path] = $query->getFileContentFuture(); + } + + $authors = array(); + + foreach (Futures($futures)->limit(8) as $path => $future) { + $queries[$path]->loadFileContentFromFuture($future); + list(, $rev_list, $blame_dict) = $queries[$path]->getBlameData(); + foreach (array_keys($this->blame[$path]) as $line) { + $commit_identifier = $rev_list[$line - 1]; + $author = idx($blame_dict[$commit_identifier], 'authorPHID'); + if ($author) { + $authors[$author][$path][] = $line; + } + } + } + + if ($authors) { + $this->conn->openTransaction(); + + foreach ($authors as $author => $paths) { + $where = array(); + foreach ($paths as $path => $lines) { + $where[] = qsprintf( + $this->conn, + '(path = %s AND line IN (%Ld))', + $this->svnRoot.'/'.$path, + $lines); + } + queryfx( + $this->conn, + 'UPDATE %T SET authorPHID = %s WHERE %Q', + PhabricatorRepository::TABLE_LINTMESSAGE, + $author, + implode(' OR ', $where)); + } + + $this->conn->saveTransaction(); + } } } diff --git a/src/applications/diffusion/controller/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/DiffusionBrowseFileController.php index 076e041a69..80898a2f71 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseFileController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseFileController.php @@ -202,11 +202,7 @@ final class DiffusionBrowseFileController extends DiffusionController { $rows = array(); foreach ($text_list as $k => $line) { $rev = $rev_list[$k]; - if (isset($blame_dict[$rev]['handle'])) { - $author = $blame_dict[$rev]['handle']->getName(); - } else { - $author = $blame_dict[$rev]['author']; - } + $author = $blame_dict[$rev]['author']; $rows[] = sprintf("%-10s %-20s %s", substr($rev, 0, 7), $author, $line); } @@ -430,11 +426,13 @@ final class DiffusionBrowseFileController extends DiffusionController { DiffusionFileContentQuery $file_query, $selected) { + $handles = array(); if ($blame_dict) { $epoch_list = ipull(ifilter($blame_dict, 'epoch'), 'epoch'); $epoch_min = min($epoch_list); $epoch_max = max($epoch_list); $epoch_range = ($epoch_max - $epoch_min) + 1; + $handles = $this->loadViewerHandles(ipull($blame_dict, 'authorPHID')); } $line_arr = array(); @@ -499,8 +497,9 @@ final class DiffusionBrowseFileController extends DiffusionController { $display_line['color'] = $color; $display_line['commit'] = $rev; - if (isset($blame['handle'])) { - $author_link = $blame['handle']->renderLink(); + $author_phid = idx($blame, 'authorPHID'); + if ($author_phid && $handles[$author_phid]) { + $author_link = $handles[$author_phid]->renderLink(); } else { $author_link = phutil_tag( 'span', diff --git a/src/applications/diffusion/controller/DiffusionLintController.php b/src/applications/diffusion/controller/DiffusionLintController.php index cb558b1cfc..086b1cf236 100644 --- a/src/applications/diffusion/controller/DiffusionLintController.php +++ b/src/applications/diffusion/controller/DiffusionLintController.php @@ -169,29 +169,30 @@ final class DiffusionLintController extends DiffusionController { } if ($owner_phids) { + $or = array(); + $or[] = qsprintf($conn, 'authorPHID IN (%Ls)', $owner_phids); + + $paths = array(); $packages = id(new PhabricatorOwnersOwner()) ->loadAllWhere('userPHID IN (%Ls)', $owner_phids); - if (!$packages) { - return array(); + if ($packages) { + $paths = id(new PhabricatorOwnersPath())->loadAllWhere( + 'packageID IN (%Ld)', + mpull($packages, 'getPackageID')); } - $paths = id(new PhabricatorOwnersPath()) - ->loadAllWhere('packageID IN (%Ld)', array_keys($packages)); - if (!$paths) { - return array(); + if ($paths) { + $repositories = id(new PhabricatorRepository())->loadAllWhere( + 'phid IN (%Ls)', + array_unique(mpull($paths, 'getRepositoryPHID'))); + $repositories = mpull($repositories, 'getID', 'getPHID'); + + $branches = id(new PhabricatorRepositoryBranch())->loadAllWhere( + 'repositoryID IN (%Ld)', + $repositories); + $branches = mgroup($branches, 'getRepositoryID'); } - $repositories = id(new PhabricatorRepository())->loadAllWhere( - 'phid IN (%Ls)', - array_unique(mpull($paths, 'getRepositoryPHID'))); - $repositories = mpull($repositories, 'getID', 'getPHID'); - - $branches = id(new PhabricatorRepositoryBranch())->loadAllWhere( - 'repositoryID IN (%Ld)', - $repositories); - $branches = mgroup($branches, 'getRepositoryID'); - - $or = array(); foreach ($paths as $path) { $branch = idx($branches, $repositories[$path->getRepositoryPHID()]); if ($branch) { @@ -207,9 +208,6 @@ final class DiffusionLintController extends DiffusionController { } } } - if (!$or) { - return array(); - } $where[] = '('.implode(' OR ', $or).')'; } diff --git a/src/applications/diffusion/controller/DiffusionLintDetailsController.php b/src/applications/diffusion/controller/DiffusionLintDetailsController.php index 0b5035acb8..0ccb3028a1 100644 --- a/src/applications/diffusion/controller/DiffusionLintDetailsController.php +++ b/src/applications/diffusion/controller/DiffusionLintDetailsController.php @@ -11,6 +11,8 @@ final class DiffusionLintDetailsController extends DiffusionController { $messages = $this->loadLintMessages($branch, $limit, $offset); $is_dir = (substr('/'.$drequest->getPath(), -1) == '/'); + $authors = $this->loadViewerHandles(ipull($messages, 'authorPHID')); + $rows = array(); foreach ($messages as $message) { $path = hsprintf( @@ -31,9 +33,15 @@ final class DiffusionLintDetailsController extends DiffusionController { )), $message['line']); + $author = $message['authorPHID']; + if ($author && $authors[$author]) { + $author = $authors[$author]->renderLink(); + } + $rows[] = array( $path, $line, + $author, ArcanistLintSeverity::getStringForSeverity($message['severity']), $message['name'], $message['description'], @@ -44,11 +52,12 @@ final class DiffusionLintDetailsController extends DiffusionController { ->setHeaders(array( 'Path', 'Line', + 'Author', 'Severity', 'Name', 'Description', )) - ->setColumnClasses(array('', 'n', '', '', '')) + ->setColumnClasses(array('', 'n')) ->setColumnVisibility(array($is_dir)); $content = array(); diff --git a/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php index 086ff4f458..f7012088ae 100644 --- a/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/DiffusionFileContentQuery.php @@ -11,8 +11,11 @@ abstract class DiffusionFileContentQuery extends DiffusionQuery { return parent::newQueryObject(__CLASS__, $request); } - final public function loadFileContent() { - $this->fileContent = $this->executeQuery(); + abstract public function getFileContentFuture(); + abstract protected function executeQueryFromFuture(Future $future); + + final public function loadFileContentFromFuture(Future $future) { + $this->fileContent = $this->executeQueryFromFuture($future); $repository = $this->getRequest()->getRepository(); $try_encoding = $repository->getDetail('encoding'); @@ -25,6 +28,14 @@ abstract class DiffusionFileContentQuery extends DiffusionQuery { return $this->fileContent; } + final protected function executeQuery() { + return $this->loadFileContentFromFuture($this->getFileContentFuture()); + } + + final public function loadFileContent() { + return $this->executeQuery(); + } + final public function getRawData() { return $this->fileContent->getCorpus(); } @@ -72,30 +83,22 @@ abstract class DiffusionFileContentQuery extends DiffusionQuery { $commit->getEpoch(); } - $commits_data = array(); if ($commits) { $commits_data = id(new PhabricatorRepositoryCommitData())->loadAllWhere( 'commitID IN (%Ls)', mpull($commits, 'getID')); - } - $phids = array(); - foreach ($commits_data as $data) { - $phids[] = $data->getCommitDetail('authorPHID'); - } - - $loader = new PhabricatorObjectHandleData(array_unique($phids)); - $loader->setViewer($this->viewer); - $handles = $loader->loadHandles(); - - foreach ($commits_data as $data) { - if ($data->getCommitDetail('authorPHID')) { - $commit_identifier = - $commits[$data->getCommitID()]->getCommitIdentifier(); - $blame_dict[$commit_identifier]['handle'] = - $handles[$data->getCommitDetail('authorPHID')]; + foreach ($commits_data as $data) { + $author_phid = $data->getCommitDetail('authorPHID'); + if (!$author_phid) { + continue; + } + $commit = $commits[$data->getCommitID()]; + $commit_identifier = $commit->getCommitIdentifier(); + $blame_dict[$commit_identifier]['authorPHID'] = $author_phid; } } + } return array($text_list, $rev_list, $blame_dict); diff --git a/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php index 4703937371..30e10c9177 100644 --- a/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php @@ -2,7 +2,7 @@ final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery { - protected function executeQuery() { + public function getFileContentFuture() { $drequest = $this->getRequest(); $repository = $drequest->getRepository(); @@ -10,16 +10,20 @@ final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery { $commit = $drequest->getCommit(); if ($this->getNeedsBlame()) { - list($corpus) = $repository->execxLocalCommand( + return $repository->getLocalCommandFuture( '--no-pager blame -c -l --date=short %s -- %s', $commit, $path); } else { - list($corpus) = $repository->execxLocalCommand( + return $repository->getLocalCommandFuture( 'cat-file blob %s:%s', $commit, $path); } + } + + protected function executeQueryFromFuture(Future $future) { + list($corpus) = $future->resolvex(); $file_content = new DiffusionFileContent(); $file_content->setCorpus($corpus); @@ -27,7 +31,6 @@ final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery { return $file_content; } - protected function tokenizeLine($line) { $m = array(); // sample lines: diff --git a/src/applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php index 512a427253..c97b53e75a 100644 --- a/src/applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php @@ -3,7 +3,7 @@ final class DiffusionMercurialFileContentQuery extends DiffusionFileContentQuery { - protected function executeQuery() { + public function getFileContentFuture() { $drequest = $this->getRequest(); $repository = $drequest->getRepository(); @@ -13,16 +13,20 @@ final class DiffusionMercurialFileContentQuery if ($this->getNeedsBlame()) { // NOTE: We're using "--number" instead of "--changeset" because there is // no way to get "--changeset" to show us the full commit hashes. - list($corpus) = $repository->execxLocalCommand( + return $repository->getLocalCommandFuture( 'annotate --user --number --rev %s -- %s', $commit, $path); } else { - list($corpus) = $repository->execxLocalCommand( + return $repository->getLocalCommandFuture( 'cat --rev %s -- %s', $commit, $path); } + } + + protected function executeQueryFromFuture(Future $future) { + list($corpus) = $future->resolvex(); $file_content = new DiffusionFileContent(); $file_content->setCorpus($corpus); diff --git a/src/applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php index 8cb6c10dc9..8c37389d19 100644 --- a/src/applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php +++ b/src/applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php @@ -2,7 +2,7 @@ final class DiffusionSvnFileContentQuery extends DiffusionFileContentQuery { - protected function executeQuery() { + public function getFileContentFuture() { $drequest = $this->getRequest(); $repository = $drequest->getRepository(); @@ -11,13 +11,17 @@ final class DiffusionSvnFileContentQuery extends DiffusionFileContentQuery { $remote_uri = $repository->getRemoteURI(); + return $repository->getRemoteCommandFuture( + '%C %s%s@%s', + $this->getNeedsBlame() ? 'blame --force' : 'cat', + $remote_uri, + phutil_escape_uri($path), + $commit); + } + + protected function executeQueryFromFuture(Future $future) { try { - list($corpus) = $repository->execxRemoteCommand( - '%C %s%s@%s', - $this->getNeedsBlame() ? 'blame --force' : 'cat', - $remote_uri, - phutil_escape_uri($path), - $commit); + list($corpus) = $future->resolvex(); } catch (CommandException $ex) { $stderr = $ex->getStdErr(); if (preg_match('/path not found$/', trim($stderr))) { diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index e3ea4e2eec..2d4d5ec191 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1161,6 +1161,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('20131302.maniphestvalue.sql'), ), + '20130304.lintauthor.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('20130304.lintauthor.sql'), + ), ); }