1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-23 14:00:56 +01:00

Implement a dedicated "diffusion.blame" API method

Summary:
Fixes T2451. Several motivations here, from strongest to weakest:

  - Currently, getting blame and file content are closely entwined. This makes fixing T9319 more difficult, and I want to fix it. I want to separate blame from content so there's more flexibility in how we approach this issue.
  - This makes pursuing T2450 easier, if it turns out to be a meaningful win.
  - If we can get a win on blame performance, we can do `arc blame` eventually if we want.

Test Plan:
  - Blamed in SVN, Git and Mercurial.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2451

Differential Revision: https://secure.phabricator.com/D14957
This commit is contained in:
epriestley 2016-01-06 04:56:27 -08:00
parent d326c6096e
commit f561dc172d
7 changed files with 236 additions and 0 deletions

View file

@ -528,6 +528,8 @@ phutil_register_library_map(array(
'DiffusionAuditorsAddAuditorsHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsAddAuditorsHeraldAction.php',
'DiffusionAuditorsAddSelfHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsAddSelfHeraldAction.php',
'DiffusionAuditorsHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsHeraldAction.php',
'DiffusionBlameConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionBlameConduitAPIMethod.php',
'DiffusionBlameQuery' => 'applications/diffusion/query/blame/DiffusionBlameQuery.php',
'DiffusionBlockHeraldAction' => 'applications/diffusion/herald/DiffusionBlockHeraldAction.php',
'DiffusionBranchQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionBranchQueryConduitAPIMethod.php',
'DiffusionBranchTableController' => 'applications/diffusion/controller/DiffusionBranchTableController.php',
@ -601,6 +603,7 @@ phutil_register_library_map(array(
'DiffusionFindSymbolsConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionFindSymbolsConduitAPIMethod.php',
'DiffusionGetLintMessagesConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionGetLintMessagesConduitAPIMethod.php',
'DiffusionGetRecentCommitsByPathConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionGetRecentCommitsByPathConduitAPIMethod.php',
'DiffusionGitBlameQuery' => 'applications/diffusion/query/blame/DiffusionGitBlameQuery.php',
'DiffusionGitBranch' => 'applications/diffusion/data/DiffusionGitBranch.php',
'DiffusionGitBranchTestCase' => 'applications/diffusion/data/__tests__/DiffusionGitBranchTestCase.php',
'DiffusionGitFileContentQuery' => 'applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php',
@ -632,6 +635,7 @@ phutil_register_library_map(array(
'DiffusionLowLevelParentsQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php',
'DiffusionLowLevelQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelQuery.php',
'DiffusionLowLevelResolveRefsQuery' => 'applications/diffusion/query/lowlevel/DiffusionLowLevelResolveRefsQuery.php',
'DiffusionMercurialBlameQuery' => 'applications/diffusion/query/blame/DiffusionMercurialBlameQuery.php',
'DiffusionMercurialFileContentQuery' => 'applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php',
'DiffusionMercurialRawDiffQuery' => 'applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php',
'DiffusionMercurialRequest' => 'applications/diffusion/request/DiffusionMercurialRequest.php',
@ -743,6 +747,7 @@ phutil_register_library_map(array(
'DiffusionSubversionServeSSHWorkflow' => 'applications/diffusion/ssh/DiffusionSubversionServeSSHWorkflow.php',
'DiffusionSubversionWireProtocol' => 'applications/diffusion/protocol/DiffusionSubversionWireProtocol.php',
'DiffusionSubversionWireProtocolTestCase' => 'applications/diffusion/protocol/__tests__/DiffusionSubversionWireProtocolTestCase.php',
'DiffusionSvnBlameQuery' => 'applications/diffusion/query/blame/DiffusionSvnBlameQuery.php',
'DiffusionSvnFileContentQuery' => 'applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php',
'DiffusionSvnRawDiffQuery' => 'applications/diffusion/query/rawdiff/DiffusionSvnRawDiffQuery.php',
'DiffusionSvnRequest' => 'applications/diffusion/request/DiffusionSvnRequest.php',
@ -4493,6 +4498,8 @@ phutil_register_library_map(array(
'DiffusionAuditorsAddAuditorsHeraldAction' => 'DiffusionAuditorsHeraldAction',
'DiffusionAuditorsAddSelfHeraldAction' => 'DiffusionAuditorsHeraldAction',
'DiffusionAuditorsHeraldAction' => 'HeraldAction',
'DiffusionBlameConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod',
'DiffusionBlameQuery' => 'DiffusionQuery',
'DiffusionBlockHeraldAction' => 'HeraldAction',
'DiffusionBranchQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod',
'DiffusionBranchTableController' => 'DiffusionController',
@ -4566,6 +4573,7 @@ phutil_register_library_map(array(
'DiffusionFindSymbolsConduitAPIMethod' => 'DiffusionConduitAPIMethod',
'DiffusionGetLintMessagesConduitAPIMethod' => 'DiffusionConduitAPIMethod',
'DiffusionGetRecentCommitsByPathConduitAPIMethod' => 'DiffusionConduitAPIMethod',
'DiffusionGitBlameQuery' => 'DiffusionBlameQuery',
'DiffusionGitBranch' => 'Phobject',
'DiffusionGitBranchTestCase' => 'PhabricatorTestCase',
'DiffusionGitFileContentQuery' => 'DiffusionFileContentQuery',
@ -4597,6 +4605,7 @@ phutil_register_library_map(array(
'DiffusionLowLevelParentsQuery' => 'DiffusionLowLevelQuery',
'DiffusionLowLevelQuery' => 'Phobject',
'DiffusionLowLevelResolveRefsQuery' => 'DiffusionLowLevelQuery',
'DiffusionMercurialBlameQuery' => 'DiffusionBlameQuery',
'DiffusionMercurialFileContentQuery' => 'DiffusionFileContentQuery',
'DiffusionMercurialRawDiffQuery' => 'DiffusionRawDiffQuery',
'DiffusionMercurialRequest' => 'DiffusionRequest',
@ -4708,6 +4717,7 @@ phutil_register_library_map(array(
'DiffusionSubversionServeSSHWorkflow' => 'DiffusionSubversionSSHWorkflow',
'DiffusionSubversionWireProtocol' => 'Phobject',
'DiffusionSubversionWireProtocolTestCase' => 'PhabricatorTestCase',
'DiffusionSvnBlameQuery' => 'DiffusionBlameQuery',
'DiffusionSvnFileContentQuery' => 'DiffusionFileContentQuery',
'DiffusionSvnRawDiffQuery' => 'DiffusionRawDiffQuery',
'DiffusionSvnRequest' => 'DiffusionRequest',

View file

@ -0,0 +1,44 @@
<?php
final class DiffusionBlameConduitAPIMethod
extends DiffusionQueryConduitAPIMethod {
public function getAPIMethodName() {
return 'diffusion.blame';
}
public function getMethodDescription() {
return pht('Get blame information for a list of paths.');
}
protected function defineReturnType() {
return 'map<string, wild>';
}
protected function defineCustomParamTypes() {
return array(
'paths' => 'required list<string>',
'commit' => 'required string',
'timeout' => 'optional int',
);
}
protected function getResult(ConduitAPIRequest $request) {
$drequest = $this->getDiffusionRequest();
$paths = $request->getValue('paths');
$blame_query = DiffusionBlameQuery::newFromDiffusionRequest($drequest)
->setPaths($paths);
$timeout = $request->getValue('timeout');
if ($timeout) {
$blame_query->setTimeout($timeout);
}
$blame = $blame_query->execute();
return $blame;
}
}

View file

@ -110,6 +110,13 @@ abstract class DiffusionQueryConduitAPIMethod
'commit' => $request->getValue('commit'),
));
if (!$drequest) {
throw new Exception(
pht(
'Repository "%s" is not a valid repository.',
$identifier));
}
// Figure out whether we're going to handle this request on this device,
// or proxy it to another node in the cluster.

View file

@ -0,0 +1,69 @@
<?php
abstract class DiffusionBlameQuery extends DiffusionQuery {
private $timeout;
private $paths;
public function setTimeout($timeout) {
$this->timeout = $timeout;
return $this;
}
public function getTimeout() {
return $this->timeout;
}
public function setPaths(array $paths) {
$this->paths = $paths;
return $this;
}
public function getPaths() {
return $this->paths;
}
abstract protected function newBlameFuture(DiffusionRequest $request, $path);
abstract protected function resolveBlameFuture(ExecFuture $future);
final public static function newFromDiffusionRequest(
DiffusionRequest $request) {
return parent::newQueryObject(__CLASS__, $request);
}
final protected function executeQuery() {
$paths = $this->getPaths();
$request = $this->getRequest();
$timeout = $this->getTimeout();
$futures = array();
foreach ($paths as $path) {
$future = $this->newBlameFuture($request, $path);
if ($timeout) {
$future->setTimeout($timeout);
}
$futures[$path] = $future;
}
$blame = array();
if ($futures) {
$futures = id(new FutureIterator($futures))
->limit(4);
foreach ($futures as $path => $future) {
$path_blame = $this->resolveBlameFuture($future);
if ($path_blame !== null) {
$blame[$path] = $path_blame;
}
}
}
return $blame;
}
}

View file

@ -0,0 +1,34 @@
<?php
final class DiffusionGitBlameQuery extends DiffusionBlameQuery {
protected function newBlameFuture(DiffusionRequest $request, $path) {
$repository = $request->getRepository();
$commit = $request->getCommit();
return $repository->getLocalCommandFuture(
'--no-pager blame -s -l %s -- %s',
$commit,
$path);
}
protected function resolveBlameFuture(ExecFuture $future) {
list($err, $stdout) = $future->resolve();
if ($err) {
return null;
}
$result = array();
$lines = phutil_split_lines($stdout);
foreach ($lines as $line) {
list($commit) = explode(' ', $line, 2);
$result[] = $commit;
}
return $result;
}
}

View file

@ -0,0 +1,36 @@
<?php
final class DiffusionMercurialBlameQuery extends DiffusionBlameQuery {
protected function newBlameFuture(DiffusionRequest $request, $path) {
$repository = $request->getRepository();
$commit = $request->getCommit();
// NOTE: We're using "--debug" to make "--changeset" give us the full
// commit hashes.
return $repository->getLocalCommandFuture(
'annotate --debug --changeset --rev %s -- %s',
$commit,
$path);
}
protected function resolveBlameFuture(ExecFuture $future) {
list($err, $stdout) = $future->resolve();
if ($err) {
return null;
}
$result = array();
$lines = phutil_split_lines($stdout);
foreach ($lines as $line) {
list($commit) = explode(':', $line, 2);
$result[] = $commit;
}
return $result;
}
}

View file

@ -0,0 +1,36 @@
<?php
final class DiffusionSvnBlameQuery extends DiffusionBlameQuery {
protected function newBlameFuture(DiffusionRequest $request, $path) {
$repository = $request->getRepository();
$commit = $request->getCommit();
return $repository->getRemoteCommandFuture(
'blame --force %s',
$repository->getSubversionPathURI($path, $commit));
}
protected function resolveBlameFuture(ExecFuture $future) {
list($err, $stdout) = $future->resolve();
if ($err) {
return null;
}
$result = array();
$matches = null;
$lines = phutil_split_lines($stdout);
foreach ($lines as $line) {
if (preg_match('/^\s*(\d+)/', $line, $matches)) {
$result[] = (int)$matches[1];
} else {
$result[] = null;
}
}
return $result;
}
}