diff --git a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php index 6595b3734a..f5926a2ebc 100644 --- a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php +++ b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php @@ -114,6 +114,25 @@ final class PhabricatorDifferentialConfigOptions "generated by an automatic process, and thus get hidden by ". "default in differential.")) ->addExample("/config\.h$/\n#/autobuilt/#", pht("Valid Setting")), + $this->newOption('differential.sticky-accept', 'bool', true) + ->setBoolOptions( + array( + pht("Accepts persist across updates"), + pht("Accepts are reset by updates"), + )) + ->setSummary( + pht("Should updating an accepted revision require re-review?")) + ->setDescription( + pht( + 'Normally, when revisions that have been "Accepted" are updated, '. + 'they remain "Accepted". This allows reviewers to suggest minor '. + 'alterations when accepting, and encourages authors to update '. + 'if they make minor changes in response to this feedback.'. + "\n\n". + 'If you want updates to always require re-review, you can disable '. + 'the "stickiness" of the "Accepted" status with this option. '. + 'This may make the process for minor changes much more burdensome '. + 'to both authors and reviewers.')), $this->newOption('differential.allow-self-accept', 'bool', false) ->setBoolOptions( array( diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 5553debde2..87b0146c4b 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -238,10 +238,16 @@ final class DifferentialTransactionEditor $actor_phid = $actor->getPHID(); $type_edge = PhabricatorTransactions::TYPE_EDGE; + $status_plan = ArcanistDifferentialRevisionStatus::CHANGES_PLANNED; + $edge_reviewer = PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER; $edge_ref_task = PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK; + $is_sticky_accept = PhabricatorEnv::getEnvConfig( + 'differential.sticky-accept'); + $downgrade_rejects = false; + $downgrade_accepts = false; if ($this->getIsCloseByCommit()) { // Never downgrade reviewers when we're closing a revision after a // commit. @@ -249,11 +255,23 @@ final class DifferentialTransactionEditor switch ($xaction->getTransactionType()) { case DifferentialTransaction::TYPE_UPDATE: $downgrade_rejects = true; + if (!$is_sticky_accept) { + // If "sticky accept" is disabled, also downgrade the accepts. + $downgrade_accepts = true; + } break; case DifferentialTransaction::TYPE_ACTION: switch ($xaction->getNewValue()) { case DifferentialAction::ACTION_REQUEST: $downgrade_rejects = true; + if ((!$is_sticky_accept) || + ($object->getStatus() != $status_plan)) { + // If the old state isn't "changes planned", downgrade the + // accepts. This exception allows an accepted revision to + // go through Plan Changes -> Request Review to return to + // "accepted" if the author didn't update the revision. + $downgrade_accepts = true; + } break; } break; @@ -265,7 +283,7 @@ final class DifferentialTransactionEditor $old_accept = DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER; $old_reject = DifferentialReviewerStatus::STATUS_REJECTED_OLDER; - if ($downgrade_rejects) { + if ($downgrade_rejects || $downgrade_accepts) { // When a revision is updated, change all "reject" to "rejected older // revision". This means we won't immediately push the update back into // "needs review", but outstanding rejects will still block it from @@ -277,15 +295,25 @@ final class DifferentialTransactionEditor $edits = array(); foreach ($object->getReviewerStatus() as $reviewer) { - if ($reviewer->getStatus() == $new_reject) { - $edits[$reviewer->getReviewerPHID()] = array( - 'data' => array( - 'status' => $old_reject, - ), - ); + if ($downgrade_rejects) { + if ($reviewer->getStatus() == $new_reject) { + $edits[$reviewer->getReviewerPHID()] = array( + 'data' => array( + 'status' => $old_reject, + ), + ); + } } - // TODO: If sticky accept is off, do a similar update for accepts. + if ($downgrade_accepts) { + if ($reviewer->getStatus() == $new_accept) { + $edits[$reviewer->getReviewerPHID()] = array( + 'data' => array( + 'status' => $old_accept, + ), + ); + } + } } if ($edits) { diff --git a/src/docs/user/userguide/differential_faq.diviner b/src/docs/user/userguide/differential_faq.diviner index c2c38e589a..aea49c4ce6 100644 --- a/src/docs/user/userguide/differential_faq.diviner +++ b/src/docs/user/userguide/differential_faq.diviner @@ -5,22 +5,25 @@ Common questions about Differential. = Why does an "accepted" revision remain accepted when it is updated? = -When a revision author updates an "accepted" revision in Differential, the -state remains "accepted". This can be confusing if you expect the revision to -change to "needs review" when it is updated. +You can configure this behavior with `differential.sticky-accept`. -This behavior is intentional, to encourage authors to update revisions when they -make minor changes after a revision is accepted. For example, a reviewer may -accept a change with a comment like this: +When a revision author updates an "Accepted" revision in Differential, the +state remains "Accepted". This can be confusing if you expect the revision to +change to "Needs Review" when it is updated. + +Although this behavior is configurable, we think stickiness is a good behavior: +stickiness encourage authors to update revisions when they make minor changes +after a revision is accepted. For example, a reviewer may accept a change with a +comment like this: > Looks great, but can you add some documentation for the foo() function > before you land it? I also caught a couple typos, see inlines. -If updating the revision reverted the status to "needs review", the author +If updating the revision reverted the status to "Needs Review", the author is discouraged from updating the revision when they make minor changes because they'll have to wait for their reviewer to have a chance to look at it again. -Instead, the "accept" state is sticky to encourage them to update the revision +Instead, the "Accepted" state is sticky to encourage them to update the revision with a comment like: > - Added docs. @@ -41,10 +44,12 @@ get an accept), solve the problem socially by telling them to stop being jerks. Unless you've configured additional layers of enforcement, there's nothing stopping them from silently changing the code before pushing it, anyway. + = How can I enable syntax highlighting? = You need to install and configure **Pygments** to highlight anything else than -PHP. Consult the configuration file for instructions. +PHP. See the `pygments.enabled` configuration setting. + = What do the whitespace options mean? = @@ -61,10 +66,15 @@ Most of these are pretty straightforward, but "Ignore Most" is not: setting. - **Ignore All**: Ignore all whitespace changes in all files. -= What does the very light green and red backgrounds mean? = -Differential uses these colors to mark changes coming from rebase - they are += What do the very light green and red backgrounds mean? = + +Differential uses these colors to mark changes coming from rebase: they are part of the diff but they were not added or removed by the author. They can -appear in diff of diffs against different bases. See -[[ https://secure.phabricator.com/D3324?vs=6468&id=6513#toc | D3324 ]] for -example. +appear in diff of diffs against different bases. + += Next Steps = + +Continue by: + + - returning to the @{article:Differential User Guide}.