From a74545c9da4bef7d566de90089c1ee7fba73a664 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 17 May 2014 16:10:54 -0700 Subject: [PATCH] Provide a rough, unstable API for reporting coverage into Diffusion Summary: Ref T4994. This stuff works: - You can dump a blob of coverage information into `diffusion.updatecoverage`. This wipes existing coverage information and replaces it. - It shows up when viewing files. - It shows up when viewing commits. This stuff does not work: - When viewing files, the Javascript hover interaction isn't tied in yet. - We always show this information, even if you're behind the commit where it was generated. - You can't do incremental updates. - There's no aggregation at the file (this file has 90% coverage), diff (the changes in this commit are 90% covered), or directory (the code in this directory has 90% coverage) levels yet. - This is probably not the final form of the UI, storage, or API, so you should expect occasional changes over time. I've marked the method as "Unstable" for now. Test Plan: - Ran `save_lint.php` to check for collateral damage; it worked fine. - Ran `save_lint.php` on a new branch to check creation. - Published some fake coverage information. - Viewed an affected commit. - Viewed an affected file. {F151915} {F151916} Reviewers: chad, btrahan Reviewed By: btrahan Subscribers: jhurwitz, epriestley, zeeg Maniphest Tasks: T5044, T4994 Differential Revision: https://secure.phabricator.com/D9022 --- resources/celerity/map.php | 6 +- .../sql/autopatches/20140509.coverage.1.sql | 8 ++ src/__phutil_library_map__.php | 2 + .../diffusion/DiffusionLintSaveRunner.php | 15 +-- ...uitAPI_diffusion_updatecoverage_Method.php | 97 +++++++++++++++++++ .../DiffusionBrowseFileController.php | 47 ++++++++- .../controller/DiffusionDiffController.php | 5 + .../diffusion/request/DiffusionRequest.php | 40 +++++++- .../storage/PhabricatorRepositoryBranch.php | 19 ++++ .../differential/changeset-view.css | 8 ++ 10 files changed, 226 insertions(+), 21 deletions(-) create mode 100644 resources/sql/autopatches/20140509.coverage.1.sql create mode 100644 src/applications/diffusion/conduit/ConduitAPI_diffusion_updatecoverage_Method.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index fa1ba81d14..c8cf00b60f 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'core.pkg.css' => '59ea1706', 'core.pkg.js' => 'b2ed04a2', 'darkconsole.pkg.js' => 'ca8671ce', - 'differential.pkg.css' => '4b8686e3', + 'differential.pkg.css' => '382ca868', 'differential.pkg.js' => '36bec171', 'diffusion.pkg.css' => '3783278d', 'diffusion.pkg.js' => '077e3ad0', @@ -55,7 +55,7 @@ return array( 'rsrc/css/application/dashboard/dashboard.css' => '2b41640b', 'rsrc/css/application/diff/inline-comment-summary.css' => '8cfd34e8', 'rsrc/css/application/differential/add-comment.css' => 'c478bcaa', - 'rsrc/css/application/differential/changeset-view.css' => '1570a1ff', + 'rsrc/css/application/differential/changeset-view.css' => 'c45747f0', 'rsrc/css/application/differential/core.css' => '7ac3cabc', 'rsrc/css/application/differential/results-table.css' => '239924f9', 'rsrc/css/application/differential/revision-comment.css' => '48186045', @@ -514,7 +514,7 @@ return array( 'conpherence-notification-css' => '403cf598', 'conpherence-update-css' => '1099a660', 'conpherence-widget-pane-css' => 'bf275a6c', - 'differential-changeset-view-css' => '1570a1ff', + 'differential-changeset-view-css' => 'c45747f0', 'differential-core-view-css' => '7ac3cabc', 'differential-inline-comment-editor' => 'f2441746', 'differential-results-table-css' => '239924f9', diff --git a/resources/sql/autopatches/20140509.coverage.1.sql b/resources/sql/autopatches/20140509.coverage.1.sql new file mode 100644 index 0000000000..f8bf09dfe2 --- /dev/null +++ b/resources/sql/autopatches/20140509.coverage.1.sql @@ -0,0 +1,8 @@ +CREATE TABLE {$NAMESPACE}_repository.repository_coverage ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + branchID INT UNSIGNED NOT NULL, + commitID INT UNSIGNED NOT NULL, + pathID INT UNSIGNED NOT NULL, + coverage LONGTEXT NOT NULL COLLATE latin1_bin, + KEY `key_path` (branchID, pathID, commitID) +) ENGINE=InnoDB, COLLATE utf8_general_ci; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 306060a84a..696021f986 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -178,6 +178,7 @@ phutil_register_library_map(array( 'ConduitAPI_diffusion_resolverefs_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_resolverefs_Method.php', 'ConduitAPI_diffusion_searchquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_searchquery_Method.php', 'ConduitAPI_diffusion_tagsquery_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_tagsquery_Method.php', + 'ConduitAPI_diffusion_updatecoverage_Method' => 'applications/diffusion/conduit/ConduitAPI_diffusion_updatecoverage_Method.php', 'ConduitAPI_feed_Method' => 'applications/feed/conduit/ConduitAPI_feed_Method.php', 'ConduitAPI_feed_publish_Method' => 'applications/feed/conduit/ConduitAPI_feed_publish_Method.php', 'ConduitAPI_feed_query_Method' => 'applications/feed/conduit/ConduitAPI_feed_query_Method.php', @@ -2838,6 +2839,7 @@ phutil_register_library_map(array( 'ConduitAPI_diffusion_resolverefs_Method' => 'ConduitAPI_diffusion_abstractquery_Method', 'ConduitAPI_diffusion_searchquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method', 'ConduitAPI_diffusion_tagsquery_Method' => 'ConduitAPI_diffusion_abstractquery_Method', + 'ConduitAPI_diffusion_updatecoverage_Method' => 'ConduitAPI_diffusion_Method', 'ConduitAPI_feed_Method' => 'ConduitAPIMethod', 'ConduitAPI_feed_publish_Method' => 'ConduitAPI_feed_Method', 'ConduitAPI_feed_query_Method' => 'ConduitAPI_feed_Method', diff --git a/src/applications/diffusion/DiffusionLintSaveRunner.php b/src/applications/diffusion/DiffusionLintSaveRunner.php index de07e680a7..23bc70a2ef 100644 --- a/src/applications/diffusion/DiffusionLintSaveRunner.php +++ b/src/applications/diffusion/DiffusionLintSaveRunner.php @@ -66,20 +66,14 @@ final class DiffusionLintSaveRunner { } $branch_name = $api->getBranchName(); - $this->branch = new PhabricatorRepositoryBranch(); - $this->conn = $this->branch->establishConnection('w'); - $this->branch = $this->branch->loadOneWhere( - 'repositoryID = %d AND name = %s', + + $this->branch = PhabricatorRepositoryBranch::loadOrCreateBranch( $project->getRepositoryID(), $branch_name); + $this->conn = $this->branch->establishConnection('w'); $this->lintCommit = null; - if (!$this->branch) { - $this->branch = id(new PhabricatorRepositoryBranch()) - ->setRepositoryID($project->getRepositoryID()) - ->setName($branch_name) - ->save(); - } else if (!$this->all) { + if (!$this->all) { $this->lintCommit = $this->branch->getLintCommit(); } @@ -95,6 +89,7 @@ final class DiffusionLintSaveRunner { } } + if (!$this->lintCommit) { $where = ($this->svnRoot ? qsprintf($this->conn, 'AND path LIKE %>', $this->svnRoot.'/') diff --git a/src/applications/diffusion/conduit/ConduitAPI_diffusion_updatecoverage_Method.php b/src/applications/diffusion/conduit/ConduitAPI_diffusion_updatecoverage_Method.php new file mode 100644 index 0000000000..3d24730235 --- /dev/null +++ b/src/applications/diffusion/conduit/ConduitAPI_diffusion_updatecoverage_Method.php @@ -0,0 +1,97 @@ + 'required phid', + 'branch' => 'required string', + 'commit' => 'required string', + 'coverage' => 'required map', + ); + } + + public function defineErrorTypes() { + return array(); + } + + protected function execute(ConduitAPIRequest $request) { + $viewer = $request->getUser(); + + $repository_phid = $request->getValue('repositoryPHID'); + $repository = id(new PhabricatorRepositoryQuery()) + ->setViewer($viewer) + ->withPHIDs(array($repository_phid)) + ->executeOne(); + + if (!$repository) { + throw new Exception( + pht('No repository exists with PHID "%s".', $repository_phid)); + } + + $commit_name = $request->getValue('commit'); + $commit = id(new DiffusionCommitQuery()) + ->setViewer($viewer) + ->withRepository($repository) + ->withIdentifiers(array($commit_name)) + ->executeOne(); + if (!$commit) { + throw new Exception( + pht('No commit exists with identifier "%s".', $commit_name)); + } + + $branch = PhabricatorRepositoryBranch::loadOrCreateBranch( + $repository->getID(), + $request->getValue('branch')); + + $coverage = $request->getValue('coverage'); + $path_map = id(new DiffusionPathIDQuery(array_keys($coverage))) + ->loadPathIDs(); + + $conn = $repository->establishConnection('w'); + + $sql = array(); + foreach ($coverage as $path => $coverage_info) { + $sql[] = qsprintf( + $conn, + '(%d, %d, %d, %s)', + $branch->getID(), + $path_map[$path], + $commit->getID(), + $coverage_info); + } + + $table_name = 'repository_coverage'; + + $conn->openTransaction(); + queryfx( + $conn, + 'DELETE FROM %T WHERE branchID = %d', + $table_name, + $branch->getID()); + + foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) { + queryfx( + $conn, + 'INSERT INTO %T (branchID, pathID, commitID, coverage) VALUES %Q', + $table_name, + $chunk); + } + $conn->saveTransaction(); + } + + +} diff --git a/src/applications/diffusion/controller/DiffusionBrowseFileController.php b/src/applications/diffusion/controller/DiffusionBrowseFileController.php index 957ae00c34..0e0825ba42 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseFileController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseFileController.php @@ -4,6 +4,7 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController { private $lintCommit; private $lintMessages; + private $coverage; public function processRequest() { $request = $this->getRequest(); @@ -68,6 +69,7 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController { } $this->loadLintMessages(); + $this->coverage = $drequest->loadCoverage(); $binary_uri = null; if (ArcanistDiffUtils::isHeuristicBinaryFile($data)) { @@ -625,7 +627,8 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController { $rows = $this->renderInlines( idx($inlines, 0, array()), - ($show_blame), + $show_blame, + (bool)$this->coverage, $engine); foreach ($display as $line) { @@ -789,6 +792,24 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController { phutil_safe_html(str_replace("\t", ' ', $line['data'])), )); + if ($this->coverage) { + require_celerity_resource('differential-changeset-view-css'); + $cov_index = $line['line'] - 1; + + if (isset($this->coverage[$cov_index])) { + $cov_class = $this->coverage[$cov_index]; + } else { + $cov_class = 'N'; + } + + $blame[] = phutil_tag( + 'td', + array( + 'class' => 'cov cov-'.$cov_class, + ), + ''); + } + $rows[] = phutil_tag( 'tr', array( @@ -800,7 +821,8 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController { $cur_inlines = $this->renderInlines( idx($inlines, $line['line'], array()), - ($show_blame), + $show_blame, + $this->coverage, $engine); foreach ($cur_inlines as $cur_inline) { $rows[] = $cur_inline; @@ -810,17 +832,34 @@ final class DiffusionBrowseFileController extends DiffusionBrowseController { return $rows; } - private function renderInlines(array $inlines, $needs_blame, $engine) { + private function renderInlines( + array $inlines, + $needs_blame, + $has_coverage, + $engine) { + $rows = array(); foreach ($inlines as $inline) { $inline_view = id(new DifferentialInlineCommentView()) ->setMarkupEngine($engine) ->setInlineComment($inline) ->render(); - $row = array_fill(0, ($needs_blame ? 5 : 1), phutil_tag('th')); + + $row = array_fill(0, ($needs_blame ? 3 : 1), phutil_tag('th')); + $row[] = phutil_tag('td', array(), $inline_view); + + if ($has_coverage) { + $row[] = phutil_tag( + 'td', + array( + 'class' => 'cov cov-I', + )); + } + $rows[] = phutil_tag('tr', array('class' => 'inline'), $row); } + return $rows; } diff --git a/src/applications/diffusion/controller/DiffusionDiffController.php b/src/applications/diffusion/controller/DiffusionDiffController.php index b8bc9680de..4541929f14 100644 --- a/src/applications/diffusion/controller/DiffusionDiffController.php +++ b/src/applications/diffusion/controller/DiffusionDiffController.php @@ -68,6 +68,11 @@ final class DiffusionDiffController extends DiffusionController { array( 'action' => 'rendering-ref'))); + $coverage = $drequest->loadCoverage(); + if ($coverage) { + $parser->setCoverage($coverage); + } + $pquery = new DiffusionPathIDQuery(array($changeset->getFilename())); $ids = $pquery->loadPathIDs(); $path_id = $ids[$changeset->getFilename()]; diff --git a/src/applications/diffusion/request/DiffusionRequest.php b/src/applications/diffusion/request/DiffusionRequest.php index 9bf439d815..a10a196b82 100644 --- a/src/applications/diffusion/request/DiffusionRequest.php +++ b/src/applications/diffusion/request/DiffusionRequest.php @@ -26,6 +26,7 @@ abstract class DiffusionRequest { private $initFromConduit = true; private $user; + private $branchObject = false; abstract protected function getSupportsBranches(); abstract protected function isStableCommit($symbol); @@ -338,12 +339,43 @@ abstract class DiffusionRequest { } public function loadBranch() { - return id(new PhabricatorRepositoryBranch())->loadOneWhere( - 'repositoryID = %d AND name = %s', - $this->getRepository()->getID(), - $this->getArcanistBranch()); + // TODO: Get rid of this and do real Queries on real objects. + + if ($this->branchObject === false) { + $this->branchObject = PhabricatorRepositoryBranch::loadBranch( + $this->getRepository()->getID(), + $this->getArcanistBranch()); + } + + return $this->branchObject; + } + + public function loadCoverage() { + // TODO: This should also die. + $branch = $this->loadBranch(); + if (!$branch) { + return; + } + + $path = $this->getPath(); + $path_map = id(new DiffusionPathIDQuery(array($path)))->loadPathIDs(); + + $coverage_row = queryfx_one( + id(new PhabricatorRepository())->establishConnection('r'), + 'SELECT * FROM %T WHERE branchID = %d AND pathID = %d + ORDER BY commitID DESC LIMIT 1', + 'repository_coverage', + $branch->getID(), + $path_map[$path]); + + if (!$coverage_row) { + return null; + } + + return idx($coverage_row, 'coverage'); } + public function loadCommit() { if (empty($this->repositoryCommit)) { $repository = $this->getRepository(); diff --git a/src/applications/repository/storage/PhabricatorRepositoryBranch.php b/src/applications/repository/storage/PhabricatorRepositoryBranch.php index 33e3ab24a6..5eb4d1f46f 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryBranch.php +++ b/src/applications/repository/storage/PhabricatorRepositoryBranch.php @@ -6,4 +6,23 @@ final class PhabricatorRepositoryBranch extends PhabricatorRepositoryDAO { protected $name; protected $lintCommit; + public static function loadBranch($repository_id, $branch_name) { + return id(new PhabricatorRepositoryBranch())->loadOneWhere( + 'repositoryID = %d AND name = %s', + $repository_id, + $branch_name); + } + + public static function loadOrCreateBranch($repository_id, $branch_name) { + $branch = self::loadBranch($repository_id, $branch_name); + if ($branch) { + return $branch; + } + + return id(new PhabricatorRepositoryBranch()) + ->setRepositoryID($repository_id) + ->setName($branch_name) + ->save(); + } + } diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index 4a7a1a2d83..be24646022 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -136,6 +136,10 @@ padding: 0; } +.diffusion-source td.cov { + padding: 0 8px; +} + td.cov-U { background: #dd8866; } @@ -152,6 +156,10 @@ td.cov-X { background: #aa00aa; } +td.cov-I { + background: {$lightgreybackground}; +} + .differential-diff td.source-cov-C, .differential-diff td.source-cov-C span.bright { background: #cceeff; -- 2.51.2