1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-09-20 01:08:50 +02:00

Drive blame generation through diffusion.blame

Summary:
Ref T2450. Ref T9319. This is still a bit messy, but not quite so bad as it was: instead of using a single call to get both blame information and file content, use `diffusion.blame` for blame information.

This will make optimizations to both blame and file content easier.

Test Plan: Viewed a bunch of blame (color on/off, blame on/off).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2450, T9319

Differential Revision: https://secure.phabricator.com/D14958
This commit is contained in:
epriestley 2016-01-06 06:15:25 -08:00
parent f561dc172d
commit 9728c65e93
10 changed files with 183 additions and 333 deletions

View file

@ -607,7 +607,6 @@ phutil_register_library_map(array(
'DiffusionGitBranch' => 'applications/diffusion/data/DiffusionGitBranch.php',
'DiffusionGitBranchTestCase' => 'applications/diffusion/data/__tests__/DiffusionGitBranchTestCase.php',
'DiffusionGitFileContentQuery' => 'applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php',
'DiffusionGitFileContentQueryTestCase' => 'applications/diffusion/query/__tests__/DiffusionGitFileContentQueryTestCase.php',
'DiffusionGitRawDiffQuery' => 'applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php',
'DiffusionGitReceivePackSSHWorkflow' => 'applications/diffusion/ssh/DiffusionGitReceivePackSSHWorkflow.php',
'DiffusionGitRequest' => 'applications/diffusion/request/DiffusionGitRequest.php',
@ -4577,7 +4576,6 @@ phutil_register_library_map(array(
'DiffusionGitBranch' => 'Phobject',
'DiffusionGitBranchTestCase' => 'PhabricatorTestCase',
'DiffusionGitFileContentQuery' => 'DiffusionFileContentQuery',
'DiffusionGitFileContentQueryTestCase' => 'PhabricatorTestCase',
'DiffusionGitRawDiffQuery' => 'DiffusionRawDiffQuery',
'DiffusionGitReceivePackSSHWorkflow' => 'DiffusionGitSSHWorkflow',
'DiffusionGitRequest' => 'DiffusionRequest',

View file

@ -257,8 +257,10 @@ final class DiffusionLintSaveRunner extends Phobject {
'path' => $path,
'commit' => $this->lintCommit,
));
$query = DiffusionFileContentQuery::newFromDiffusionRequest($drequest)
->setNeedsBlame(true);
// TODO: Restore blame information / generally fix this workflow.
$query = DiffusionFileContentQuery::newFromDiffusionRequest($drequest);
$queries[$path] = $query;
$futures[$path] = $query->getFileContentFuture();
}

View file

@ -19,7 +19,6 @@ final class DiffusionFileContentQueryConduitAPIMethod
return array(
'path' => 'required string',
'commit' => 'required string',
'needsBlame' => 'optional bool',
'timeout' => 'optional int',
'byteLimit' => 'optional int',
);
@ -27,12 +26,9 @@ final class DiffusionFileContentQueryConduitAPIMethod
protected function getResult(ConduitAPIRequest $request) {
$drequest = $this->getDiffusionRequest();
$needs_blame = $request->getValue('needsBlame');
$file_query = DiffusionFileContentQuery::newFromDiffusionRequest(
$drequest);
$file_query
->setViewer($request->getUser())
->setNeedsBlame($needs_blame);
$file_query = DiffusionFileContentQuery::newFromDiffusionRequest($drequest)
->setViewer($request->getUser());
$timeout = $request->getValue('timeout');
if ($timeout) {
@ -46,11 +42,7 @@ final class DiffusionFileContentQueryConduitAPIMethod
$file_content = $file_query->loadFileContent();
if ($needs_blame) {
list($text_list, $rev_list, $blame_dict) = $file_query->getBlameData();
} else {
$text_list = $rev_list = $blame_dict = array();
}
$text_list = $rev_list = $blame_dict = array();
$file_content
->setBlameDict($blame_dict)

View file

@ -130,7 +130,6 @@ final class DiffusionBrowseController extends DiffusionController {
$params = array(
'commit' => $drequest->getCommit(),
'path' => $drequest->getPath(),
'needsBlame' => $needs_blame,
);
$byte_limit = null;
@ -572,7 +571,24 @@ final class DiffusionBrowseController extends DiffusionController {
$path,
$data) {
$viewer = $this->getViewer();
$blame_handles = array();
if ($needs_blame) {
$blame = $this->loadBlame($path, $drequest->getCommit());
if ($blame) {
$author_phids = mpull($blame, 'getAuthorPHID');
$blame_handles = $viewer->loadHandles($author_phids);
}
} else {
$blame = array();
}
$file_corpus = $file_content->getCorpus();
if (!$show_color) {
$lines = phutil_split_lines($file_corpus);
$style =
'border: none; width: 100%; height: 80em; font-family: monospace';
if (!$show_blame) {
@ -581,18 +597,24 @@ final class DiffusionBrowseController extends DiffusionController {
array(
'style' => $style,
),
$file_content->getCorpus());
$file_corpus);
} else {
$text_list = $file_content->getTextList();
$rev_list = $file_content->getRevList();
$blame_dict = $file_content->getBlameDict();
$rows = array();
foreach ($text_list as $k => $line) {
$rev = $rev_list[$k];
$author = $blame_dict[$rev]['author'];
$rows[] =
sprintf('%-10s %-20s %s', substr($rev, 0, 7), $author, $line);
foreach ($lines as $line_number => $line) {
$commit = idx($blame, $line_number);
if ($commit) {
$author = $commit->renderAuthorShortName($blame_handles);
$commit_name = $commit->getShortName();
} else {
$author = null;
$commit_name = null;
}
$rows[] = sprintf(
'%-10s %-20s %s',
$commit_name,
$author,
$line);
}
$corpus = phutil_tag(
@ -600,22 +622,21 @@ final class DiffusionBrowseController extends DiffusionController {
array(
'style' => $style,
),
implode("\n", $rows));
implode('', $rows));
}
} else {
require_celerity_resource('syntax-highlighting-css');
$text_list = $file_content->getTextList();
$rev_list = $file_content->getRevList();
$blame_dict = $file_content->getBlameDict();
$text_list = implode("\n", $text_list);
$text_list = PhabricatorSyntaxHighlighter::highlightWithFilename(
$highlighted = PhabricatorSyntaxHighlighter::highlightWithFilename(
$path,
$text_list);
$text_list = explode("\n", $text_list);
$file_corpus);
$lines = phutil_split_lines($highlighted);
$rows = $this->buildDisplayRows($text_list, $rev_list, $blame_dict,
$needs_blame, $drequest, $show_blame, $show_color);
$rows = $this->buildDisplayRows(
$lines,
$blame,
$show_blame,
$show_color);
$corpus_table = javelin_tag(
'table',
@ -824,25 +845,26 @@ final class DiffusionBrowseController extends DiffusionController {
private function buildDisplayRows(
array $text_list,
array $rev_list,
array $blame_dict,
$needs_blame,
DiffusionRequest $drequest,
$show_blame,
$show_color) {
array $lines,
array $blame,
$show_color,
$show_blame) {
$drequest = $this->getDiffusionRequest();
$handles = array();
if ($blame_dict) {
$epoch_list = ipull(ifilter($blame_dict, 'epoch'), 'epoch');
if ($blame) {
$epoch_list = mpull($blame, 'getEpoch', 'getID');
$epoch_list = array_filter($epoch_list);
$epoch_list = array_unique($epoch_list);
$epoch_list = array_values($epoch_list);
$epoch_min = min($epoch_list);
$epoch_max = max($epoch_list);
$epoch_range = ($epoch_max - $epoch_min) + 1;
$author_phids = ipull(ifilter($blame_dict, 'authorPHID'), 'authorPHID');
$handles = $this->loadViewerHandles($author_phids);
}
$line_arr = array();
$line_str = $drequest->getLine();
$ranges = explode(',', $line_str);
@ -864,9 +886,9 @@ final class DiffusionBrowseController extends DiffusionController {
$display = array();
$line_number = 1;
$last_rev = null;
$last_commit = null;
$color = null;
foreach ($text_list as $k => $line) {
foreach ($lines as $line_index => $line) {
$display_line = array(
'epoch' => null,
'commit' => null,
@ -882,17 +904,26 @@ final class DiffusionBrowseController extends DiffusionController {
// with same color; otherwise generate blame info. The newer a change
// is, the more saturated the color.
$rev = idx($rev_list, $k, $last_rev);
$commit = idx($blame, $line_index, $last_commit);
if ($last_rev == $rev) {
if ($commit && $last_commit &&
($last_commit->getID() == $commit->getID())) {
$display_line['color'] = $color;
} else {
$blame = $blame_dict[$rev];
if (!isset($blame['epoch'])) {
$color = '#ffd'; // Render as warning.
if ($commit) {
$epoch = $commit->getEpoch();
} else {
$color_ratio = ($blame['epoch'] - $epoch_min) / $epoch_range;
$epoch = null;
}
if (!$epoch) {
if (!$blame) {
$color = '#f6f6f6';
} else {
$color = '#ffd'; // Render as warning.
}
} else {
$color_ratio = ($epoch - $epoch_min) / $epoch_range;
$color_value = 0xE6 * (1.0 - $color_ratio);
$color = sprintf(
'#%02x%02x%02x',
@ -901,19 +932,16 @@ final class DiffusionBrowseController extends DiffusionController {
$color_value);
}
$display_line['epoch'] = idx($blame, 'epoch');
$display_line['epoch'] = $epoch;
$display_line['color'] = $color;
$display_line['commit'] = $rev;
$author_phid = idx($blame, 'authorPHID');
if ($author_phid && $handles[$author_phid]) {
$author_link = $handles[$author_phid]->renderLink();
if ($commit) {
$display_line['commit'] = $commit;
} else {
$author_link = $blame['author'];
$display_line['commit'] = null;
}
$display_line['author'] = $author_link;
$last_rev = $rev;
$last_commit = $commit;
}
}
@ -936,15 +964,7 @@ final class DiffusionBrowseController extends DiffusionController {
$request = $this->getRequest();
$viewer = $request->getUser();
$commits = array_filter(ipull($display, 'commit'));
if ($commits) {
$commits = id(new DiffusionCommitQuery())
->setViewer($viewer)
->withRepository($drequest->getRepository())
->withIdentifiers($commits)
->execute();
$commits = mpull($commits, null, 'getCommitIdentifier');
}
$commits = mpull($blame, null, 'getCommitIdentifier');
$revision_ids = id(new DifferentialRevision())
->loadIDsByCommitPHIDs(mpull($commits, 'getPHID'));
@ -957,9 +977,9 @@ final class DiffusionBrowseController extends DiffusionController {
}
$phids = array();
foreach ($commits as $commit) {
if ($commit->getAuthorPHID()) {
$phids[] = $commit->getAuthorPHID();
foreach ($commits as $blame_commit) {
if ($blame_commit->getAuthorPHID()) {
$phids[] = $blame_commit->getAuthorPHID();
}
}
foreach ($revisions as $revision) {
@ -1002,7 +1022,6 @@ final class DiffusionBrowseController extends DiffusionController {
$engine);
foreach ($display as $line) {
$line_href = $drequest->generateURI(
array(
'action' => 'browse',
@ -1023,11 +1042,11 @@ final class DiffusionBrowseController extends DiffusionController {
if (idx($line, 'commit')) {
$commit = $line['commit'];
if (idx($commits, $commit)) {
if ($commit) {
$tooltip = $this->renderCommitTooltip(
$commits[$commit],
$commit,
$handles,
$line['author']);
$commit->renderAuthorLink($handles));
} else {
$tooltip = null;
}
@ -1041,7 +1060,7 @@ final class DiffusionBrowseController extends DiffusionController {
'href' => $drequest->generateURI(
array(
'action' => 'commit',
'commit' => $line['commit'],
'commit' => $commit->getCommitIdentifier(),
)),
'sigil' => 'has-tooltip',
'meta' => array(
@ -1050,14 +1069,11 @@ final class DiffusionBrowseController extends DiffusionController {
'size' => 600,
),
),
id(new PhutilUTF8StringTruncator())
->setMaximumGlyphs(9)
->setTerminator('')
->truncateString($line['commit']));
$commit->getShortName());
$revision_id = null;
if (idx($commits, $commit)) {
$revision_id = idx($revision_ids, $commits[$commit]->getPHID());
if ($commit) {
$revision_id = idx($revision_ids, $commit->getPHID());
}
if ($revision_id) {
@ -1207,7 +1223,7 @@ final class DiffusionBrowseController extends DiffusionController {
private function renderInlines(
array $inlines,
$needs_blame,
$show_blame,
$has_coverage,
$engine) {
@ -1222,7 +1238,7 @@ final class DiffusionBrowseController extends DiffusionController {
->setInlineComment($inline)
->render();
$row = array_fill(0, ($needs_blame ? 3 : 1), phutil_tag('th'));
$row = array_fill(0, ($show_blame ? 3 : 1), phutil_tag('th'));
$row[] = phutil_tag('td', array(), $inline_view);
@ -1722,4 +1738,44 @@ final class DiffusionBrowseController extends DiffusionController {
return $view;
}
private function loadBlame($path, $commit) {
$blame = $this->callConduitWithDiffusionRequest(
'diffusion.blame',
array(
'commit' => $commit,
'paths' => array($path),
));
$identifiers = idx($blame, $path, array());
if ($identifiers) {
$viewer = $this->getViewer();
$drequest = $this->getDiffusionRequest();
$repository = $drequest->getRepository();
$commits = id(new DiffusionCommitQuery())
->setViewer($viewer)
->withRepository($repository)
->withIdentifiers($identifiers)
// TODO: We only fetch this to improve author display behavior, but
// shouldn't really need to?
->needCommitData(true)
->execute();
$commits = mpull($commits, null, 'getCommitIdentifier');
} else {
$commits = array();
}
foreach ($identifiers as $key => $identifier) {
$commit = idx($commits, $identifier);
if ($commit) {
$identifiers[$key] = $commit;
} else {
$identifiers[$key] = null;
}
}
return $identifiers;
}
}

View file

@ -1,32 +0,0 @@
<?php
final class DiffusionGitFileContentQueryTestCase extends PhabricatorTestCase {
public function testAuthorName() {
// A normal case - no parenthesis in user name
$result = DiffusionGitFileContentQuery::match(
'8220d5d54f6d5d5552a636576cbe9c35f15b65b2 '.
'(Andrew Gallagher 2010-12-03 324) $somevar = $this->call()');
$this->assertEqual($result[0], '8220d5d54f6d5d5552a636576cbe9c35f15b65b2');
$this->assertEqual($result[1], 'Andrew Gallagher');
$this->assertEqual($result[2], ' $somevar = $this->call()');
// User name like 'Jimmy (He) Zhang'
$result = DiffusionGitFileContentQuery::match(
'8220d5d54f6d5d5552a636576cbe9c35f15b65b2 '.
'( Jimmy (He) Zhang 2013-10-11 5) '.
'code(); "(string literal 9999-99-99 2)"; more_code();');
$this->assertEqual($result[1], 'Jimmy (He) Zhang');
$this->assertEqual($result[2],
' code(); "(string literal 9999-99-99 2)"; more_code();');
// User name like 'Scott Shapiro (Ads Product Marketing)'
$result = DiffusionGitFileContentQuery::match(
'8220d5d54f6d5d5552a636576cbe9c35f15b65b2 '.
'( Scott Shapiro (Ads Product Marketing) 2013-10-11 5) '.
'code(); "(string literal 9999-99-99 2)"; more_code();');
$this->assertEqual($result[1], 'Scott Shapiro (Ads Product Marketing)');
$this->assertEqual($result[2],
' code(); "(string literal 9999-99-99 2)"; more_code();');
}
}

View file

@ -8,7 +8,6 @@
*/
abstract class DiffusionFileContentQuery extends DiffusionQuery {
private $needsBlame;
private $fileContent;
private $viewer;
private $timeout;
@ -32,6 +31,15 @@ abstract class DiffusionFileContentQuery extends DiffusionQuery {
return $this->byteLimit;
}
public function setViewer(PhabricatorUser $user) {
$this->viewer = $user;
return $this;
}
public function getViewer() {
return $this->viewer;
}
final public static function newFromDiffusionRequest(
DiffusionRequest $request) {
return parent::newQueryObject(__CLASS__, $request);
@ -90,110 +98,4 @@ abstract class DiffusionFileContentQuery extends DiffusionQuery {
return $this->fileContent->getCorpus();
}
/**
* Pretty hairy function. If getNeedsBlame is false, this returns
*
* ($text_list, array(), array())
*
* Where $text_list is the raw file content with trailing new lines stripped.
*
* If getNeedsBlame is true, this returns
*
* ($text_list, $line_rev_dict, $blame_dict)
*
* Where $text_list is just the lines of code -- the raw file content will
* contain lots of blame data, $line_rev_dict is a dictionary of line number
* => revision id, and $blame_dict is another complicated data structure.
* In detail, $blame_dict contains [revision id][author] keys, as well
* as [commit id][authorPhid] and [commit id][epoch] keys.
*
* @return ($text_list, $line_rev_dict, $blame_dict)
*/
final public function getBlameData() {
$raw_data = preg_replace('/\n$/', '', $this->getRawData());
$text_list = array();
$line_rev_dict = array();
$blame_dict = array();
if (!$this->getNeedsBlame()) {
$text_list = explode("\n", $raw_data);
} else if ($raw_data != '') {
$lines = array();
foreach (explode("\n", $raw_data) as $k => $line) {
$lines[$k] = $this->tokenizeLine($line);
list($rev_id, $author, $text) = $lines[$k];
$text_list[$k] = $text;
$line_rev_dict[$k] = $rev_id;
}
$line_rev_dict = $this->processRevList($line_rev_dict);
foreach ($lines as $k => $line) {
list($rev_id, $author, $text) = $line;
$rev_id = $line_rev_dict[$k];
if (!isset($blame_dict[$rev_id])) {
$blame_dict[$rev_id]['author'] = $author;
}
}
$repository = $this->getRequest()->getRepository();
$commits = id(new DiffusionCommitQuery())
->setViewer($this->getViewer())
->withDefaultRepository($repository)
->withIdentifiers(array_unique($line_rev_dict))
->execute();
foreach ($commits as $commit) {
$blame_dict[$commit->getCommitIdentifier()]['epoch'] =
$commit->getEpoch();
}
if ($commits) {
$commits_data = id(new PhabricatorRepositoryCommitData())->loadAllWhere(
'commitID IN (%Ls)',
mpull($commits, 'getID'));
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, $line_rev_dict, $blame_dict);
}
abstract protected function tokenizeLine($line);
public function setNeedsBlame($needs_blame) {
$this->needsBlame = $needs_blame;
return $this;
}
public function getNeedsBlame() {
return $this->needsBlame;
}
public function setViewer(PhabricatorUser $user) {
$this->viewer = $user;
return $this;
}
public function getViewer() {
return $this->viewer;
}
protected function processRevList(array $rev_list) {
return $rev_list;
}
}

View file

@ -9,17 +9,10 @@ final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery {
$path = $drequest->getPath();
$commit = $drequest->getCommit();
if ($this->getNeedsBlame()) {
return $repository->getLocalCommandFuture(
'--no-pager blame -c -l --date=short %s -- %s',
$commit,
$path);
} else {
return $repository->getLocalCommandFuture(
'cat-file blob %s:%s',
$commit,
$path);
}
return $repository->getLocalCommandFuture(
'cat-file blob %s:%s',
$commit,
$path);
}
protected function executeQueryFromFuture(Future $future) {
@ -31,28 +24,4 @@ final class DiffusionGitFileContentQuery extends DiffusionFileContentQuery {
return $file_content;
}
protected function tokenizeLine($line) {
return self::match($line);
}
public static function match($line) {
$m = array();
// sample lines:
//
// d1b4fcdd2a7c8c0f8cbdd01ca839d992135424dc
// ( hzhao 2009-05-01 202)function print();
//
// 8220d5d54f6d5d5552a636576cbe9c35f15b65b2
// (Andrew Gallagher 2010-12-03 324)
// // Add the lines for trailing context
preg_match(
'/^\s*?(\S+?)\s*\(\s*(.*?)\s+\d{4}-\d{2}-\d{2}\s+\d+\)(.*)?$/',
$line,
$m);
$rev_id = $m[1];
$author = $m[2];
$text = idx($m, 3);
return array($rev_id, $author, $text);
}
}

View file

@ -10,19 +10,10 @@ final class DiffusionMercurialFileContentQuery
$path = $drequest->getPath();
$commit = $drequest->getCommit();
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.
return $repository->getLocalCommandFuture(
'annotate --user --number --rev %s -- %s',
$commit,
$path);
} else {
return $repository->getLocalCommandFuture(
'cat --rev %s -- %s',
$commit,
$path);
}
return $repository->getLocalCommandFuture(
'cat --rev %s -- %s',
$commit,
$path);
}
protected function executeQueryFromFuture(Future $future) {
@ -34,45 +25,4 @@ final class DiffusionMercurialFileContentQuery
return $file_content;
}
protected function tokenizeLine($line) {
$matches = null;
preg_match(
'/^(.*?)\s+([0-9]+): (.*)$/',
$line,
$matches);
return array($matches[2], $matches[1], $matches[3]);
}
/**
* Convert local revision IDs into full commit identifier hashes.
*/
protected function processRevList(array $rev_list) {
$drequest = $this->getRequest();
$repository = $drequest->getRepository();
$revs = array_unique($rev_list);
foreach ($revs as $key => $rev) {
$revs[$key] = '--rev '.(int)$rev;
}
list($stdout) = $repository->execxLocalCommand(
'log --template=%s %C',
'{rev} {node}\\n',
implode(' ', $revs));
$map = array();
foreach (explode("\n", trim($stdout)) as $line) {
list($rev, $node) = explode(' ', $line);
$map[$rev] = $node;
}
foreach ($rev_list as $k => $rev) {
$rev_list[$k] = $map[$rev];
}
return $rev_list;
}
}

View file

@ -10,8 +10,7 @@ final class DiffusionSvnFileContentQuery extends DiffusionFileContentQuery {
$commit = $drequest->getCommit();
return $repository->getRemoteCommandFuture(
'%C %s',
$this->getNeedsBlame() ? 'blame --force' : 'cat',
'cat %s',
$repository->getSubversionPathURI($path, $commit));
}
@ -41,16 +40,4 @@ final class DiffusionSvnFileContentQuery extends DiffusionFileContentQuery {
return $file_content;
}
protected function tokenizeLine($line) {
// sample line:
// 347498 yliu function print();
$m = array();
preg_match('/^\s*(\d+)\s+(\S+)(?: (.*))?$/', $line, $m);
$rev_id = $m[1];
$author = $m[2];
$text = idx($m, 3);
return array($rev_id, $author, $text);
}
}

View file

@ -266,6 +266,32 @@ final class PhabricatorRepositoryCommit
return $repository->formatCommitName($identifier);
}
public function getShortName() {
$identifier = $this->getCommitIdentifier();
return substr($identifier, 0, 9);
}
public function renderAuthorLink($handles) {
$author_phid = $this->getAuthorPHID();
if ($author_phid && isset($handles[$author_phid])) {
return $handles[$author_phid]->renderLink();
}
return $this->renderAuthorShortName($handles);
}
public function renderAuthorShortName($handles) {
$author_phid = $this->getAuthorPHID();
if ($author_phid && isset($handles[$author_phid])) {
return $handles[$author_phid]->getName();
}
$data = $this->getCommitData();
$name = $data->getAuthorName();
$parsed = new PhutilEmailAddress($name);
return nonempty($parsed->getDisplayName(), $parsed->getAddress());
}
/* -( PhabricatorPolicyInterface )----------------------------------------- */