Return Diffusion diffs through Files, not directly over Conduit

Summary:
Fixes T10423. Ref T11524. This changes `diffusion.rawdiffquery` to return a file PHID instead of a blob of data.

This is better in general, but particularly better for huge diffs (as in T10423) and diffs with non-utf8 data (as in T10423).

Test Plan:
  - Used `bin/differential extract` to extract a latin1 diff, got a clean diff.
  - Used `bin/repository reparse --herald` to rerun herald on a latin1 diff, got a clean result.
  - Pushed latin1 diffs to test commit hooks.
  - Triggered the the too large / too slow logic.
  - Viewed latin1 diffs in Diffusion.
  - Used "blame past this change" in Diffusion to hit the `before` logic.

Reviewers: chad

Reviewed By: chad

Subscribers: eadler

Maniphest Tasks: T10423, T11524

Differential Revision: https://secure.phabricator.com/D16460

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',

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);

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();

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);
     }

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);
   }
 
 }

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;
 

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();
   }

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

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);
 

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;
   }
 
-
 }

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();

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();

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();

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) {
+      // 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;
   }
 
 }

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;
   }
 
 }

src/applications/diffusion/query/rawdiff/DiffusionRawDiffQuery.php

@@ -1,40 +1,17 @@
 <?php
 
-abstract class DiffusionRawDiffQuery extends DiffusionQuery {
+abstract class DiffusionRawDiffQuery
+  extends DiffusionFileFutureQuery {
 
-  private $timeout;
   private $linesOfContext = 65535;
   private $anchorCommit;
   private $againstCommit;
-  private $byteLimit;
 
   final public static function newFromDiffusionRequest(
     DiffusionRequest $request) {
     return parent::newQueryObject(__CLASS__, $request);
   }
 
-  final public function loadRawDiff() {
-    return $this->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());
-    }
-  }
-
 }

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;
   }
 
 }

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,
+      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();
   }
 
 }