From 2ae0cb797da57fbfd940671fef612b4d1edbd0fe Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 17 Dec 2012 12:53:38 -0800 Subject: [PATCH] Remove ArcanistRepositoryAPI::setDefaultBaseCommit() Summary: This method is used in three cases: # For unit tests, to set the range to 'HEAD^' or '.^' in an agnostic way. # For "amend", to set the range to the commit to be amended (also 'HEAD^' or '.^'). # For "patch" and "upgrade" so we don't fail just because there's an invalid "base" rule somewhere in the config when doing clean-working-copy tests. For cases (1) and (2), introduce an "arc:this" rule to mean "the current commit". For case (3), remove the call; it is no longer necessary to check the commit range in order to do tests for the working copy state after D4095. Test Plan: Ran unit tests, "arc upgrade", "arc patch", "arc amend". Reviewers: vrana Reviewed By: vrana CC: aran Differential Revision: https://secure.phabricator.com/D4096 --- src/parser/ArcanistBaseCommitParser.php | 1 + src/parser/__tests__/ArcanistBundleTestCase.php | 8 ++++++-- src/repository/api/ArcanistGitAPI.php | 10 +++++----- src/repository/api/ArcanistMercurialAPI.php | 11 ++++++----- src/repository/api/ArcanistRepositoryAPI.php | 15 --------------- .../ArcanistRepositoryAPIStateTestCase.php | 5 +---- src/workflow/ArcanistAmendWorkflow.php | 5 +---- src/workflow/ArcanistPatchWorkflow.php | 4 ---- src/workflow/ArcanistUpgradeWorkflow.php | 4 ---- 9 files changed, 20 insertions(+), 43 deletions(-) diff --git a/src/parser/ArcanistBaseCommitParser.php b/src/parser/ArcanistBaseCommitParser.php index 5d8a1fb9..215e9ee1 100644 --- a/src/parser/ArcanistBaseCommitParser.php +++ b/src/parser/ArcanistBaseCommitParser.php @@ -151,6 +151,7 @@ final class ArcanistBaseCommitParser { case 'outgoing': case 'bookmark': case 'amended': + case 'this': return $this->api->resolveBaseCommitRule($rule, $source); default: $matches = null; diff --git a/src/parser/__tests__/ArcanistBundleTestCase.php b/src/parser/__tests__/ArcanistBundleTestCase.php index 747286f1..0652e166 100644 --- a/src/parser/__tests__/ArcanistBundleTestCase.php +++ b/src/parser/__tests__/ArcanistBundleTestCase.php @@ -58,8 +58,12 @@ final class ArcanistBundleTestCase extends ArcanistTestCase { list($commit_hash, $tree_hash, $subject) = explode(' ', $commit, 3); execx('git reset --hard %s --', $commit_hash); - $repository_api = new ArcanistGitAPI($fixture->getPath()); - $repository_api->setDefaultBaseCommit(); + $fixture_path = $fixture->getPath(); + $working_copy = ArcanistWorkingCopyIdentity::newFromPath($fixture_path); + + $repository_api = ArcanistRepositoryAPI::newAPIFromWorkingCopyIdentity( + $working_copy); + $repository_api->setBaseCommitArgumentRules('arc:this'); $diff = $repository_api->getFullGitDiff(); $parser = new ArcanistDiffParser(); diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php index f3de6260..87673012 100644 --- a/src/repository/api/ArcanistGitAPI.php +++ b/src/repository/api/ArcanistGitAPI.php @@ -715,11 +715,6 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { return true; } - public function setDefaultBaseCommit() { - $this->setRelativeCommit('HEAD^'); - return $this; - } - public function hasLocalCommit($commit) { try { if (!$this->getCanonicalRevisionName($commit)) { @@ -987,6 +982,11 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { return $upstream_merge_base; } break; + case 'this': + $this->setBaseCommitExplanation( + "you specified '{$rule}' in your {$source} 'base' ". + "configuration."); + return 'HEAD^'; } default: return null; diff --git a/src/repository/api/ArcanistMercurialAPI.php b/src/repository/api/ArcanistMercurialAPI.php index 50e28014..ecadd58d 100644 --- a/src/repository/api/ArcanistMercurialAPI.php +++ b/src/repository/api/ArcanistMercurialAPI.php @@ -442,11 +442,6 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { return true; } - public function setDefaultBaseCommit() { - $this->setRelativeCommit('.^'); - return $this; - } - public function hasLocalCommit($commit) { try { $this->getCanonicalRevisionName($commit); @@ -734,6 +729,12 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { "your {$source} 'base' configuration"); return trim($bookmark_base); } + break; + case 'this': + $this->setBaseCommitExplanation( + "you specified '{$rule}' in your {$source} 'base' ". + "configuration."); + return '.^'; } break; default: diff --git a/src/repository/api/ArcanistRepositoryAPI.php b/src/repository/api/ArcanistRepositoryAPI.php index 9af523ab..c7a7e405 100644 --- a/src/repository/api/ArcanistRepositoryAPI.php +++ b/src/repository/api/ArcanistRepositoryAPI.php @@ -322,21 +322,6 @@ abstract class ArcanistRepositoryAPI { return $this->getWorkingCopyRevision(); } - /** - * Set the base commit to a reasonable default value so that working copy - * status checks can do something meaningful and won't invoke configured - * 'base' rules. - * - * This is primarily useful for workflows which do not operate on commit - * ranges but need to verify the working copy is not dirty, like "amend", - * "upgrade" and "patch". - * - * @return this - */ - public function setDefaultBaseCommit() { - throw new ArcanistCapabilityNotSupportedException($this); - } - public function getChangedFiles($since_commit) { throw new ArcanistCapabilityNotSupportedException($this); } diff --git a/src/repository/api/__tests__/ArcanistRepositoryAPIStateTestCase.php b/src/repository/api/__tests__/ArcanistRepositoryAPIStateTestCase.php index e139d3d0..d3c89f1f 100644 --- a/src/repository/api/__tests__/ArcanistRepositoryAPIStateTestCase.php +++ b/src/repository/api/__tests__/ArcanistRepositoryAPIStateTestCase.php @@ -14,10 +14,7 @@ final class ArcanistRepositoryAPIStateTestCase extends ArcanistTestCase { $api = ArcanistRepositoryAPI::newAPIFromWorkingCopyIdentity( $working_copy); - - if ($api->supportsRelativeLocalCommits()) { - $api->setDefaultBaseCommit(); - } + $api->setBaseCommitArgumentRules('arc:this'); $this->assertCorrectState($test, $api); } diff --git a/src/workflow/ArcanistAmendWorkflow.php b/src/workflow/ArcanistAmendWorkflow.php index 0aed0d22..ceb504d6 100644 --- a/src/workflow/ArcanistAmendWorkflow.php +++ b/src/workflow/ArcanistAmendWorkflow.php @@ -90,10 +90,7 @@ EOTEXT $revision_id = $this->normalizeRevisionID($this->getArgument('revision')); } - if ($repository_api->supportsRelativeLocalCommits()) { - $repository_api->setDefaultBaseCommit(); - } - + $repository_api->setBaseCommitArgumentRules('arc:this'); $in_working_copy = $repository_api->loadWorkingCopyDifferentialRevisions( $this->getConduit(), array( diff --git a/src/workflow/ArcanistPatchWorkflow.php b/src/workflow/ArcanistPatchWorkflow.php index e04a6377..d6b45646 100644 --- a/src/workflow/ArcanistPatchWorkflow.php +++ b/src/workflow/ArcanistPatchWorkflow.php @@ -853,10 +853,6 @@ EOTEXT private function sanityCheck(ArcanistBundle $bundle) { $repository_api = $this->getRepositoryAPI(); - if ($repository_api->supportsRelativeLocalCommits()) { - $repository_api->setDefaultBaseCommit(); - } - // Require clean working copy $this->requireCleanWorkingCopy(); diff --git a/src/workflow/ArcanistUpgradeWorkflow.php b/src/workflow/ArcanistUpgradeWorkflow.php index 348686ce..803fc0d2 100644 --- a/src/workflow/ArcanistUpgradeWorkflow.php +++ b/src/workflow/ArcanistUpgradeWorkflow.php @@ -50,10 +50,6 @@ EOTEXT $repository_api = ArcanistRepositoryAPI::newAPIFromWorkingCopyIdentity( $working_copy); - if ($repository_api->supportsRelativeLocalCommits()) { - $repository_api->setDefaultBaseCommit(); - } - $this->setRepositoryAPI($repository_api); // Require no local changes.