--range support for git

Summary:
This adds support for passing range of commits for arc diff. This is useful when you want to submit code reviews for past commits without mucking around with the working copy.

This will probably require changes :)

Test Plan: Tested locally, but totally need to add tests for this

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

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

src/parser/__tests__/ArcanistBundleTestCase.php

@@ -84,7 +84,9 @@ final class ArcanistBundleTestCase extends ArcanistTestCase {
         $configuration_manager);
 
       $repository_api->setBaseCommitArgumentRules('arc:this');
-      $diff = $repository_api->getFullGitDiff();
+      $diff = $repository_api->getFullGitDiff(
+        $repository_api->getBaseCommit(),
+        $repository_api->getHeadCommit());
 
       $parser = new ArcanistDiffParser();
       $parser->setRepositoryAPI($repository_api);

src/repository/api/ArcanistGitAPI.php

@@ -16,6 +16,9 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
    */
   const GIT_MAGIC_ROOT_COMMIT = '4b825dc642cb6eb9a060e54bf8d69288fbee4904';
 
+  private $symbolicHeadCommit = 'HEAD';
+  private $resolvedHeadCommit;
+
   public static function newHookAPI($root) {
     return new ArcanistGitAPI($root);
   }
@@ -75,6 +78,21 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     return !$this->repositoryHasNoCommits;
   }
 
+  /**
+   * Tests if a child commit is descendant of a parent commit.
+   * If child and parent are the same, it returns false.
+   * @param child commit SHA.
+   * @param parent commit SHA.
+   * @return bool
+   */
+  private function isDescendant($child, $parent) {
+    list($common_ancestor) =
+      $this->execxLocal('merge-base %s %s', $child, $parent);
+    $common_ancestor = trim($common_ancestor);
+
+    return $common_ancestor == $parent && $common_ancestor != $child;
+  }
+
   public function getLocalCommitInformation() {
     if ($this->repositoryHasNoCommits) {
       // Zero commits.
@@ -106,7 +124,16 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
       // this as being the commits X and Y. If we log "B..Y", we only show
       // Y. With "Y --not B", we show X and Y.
 
-      $against = csprintf('%s --not %s', 'HEAD', $this->getBaseCommit());
+      $base_commit = $this->getBaseCommit();
+      $head_commit = $this->getHeadCommit();
+      if ($this->isDescendant($head_commit, $base_commit) === false) {
+        throw new ArcanistUsageException(
+          "base commit ${base_commit} is not a child of head commit ".
+          "${head_commit}");
+      }
+
+      $against = csprintf('%s --not %s',
+        $this->getHeadCommit(), $this->getBaseCommit());
     }
 
     // NOTE: Windows escaping of "%" symbols apparently is inherently broken;
@@ -161,8 +188,9 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
       }
 
       list($err, $merge_base) = $this->execManualLocal(
-        'merge-base %s HEAD',
-        $symbolic_commit);
+        'merge-base %s %s',
+        $symbolic_commit,
+        $this->getHeadCommit());
       if ($err) {
         throw new ArcanistUsageException(
           "Unable to find any git commit named '{$symbolic_commit}' in ".
@@ -170,8 +198,8 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
       }
 
       $this->setBaseCommitExplanation(
-        "it is the merge-base of '{$symbolic_commit}' and HEAD, as you ".
-        "explicitly specified.");
+        "it is the merge-base of '{$symbolic_commit}' and ".
+        "{$this->symbolicHeadCommit}, as you explicitly specified.");
       return trim($merge_base);
     }
 
@@ -301,6 +329,40 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     return trim($merge_base);
   }
 
+  public function getHeadCommit() {
+    if ($this->resolvedHeadCommit === null) {
+      $this->resolvedHeadCommit =
+        $this->resolveCommit($this->symbolicHeadCommit);
+    }
+
+    return $this->resolvedHeadCommit;
+  }
+
+  final public function setHeadCommit($symbolic_commit) {
+    $this->symbolicHeadCommit = $symbolic_commit;
+    $this->reloadCommitRange();
+    return $this;
+  }
+
+  /**
+   * Translates a symbolic commit (like "HEAD^") to a commit identifier.
+   * @param string_symbol commit.
+   * @return string the commit SHA.
+   */
+  private function resolveCommit($symbolic_commit) {
+    list($err, $commit_hash) = $this->execManualLocal(
+      'rev-parse %s',
+      $symbolic_commit);
+
+    if ($err) {
+      throw new ArcanistUsageException(
+        "Unable to find any git commit named '{$symbolic_commit}' in ".
+        "this repository.");
+    }
+
+    return trim($commit_hash);
+  }
+
   private function getDiffFullOptions($detect_moves_and_renames = true) {
     $options = array(
       self::getDiffBaseOptions(),
@@ -332,11 +394,22 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     return implode(' ', $options);
   }
 
-  public function getFullGitDiff() {
+  /**
+   * @param the base revision
+   * @param head revision. If this is null, the generated diff will include the
+   * working copy
+   */
+  public function getFullGitDiff($base, $head=null) {
     $options = $this->getDiffFullOptions();
+
+    $diff_revision = $base;
+    if ($head) {
+      $diff_revision .= '..'.$head;
+    }
+
     list($stdout) = $this->execxLocal(
       "diff {$options} %s --",
-      $this->getBaseCommit());
+      $diff_revision);
     return $stdout;
   }
 
@@ -401,8 +474,9 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
     } else {
       // 2..N commits.
       list($stdout) = $this->execxLocal(
-        'log --first-parent --format=medium %s..HEAD',
-        $this->getBaseCommit());
+        'log --first-parent --format=medium %s..%s',
+        $this->getBaseCommit(),
+        $this->getHeadCommit());
     }
     return $stdout;
   }
@@ -821,7 +895,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
   }
 
   public function getAllLocalChanges() {
-    $diff = $this->getFullGitDiff();
+    $diff = $this->getFullGitDiff($this->getBaseCommit());
     if (!strlen(trim($diff))) {
       return array();
     }

src/repository/api/ArcanistRepositoryAPI.php

@@ -575,6 +575,10 @@ abstract class ArcanistRepositoryAPI {
     return $this;
   }
 
+  public function setHeadCommit($symbolic_commit) {
+    throw new ArcanistCapabilityNotSupportedException($this);
+  }
+
   final public function getBaseCommit() {
     if (!$this->supportsCommitRanges()) {
       throw new ArcanistCapabilityNotSupportedException($this);
@@ -588,6 +592,10 @@ abstract class ArcanistRepositoryAPI {
     return $this->resolvedBaseCommit;
   }
 
+  public function getHeadCommit() {
+    throw new ArcanistCapabilityNotSupportedException($this);
+  }
+
   final public function reloadCommitRange() {
     $this->resolvedBaseCommit = null;
     $this->baseCommitExplanation = null;

src/workflow/ArcanistDiffWorkflow.php

@@ -395,6 +395,18 @@ EOTEXT
         ),
       ),
       '*' => 'paths',
+      'head' => array(
+        'param' => 'commit',
+        'help' => "specify the head commit.\n".
+          "This disables many Arcanist/Phabricator features which depend on ".
+          "having access to the working copy.",
+        'supports' => array('git'),
+        'conflicts' => array(
+          'lintall'   => '--head suppresses lint.',
+          'advice'    => '--head suppresses lint.',
+        ),
+
+      )
     );
 
     if (phutil_is_windows()) {
@@ -433,8 +445,19 @@ EOTEXT
         array_unshift($argv, '--ansi');
       }
 
-      if ($this->getRepositoryAPI()->supportsCommitRanges()) {
-        $this->getRepositoryAPI()->getBaseCommit();
+      $repo = $this->getRepositoryAPI();
+      $head_commit = $this->getArgument('head');
+      $range_supported = $repo->supportsCommitRanges();
+      if ($head_commit) {
+        if (!$range_supported) {
+          throw new ArcanistUsageException('Ranged are not supported');
+        }
+
+        $repo->setHeadCommit($head_commit);
+      }
+
+      if ($range_supported) {
+        $repo->getBaseCommit();
       }
 
       $script = phutil_get_library_root('arcanist').'/../scripts/arcanist.php';
@@ -661,7 +684,9 @@ EOTEXT
         if ($repository_api instanceof ArcanistSubversionAPI) {
           $repository_api->limitStatusToPaths($this->getArgument('paths'));
         }
-        $this->requireCleanWorkingCopy();
+        if (!$this->getArgument('head')) {
+          $this->requireCleanWorkingCopy();
+        }
       } catch (ArcanistUncommittedChangesException $ex) {
         if ($repository_api instanceof ArcanistMercurialAPI) {
           $use_dirty_changes = false;
@@ -937,7 +962,9 @@ EOTEXT
         $repository_api,
         $paths);
     } else if ($repository_api instanceof ArcanistGitAPI) {
-      $diff = $repository_api->getFullGitDiff();
+      $diff = $repository_api->getFullGitDiff(
+        $repository_api->getBaseCommit(),
+        $repository_api->getHeadCommit());
       if (!strlen($diff)) {
         throw new ArcanistUsageException(
           'No changes found. (Did you specify the wrong commit range?)');
@@ -1216,7 +1243,8 @@ EOTEXT
   private function runLint() {
     if ($this->getArgument('nolint') ||
         $this->getArgument('only') ||
-        $this->isRawDiffSource()) {
+        $this->isRawDiffSource() ||
+        $this->getArgument('head')) {
       return ArcanistLintWorkflow::RESULT_SKIP;
     }
 
@@ -1297,7 +1325,8 @@ EOTEXT
   private function runUnit() {
     if ($this->getArgument('nounit') ||
         $this->getArgument('only') ||
-        $this->isRawDiffSource()) {
+        $this->isRawDiffSource() ||
+        $this->getArgument('head')) {
       return ArcanistUnitWorkflow::RESULT_SKIP;
     }
 

src/workflow/ArcanistExportWorkflow.php

@@ -178,7 +178,9 @@ EOTEXT
 
         if ($repository_api instanceof ArcanistGitAPI) {
           $this->parseBaseCommitArgument($this->getArgument('paths'));
-          $diff = $repository_api->getFullGitDiff();
+          $diff = $repository_api->getFullGitDiff(
+            $repository_api->getBaseCommit(),
+            $repository_api->getHeadCommit());
           $changes = $parser->parseDiff($diff);
           $authors = $this->getConduit()->callMethodSynchronous(
             'user.query',

src/workflow/ArcanistWhichWorkflow.php

@@ -61,6 +61,10 @@ EOTEXT
         ),
         'supports' => array('git', 'hg'),
       ),
+      'head' => array(
+        'param' => 'commit',
+        'help' => 'specify the head commit.'
+      ),
       '*' => 'commit',
     );
   }
@@ -83,7 +87,19 @@ EOTEXT
     $repository_api->setBaseCommitArgumentRules(
       $this->getArgument('base', ''));
 
-    if ($repository_api->supportsCommitRanges()) {
+    $supports_ranges = $repository_api->supportsCommitRanges();
+
+    if ($this->getArgument('head')) {
+      if ($supports_ranges === false) {
+        throw new Exception('--head is not supported in this VCS');
+      }
+
+      $head_commit = $this->getArgument('head');
+      $arg .= " --head {$head_commit}";
+      $repository_api->setHeadCommit($head_commit);
+    }
+
+    if ($supports_ranges) {
       $relative = $repository_api->getBaseCommit();
 
       if ($this->getArgument('show-base')) {
@@ -111,7 +127,8 @@ EOTEXT
       $relative = substr($relative, 0, 16);
 
       if ($repository_api instanceof ArcanistGitAPI) {
-        $command = "git diff {$relative}..HEAD";
+        $head = $this->getArgument('head', 'HEAD');
+        $command = "git diff {$relative}..{$head}";
       } else if ($repository_api instanceof ArcanistMercurialAPI) {
         $command = "hg diff --rev {$relative}";
       } else {