From b61e4eacf1d4097a270a285eca87778156d1f194 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Fri, 2 Dec 2011 16:21:14 -0800 Subject: [PATCH] Arc - add a sanity check to arc patch workflows to make sure the vcs base revision is the correct base revision relative to the patch. Summary: What the title says. If not correct, warn the user. This check honors the --force flag to skip all these checks. This change also includes moving some Differential constants into Arc so they can be used for both projects. There is a corresponding phabricator diff (incoming) to address this part of the change. Test Plan: For a project with actual diffs, a git repository tracked by phabricator, *AND* development in master branch only, do some... - git reset --hard HEAD^1 - arc patch DX, where X is what got us to HEAD in the first place - verify successful patch ...then... - git reset --hard HEAD^^ - arc patch DX, where X is what got us to HEAD in the first place - verify warning - verify Y versus N continues versus stops appropriately Note if development were done outside the master branch this warning message will fire early / often as git commit hashes are based on the commit *and* the rest of the source code the commit is made against. This is (unfortunately) the "typical" case so this warning is pretty active at the moment. T201 will eventually land and when parsing a given commit update the corresponding diff. Reviewers: epriestley Reviewed By: epriestley CC: aran, btrahan, epriestley Differential Revision: https://secure.phabricator.com/D1328 --- src/__phutil_library_map__.php | 2 + .../ArcanistDifferentialRevisionHash.php | 35 +++++++ .../constants/revisionhash/__init__.php | 10 ++ .../ArcanistDifferentialRevisionStatus.php | 39 ++++++++ .../constants/revisionstatus/__init__.php | 12 +++ src/parser/bundle/ArcanistBundle.php | 19 +++- .../api/base/ArcanistRepositoryAPI.php | 6 +- src/repository/api/git/ArcanistGitAPI.php | 10 +- .../api/mercurial/ArcanistMercurialAPI.php | 2 +- .../api/subversion/ArcanistSubversionAPI.php | 6 +- src/workflow/base/ArcanistBaseWorkflow.php | 1 + .../export/ArcanistExportWorkflow.php | 4 +- src/workflow/patch/ArcanistPatchWorkflow.php | 95 ++++++++++++++++++- src/workflow/patch/__init__.php | 2 + 14 files changed, 229 insertions(+), 14 deletions(-) create mode 100644 src/differential/constants/revisionhash/ArcanistDifferentialRevisionHash.php create mode 100644 src/differential/constants/revisionhash/__init__.php create mode 100644 src/differential/constants/revisionstatus/ArcanistDifferentialRevisionStatus.php create mode 100644 src/differential/constants/revisionstatus/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 7a620a74..467d61e1 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -33,7 +33,9 @@ phutil_register_library_map(array( 'ArcanistDiffWorkflow' => 'workflow/diff', 'ArcanistDifferentialCommitMessage' => 'differential/commitmessage', 'ArcanistDifferentialCommitMessageParserException' => 'differential/commitmessage', + 'ArcanistDifferentialRevisionHash' => 'differential/constants/revisionhash', 'ArcanistDifferentialRevisionRef' => 'differential/revision', + 'ArcanistDifferentialRevisionStatus' => 'differential/constants/revisionstatus', 'ArcanistDownloadWorkflow' => 'workflow/download', 'ArcanistEventType' => 'events/constant/type', 'ArcanistExportWorkflow' => 'workflow/export', diff --git a/src/differential/constants/revisionhash/ArcanistDifferentialRevisionHash.php b/src/differential/constants/revisionhash/ArcanistDifferentialRevisionHash.php new file mode 100644 index 00000000..cbbc43d0 --- /dev/null +++ b/src/differential/constants/revisionhash/ArcanistDifferentialRevisionHash.php @@ -0,0 +1,35 @@ + 'Needs Review', + self::NEEDS_REVISION => 'Needs Revision', + self::ACCEPTED => 'Accepted', + self::COMMITTED => 'Committed', + self::ABANDONED => 'Abandoned', + ); + + return idx($map, coalesce($status, '?'), 'Unknown'); + } + +} diff --git a/src/differential/constants/revisionstatus/__init__.php b/src/differential/constants/revisionstatus/__init__.php new file mode 100644 index 00000000..e407b4c9 --- /dev/null +++ b/src/differential/constants/revisionstatus/__init__.php @@ -0,0 +1,12 @@ +conduit = $conduit; @@ -41,6 +42,14 @@ class ArcanistBundle { return $this->projectID; } + public function setBaseRevision($base_revision) { + $this->baseRevision = $base_revision; + } + + public function getBaseRevision() { + return $this->baseRevision; + } + public static function newFromChanges(array $changes) { $obj = new ArcanistBundle(); $obj->changes = $changes; @@ -65,10 +74,12 @@ class ArcanistBundle { $meta_info = $future->resolveJSON(); $version = idx($meta_info, 'version', 0); $project_name = idx($meta_info, 'projectName'); + $base_revision = idx($meta_info, 'baseRevision'); // this arc bundle was probably made before we started storing meta info } else { $version = 0; $project_name = null; + $base_revision = null; } $future = new ExecFuture( @@ -92,6 +103,7 @@ class ArcanistBundle { $obj->changes = $changes; $obj->diskPath = $path; $obj->setProjectID($project_name); + $obj->setBaseRevision($base_revision); return $obj; } @@ -138,8 +150,11 @@ class ArcanistBundle { $blobs[$phid] = $this->getBlob($phid); } - $meta_info = array('version' => 1, - 'projectName' => $this->getProjectID()); + $meta_info = array( + 'version' => 2, + 'projectName' => $this->getProjectID(), + 'baseRevision' => $this->getBaseRevision(), + ); $dir = Filesystem::createTemporaryDirectory(); Filesystem::createDirectory($dir.'/hunks'); diff --git a/src/repository/api/base/ArcanistRepositoryAPI.php b/src/repository/api/base/ArcanistRepositoryAPI.php index c447c5d8..e5a280e5 100644 --- a/src/repository/api/base/ArcanistRepositoryAPI.php +++ b/src/repository/api/base/ArcanistRepositoryAPI.php @@ -1,7 +1,7 @@ getPath(), + 'HEAD'); + return rtrim($stdout, "\n"); + } + public function supportsRelativeLocalCommits() { return true; } diff --git a/src/repository/api/mercurial/ArcanistMercurialAPI.php b/src/repository/api/mercurial/ArcanistMercurialAPI.php index 127df939..fd893b42 100644 --- a/src/repository/api/mercurial/ArcanistMercurialAPI.php +++ b/src/repository/api/mercurial/ArcanistMercurialAPI.php @@ -304,7 +304,7 @@ class ArcanistMercurialAPI extends ArcanistRepositoryAPI { } } - private function getWorkingCopyRevision() { + public function getWorkingCopyRevision() { // In Mercurial, "tip" means the tip of the current branch, not what's in // the working copy. The tip may be ahead of the working copy. We need to // use "hg summary" to figure out what is actually in the working copy. diff --git a/src/repository/api/subversion/ArcanistSubversionAPI.php b/src/repository/api/subversion/ArcanistSubversionAPI.php index e73a1e37..9656d17a 100644 --- a/src/repository/api/subversion/ArcanistSubversionAPI.php +++ b/src/repository/api/subversion/ArcanistSubversionAPI.php @@ -1,7 +1,7 @@ getSourceControlBaseRevision(); + } + public function supportsLocalBranchMerge() { return false; } diff --git a/src/workflow/base/ArcanistBaseWorkflow.php b/src/workflow/base/ArcanistBaseWorkflow.php index 6bcb5375..1400263a 100644 --- a/src/workflow/base/ArcanistBaseWorkflow.php +++ b/src/workflow/base/ArcanistBaseWorkflow.php @@ -752,6 +752,7 @@ class ArcanistBaseWorkflow { $bundle = ArcanistBundle::newFromChanges($changes); $bundle->setConduit($conduit); $bundle->setProjectID($diff['projectName']); + $bundle->setBaseRevision($diff['sourceControlBaseRevision']); return $bundle; } diff --git a/src/workflow/export/ArcanistExportWorkflow.php b/src/workflow/export/ArcanistExportWorkflow.php index ad39e9ff..ff9b5daf 100644 --- a/src/workflow/export/ArcanistExportWorkflow.php +++ b/src/workflow/export/ArcanistExportWorkflow.php @@ -1,7 +1,7 @@ setProjectID($this->getWorkingCopy()->getProjectID()); + $bundle->setBaseRevision( + $repository_api->getSourceControlBaseRevision()); break; case self::SOURCE_REVISION: $bundle = $this->loadRevisionBundleFromConduit( diff --git a/src/workflow/patch/ArcanistPatchWorkflow.php b/src/workflow/patch/ArcanistPatchWorkflow.php index f28303ec..eaaa100f 100644 --- a/src/workflow/patch/ArcanistPatchWorkflow.php +++ b/src/workflow/patch/ArcanistPatchWorkflow.php @@ -1,7 +1,7 @@ getSource() == self::SOURCE_REVISION) || - ($this->getSource() == self::SOURCE_DIFF); + return ($this->getSource() != self::SOURCE_PATCH); } public function requiresRepositoryAPI() { @@ -428,12 +427,12 @@ EOTEXT */ private function sanityCheckPatch(ArcanistBundle $bundle) { - // Check to see if the bundle project id matches the working copy + // Check to see if the bundle's project id matches the working copy // project id $bundle_project_id = $bundle->getProjectID(); $working_copy_project_id = $this->getWorkingCopy()->getProjectID(); if (empty($bundle_project_id)) { - // this means $source is SOURCE_PATCH || SOURCE_BUNDLE + // this means $source is SOURCE_PATCH || SOURCE_BUNDLE w/ $version = 0 // they don't come with a project id so just do nothing } else if ($bundle_project_id != $working_copy_project_id) { $ok = phutil_console_confirm( @@ -447,6 +446,69 @@ EOTEXT } } + // Check to see if the bundle's base revision matches the working copy + // base revision + $bundle_base_rev = $bundle->getBaseRevision(); + if (empty($bundle_base_rev)) { + // this means $source is SOURCE_PATCH || SOURCE_BUNDLE w/ $version < 2 + // they don't have a base rev so just do nothing + } else { + $repository_api = $this->getRepositoryAPI(); + $source_base_rev = $repository_api->getWorkingCopyRevision(); + + if ($source_base_rev != $bundle_base_rev) { + // we have a problem...! lots of work because we need to ask + // differential for revision information for these base revisions + // to improve our error message. + $bundle_base_rev_str = null; + $source_base_rev_str = null; + + // SVN doesn't store these hashes, so we're basically done already + // and will have a relatively "lame" error message + if ($repository_api instanceof ArcanistSubversionAPI) { + $hash_type = null; + } else if ($repository_api instanceof ArcanistGitAPI) { + $hash_type = ArcanistDifferentialRevisionHash::HASH_GIT_COMMIT; + } else if ($repository_api instanceof ArcanistMercurialAPI) { + $hash_type = ArcanistDifferentialRevisionHash::HASH_MERCURIAL_COMMIT; + } else { + $hash_type = null; + } + + if ($hash_type) { + // 2 round trips because even though we could send off one query + // we wouldn't be able to tell which revisions were for which hash + $hash = array($hash_type, $bundle_base_rev); + $bundle_revision = $this->loadRevisionFromHash($hash); + $hash = array($hash_type, $source_base_rev); + $source_revision = $this->loadRevisionFromHash($hash); + + if ($bundle_revision) { + $bundle_base_rev_str = $bundle_base_rev . + ' \ D' . $bundle_revision['id']; + } + if ($source_revision) { + $source_base_rev_str = $source_base_rev . + ' \ D' . $source_revision['id']; + } + } + $bundle_base_rev_str = nonempty($bundle_base_rev_str, + $bundle_base_rev); + $source_base_rev_str = nonempty($source_base_rev_str, + $source_base_rev); + + $ok = phutil_console_confirm( + "This diff is against commit {$bundle_base_rev_str}, but the ". + "working copy is at {$source_base_rev_str}. ". + "Still try to apply it?", + $default_no = false + ); + if (!$ok) { + throw new ArcanistUserAbortException(); + } + } + } + // TODO -- more sanity checks here } @@ -473,4 +535,27 @@ EOTEXT $dir)); } } + + private function loadRevisionFromHash($hash) { + $conduit = $this->getConduit(); + + $revisions = $conduit->callMethodSynchronous( + 'differential.query', + array( + 'commitHashes' => array($hash), + ) + ); + + + // grab the latest committed revision only + $found_revision = null; + $revisions = isort($revisions, 'dateModified'); + foreach ($revisions as $revision) { + if ($revision['status'] == + ArcanistDifferentialRevisionStatus::COMMITTED) { + $found_revision = $revision; + } + } + return $found_revision; + } } diff --git a/src/workflow/patch/__init__.php b/src/workflow/patch/__init__.php index 3449002a..35015c6f 100644 --- a/src/workflow/patch/__init__.php +++ b/src/workflow/patch/__init__.php @@ -6,6 +6,8 @@ +phutil_require_module('arcanist', 'differential/constants/revisionhash'); +phutil_require_module('arcanist', 'differential/constants/revisionstatus'); phutil_require_module('arcanist', 'exception/usage'); phutil_require_module('arcanist', 'exception/usage/userabort'); phutil_require_module('arcanist', 'parser/bundle');