Skip to content

Commit

Permalink
Automatically detect when to mark revisions committed
Browse files Browse the repository at this point in the history 
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
epriestley committed Sep 27, 2011
1 parent fe5355e commit 2fd37a1
Show file tree
Hide file tree
Showing 7 changed files with 97 additions and 65 deletions.
1 change: 0 additions & 1 deletion .arcconfig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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/"
Expand Down
6 changes: 6 additions & 0 deletions src/repository/api/subversion/ArcanistSubversionAPI.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.";
}

}
29 changes: 7 additions & 22 deletions src/workflow/amend/ArcanistAmendWorkflow.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
19 changes: 7 additions & 12 deletions src/workflow/commit/ArcanistCommitWorkflow.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
13 changes: 0 additions & 13 deletions src/workflow/git-hook-pre-receive/ArcanistGitHookPreReceiveWorkflow.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
83 changes: 69 additions & 14 deletions src/workflow/mark-committed/ArcanistMarkCommittedWorkflow.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
);
}
Expand All @@ -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();

Expand All @@ -73,30 +89,69 @@ public function run() {
),
));

$revision = null;
try {
$revision_id = reset($revision_list);
$revision_id = $this->normalizeRevisionID($revision_id);
$revision = $this->chooseRevision(
$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;
}
Expand Down
11 changes: 8 additions & 3 deletions src/workflow/merge/ArcanistMergeWorkflow.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}

}

0 comments on commit 2fd37a1

Please sign in to comment.