From 6b192f3178f94ea5301c74c97f4de8ba69a7fcdb Mon Sep 17 00:00:00 2001 From: Tal Shiri Date: Wed, 11 Jun 2014 14:37:01 -0700 Subject: [PATCH] --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 --- .../__tests__/ArcanistBundleTestCase.php | 4 +- src/repository/api/ArcanistGitAPI.php | 94 +++++++++++++++++-- src/repository/api/ArcanistRepositoryAPI.php | 8 ++ src/workflow/ArcanistDiffWorkflow.php | 41 ++++++-- src/workflow/ArcanistExportWorkflow.php | 4 +- src/workflow/ArcanistWhichWorkflow.php | 21 ++++- 6 files changed, 152 insertions(+), 20 deletions(-) diff --git a/src/parser/__tests__/ArcanistBundleTestCase.php b/src/parser/__tests__/ArcanistBundleTestCase.php index 6dc2a0a0..19f251d9 100644 --- a/src/parser/__tests__/ArcanistBundleTestCase.php +++ b/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); diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php index 45bae424..3f24d984 100644 --- a/src/repository/api/ArcanistGitAPI.php +++ b/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(); } diff --git a/src/repository/api/ArcanistRepositoryAPI.php b/src/repository/api/ArcanistRepositoryAPI.php index 69a89b57..10da4697 100644 --- a/src/repository/api/ArcanistRepositoryAPI.php +++ b/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; diff --git a/src/workflow/ArcanistDiffWorkflow.php b/src/workflow/ArcanistDiffWorkflow.php index 34695d24..fead0459 100644 --- a/src/workflow/ArcanistDiffWorkflow.php +++ b/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; } diff --git a/src/workflow/ArcanistExportWorkflow.php b/src/workflow/ArcanistExportWorkflow.php index ae6d33f4..481f0c5b 100644 --- a/src/workflow/ArcanistExportWorkflow.php +++ b/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', diff --git a/src/workflow/ArcanistWhichWorkflow.php b/src/workflow/ArcanistWhichWorkflow.php index d8b2815d..1160d4e8 100644 --- a/src/workflow/ArcanistWhichWorkflow.php +++ b/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 {