Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

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
  • Loading branch information...
commit 2fd37a1728bdf8346c3c9ab0e9984b63372c3ed8 1 parent fe5355e
@epriestley epriestley authored
View
1  .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/"
View
6 src/repository/api/subversion/ArcanistSubversionAPI.php
@@ -490,4 +490,10 @@ public function supportsLocalBranchMerge() {
return false;
}
+ public function getFinalizedRevisionMessage() {
+ // In other VCSes we give push instructions here, but it never makes sense
+ // in SVN.
+ return "Done.";
+ }
+
}
View
29 src/workflow/amend/ArcanistAmendWorkflow.php
@@ -126,28 +126,13 @@ public function run() {
$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;
View
19 src/workflow/commit/ArcanistCommitWorkflow.php
@@ -129,18 +129,13 @@ public function run() {
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;
}
View
13 src/workflow/git-hook-pre-receive/ArcanistGitHookPreReceiveWorkflow.php
@@ -57,19 +57,6 @@ public function run() {
".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.
View
83 src/workflow/mark-committed/ArcanistMarkCommittedWorkflow.php
@@ -28,16 +28,23 @@ public function getCommandHelp() {
**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 @@ public function requiresAuthentication() {
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 @@ public function run() {
),
));
+ $revision = null;
try {
$revision_id = reset($revision_list);
$revision_id = $this->normalizeRevisionID($revision_id);
@@ -80,23 +97,61 @@ public function run() {
$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();
-
- echo "Marking revision D{$revision_id} '{$revision_name}' committed...\n";
+ 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";
+
+ $conduit->callMethodSynchronous(
+ 'differential.markcommitted',
+ array(
+ 'revision_id' => $revision_id,
+ ));
+ }
+ }
- $conduit->callMethodSynchronous(
- 'differential.markcommitted',
+ $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;
}
View
11 src/workflow/merge/ArcanistMergeWorkflow.php
@@ -144,15 +144,20 @@ public function run() {
$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');
}
}
Please sign in to comment.
Something went wrong with that request. Please try again.