From 2fd37a1728bdf8346c3c9ab0e9984b63372c3ed8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 27 Sep 2011 09:37:11 -0700 Subject: [PATCH] Automatically detect when to mark revisions committed Summary: See D945. We have this kludgy "remote_hooks_installed" mess right now, but we have enough information on the server nowadays to figure this out without it. Also reduce code duplication by sharing the "mark-committed" workflow. This causes "arc merge" to effect commit marks. Test Plan: In Git, Mercurial and SVN working copies ran like a million amend/merge/commit/mark-committed commands with and without --finalize in various states of revision completion. This change is really hard to exhaustively test because of the number of combinations of VCS, revision state, command, command flags, repository state and tracking state. All the reasonable tests I could come up with worked correctly, though. Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran Reviewed By: jungejason CC: aran, jungejason Differential Revision: 967 --- .arcconfig | 1 - .../api/subversion/ArcanistSubversionAPI.php | 6 ++ src/workflow/amend/ArcanistAmendWorkflow.php | 29 ++----- .../commit/ArcanistCommitWorkflow.php | 19 ++--- .../ArcanistGitHookPreReceiveWorkflow.php | 13 --- .../ArcanistMarkCommittedWorkflow.php | 81 ++++++++++++++++--- src/workflow/merge/ArcanistMergeWorkflow.php | 11 ++- 7 files changed, 96 insertions(+), 64 deletions(-) diff --git a/.arcconfig b/.arcconfig index cc74a857..10c537f3 100644 --- a/.arcconfig +++ b/.arcconfig @@ -3,7 +3,6 @@ "conduit_uri" : "https://secure.phabricator.com/", "lint_engine" : "PhutilLintEngine", "unit_engine" : "PhutilUnitTestEngine", - "remote_hooks_installed" : true, "copyright_holder" : "Facebook, Inc.", "phutil_libraries" : { "arcanist" : "src/" diff --git a/src/repository/api/subversion/ArcanistSubversionAPI.php b/src/repository/api/subversion/ArcanistSubversionAPI.php index 26b27dfc..522ed172 100644 --- a/src/repository/api/subversion/ArcanistSubversionAPI.php +++ b/src/repository/api/subversion/ArcanistSubversionAPI.php @@ -490,4 +490,10 @@ EODIFF; return false; } + public function getFinalizedRevisionMessage() { + // In other VCSes we give push instructions here, but it never makes sense + // in SVN. + return "Done."; + } + } diff --git a/src/workflow/amend/ArcanistAmendWorkflow.php b/src/workflow/amend/ArcanistAmendWorkflow.php index de5a33db..78749f3c 100644 --- a/src/workflow/amend/ArcanistAmendWorkflow.php +++ b/src/workflow/amend/ArcanistAmendWorkflow.php @@ -126,28 +126,13 @@ EOTEXT $repository_api->amendGitHeadCommit($message); echo "Amended commit message.\n"; - $working_copy = $this->getWorkingCopy(); - $remote_hooks = $working_copy->getConfig('remote_hooks_installed', false); - if (!$remote_hooks) { - echo "According to .arcconfig, remote commit hooks are not installed ". - "for this project, so the revision will be marked committed now. ". - "Consult the documentation for instructions on installing hooks.". - "\n\n"; - $mark_workflow = $this->buildChildWorkflow( - 'mark-committed', - array($revision_id)); - $mark_workflow->run(); - } - - $remote_message = ArcanistDifferentialCommitMessage::newFromRawCorpus( - $message); - $remote_message->pullDataFromConduit($conduit); - if ($remote_message->getFieldValue('reviewedByGUIDs') || - $remote_message->getFieldValue('reviewedByPHIDs')) { - echo phutil_console_wrap( - "You may now push this commit upstream, as appropriate (e.g. with ". - "'git push', or 'git svn dcommit', or by printing and faxing it).\n"); - } + $mark_workflow = $this->buildChildWorkflow( + 'mark-committed', + array( + '--finalize', + $revision_id, + )); + $mark_workflow->run(); } return 0; diff --git a/src/workflow/commit/ArcanistCommitWorkflow.php b/src/workflow/commit/ArcanistCommitWorkflow.php index ab6fb525..27b29aa5 100644 --- a/src/workflow/commit/ArcanistCommitWorkflow.php +++ b/src/workflow/commit/ArcanistCommitWorkflow.php @@ -129,18 +129,13 @@ EOTEXT throw new Exception("Executing 'svn commit' failed!"); } - $working_copy = $this->getWorkingCopy(); - $remote_hooks = $working_copy->getConfig('remote_hooks_installed', false); - if (!$remote_hooks) { - echo "According to .arcconfig, remote commit hooks are not installed ". - "for this project, so the revision will be marked committed now. ". - "Consult the documentation for instructions on installing hooks.". - "\n\n"; - $mark_workflow = $this->buildChildWorkflow( - 'mark-committed', - array($revision_id)); - $mark_workflow->run(); - } + $mark_workflow = $this->buildChildWorkflow( + 'mark-committed', + array( + '--finalize', + $revision_id, + )); + $mark_workflow->run(); return $err; } diff --git a/src/workflow/git-hook-pre-receive/ArcanistGitHookPreReceiveWorkflow.php b/src/workflow/git-hook-pre-receive/ArcanistGitHookPreReceiveWorkflow.php index af648e76..1f36277e 100644 --- a/src/workflow/git-hook-pre-receive/ArcanistGitHookPreReceiveWorkflow.php +++ b/src/workflow/git-hook-pre-receive/ArcanistGitHookPreReceiveWorkflow.php @@ -57,19 +57,6 @@ EOTEXT ".arcconfig."); } - if (!$working_copy->getConfig('remote_hooks_installed')) { - echo phutil_console_wrap( - "\n". - "NOTE: Arcanist is installed as a git pre-receive hook in the git ". - "remote you are pushing to, but the project's '.arcconfig' does not ". - "have the 'remote_hooks_installed' flag set. Until you set the flag, ". - "some code will run needlessly in both the local and remote, and ". - "revisions will be marked 'committed' in Differential when they are ". - "amended rather than when they are actually pushed to the remote ". - "origin.". - "\n\n"); - } - // Git repositories have special rules in pre-receive hooks. We need to // construct the API against the .git directory instead of the project // root or commands don't work properly. diff --git a/src/workflow/mark-committed/ArcanistMarkCommittedWorkflow.php b/src/workflow/mark-committed/ArcanistMarkCommittedWorkflow.php index 8ed7ac83..74c0eee4 100644 --- a/src/workflow/mark-committed/ArcanistMarkCommittedWorkflow.php +++ b/src/workflow/mark-committed/ArcanistMarkCommittedWorkflow.php @@ -28,16 +28,23 @@ class ArcanistMarkCommittedWorkflow extends ArcanistBaseWorkflow { **mark-committed** __revision__ Supports: git, svn Manually mark a revision as committed. You should not normally need - to do this; arc commit (svn), arc amend (git) or commit hooks in the - master remote repository should do it for you. However, if these - mechanisms have failed for some reason you can use this command to - manually change a revision status from "Accepted" to "Committed". + to do this; arc commit (svn), arc amend (git), arc merge (git, hg) or + repository tracking on the master remote repository should do it for + you. However, if these mechanisms have failed for some reason you can + use this command to manually change a revision status from "Accepted" + to "Committed". EOTEXT ); } public function getArguments() { return array( + 'finalize' => array( + 'help' => + "Mark committed only if the repository is untracked and the ". + "revision is accepted. Continue even if the mark can't happen. This ". + "is a soft version of 'mark-committed' used by other workflows.", + ), '*' => 'revision', ); } @@ -50,7 +57,16 @@ EOTEXT return true; } + public function requiresRepositoryAPI() { + // NOTE: Technically we only use this to generate the right message at + // the end, and you can even get the wrong message (e.g., if you run + // "arc mark-committed D123" from a git repository, but D123 is an SVN + // revision). We could be smarter about this, but it's just display fluff. + return true; + } + public function run() { + $is_finalize = $this->getArgument('finalize'); $conduit = $this->getConduit(); @@ -73,6 +89,7 @@ EOTEXT ), )); + $revision = null; try { $revision_id = reset($revision_list); $revision_id = $this->normalizeRevisionID($revision_id); @@ -80,23 +97,61 @@ EOTEXT $revision_data, $revision_id); } catch (ArcanistChooseInvalidRevisionException $ex) { - throw new ArcanistUsageException( - "Revision D{$revision_id} is not committable. You can only mark ". - "revisions which have been 'accepted' as committed."); + if (!$is_finalize) { + throw new ArcanistUsageException( + "Revision D{$revision_id} is not committable. You can only mark ". + "revisions which have been 'accepted' as committed."); + } } - $revision_id = $revision->getID(); - $revision_name = $revision->getName(); + if ($revision) { + $actually_mark = true; + if ($is_finalize) { + $project_info = $conduit->callMethodSynchronous( + 'arcanist.projectinfo', + array( + 'name' => $this->getWorkingCopy()->getProjectID(), + )); + if ($project_info['tracked']) { + $actually_mark = false; + } + } + if ($actually_mark) { + $revision_id = $revision->getID(); + $revision_name = $revision->getName(); - echo "Marking revision D{$revision_id} '{$revision_name}' committed...\n"; + echo "Marking revision D{$revision_id} '{$revision_name}' ". + "committed...\n"; - $conduit->callMethodSynchronous( - 'differential.markcommitted', + $conduit->callMethodSynchronous( + 'differential.markcommitted', + array( + 'revision_id' => $revision_id, + )); + } + } + + $revision_info = $conduit->callMethodSynchronous( + 'differential.getrevision', array( 'revision_id' => $revision_id, )); + $status = $revision_info['statusName']; + if ($status == 'Accepted' || $status == 'Committed') { + // If this has already been attached to commits, don't show the + // "you can push this commit" message since we know it's been committed + // already. + $is_finalized = empty($revision_info['commits']); + } else { + $is_finalized = false; + } - echo "Done.\n"; + if ($is_finalized) { + $message = $this->getRepositoryAPI()->getFinalizedRevisionMessage(); + echo phutil_console_wrap($message)."\n"; + } else { + echo "Done.\n"; + } return 0; } diff --git a/src/workflow/merge/ArcanistMergeWorkflow.php b/src/workflow/merge/ArcanistMergeWorkflow.php index 31cf3d74..881e1fb9 100644 --- a/src/workflow/merge/ArcanistMergeWorkflow.php +++ b/src/workflow/merge/ArcanistMergeWorkflow.php @@ -144,15 +144,20 @@ EOTEXT $repository_api->performLocalBranchMerge($branch, $message); echo "Merged '{$branch}'.\n"; - $done_message = $repository_api->getFinalizedRevisionMessage(); - echo phutil_console_wrap($done_message."\n"); + $mark_workflow = $this->buildChildWorkflow( + 'mark-committed', + array( + '--finalize', + $revision_id, + )); + $mark_workflow->run(); } return 0; } protected function getSupportedRevisionControlSystems() { - return array('git'); + return array('git', 'hg'); } }