mirror of
https://we.phorge.it/source/arcanist.git
synced 2024-11-29 10:12:41 +01:00
In arc diff
, always check that you own a revision before trying to do anything with it
Summary: Some users assume they can update anything, not just revisions they own (see https://github.com/facebook/arcanist/issues/54). Currently, if you `arc patch` or `arc amend` and get a commit message, then `arc diff` for a revision you don't own, we: - Fail early with a very confusing message ("You can not review a revision you own!") if you are on the "Reviewers" line, until D3820. - Or fail very very late with a good error message, but after lint, unit and update messages. Instead, check that you own the revision as early as we can. Test Plan: Tried to update revisions I didn't own, got good error messages early on (with D3820). Reviewers: btrahan, vrana Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D3821
This commit is contained in:
parent
6be5cfd104
commit
519e23f3a4
1 changed files with 58 additions and 20 deletions
|
@ -1431,6 +1431,7 @@ EOTEXT
|
||||||
return $this->getCommitMessageFromUser();
|
return $this->getCommitMessageFromUser();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
if (!$is_raw && !$is_create && !$is_update) {
|
if (!$is_raw && !$is_create && !$is_update) {
|
||||||
$repository_api = $this->getRepositoryAPI();
|
$repository_api = $this->getRepositoryAPI();
|
||||||
$revisions = $repository_api->loadWorkingCopyDifferentialRevisions(
|
$revisions = $repository_api->loadWorkingCopyDifferentialRevisions(
|
||||||
|
@ -1699,12 +1700,7 @@ EOTEXT
|
||||||
"Revision '{$revision_id}' does not exist!");
|
"Revision '{$revision_id}' does not exist!");
|
||||||
}
|
}
|
||||||
|
|
||||||
if ($revision['authorPHID'] != $this->getUserPHID()) {
|
$this->checkRevisionOwnership($revision);
|
||||||
$rev_title = $revision['title'];
|
|
||||||
throw new ArcanistUsageException(
|
|
||||||
"You don't own revision D{$id} '{$rev_title}'. You can only update ".
|
|
||||||
"revisions you own.");
|
|
||||||
}
|
|
||||||
|
|
||||||
$message = $this->getConduit()->callMethodSynchronous(
|
$message = $this->getConduit()->callMethodSynchronous(
|
||||||
'differential.getcommitmessage',
|
'differential.getcommitmessage',
|
||||||
|
@ -1726,6 +1722,17 @@ EOTEXT
|
||||||
*/
|
*/
|
||||||
private function validateCommitMessage(
|
private function validateCommitMessage(
|
||||||
ArcanistDifferentialCommitMessage $message) {
|
ArcanistDifferentialCommitMessage $message) {
|
||||||
|
$futures = array();
|
||||||
|
|
||||||
|
$revision_id = $message->getRevisionID();
|
||||||
|
if ($revision_id) {
|
||||||
|
$futures['revision'] = $this->getConduit()->callMethod(
|
||||||
|
'differential.query',
|
||||||
|
array(
|
||||||
|
'ids' => array($revision_id),
|
||||||
|
));
|
||||||
|
}
|
||||||
|
|
||||||
$reviewers = $message->getFieldValue('reviewerPHIDs');
|
$reviewers = $message->getFieldValue('reviewerPHIDs');
|
||||||
if (!$reviewers) {
|
if (!$reviewers) {
|
||||||
$confirm = "You have not specified any reviewers. Continue anyway?";
|
$confirm = "You have not specified any reviewers. Continue anyway?";
|
||||||
|
@ -1733,28 +1740,45 @@ EOTEXT
|
||||||
throw new ArcanistUsageException('Specify reviewers and retry.');
|
throw new ArcanistUsageException('Specify reviewers and retry.');
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
$users = $this->getConduit()->callMethodSynchronous(
|
$futures['reviewers'] = $this->getConduit()->callMethod(
|
||||||
'user.query',
|
'user.query',
|
||||||
array(
|
array(
|
||||||
'phids' => $reviewers,
|
'phids' => $reviewers,
|
||||||
));
|
));
|
||||||
|
}
|
||||||
|
|
||||||
|
foreach (Futures($futures) as $key => $future) {
|
||||||
|
$result = $future->resolve();
|
||||||
|
switch ($key) {
|
||||||
|
case 'revision':
|
||||||
|
if (empty($result)) {
|
||||||
|
throw new ArcanistUsageException(
|
||||||
|
"There is no revision D{$revision_id}.");
|
||||||
|
}
|
||||||
|
$this->checkRevisionOwnership(head($result));
|
||||||
|
break;
|
||||||
|
case 'reviewers':
|
||||||
$untils = array();
|
$untils = array();
|
||||||
foreach ($users as $user) {
|
foreach ($result as $user) {
|
||||||
if (idx($user, 'currentStatus') == 'away') {
|
if (idx($user, 'currentStatus') == 'away') {
|
||||||
$untils[] = $user['currentStatusUntil'];
|
$untils[] = $user['currentStatusUntil'];
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
if (count($untils) == count($reviewers)) {
|
if (count($untils) == count($reviewers)) {
|
||||||
$until = date('l, M j Y', min($untils));
|
$until = date('l, M j Y', min($untils));
|
||||||
$confirm = "All reviewers are away until {$until}. Continue anyway?";
|
$confirm = "All reviewers are away until {$until}. ".
|
||||||
|
"Continue anyway?";
|
||||||
if (!phutil_console_confirm($confirm)) {
|
if (!phutil_console_confirm($confirm)) {
|
||||||
throw new ArcanistUsageException(
|
throw new ArcanistUsageException(
|
||||||
'Specify available reviewers and retry.');
|
'Specify available reviewers and retry.');
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
break;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* @task message
|
* @task message
|
||||||
|
@ -2349,4 +2373,18 @@ EOTEXT
|
||||||
return $event->getValue('fields');
|
return $event->getValue('fields');
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private function checkRevisionOwnership(array $revision) {
|
||||||
|
if ($revision['authorPHID'] == $this->getUserPHID()) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
$id = $revision['id'];
|
||||||
|
$title = $revision['title'];
|
||||||
|
|
||||||
|
throw new ArcanistUsageException(
|
||||||
|
"You don't own revision D{$id} '{$title}'. You can only update ".
|
||||||
|
"revisions you own. You can 'Commandeer' this revision from the web ".
|
||||||
|
"interface if you want to become the owner.");
|
||||||
|
}
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|
Loading…
Reference in a new issue