Move some VCS-specific logic into VCS APIs

Summary:
We have some git-specific logic on main pathways which should be in the API
class, move it around so "arc lint" with an engine works under Mercurial. This
resovles the error @makinde reported:

> PHP Catchable fatal error:  Argument 1 passed to
ArcanistBaseWorkflow::parseGitRelativeCommit() must be an instance of
ArcanistGitAPI, instance of ArcanistMercurialAPI given, called in
/home/makinde/.arc_install/arcanist/src/workflow/lint/ArcanistLintWorkflow.php
on line 131 and defined in
/home/makinde/.arc_install/arcanist/src/workflow/base/ArcanistBaseWorkflow.php
on line 830

Test Plan: Ran "arc diff" in git and hg working copies, plus "arc lint" with a
configured "lint_engine".

Reviewers: Makinde, aran, jungejason, nh, tuomaspelkonen

Reviewed By: Makinde

CC: aran, Makinde

Differential Revision: 935

src/repository/api/base/ArcanistRepositoryAPI.php

@@ -159,4 +159,15 @@ abstract class ArcanistRepositoryAPI {
   abstract public function getCurrentFileData($path);
   abstract public function getLocalCommitInformation();
 
+
+  abstract public function supportsRelativeLocalCommits();
+
+  public function parseRelativeLocalCommit(array $argv) {
+    throw new Exception("This VCS does not support relative local commits.");
+  }
+
+  public function getAllLocalChanges() {
+    throw new Exception("This VCS does not support getting all local changes.");
+  }
+
 }

src/repository/api/git/ArcanistGitAPI.php

@@ -512,4 +512,37 @@ class ArcanistGitAPI extends ArcanistRepositoryAPI {
     return trim($owner);
   }
 
+  public function supportsRelativeLocalCommits() {
+    return true;
+  }
+
+  public function parseRelativeLocalCommit(array $argv) {
+    if (count($argv) == 0) {
+      return;
+    }
+    if (count($argv) != 1) {
+      throw new ArcanistUsageException("Specify only one commit.");
+    }
+    $base = reset($argv);
+    if ($base == ArcanistGitAPI::GIT_MAGIC_ROOT_COMMIT) {
+      $merge_base = $base;
+    } else {
+      list($err, $merge_base) = exec_manual(
+        '(cd %s; git merge-base %s HEAD)',
+        $this->getPath(),
+        $base);
+      if ($err) {
+        throw new ArcanistUsageException(
+          "Unable to parse git commit name '{$base}'.");
+      }
+    }
+    $this->setRelativeCommit(trim($merge_base));
+  }
+
+  public function getAllLocalChanges() {
+    $diff = $this->getFullGitDiff();
+    $parser = new ArcanistDiffParser();
+    return $parser->parseDiff($diff);
+  }
+
 }

src/repository/api/git/__init__.php

@@ -6,6 +6,8 @@
 
 
 
+phutil_require_module('arcanist', 'exception/usage');
+phutil_require_module('arcanist', 'parser/diff');
 phutil_require_module('arcanist', 'repository/api/base');
 
 phutil_require_module('phutil', 'future');

src/repository/api/mercurial/ArcanistMercurialAPI.php

@@ -374,4 +374,26 @@ class ArcanistMercurialAPI extends ArcanistRepositoryAPI {
     return trim($match[1]);
   }
 
+  public function supportsRelativeLocalCommits() {
+    return true;
+  }
+
+  public function parseRelativeLocalCommit(array $argv) {
+    if (count($argv) == 0) {
+      return;
+    }
+    if (count($argv) != 1) {
+      throw new ArcanistUsageException("Specify only one commit.");
+    }
+    // This does the "hg id" call we need to normalize/validate the revision
+    // identifier.
+    $this->setRelativeCommit(reset($argv));
+  }
+
+  public function getAllLocalChanges() {
+    $diff = $this->getFullMercurialDiff();
+    $parser = new ArcanistDiffParser();
+    return $parser->parseDiff($diff);
+  }
+
 }

src/repository/api/subversion/ArcanistSubversionAPI.php

@@ -482,4 +482,8 @@ EODIFF;
     return null;
   }
 
+  public function supportsRelativeCommits() {
+    return false;
+  }
+
 }

src/workflow/base/ArcanistBaseWorkflow.php

@@ -768,6 +768,8 @@ class ArcanistBaseWorkflow {
     $repository_api = $this->getRepositoryAPI();
 
     if ($repository_api instanceof ArcanistSubversionAPI) {
+      // NOTE: In SVN, we don't currently support a "get all local changes"
+      // operation, so special case it.
       if (empty($this->changeCache[$path])) {
         $diff = $repository_api->getRawDiffText($path);
         $parser = new ArcanistDiffParser();
@@ -777,15 +779,15 @@ class ArcanistBaseWorkflow {
         }
         $this->changeCache[$path] = reset($changes);
       }
-    } else {
+    } else if ($repository_api->supportsRelativeLocalCommits()) {
       if (empty($this->changeCache)) {
-        $diff = $repository_api->getFullGitDiff();
+        $changes = $repository_api->getAllLocalChanges();
-        $parser = new ArcanistDiffParser();
-        $changes = $parser->parseDiff($diff);
         foreach ($changes as $change) {
           $this->changeCache[$change->getCurrentPath()] = $change;
         }
       }
+    } else {
+      throw new Exception("Missing VCS support.");
     }
 
     if (empty($this->changeCache[$path])) {
@@ -827,30 +829,6 @@ class ArcanistBaseWorkflow {
     }
   }
 
-  protected function parseGitRelativeCommit(ArcanistGitAPI $api, array $argv) {
-    if (count($argv) == 0) {
-      return;
-    }
-    if (count($argv) != 1) {
-      throw new ArcanistUsageException(
-        "Specify exactly one commit.");
-    }
-    $base = reset($argv);
-    if ($base == ArcanistGitAPI::GIT_MAGIC_ROOT_COMMIT) {
-      $merge_base = $base;
-    } else {
-      list($err, $merge_base) = exec_manual(
-        '(cd %s; git merge-base %s HEAD)',
-        $api->getPath(),
-        $base);
-      if ($err) {
-        throw new ArcanistUsageException(
-          "Unable to parse git commit name '{$base}'.");
-      }
-    }
-    $api->setRelativeCommit(trim($merge_base));
-  }
-
   protected function normalizeRevisionID($revision_id) {
     return ltrim(strtoupper($revision_id), 'D');
   }

src/workflow/base/__init__.php

@@ -13,7 +13,6 @@ phutil_require_module('arcanist', 'exception/usage/userabort');
 phutil_require_module('arcanist', 'parser/bundle');
 phutil_require_module('arcanist', 'parser/diff');
 phutil_require_module('arcanist', 'parser/diff/change');
-phutil_require_module('arcanist', 'repository/api/git');
 
 phutil_require_module('phutil', 'conduit/client');
 phutil_require_module('phutil', 'console');

src/workflow/diff/ArcanistDiffWorkflow.php

@@ -603,17 +603,10 @@ EOTEXT
         }
       }
 
-    } else if ($repository_api instanceof ArcanistGitAPI) {
+    } else if ($repository_api->supportsRelativeLocalCommits()) {
-      $this->parseGitRelativeCommit(
+      $repository_api->parseRelativeLocalCommit(
-        $repository_api,
         $this->getArgument('paths', array()));
       $paths = $repository_api->getWorkingCopyStatus();
-    } else if ($repository_api instanceof ArcanistMercurialAPI) {
-      // TODO: Unify this and the previous block.
-
-      // TODO: Parse the relative commit.
-
-      $paths = $repository_api->getWorkingCopyStatus();
     } else {
       throw new Exception("Unknown VCS!");
     }

src/workflow/lint/ArcanistLintWorkflow.php

@@ -125,11 +125,12 @@ EOTEXT
             unset($paths[$path]);
           }
         }
-      } else {
+      } else if ($repository_api->supportsRelativeLocalCommits()) {
-        $this->parseGitRelativeCommit(
+        $repository_api->parseRelativeLocalCommit(
-          $repository_api,
+          $this->getArgument('paths', array()));
-          $this->getArgument('paths'));
         $paths = $repository_api->getWorkingCopyStatus();
+      } else {
+        throw new Exception("Unknown VCS!");
       }
 
       foreach ($paths as $path => $flags) {
@@ -139,7 +140,6 @@ EOTEXT
       }
 
       $paths = array_keys($paths);
-
     } else {
       $paths = $this->getArgument('paths');
       if (empty($paths)) {