From 44959afd4b88b5d3d283d1fec62452251343ed26 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 25 Aug 2011 16:02:03 -0700 Subject: [PATCH] Add an "arc merge" workflow Summary: This should support conservative rewrite policies in git fairly well, under an assumed workflow of: - Develop in local branches, never rewrite history. - Commit with "-m" or by typing a brief, non-template commit message describing the checkpoint. - Provide rich information in the web console (reviewers, etc.) - Finalize with "git checkout master && arc merge branch && git push" or some flavor thereof. This supports Mercurial somewhat. The major problem is that "hg merge" fails if the local is a fastforward of the remote, at which point there's nowhere we can throw the commit message. Oh well. Just push it and we'll do our best to link them up based on local commit info. I am increasingly forming an opinion that Mercurial is "saftey-scissors git". But also maybe I have no clue what I'm doing. I just don't understand why anyone would think it's a good idea to have a trunk consisting of ~50% known-broken revisions, random checkpoint parts, whitespace changes, typo fixes, etc. If you use git with branching you can avoid this by making a trunk out of merges or with rebase/amend, but there seems to be no way to have "one commit = one idea" in any real sense in Mercurial. Test Plan: Execute "arc merge" in git and mercurial. Reviewers: fratrik, Makinde, aran, jungejason, tuomaspelkonen Reviewed By: Makinde CC: aran, epriestley, Makinde Differential Revision: 860 --- src/__phutil_library_map__.php | 3 + .../api/base/ArcanistRepositoryAPI.php | 14 +- src/repository/api/base/__init__.php | 1 + src/repository/api/git/ArcanistGitAPI.php | 25 +++ .../api/mercurial/ArcanistMercurialAPI.php | 28 ++++ .../api/subversion/ArcanistSubversionAPI.php | 4 + ...rcanistCapabilityNotSupportedException.php | 28 ++++ .../exception/notsupported/__init__.php | 10 ++ src/workflow/merge/ArcanistMergeWorkflow.php | 158 ++++++++++++++++++ src/workflow/merge/__init__.php | 16 ++ 10 files changed, 285 insertions(+), 2 deletions(-) create mode 100644 src/workflow/exception/notsupported/ArcanistCapabilityNotSupportedException.php create mode 100644 src/workflow/exception/notsupported/__init__.php create mode 100644 src/workflow/merge/ArcanistMergeWorkflow.php create mode 100644 src/workflow/merge/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 7d9ac707..b89c0d4c 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -16,6 +16,7 @@ phutil_register_library_map(array( 'ArcanistBranchWorkflow' => 'workflow/branch', 'ArcanistBundle' => 'parser/bundle', 'ArcanistCallConduitWorkflow' => 'workflow/call-conduit', + 'ArcanistCapabilityNotSupportedException' => 'workflow/exception/notsupported', 'ArcanistChooseInvalidRevisionException' => 'exception', 'ArcanistChooseNoRevisionsException' => 'exception', 'ArcanistCommitWorkflow' => 'workflow/commit', @@ -56,6 +57,7 @@ phutil_register_library_map(array( 'ArcanistListWorkflow' => 'workflow/list', 'ArcanistMarkCommittedWorkflow' => 'workflow/mark-committed', 'ArcanistMercurialAPI' => 'repository/api/mercurial', + 'ArcanistMergeWorkflow' => 'workflow/merge', 'ArcanistNoEffectException' => 'exception/usage/noeffect', 'ArcanistNoEngineException' => 'exception/usage/noengine', 'ArcanistNoLintLinter' => 'lint/linter/nolint', @@ -121,6 +123,7 @@ phutil_register_library_map(array( 'ArcanistListWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistMarkCommittedWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistMercurialAPI' => 'ArcanistRepositoryAPI', + 'ArcanistMergeWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistNoEffectException' => 'ArcanistUsageException', 'ArcanistNoEngineException' => 'ArcanistUsageException', 'ArcanistNoLintLinter' => 'ArcanistLinter', diff --git a/src/repository/api/base/ArcanistRepositoryAPI.php b/src/repository/api/base/ArcanistRepositoryAPI.php index b3031ce1..563b6c7f 100644 --- a/src/repository/api/base/ArcanistRepositoryAPI.php +++ b/src/repository/api/base/ArcanistRepositoryAPI.php @@ -163,11 +163,21 @@ abstract class ArcanistRepositoryAPI { abstract public function supportsRelativeLocalCommits(); public function parseRelativeLocalCommit(array $argv) { - throw new Exception("This VCS does not support relative local commits."); + throw new ArcanistCapabilityNotSupportedException($this); } public function getAllLocalChanges() { - throw new Exception("This VCS does not support getting all local changes."); + throw new ArcanistCapabilityNotSupportedException($this); + } + + abstract public function supportsLocalBranchMerge(); + + public function performLocalBranchMerge($branch, $message) { + throw new ArcanistCapabilityNotSupportedException($this); + } + + public function getFinalizedRevisionMessage() { + throw new ArcanistCapabilityNotSupportedException($this); } } diff --git a/src/repository/api/base/__init__.php b/src/repository/api/base/__init__.php index f2fcc047..e78e816c 100644 --- a/src/repository/api/base/__init__.php +++ b/src/repository/api/base/__init__.php @@ -7,6 +7,7 @@ phutil_require_module('arcanist', 'exception/usage'); +phutil_require_module('arcanist', 'workflow/exception/notsupported'); phutil_require_module('phutil', 'console'); phutil_require_module('phutil', 'filesystem'); diff --git a/src/repository/api/git/ArcanistGitAPI.php b/src/repository/api/git/ArcanistGitAPI.php index db16cd87..6df4e49c 100644 --- a/src/repository/api/git/ArcanistGitAPI.php +++ b/src/repository/api/git/ArcanistGitAPI.php @@ -545,4 +545,29 @@ class ArcanistGitAPI extends ArcanistRepositoryAPI { return $parser->parseDiff($diff); } + public function supportsLocalBranchMerge() { + return true; + } + + public function performLocalBranchMerge($branch, $message) { + if (!$branch) { + throw new ArcanistUsageException( + "Under git, you must specify the branch you want to merge."); + } + $err = phutil_passthru( + '(cd %s && git merge --no-ff -m %s %s)', + $this->getPath(), + $message, + $branch); + + if ($err) { + throw new ArcanistUsageException("Merge failed!"); + } + } + + public function getFinalizedRevisionMessage() { + return "You may now push this commit upstream, as appropriate (e.g. with ". + "'git push', or 'git svn dcommit', or by printing and faxing it)."; + } + } diff --git a/src/repository/api/mercurial/ArcanistMercurialAPI.php b/src/repository/api/mercurial/ArcanistMercurialAPI.php index db78f0b5..8a02ae97 100644 --- a/src/repository/api/mercurial/ArcanistMercurialAPI.php +++ b/src/repository/api/mercurial/ArcanistMercurialAPI.php @@ -396,4 +396,32 @@ class ArcanistMercurialAPI extends ArcanistRepositoryAPI { return $parser->parseDiff($diff); } + public function supportsLocalBranchMerge() { + return true; + } + + public function performLocalBranchMerge($branch, $message) { + if ($branch) { + $err = phutil_passthru( + '(cd %s && hg merge --rev %s && hg commit -m %s)', + $this->getPath(), + $branch, + $message); + } else { + $err = phutil_passthru( + '(cd %s && hg merge && hg commit -m %s)', + $this->getPath(), + $message); + } + + if ($err) { + throw new ArcanistUsageException("Merge failed!"); + } + } + + public function getFinalizedRevisionMessage() { + return "You may now push this commit upstream, as appropriate (e.g. with ". + "'hg push' or by printing and faxing it)."; + } + } diff --git a/src/repository/api/subversion/ArcanistSubversionAPI.php b/src/repository/api/subversion/ArcanistSubversionAPI.php index a9179803..418d8575 100644 --- a/src/repository/api/subversion/ArcanistSubversionAPI.php +++ b/src/repository/api/subversion/ArcanistSubversionAPI.php @@ -486,4 +486,8 @@ EODIFF; return false; } + public function supportsLocalBranchMerge() { + return false; + } + } diff --git a/src/workflow/exception/notsupported/ArcanistCapabilityNotSupportedException.php b/src/workflow/exception/notsupported/ArcanistCapabilityNotSupportedException.php new file mode 100644 index 00000000..f57fcf86 --- /dev/null +++ b/src/workflow/exception/notsupported/ArcanistCapabilityNotSupportedException.php @@ -0,0 +1,28 @@ +getSourceControlSystemName(); + parent::__construct( + "This repository API ('{$name}') does not support the requested ". + "capability."); + } + +} diff --git a/src/workflow/exception/notsupported/__init__.php b/src/workflow/exception/notsupported/__init__.php new file mode 100644 index 00000000..b432dd06 --- /dev/null +++ b/src/workflow/exception/notsupported/__init__.php @@ -0,0 +1,10 @@ +" or "hg merge --rev " of a + reviewed branch, but give the merge commit a useful commit message + with information from Differential. + + In Git, this operates like "git merge " and should be executed + from the branch you want to merge __into__, just like "git merge". + Branch is required. + + In Mercurial, this operates like "hg merge" (default) or + "hg merge --rev " and should be executed from the branch you + want to merge __from__, just like "hg merge". It will also effect an + "hg commit" with a rich commit message. + +EOTEXT + ); + } + + public function requiresWorkingCopy() { + return true; + } + + public function requiresConduit() { + return true; + } + + public function requiresAuthentication() { + return true; + } + + public function requiresRepositoryAPI() { + return true; + } + + public function getArguments() { + return array( + 'show' => array( + 'help' => + "Don't merge, just show the commit message." + ), + 'revision' => array( + 'param' => 'revision', + 'help' => + "Use the message for a specific revision. If 'arc' can't figure ". + "out which revision you want, you can tell it explicitly.", + ), + '*' => 'branch', + ); + } + + public function run() { + $this->writeStatusMessage( + phutil_console_format( + "**WARNING:** 'arc merge' is new and experimental.\n")); + + $repository_api = $this->getRepositoryAPI(); + if (!$repository_api->supportsLocalBranchMerge()) { + $name = $repository_api->getSourceControlSystemName(); + throw new ArcanistUsageException( + "This source control system ('{$name}') does not support 'arc merge'."); + } + + if ($repository_api->getUncommittedChanges()) { + throw new ArcanistUsageException( + "You have uncommitted changes in this working copy. Commit ". + "(or revert) them before proceeding."); + } + + $branch = $this->getArgument('branch'); + if (count($branch) > 1) { + throw new ArcanistUsageException("Specify only one branch to merge."); + } else { + $branch = head($branch); + } + + $conduit = $this->getConduit(); + + $revisions = $conduit->callMethodSynchronous( + 'differential.find', + array( + 'guids' => array($this->getUserPHID()), + 'query' => 'committable', + )); + + // TODO: Make an effort to guess which revision the user means here. Branch + // name is a very strong heuristic but Conduit doesn't make it easy to get + // right now. We now also have "commits:local" after D857. Between these + // we should be able to get this right automatically in essentially every + // reasonable case. + + try { + $revision = $this->chooseRevision( + $revisions, + $this->getArgument('revision'), + 'Which revision do you want to merge?'); + $revision_id = $revision->getID(); + } catch (ArcanistChooseInvalidRevisionException $ex) { + throw new ArcanistUsageException( + "You can only merge Differential revisions which have been accepted."); + } catch (ArcanistChooseNoRevisionsException $ex) { + throw new ArcanistUsageException( + "You have no accepted Differential revisions."); + } + + $message = $conduit->callMethodSynchronous( + 'differential.getcommitmessage', + array( + 'revision_id' => $revision_id, + 'edit' => false, + )); + + if ($this->getArgument('show')) { + echo $message."\n"; + } else { + $repository_api->performLocalBranchMerge($branch, $message); + echo "Merged '{$branch}'.\n"; + + $done_message = $repository_api->getFinalizedRevisionMessage(); + echo phutil_console_wrap($done_message."\n"); + } + + return 0; + } + + protected function getSupportedRevisionControlSystems() { + return array('git'); + } + +} diff --git a/src/workflow/merge/__init__.php b/src/workflow/merge/__init__.php new file mode 100644 index 00000000..1f04a258 --- /dev/null +++ b/src/workflow/merge/__init__.php @@ -0,0 +1,16 @@ +