Use Conduit in PhabricatorRepositoryGitCommitChangeParserWorker

Summary:
Ref T2783.  This allows this worker to run on a machine different to the one that stores the repository, by routing the execution of Git over Conduit calls.

This API method is super gross, but fixing it isn't straightforward and it runs into other complicated considerations. We can fix it later; for now, just define it as "internal" to limit how much mess this creates.

"Internal" methods do not appear on the console.

Test Plan: Ran `bin/repository reparse --change <commit> --trace` on several commits, saw daemons make a Conduit call instead of running a `git` command.

Reviewers: hach-que, chad

Reviewed By: chad

Subscribers: joshuaspence, Korvin, epriestley

Maniphest Tasks: T2783

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

src/__phutil_library_map__.php

@@ -650,6 +650,7 @@ phutil_register_library_map(array(
     'DiffusionHovercardEngineExtension' => 'applications/diffusion/engineextension/DiffusionHovercardEngineExtension.php',
     'DiffusionInlineCommentController' => 'applications/diffusion/controller/DiffusionInlineCommentController.php',
     'DiffusionInlineCommentPreviewController' => 'applications/diffusion/controller/DiffusionInlineCommentPreviewController.php',
+    'DiffusionInternalGitRawDiffQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php',
     'DiffusionLastModifiedController' => 'applications/diffusion/controller/DiffusionLastModifiedController.php',
     'DiffusionLastModifiedQueryConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionLastModifiedQueryConduitAPIMethod.php',
     'DiffusionLintController' => 'applications/diffusion/controller/DiffusionLintController.php',
@@ -4836,6 +4837,7 @@ phutil_register_library_map(array(
     'DiffusionHovercardEngineExtension' => 'PhabricatorHovercardEngineExtension',
     'DiffusionInlineCommentController' => 'PhabricatorInlineCommentController',
     'DiffusionInlineCommentPreviewController' => 'PhabricatorInlineCommentPreviewController',
+    'DiffusionInternalGitRawDiffQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod',
     'DiffusionLastModifiedController' => 'DiffusionController',
     'DiffusionLastModifiedQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod',
     'DiffusionLintController' => 'DiffusionController',

src/applications/conduit/method/ConduitAPIMethod.php

@@ -52,6 +52,9 @@ abstract class ConduitAPIMethod
 
   abstract protected function execute(ConduitAPIRequest $request);
 
+  public function isInternalAPI() {
+    return false;
+  }
 
   public function getParamTypes() {
     $types = $this->defineParamTypes();

src/applications/conduit/query/PhabricatorConduitMethodQuery.php

@@ -9,6 +9,7 @@ final class PhabricatorConduitMethodQuery
   private $applicationNames;
   private $nameContains;
   private $methods;
+  private $isInternal;
 
   public function withMethods(array $methods) {
     $this->methods = $methods;
@@ -40,6 +41,11 @@ final class PhabricatorConduitMethodQuery
     return $this;
   }
 
+  public function withIsInternal($is_internal) {
+    $this->isInternal = $is_internal;
+    return $this;
+  }
+
   protected function loadPage() {
     $methods = $this->getAllMethods();
     $methods = $this->filterMethods($methods);
@@ -112,6 +118,14 @@ final class PhabricatorConduitMethodQuery
       }
     }
 
+    if ($this->isInternal !== null) {
+      foreach ($methods as $key => $method) {
+        if ($method->isInternalAPI() !== $this->isInternal) {
+          unset($methods[$key]);
+        }
+      }
+    }
+
     return $methods;
   }
 

src/applications/conduit/query/PhabricatorConduitSearchEngine.php

@@ -37,6 +37,7 @@ final class PhabricatorConduitSearchEngine
     $query->withIsStable($saved->getParameter('isStable'));
     $query->withIsUnstable($saved->getParameter('isUnstable'));
     $query->withIsDeprecated($saved->getParameter('isDeprecated'));
+    $query->withIsInternal(false);
 
     $names = $saved->getParameter('applicationNames', array());
     if ($names) {

src/applications/diffusion/conduit/DiffusionInternalGitRawDiffQueryConduitAPIMethod.php

@@ -0,0 +1,74 @@
+<?php
+
+final class DiffusionInternalGitRawDiffQueryConduitAPIMethod
+  extends DiffusionQueryConduitAPIMethod {
+
+  public function isInternalAPI() {
+    return true;
+  }
+
+  public function getAPIMethodName() {
+    return 'diffusion.internal.gitrawdiffquery';
+  }
+
+  public function getMethodDescription() {
+    return pht('Internal method for getting raw diff information.');
+  }
+
+  protected function defineReturnType() {
+    return 'string';
+  }
+
+  protected function defineCustomParamTypes() {
+    return array(
+      'commit' => 'required string',
+    );
+  }
+
+  protected function getResult(ConduitAPIRequest $request) {
+    $drequest = $this->getDiffusionRequest();
+    $repository = $drequest->getRepository();
+
+    if (!$repository->isGit()) {
+      throw new Exception(
+        pht(
+          'This API method can only be called on Git repositories.'));
+    }
+
+    // Check if the commit has parents. We're testing to see whether it is the
+    // first commit in history (in which case we must use "git log") or some
+    // other commit (in which case we can use "git diff"). We'd rather use
+    // "git diff" because it has the right behavior for merge commits, but
+    // it requires the commit to have a parent that we can diff against. The
+    // first commit doesn't, so "commit^" is not a valid ref.
+    list($parents) = $repository->execxLocalCommand(
+      'log -n1 --format=%s %s',
+      '%P',
+      $request->getValue('commit'));
+
+    $use_log = !strlen(trim($parents));
+    if ($use_log) {
+      // This is the first commit so we need to use "log". We know it's not a
+      // merge commit because it couldn't be merging anything, so this is safe.
+
+      // NOTE: "--pretty=format: " is to disable diff output, we only want the
+      // part we get from "--raw".
+      list($raw) = $repository->execxLocalCommand(
+        'log -n1 -M -C -B --find-copies-harder --raw -t '.
+          '--pretty=format: --abbrev=40 %s',
+        $request->getValue('commit'));
+    } else {
+      // Otherwise, we can use "diff", which will give us output for merges.
+      // We diff against the first parent, as this is generally the expectation
+      // and results in sensible behavior.
+      list($raw) = $repository->execxLocalCommand(
+        'diff -n1 -M -C -B --find-copies-harder --raw -t '.
+          '--abbrev=40 %s^1 %s',
+        $request->getValue('commit'),
+        $request->getValue('commit'));
+    }
+
+    return $raw;
+  }
+
+}

src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryGitCommitChangeParserWorker.php

@@ -7,38 +7,18 @@ final class PhabricatorRepositoryGitCommitChangeParserWorker
     PhabricatorRepository $repository,
     PhabricatorRepositoryCommit $commit) {
 
-    // Check if the commit has parents. We're testing to see whether it is the
+    $viewer = PhabricatorUser::getOmnipotentUser();
-    // first commit in history (in which case we must use "git log") or some
+    $raw = DiffusionQuery::callConduitWithDiffusionRequest(
-    // other commit (in which case we can use "git diff"). We'd rather use
+      $viewer,
-    // "git diff" because it has the right behavior for merge commits, but
+      DiffusionRequest::newFromDictionary(
-    // it requires the commit to have a parent that we can diff against. The
+        array(
-    // first commit doesn't, so "commit^" is not a valid ref.
+          'repository' => $repository,
-    list($parents) = $repository->execxLocalCommand(
+          'user' => $viewer,
-      'log -n1 --format=%s %s',
+        )),
-      '%P',
+      'diffusion.internal.gitrawdiffquery',
-      $commit->getCommitIdentifier());
+      array(
-
+        'commit' => $commit->getCommitIdentifier(),
-    $use_log = !strlen(trim($parents));
+      ));
-    if ($use_log) {
-      // This is the first commit so we need to use "log". We know it's not a
-      // merge commit because it couldn't be merging anything, so this is safe.
-
-      // NOTE: "--pretty=format: " is to disable diff output, we only want the
-      // part we get from "--raw".
-      list($raw) = $repository->execxLocalCommand(
-        'log -n1 -M -C -B --find-copies-harder --raw -t '.
-          '--pretty=format: --abbrev=40 %s',
-        $commit->getCommitIdentifier());
-    } else {
-      // Otherwise, we can use "diff", which will give us output for merges.
-      // We diff against the first parent, as this is generally the expectation
-      // and results in sensible behavior.
-      list($raw) = $repository->execxLocalCommand(
-        'diff -n1 -M -C -B --find-copies-harder --raw -t '.
-          '--abbrev=40 %s^1 %s',
-        $commit->getCommitIdentifier(),
-        $commit->getCommitIdentifier());
-    }
 
     $changes = array();
     $move_away = array();