diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b0b4ba8622..60852bface 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -5260,7 +5260,7 @@ phutil_register_library_map(array( 'DiffusionQueryCommitsConduitAPIMethod' => 'DiffusionConduitAPIMethod', 'DiffusionQueryConduitAPIMethod' => 'DiffusionConduitAPIMethod', 'DiffusionQueryPathsConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', - 'DiffusionRawDiffQuery' => 'DiffusionQuery', + 'DiffusionRawDiffQuery' => 'DiffusionFileFutureQuery', 'DiffusionRawDiffQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionReadmeView' => 'DiffusionView', 'DiffusionRefDatasource' => 'PhabricatorTypeaheadDatasource', diff --git a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php index 0fc6391987..05e19bbe71 100644 --- a/src/applications/differential/engine/DifferentialDiffExtractionEngine.php +++ b/src/applications/differential/engine/DifferentialDiffExtractionEngine.php @@ -36,7 +36,7 @@ final class DifferentialDiffExtractionEngine extends Phobject { 'repository' => $repository, )); - $raw_diff = DiffusionQuery::callConduitWithDiffusionRequest( + $diff_info = DiffusionQuery::callConduitWithDiffusionRequest( $viewer, $drequest, 'diffusion.rawdiffquery', @@ -44,6 +44,21 @@ final class DifferentialDiffExtractionEngine extends Phobject { 'commit' => $identifier, )); + $file_phid = $diff_info['filePHID']; + $diff_file = id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs(array($file_phid)) + ->executeOne(); + if (!$diff_file) { + throw new Exception( + pht( + 'Failed to load file ("%s") returned by "%s".', + $file_phid, + 'diffusion.rawdiffquery')); + } + + $raw_diff = $diff_file->loadFileData(); + // TODO: Support adds, deletes and moves under SVN. if (strlen($raw_diff)) { $changes = id(new ArcanistDiffParser())->parseDiff($raw_diff); diff --git a/src/applications/diffusion/DiffusionLintSaveRunner.php b/src/applications/diffusion/DiffusionLintSaveRunner.php index bcfeefd510..980234f95d 100644 --- a/src/applications/diffusion/DiffusionLintSaveRunner.php +++ b/src/applications/diffusion/DiffusionLintSaveRunner.php @@ -262,7 +262,7 @@ final class DiffusionLintSaveRunner extends Phobject { $query = DiffusionFileContentQuery::newFromDiffusionRequest($drequest); $queries[$path] = $query; - $futures[$path] = $query->getFileContentFuture(); + $futures[$path] = new ImmediateFuture($query->executeInline()); } $authors = array(); diff --git a/src/applications/diffusion/conduit/DiffusionDiffQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionDiffQueryConduitAPIMethod.php index 19295f2b20..d8ce38b5d5 100644 --- a/src/applications/diffusion/conduit/DiffusionDiffQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionDiffQueryConduitAPIMethod.php @@ -207,7 +207,7 @@ final class DiffusionDiffQueryConduitAPIMethod $raw_query = DiffusionRawDiffQuery::newFromDiffusionRequest($drequest) ->setAnchorCommit($effective_commit); - $raw_diff = $raw_query->loadRawDiff(); + $raw_diff = $raw_query->executeInline(); if (!$raw_diff) { return $this->getEmptyResult(2); } diff --git a/src/applications/diffusion/conduit/DiffusionRawDiffQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionRawDiffQueryConduitAPIMethod.php index 282dc7eb9b..4fab6baae1 100644 --- a/src/applications/diffusion/conduit/DiffusionRawDiffQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionRawDiffQueryConduitAPIMethod.php @@ -21,39 +21,27 @@ final class DiffusionRawDiffQueryConduitAPIMethod return array( 'commit' => 'required string', 'path' => 'optional string', - 'timeout' => 'optional int', - 'byteLimit' => 'optional int', 'linesOfContext' => 'optional int', 'againstCommit' => 'optional string', - ); + ) + DiffusionFileFutureQuery::getConduitParameters(); } protected function getResult(ConduitAPIRequest $request) { $drequest = $this->getDiffusionRequest(); - $raw_query = DiffusionRawDiffQuery::newFromDiffusionRequest($drequest); - - $timeout = $request->getValue('timeout'); - if ($timeout !== null) { - $raw_query->setTimeout($timeout); - } + $query = DiffusionRawDiffQuery::newFromDiffusionRequest($drequest); $lines_of_context = $request->getValue('linesOfContext'); if ($lines_of_context !== null) { - $raw_query->setLinesOfContext($lines_of_context); + $query->setLinesOfContext($lines_of_context); } $against_commit = $request->getValue('againstCommit'); if ($against_commit !== null) { - $raw_query->setAgainstCommit($against_commit); + $query->setAgainstCommit($against_commit); } - $byte_limit = $request->getValue('byteLimit'); - if ($byte_limit !== null) { - $raw_query->setByteLimit($byte_limit); - } - - return $raw_query->loadRawDiff(); + return $query->respondToConduitRequest($request); } } diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index 3ae076a5d0..9d51146534 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -1527,19 +1527,36 @@ final class DiffusionBrowseController extends DiffusionController { private function getBeforeLineNumber($target_commit) { $drequest = $this->getDiffusionRequest(); + $viewer = $this->getViewer(); $line = $drequest->getLine(); if (!$line) { return null; } - $raw_diff = $this->callConduitWithDiffusionRequest( + $diff_info = $this->callConduitWithDiffusionRequest( 'diffusion.rawdiffquery', array( 'commit' => $drequest->getCommit(), 'path' => $drequest->getPath(), 'againstCommit' => $target_commit, )); + + $file_phid = $diff_info['filePHID']; + $file = id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs(array($file_phid)) + ->executeOne(); + if (!$file) { + throw new Exception( + pht( + 'Failed to load file ("%s") returned by "%s".', + $file_phid, + 'diffusion.rawdiffquery.')); + } + + $raw_diff = $file->loadFileData(); + $old_line = 0; $new_line = 0; diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index c27006350f..3ec28d31a5 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -1027,24 +1027,26 @@ final class DiffusionCommitController extends DiffusionController { } private function buildRawDiffResponse(DiffusionRequest $drequest) { - $raw_diff = $this->callConduitWithDiffusionRequest( + $diff_info = $this->callConduitWithDiffusionRequest( 'diffusion.rawdiffquery', array( 'commit' => $drequest->getCommit(), 'path' => $drequest->getPath(), )); - $file = PhabricatorFile::buildFromFileDataOrHash( - $raw_diff, - array( - 'name' => $drequest->getCommit().'.diff', - 'ttl' => (60 * 60 * 24), - 'viewPolicy' => PhabricatorPolicies::POLICY_NOONE, - )); + $file_phid = $diff_info['filePHID']; - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $file->attachToObject($drequest->getRepository()->getPHID()); - unset($unguarded); + $file = id(new PhabricatorFileQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs(array($file_phid)) + ->executeOne(); + if (!$file) { + throw new Exception( + pht( + 'Failed to load file ("%s") returned by "%s".', + $file_phid, + 'diffusion.rawdiffquery')); + } return $file->getRedirectResponse(); } diff --git a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php index 50fc828364..2813927e40 100644 --- a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php +++ b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php @@ -1095,7 +1095,7 @@ final class DiffusionCommitHookEngine extends Phobject { ->setTimeout($time_limit) ->setByteLimit($byte_limit) ->setLinesOfContext(0) - ->loadRawDiff(); + ->executeInline(); break; case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: // TODO: This diff has 3 lines of context, which produces slightly diff --git a/src/applications/diffusion/herald/HeraldCommitAdapter.php b/src/applications/diffusion/herald/HeraldCommitAdapter.php index bd34fe23cc..759b8afa6f 100644 --- a/src/applications/diffusion/herald/HeraldCommitAdapter.php +++ b/src/applications/diffusion/herald/HeraldCommitAdapter.php @@ -204,25 +204,50 @@ final class HeraldCommitAdapter } private function loadCommitDiff() { - $byte_limit = self::getEnormousByteLimit(); + $viewer = PhabricatorUser::getOmnipotentUser(); - $raw = $this->callConduit( + $byte_limit = self::getEnormousByteLimit(); + $time_limit = self::getEnormousTimeLimit(); + + $diff_info = $this->callConduit( 'diffusion.rawdiffquery', array( 'commit' => $this->commit->getCommitIdentifier(), - 'timeout' => self::getEnormousTimeLimit(), + 'timeout' => $time_limit, 'byteLimit' => $byte_limit, 'linesOfContext' => 0, )); - if (strlen($raw) >= $byte_limit) { + if ($diff_info['tooHuge']) { throw new Exception( pht( - 'The raw text of this change is enormous (larger than %d bytes). '. + 'The raw text of this change is enormous (larger than %s byte(s)). '. 'Herald can not process it.', - $byte_limit)); + new PhutilNumber($byte_limit))); } + if ($diff_info['tooSlow']) { + throw new Exception( + pht( + 'The raw text of this change took too long to process (longer '. + 'than %s second(s)). Herald can not process it.', + new PhutilNumber($time_limit))); + } + + $file_phid = $diff_info['filePHID']; + $diff_file = id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs(array($file_phid)) + ->executeOne(); + if (!$diff_file) { + throw new Exception( + pht( + 'Failed to load diff ("%s") for this change.', + $file_phid)); + } + + $raw = $diff_file->loadFileData(); + $parser = new ArcanistDiffParser(); $changes = $parser->parseDiff($raw); diff --git a/src/applications/diffusion/query/DiffusionFileFutureQuery.php b/src/applications/diffusion/query/DiffusionFileFutureQuery.php index 595b9fe5ae..8ce59028e2 100644 --- a/src/applications/diffusion/query/DiffusionFileFutureQuery.php +++ b/src/applications/diffusion/query/DiffusionFileFutureQuery.php @@ -42,7 +42,7 @@ abstract class DiffusionFileFutureQuery return $this->didHitTimeLimit; } - abstract protected function getFileContentFuture(); + abstract protected function newQueryFuture(); final public function respondToConduitRequest(ConduitAPIRequest $request) { $drequest = $this->getRequest(); @@ -82,13 +82,13 @@ abstract class DiffusionFileFutureQuery } final public function executeInline() { - $future = $this->getFileContentFuture(); + $future = $this->newConfiguredQueryFuture(); list($stdout) = $future->resolvex(); - return $future; + return $stdout; } final protected function executeQuery() { - $future = $this->newConfiguredFileContentFuture(); + $future = $this->newQueryFuture(); $drequest = $this->getRequest(); @@ -134,8 +134,8 @@ abstract class DiffusionFileFutureQuery return $file; } - private function newConfiguredFileContentFuture() { - $future = $this->getFileContentFuture(); + private function newConfiguredQueryFuture() { + $future = $this->newQueryFuture(); if ($this->getTimeout()) { $future->setTimeout($this->getTimeout()); @@ -149,5 +149,4 @@ abstract class DiffusionFileFutureQuery return $future; } - } diff --git a/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionGitFileContentQuery.php index 279abe7cce..e0fb8bd9ee 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 getFileContentFuture() { + protected function newQueryFuture() { $drequest = $this->getRequest(); $repository = $drequest->getRepository(); diff --git a/src/applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionMercurialFileContentQuery.php index e88bf82e63..f47f0936d9 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 getFileContentFuture() { + protected function newQueryFuture() { $drequest = $this->getRequest(); $repository = $drequest->getRepository(); diff --git a/src/applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php b/src/applications/diffusion/query/filecontent/DiffusionSvnFileContentQuery.php index 35f002297c..8060dd5dfd 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 getFileContentFuture() { + protected function newQueryFuture() { $drequest = $this->getRequest(); $repository = $drequest->getRepository(); diff --git a/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php b/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php index c7fdeb0e38..18e5a9ec9a 100644 --- a/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php +++ b/src/applications/diffusion/query/rawdiff/DiffusionGitRawDiffQuery.php @@ -2,7 +2,7 @@ final class DiffusionGitRawDiffQuery extends DiffusionRawDiffQuery { - protected function executeQuery() { + protected function newQueryFuture() { $drequest = $this->getRequest(); $repository = $drequest->getRepository(); @@ -17,54 +17,34 @@ final class DiffusionGitRawDiffQuery extends DiffusionRawDiffQuery { '--dst-prefix=b/', '-U'.(int)$this->getLinesOfContext(), ); - $options = implode(' ', $options); $against = $this->getAgainstCommit(); if ($against === null) { - $against = $commit.'^'; + // Check if this is the root commit by seeing if it has parents, since + // `git diff X^ X` does not work if "X" is the initial commit. + list($parents) = $repository->execxLocalCommand( + 'log --format=%s %s --', + '%P', + $commit); + + if (strlen(trim($parents))) { + $against = $commit.'^'; + } else { + $against = ArcanistGitAPI::GIT_MAGIC_ROOT_COMMIT; + } } - // If there's no path, get the entire raw diff. - $path = nonempty($drequest->getPath(), '.'); + $path = $drequest->getPath(); + if (!strlen($path)) { + $path = '.'; + } - $future = $repository->getLocalCommandFuture( - 'diff %C %s %s -- %s', + return $repository->getLocalCommandFuture( + 'diff %Ls %s %s -- %s', $options, $against, $commit, $path); - - $this->configureFuture($future); - - try { - list($raw_diff) = $future->resolvex(); - } catch (CommandException $ex) { - // Check if this is the root commit by seeing if it has parents. - list($parents) = $repository->execxLocalCommand( - 'log --format=%s %s --', - '%P', // "parents" - $commit); - - if (strlen(trim($parents))) { - throw $ex; - } - - // No parents means we're looking at the root revision. Diff against - // the empty tree hash instead, since there is no parent so "^" does - // not work. See ArcanistGitAPI for more discussion. - $future = $repository->getLocalCommandFuture( - 'diff %C %s %s -- %s', - $options, - ArcanistGitAPI::GIT_MAGIC_ROOT_COMMIT, - $commit, - $drequest->getPath()); - - $this->configureFuture($future); - - list($raw_diff) = $future->resolvex(); - } - - return $raw_diff; } } diff --git a/src/applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php b/src/applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php index 37aeed045e..39eed01226 100644 --- a/src/applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php +++ b/src/applications/diffusion/query/rawdiff/DiffusionMercurialRawDiffQuery.php @@ -2,11 +2,7 @@ final class DiffusionMercurialRawDiffQuery extends DiffusionRawDiffQuery { - protected function executeQuery() { - return $this->executeRawDiffCommand(); - } - - protected function executeRawDiffCommand() { + protected function newQueryFuture() { $drequest = $this->getRequest(); $repository = $drequest->getRepository(); @@ -30,11 +26,7 @@ final class DiffusionMercurialRawDiffQuery extends DiffusionRawDiffQuery { $commit, $path); - $this->configureFuture($future); - - list($raw_diff) = $future->resolvex(); - - return $raw_diff; + return $future; } } diff --git a/src/applications/diffusion/query/rawdiff/DiffusionRawDiffQuery.php b/src/applications/diffusion/query/rawdiff/DiffusionRawDiffQuery.php index da16b9e609..37d8a68068 100644 --- a/src/applications/diffusion/query/rawdiff/DiffusionRawDiffQuery.php +++ b/src/applications/diffusion/query/rawdiff/DiffusionRawDiffQuery.php @@ -1,40 +1,17 @@ executeQuery(); - } - - final public function setTimeout($timeout) { - $this->timeout = $timeout; - return $this; - } - - final public function getTimeout() { - return $this->timeout; - } - - public function setByteLimit($byte_limit) { - $this->byteLimit = $byte_limit; - return $this; - } - - public function getByteLimit() { - return $this->byteLimit; - } - final public function setLinesOfContext($lines_of_context) { $this->linesOfContext = $lines_of_context; return $this; @@ -66,15 +43,4 @@ abstract class DiffusionRawDiffQuery extends DiffusionQuery { return $this->getRequest()->getStableCommit(); } - protected function configureFuture(ExecFuture $future) { - if ($this->getTimeout()) { - $future->setTimeout($this->getTimeout()); - } - - if ($this->getByteLimit()) { - $future->setStdoutSizeLimit($this->getByteLimit()); - $future->setStderrSizeLimit($this->getByteLimit()); - } - } - } diff --git a/src/applications/diffusion/query/rawdiff/DiffusionSvnRawDiffQuery.php b/src/applications/diffusion/query/rawdiff/DiffusionSvnRawDiffQuery.php index bc8384cb75..e492016fe3 100644 --- a/src/applications/diffusion/query/rawdiff/DiffusionSvnRawDiffQuery.php +++ b/src/applications/diffusion/query/rawdiff/DiffusionSvnRawDiffQuery.php @@ -2,7 +2,7 @@ final class DiffusionSvnRawDiffQuery extends DiffusionRawDiffQuery { - protected function executeQuery() { + protected function newQueryFuture() { $drequest = $this->getRequest(); $repository = $drequest->getRepository(); @@ -22,10 +22,7 @@ final class DiffusionSvnRawDiffQuery extends DiffusionRawDiffQuery { $commit, $repository->getSubversionPathURI($drequest->getPath())); - $this->configureFuture($future); - - list($raw_diff) = $future->resolvex(); - return $raw_diff; + return $future; } } diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php index 4bd9344409..9fb6667924 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php @@ -105,40 +105,64 @@ final class PhabricatorRepositoryCommitHeraldWorker private function loadRawPatchText( PhabricatorRepository $repository, PhabricatorRepositoryCommit $commit) { + $viewer = PhabricatorUser::getOmnipotentUser(); + + $identifier = $commit->getCommitIdentifier(); $drequest = DiffusionRequest::newFromDictionary( array( - 'user' => PhabricatorUser::getOmnipotentUser(), + 'user' => $viewer, 'repository' => $repository, - 'commit' => $commit->getCommitIdentifier(), )); - $raw_query = DiffusionRawDiffQuery::newFromDiffusionRequest($drequest); - $raw_query->setLinesOfContext(3); - $time_key = 'metamta.diffusion.time-limit'; $byte_key = 'metamta.diffusion.byte-limit'; $time_limit = PhabricatorEnv::getEnvConfig($time_key); $byte_limit = PhabricatorEnv::getEnvConfig($byte_key); - if ($time_limit) { - $raw_query->setTimeout($time_limit); + $diff_info = DiffusionQuery::callConduitWithDiffusionRequest( + $viewer, + $drequest, + 'diffusion.rawdiffquery', + array( + 'commit' => $identifier, + 'linesOfContext' => 3, + 'timeout' => $time_limit, + 'byteLimit' => $byte_limit, + )); + + if ($diff_info['tooSlow']) { + throw new Exception( + pht( + 'Patch generation took longer than configured limit ("%s") of '. + '%s second(s).', + $time_key, + new PhutilNumber($time_limit))); } - $raw_diff = $raw_query->loadRawDiff(); - - $size = strlen($raw_diff); - if ($byte_limit && $size > $byte_limit) { - $pretty_size = phutil_format_bytes($size); + if ($diff_info['tooHuge']) { $pretty_limit = phutil_format_bytes($byte_limit); - throw new Exception(pht( - 'Patch size of %s exceeds configured byte size limit (%s) of %s.', - $pretty_size, - $byte_key, - $pretty_limit)); + throw new Exception( + pht( + 'Patch size exceeds configured byte size limit ("%s") of %s.', + $byte_key, + $pretty_limit)); } - return $raw_diff; + $file_phid = $diff_info['filePHID']; + $file = id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs(array($file_phid)) + ->executeOne(); + if (!$file) { + throw new Exception( + pht( + 'Failed to load file ("%s") returned by "%s".', + $file_phid, + 'diffusion.rawdiffquery')); + } + + return $file->loadFileData(); } }