From ae7488f71005de6d33e748ac0c46fb2ab9d9877a Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 15 Aug 2011 21:06:58 -0700 Subject: [PATCH] Drive commit message rendering from field specifications Summary: When rendering commit messages, drive all the logic through field specification classes instead of the hard-coded DifferentialCommitMessageData class. This removes DifferentialCommitMessageData and support classes. Note that this effectively reverts D546, and will cause a minor break for Facebook (Task IDs will no longer render in commit messages generated by "arc amend", and will not be editable via "arc diff --edit"). This can be resolved by implementing the feature as a custom field. While I've been able to preserve the task ID functionality elsewhere, I felt this implementation was too complex to reasonably leave hooks for, and the break is pretty minor. Test Plan: - Made numerous calls to differential.getcommitmessage across many diffs in various states, with and without 'edit' and with and without various field overrides. - General behavior seems correct (messages look accurate, and have the expected information). Special fields like "Reviewed By" and "git-svn-id" seem to work correctly. - Edit behavior seems correct (edit mode shows all editable fields, hides fields like "Reviewed By"). - Field overwrite behavior seems correct (overwritable fields show the correct values when overwritten, ignore provided values otherwise). Reviewed By: jungejason Reviewers: jungejason, tuomaspelkonen, aran CC: aran, jungejason Differential Revision: 814 --- conf/default.conf.php | 6 - src/__phutil_library_map__.php | 3 - ...I_differential_getcommitmessage_Method.php | 114 ++++++---- .../getcommitmessage/__init__.php | 3 +- .../DifferentialCommitMessageData.php | 196 ------------------ .../DifferentialCommitMessageField.php | 49 ----- .../DifferentialCommitMessageModifier.php | 39 ---- .../data/commitmessage/__init__.php | 19 -- .../revision/DifferentialRevisionEditor.php | 1 + .../base/DifferentialFieldSpecification.php | 89 ++++++++ ...rentialBlameRevisionFieldSpecification.php | 12 ++ .../ccs/DifferentialCCsFieldSpecification.php | 25 ++- ...DifferentialGitSVNIDFieldSpecification.php | 12 ++ ...fferentialRevertPlanFieldSpecification.php | 12 ++ ...fferentialReviewedByFieldSpecification.php | 54 +++++ .../specification/reviewedby/__init__.php | 2 + ...ifferentialReviewersFieldSpecification.php | 26 ++- ...fferentialRevisionIDFieldSpecification.php | 12 ++ .../DifferentialSummaryFieldSpecification.php | 17 +- ...DifferentialTestPlanFieldSpecification.php | 17 +- .../DifferentialTitleFieldSpecification.php | 17 +- 21 files changed, 365 insertions(+), 360 deletions(-) delete mode 100644 src/applications/differential/data/commitmessage/DifferentialCommitMessageData.php delete mode 100644 src/applications/differential/data/commitmessage/DifferentialCommitMessageField.php delete mode 100644 src/applications/differential/data/commitmessage/DifferentialCommitMessageModifier.php delete mode 100644 src/applications/differential/data/commitmessage/__init__.php diff --git a/conf/default.conf.php b/conf/default.conf.php index 9d79cbd2be..7578ee2e40 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -476,18 +476,12 @@ return array( '/.*/' => 80, ), - // Class for appending custom fields to be included in the commit - // messages generated by "arc amend". Should inherit - // DifferentialCommitMessageModifier - 'differential.modify-commit-message-class' => null, - // List of file regexps were whitespace is meaningful and should not // use 'ignore-all' by default 'differential.whitespace-matters' => array( '/\.py$/', ), - 'differential.field-selector' => 'DifferentialDefaultFieldSelector', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 3e2e54abe6..3647f3116f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -154,9 +154,6 @@ phutil_register_library_map(array( 'DifferentialCommentPreviewController' => 'applications/differential/controller/commentpreview', 'DifferentialCommentSaveController' => 'applications/differential/controller/commentsave', 'DifferentialCommitMessage' => 'applications/differential/parser/commitmessage', - 'DifferentialCommitMessageData' => 'applications/differential/data/commitmessage', - 'DifferentialCommitMessageField' => 'applications/differential/data/commitmessage', - 'DifferentialCommitMessageModifier' => 'applications/differential/data/commitmessage', 'DifferentialCommitMessageParserException' => 'applications/differential/parser/commitmessage/exception', 'DifferentialCommitsFieldSpecification' => 'applications/differential/field/specification/commits', 'DifferentialController' => 'applications/differential/controller/base', diff --git a/src/applications/conduit/method/differential/getcommitmessage/ConduitAPI_differential_getcommitmessage_Method.php b/src/applications/conduit/method/differential/getcommitmessage/ConduitAPI_differential_getcommitmessage_Method.php index c24d901635..f2685128cb 100644 --- a/src/applications/conduit/method/differential/getcommitmessage/ConduitAPI_differential_getcommitmessage_Method.php +++ b/src/applications/conduit/method/differential/getcommitmessage/ConduitAPI_differential_getcommitmessage_Method.php @@ -51,57 +51,87 @@ class ConduitAPI_differential_getcommitmessage_Method extends ConduitAPIMethod { throw new ConduitException('ERR_NOT_FOUND'); } - $edit = $request->getValue('edit'); - $mode = $edit - ? DifferentialCommitMessageData::MODE_EDIT - : DifferentialCommitMessageData::MODE_AMEND; + $revision->loadRelationships(); - $message_data = new DifferentialCommitMessageData($revision, $mode); - $message_data->prepare(); + $is_edit = $request->getValue('edit'); - if ($mode == DifferentialCommitMessageData::MODE_EDIT) { + $aux_fields = DifferentialFieldSelector::newSelector() + ->getFieldSpecifications(); + + foreach ($aux_fields as $key => $aux_field) { + $aux_field->setRevision($revision); + if (!$aux_field->shouldAppearOnCommitMessage()) { + unset($aux_fields[$key]); + } + } + + $aux_fields = DifferentialAuxiliaryField::loadFromStorage( + $revision, + $aux_fields); + $aux_fields = mpull($aux_fields, null, 'getCommitMessageKey'); + + if ($is_edit) { $fields = $request->getValue('fields'); - if ($fields) { + foreach ($fields as $field => $value) { - static $simple_fields = array( - 'title' => 'Title', - 'summary' => 'Summary', - 'testPlan' => 'Test Plan', - 'blameRevision' => 'Blame Revision', - 'revertPlan' => 'Revert Plan', - 'tasks' => 'Tasks', - ); - - foreach ($fields as $field => $value) { - if (isset($simple_fields[$field])) { - $message_data->setField($simple_fields[$field], $value); - } else { - $overwrite = true; - static $overwrite_map = array( - 'reviewerPHIDs' => 'Reviewers', - 'ccPHIDs' => 'CC', - 'taskPHIDs' => 'Tasks', - ); - switch ($field) { - case 'reviewerPHIDs': - case 'ccPHIDs': - $handles = id(new PhabricatorObjectHandleData($value)) - ->loadHandles($handles); - $value = implode(', ', mpull($handles, 'getName')); - break; - default: - $overwrite = false; - break; - } - if ($overwrite) { - $message_data->setField($overwrite_map[$field], $value); - } + $aux_field = idx($aux_fields, $field); + if (!$aux_field) { + if ($field == 'tasks') { + // TODO: Remove, backcompat for Facebook. + continue; } + throw new Exception( + "Commit message includes field '{$field}' which does not ". + "correspond to any configured field."); + } + + if ($aux_field->shouldOverwriteWhenCommitMessageIsEdited()) { + $aux_field->setValueFromParsedCommitMessage($value); } } } - $commit_message = $message_data->getCommitMessage(); + + $aux_phids = array(); + foreach ($aux_fields as $field_key => $field) { + $aux_phids[$field_key] = $field->getRequiredHandlePHIDsForCommitMessage(); + } + $phids = array_unique(array_mergev($aux_phids)); + $handles = id(new PhabricatorObjectHandleData($phids))->loadHandles(); + foreach ($aux_fields as $field_key => $field) { + $field->setHandles(array_select_keys($handles, $aux_phids[$field_key])); + } + + + $commit_message = array(); + foreach ($aux_fields as $field_key => $field) { + $value = $field->renderValueForCommitMessage($is_edit); + $label = $field->renderLabelForCommitMessage(); + if ($value === null || !strlen($value)) { + if ($field_key === 'title') { + $commit_message[] = '<>'; + } else { + if ($field->shouldAppearOnCommitMessageTemplate() && $is_edit) { + $commit_message[] = $label.': '; + } + } + } else { + if ($field_key === 'title') { + $commit_message[] = $value; + } else { + $value = str_replace( + array("\r\n", "\r"), + array("\n", "\n"), + $value); + if (strpos($value, "\n") !== false) { + $commit_message[] = "{$label}:\n{$value}"; + } else { + $commit_message[] = "{$label}: {$value}"; + } + } + } + } + $commit_message = implode("\n\n", $commit_message); return wordwrap($commit_message, 80); } diff --git a/src/applications/conduit/method/differential/getcommitmessage/__init__.php b/src/applications/conduit/method/differential/getcommitmessage/__init__.php index 2de8f9d9d0..85c8391593 100644 --- a/src/applications/conduit/method/differential/getcommitmessage/__init__.php +++ b/src/applications/conduit/method/differential/getcommitmessage/__init__.php @@ -8,7 +8,8 @@ phutil_require_module('phabricator', 'applications/conduit/method/base'); phutil_require_module('phabricator', 'applications/conduit/protocol/exception'); -phutil_require_module('phabricator', 'applications/differential/data/commitmessage'); +phutil_require_module('phabricator', 'applications/differential/field/selector/base'); +phutil_require_module('phabricator', 'applications/differential/storage/auxiliaryfield'); phutil_require_module('phabricator', 'applications/differential/storage/revision'); phutil_require_module('phabricator', 'applications/phid/handle/data'); diff --git a/src/applications/differential/data/commitmessage/DifferentialCommitMessageData.php b/src/applications/differential/data/commitmessage/DifferentialCommitMessageData.php deleted file mode 100644 index 46c1a97f08..0000000000 --- a/src/applications/differential/data/commitmessage/DifferentialCommitMessageData.php +++ /dev/null @@ -1,196 +0,0 @@ -revision = $revision; - $this->mode = $mode; - $comments = id(new DifferentialComment())->loadAllWhere( - 'revisionID = %d', - $revision->getID()); - $this->comments = $comments; - } - - protected function getCommenters() { - $revision = $this->revision; - - $map = array(); - foreach ($this->comments as $comment) { - $map[$comment->getAuthorPHID()] = true; - } - - unset($map[$revision->getAuthorPHID()]); - if ($this->getReviewer()) { - unset($map[$this->getReviewer()]); - } - - return array_keys($map); - } - - private function getReviewer() { - $reviewer = null; - foreach ($this->comments as $comment) { - if ($comment->getAction() == DifferentialAction::ACTION_ACCEPT) { - $reviewer = $comment->getAuthorPHID(); - } else if ($comment->getAction() == DifferentialAction::ACTION_REJECT) { - $reviewer = null; - } - } - return $reviewer; - } - - public function prepare() { - $revision = $this->revision; - - if ($revision->getSummary()) { - $this->setField('Summary', $revision->getSummary()); - } - - $this->setField('Test Plan', $revision->getTestPlan()); - - $reviewer = null; - $commenters = array(); - $revision->loadRelationships(); - - if ($this->mode == self::MODE_AMEND) { - $reviewer = $this->getReviewer(); - $commenters = $this->getCommenters(); - } - - $reviewers = $revision->getReviewers(); - $ccphids = $revision->getCCPHIDs(); - - $phids = array_merge($ccphids, $commenters, $reviewers); - if ($reviewer) { - $phids[] = $reviewer; - } - - $handles = id(new PhabricatorObjectHandleData($phids)) - ->loadHandles(); - - if ($this->mode == self::MODE_AMEND) { - if ($reviewer) { - $this->setField('Reviewed By', $handles[$reviewer]->getName()); - } - } - - if ($reviewers) { - $reviewer_names = array(); - foreach ($reviewers as $uid) { - $reviewer_names[] = $handles[$uid]->getName(); - } - $reviewer_names = implode(', ', $reviewer_names); - $this->setField('Reviewers', $reviewer_names); - } - - $user_handles = array_select_keys($handles, $commenters); - if ($user_handles) { - $commenters = implode(', ', mpull($user_handles, 'getName')); - $this->setField('Commenters', $commenters); - } - - $cc_handles = array_select_keys($handles, $ccphids); - if ($cc_handles) { - $cc = implode(', ', mpull($cc_handles, 'getName')); - $this->setField('CC', $cc); - } - - if ($revision->getRevertPlan()) { - $this->setField('Revert Plan', $revision->getRevertPlan()); - } - - if ($revision->getBlameRevision()) { - $this->setField('Blame Revision', $revision->getBlameRevision()); - } - - if ($this->mode == self::MODE_EDIT) { - // In edit mode, include blank fields. - $blank_fields = array('Summary', 'Reviewers', 'CC', 'Revert Plan', - 'Blame Revision'); - foreach ($blank_fields as $blank_field) { - if (!$this->getField($blank_field)) { - $this->setField($blank_field, ''); - } - } - } - - $this->setField('Title', $revision->getTitle()); - $this->setField('Differential Revision', $revision->getID()); - - // append custom commit message fields - $modify_class = PhabricatorEnv::getEnvConfig( - 'differential.modify-commit-message-class'); - - if ($modify_class) { - $modifier = newv($modify_class, array($revision)); - $this->fields = $modifier->modifyFields($this->fields); - } - } - - public function setField($name, $value) { - $field = $this->getField($name); - if ($field) { - $field->setValue($value); - } else { - $this->fields[] = new DifferentialCommitMessageField($name, $value); - } - return $this; - } - - private function getField($name) { - foreach ($this->fields as $field) { - if ($field->getName() == $name) { - return $field; - } - } - return null; - } - - public function getCommitMessage() { - $fields = $this->fields; - - $message = array(); - - $title = $this->getField('Title'); - $message[] = $title->getValue() . "\n"; - - foreach ($fields as $field) { - if ($field->getName() != 'Title') { - $message[] = $field->render(); - } - } - - $message = implode("\n", $message); - $message = str_replace( - array("\r\n", "\r"), - array("\n", "\n"), - $message); - $message = $message."\n"; - - return $message; - } -} diff --git a/src/applications/differential/data/commitmessage/DifferentialCommitMessageField.php b/src/applications/differential/data/commitmessage/DifferentialCommitMessageField.php deleted file mode 100644 index b70d5ec03a..0000000000 --- a/src/applications/differential/data/commitmessage/DifferentialCommitMessageField.php +++ /dev/null @@ -1,49 +0,0 @@ -name = $name; - $this->value = $value; - } - - public function getName() { - return $this->name; - } - - public function getValue() { - return $this->value; - } - - public function setValue($value) { - $this->value = trim($value); - } - - public function render() { - $str = "{$this->name}:\n{$this->value}\n"; - if (!strpos($this->value, "\n")) { - $str = "{$this->name}: {$this->value}"; - } - return $str; - } -} diff --git a/src/applications/differential/data/commitmessage/DifferentialCommitMessageModifier.php b/src/applications/differential/data/commitmessage/DifferentialCommitMessageModifier.php deleted file mode 100644 index 58a65cd368..0000000000 --- a/src/applications/differential/data/commitmessage/DifferentialCommitMessageModifier.php +++ /dev/null @@ -1,39 +0,0 @@ -revision = $revision; - } - - /** - * Implementation of this function should remove, modify, or append - * fields to the $fields representing the fields for the given - * $revision. It should return the modified dict. These fields are - * included in the commit message generated by 'arc amend'. - * - * @param array The array of fields to modify - * @return array The updated array of fields - */ - abstract public function modifyFields(array $fields); -} diff --git a/src/applications/differential/data/commitmessage/__init__.php b/src/applications/differential/data/commitmessage/__init__.php deleted file mode 100644 index c6ce1f81a8..0000000000 --- a/src/applications/differential/data/commitmessage/__init__.php +++ /dev/null @@ -1,19 +0,0 @@ -revision; + $revision->loadRelationships(); $aux_fields = DifferentialFieldSelector::newSelector() ->getFieldSpecifications(); diff --git a/src/applications/differential/field/specification/base/DifferentialFieldSpecification.php b/src/applications/differential/field/specification/base/DifferentialFieldSpecification.php index 96b5103000..bf2acb8faf 100644 --- a/src/applications/differential/field/specification/base/DifferentialFieldSpecification.php +++ b/src/applications/differential/field/specification/base/DifferentialFieldSpecification.php @@ -348,6 +348,71 @@ abstract class DifferentialFieldSpecification { throw new DifferentialFieldSpecificationIncompleteException($this); } + /** + * In revision control systems which read revision information from the + * working copy, the user may edit the commit message outside of invoking + * "arc diff --edit". When they do this, only some fields (those fields which + * can not be edited by other users) are safe to overwrite. For instance, it + * is fine to overwrite "Summary" because no one else can edit it, but not + * to overwrite "Reviewers" because reviewers may have been added or removed + * via the web interface. + * + * If a field is safe to overwrite when edited in a working copy commit + * message, return true. If the authoritative value should always be used, + * return false. By default, fields can not be overwritten. + * + * @return bool True to indicate the field is save to overwrite. + * @task commit + */ + public function shouldOverwriteWhenCommitMessageIsEdited() { + return false; + } + + /** + * Return true if this field should be suggested to the user during + * "arc diff --edit". Basicially, return true if the field is something the + * user might want to fill out (like "Summary"), and false if it's a + * system/display/readonly field (like "Differential Revision"). If this + * method returns true, the field will be rendered even if it has no value + * during edit and update operations. + * + * @return bool True to indicate the field should appear in the edit template. + * @task commit + */ + public function shouldAppearOnCommitMessageTemplate() { + return true; + } + + /** + * Render a human-readable label for this field, like "Summary" or + * "Test Plan". This is distinct from the commit message key, but generally + * they should be similar. + * + * @return string Human-readable field label for commit messages. + * @task commit + */ + public function renderLabelForCommitMessage() { + throw new DifferentialFieldSpecificationIncompleteException($this); + } + + /** + * Render a human-readable value for this field when it appears in commit + * messages (for instance, lists of users should be rendered as user names). + * + * The ##$is_edit## parameter allows you to distinguish between commit + * messages being rendered for editing and those being rendered for amending + * or commit. Some fields may decline to render a value in one mode (for + * example, "Reviewed By" appears only when doing commit/amend, not while + * editing). + * + * @param bool True if the message is being edited. + * @return string Human-readable field value. + * @task commit + */ + public function renderValueForCommitMessage($is_edit) { + throw new DifferentialFieldSpecificationIncompleteException($this); + } + /* -( Loading Additional Data )-------------------------------------------- */ @@ -402,6 +467,21 @@ abstract class DifferentialFieldSpecification { return $this->getRequiredHandlePHIDs(); } + /** + * Specify which @{class:PhabricatorObjectHandles} need to be loaded for your + * field to render correctly on the commit message interface. + * + * This is a more specific version of @{method:getRequiredHandlePHIDs} which + * can be overridden to improve field performance by loading only data you + * need. + * + * @return list List of PHIDs to load handles for. + * @task load + */ + public function getRequiredHandlePHIDsForCommitMessage() { + return $this->getRequiredHandlePHIDs(); + } + /** * Specify which diff properties this field needs to load. @@ -422,9 +502,18 @@ abstract class DifferentialFieldSpecification { */ final public function setRevision(DifferentialRevision $revision) { $this->revision = $revision; + $this->didSetRevision(); return $this; } + /** + * @task context + */ + protected function didSetRevision() { + return; + } + + /** * @task context */ diff --git a/src/applications/differential/field/specification/blamerev/DifferentialBlameRevisionFieldSpecification.php b/src/applications/differential/field/specification/blamerev/DifferentialBlameRevisionFieldSpecification.php index c058590cee..5fdcea13be 100644 --- a/src/applications/differential/field/specification/blamerev/DifferentialBlameRevisionFieldSpecification.php +++ b/src/applications/differential/field/specification/blamerev/DifferentialBlameRevisionFieldSpecification.php @@ -91,4 +91,16 @@ final class DifferentialBlameRevisionFieldSpecification return $this; } + public function shouldOverwriteWhenCommitMessageIsEdited() { + return true; + } + + public function renderLabelForCommitMessage() { + return 'Blame Revision'; + } + + public function renderValueForCommitMessage($is_edit) { + return $this->value; + } + } diff --git a/src/applications/differential/field/specification/ccs/DifferentialCCsFieldSpecification.php b/src/applications/differential/field/specification/ccs/DifferentialCCsFieldSpecification.php index 701fb47d12..74107b02a0 100644 --- a/src/applications/differential/field/specification/ccs/DifferentialCCsFieldSpecification.php +++ b/src/applications/differential/field/specification/ccs/DifferentialCCsFieldSpecification.php @@ -53,14 +53,21 @@ final class DifferentialCCsFieldSpecification } public function shouldAppearOnEdit() { - $this->ccs = $this->getCCPHIDs(); return true; } + protected function didSetRevision() { + $this->ccs = $this->getCCPHIDs(); + } + public function getRequiredHandlePHIDsForRevisionEdit() { return $this->ccs; } + public function getRequiredHandlePHIDsForCommitMessage() { + return $this->ccs; + } + public function setValueFromRequest(AphrontRequest $request) { $this->ccs = $request->getArr('cc'); return $this; @@ -95,4 +102,20 @@ final class DifferentialCCsFieldSpecification return $this; } + public function renderLabelForCommitMessage() { + return 'CC'; + } + + public function renderValueForCommitMessage($is_edit) { + if (!$this->ccs) { + return null; + } + + $names = array(); + foreach ($this->ccs as $phid) { + $names[] = $this->getHandle($phid)->getName(); + } + return implode(', ', $names); + } + } diff --git a/src/applications/differential/field/specification/gitsvnid/DifferentialGitSVNIDFieldSpecification.php b/src/applications/differential/field/specification/gitsvnid/DifferentialGitSVNIDFieldSpecification.php index c908946303..1fcff06fbf 100644 --- a/src/applications/differential/field/specification/gitsvnid/DifferentialGitSVNIDFieldSpecification.php +++ b/src/applications/differential/field/specification/gitsvnid/DifferentialGitSVNIDFieldSpecification.php @@ -25,6 +25,10 @@ final class DifferentialGitSVNIDFieldSpecification return true; } + public function shouldAppearOnCommitMessageTemplate() { + return false; + } + public function getCommitMessageKey() { return 'gitSVNID'; } @@ -34,4 +38,12 @@ final class DifferentialGitSVNIDFieldSpecification return $this; } + public function renderLabelForCommitMessage() { + return 'git-svn-id'; + } + + public function renderValueForCommitMessage($is_edit) { + return null; + } + } diff --git a/src/applications/differential/field/specification/revertplan/DifferentialRevertPlanFieldSpecification.php b/src/applications/differential/field/specification/revertplan/DifferentialRevertPlanFieldSpecification.php index e84950363f..790758e4a0 100644 --- a/src/applications/differential/field/specification/revertplan/DifferentialRevertPlanFieldSpecification.php +++ b/src/applications/differential/field/specification/revertplan/DifferentialRevertPlanFieldSpecification.php @@ -91,4 +91,16 @@ final class DifferentialRevertPlanFieldSpecification return $this; } + public function shouldOverwriteWhenCommitMessageIsEdited() { + return true; + } + + public function renderLabelForCommitMessage() { + return 'Revert Plan'; + } + + public function renderValueForCommitMessage($is_edit) { + return $this->value; + } + } diff --git a/src/applications/differential/field/specification/reviewedby/DifferentialReviewedByFieldSpecification.php b/src/applications/differential/field/specification/reviewedby/DifferentialReviewedByFieldSpecification.php index f5904e2ee2..78158e4d57 100644 --- a/src/applications/differential/field/specification/reviewedby/DifferentialReviewedByFieldSpecification.php +++ b/src/applications/differential/field/specification/reviewedby/DifferentialReviewedByFieldSpecification.php @@ -21,6 +21,32 @@ final class DifferentialReviewedByFieldSpecification private $reviewedBy; + protected function didSetRevision() { + $this->reviewedBy = array(); + $revision = $this->getRevision(); + + $status = $revision->getStatus(); + if ($status == DifferentialRevisionStatus::ACCEPTED || + $status == DifferentialRevisionStatus::COMMITTED) { + $reviewer = null; + $comments = $revision->loadComments(); + foreach ($comments as $comment) { + $action = $comment->getAction(); + if ($action == DifferentialAction::ACTION_ACCEPT) { + $reviewer = $comment->getAuthorPHID(); + } else if ($action == DifferentialAction::ACTION_REJECT || + $action == DifferentialAction::ACTION_ABANDON || + $action == DifferentialAction::ACTION_RETHINK) { + $reviewer = null; + } + } + + if ($reviewer) { + $this->reviewedBy = array($reviewer); + } + } + } + public function shouldAppearOnCommitMessage() { return true; } @@ -34,5 +60,33 @@ final class DifferentialReviewedByFieldSpecification return $this; } + public function shouldAppearOnCommitMessageTemplate() { + return false; + } + + public function renderLabelForCommitMessage() { + return 'Reviewed By'; + } + + public function getRequiredHandlePHIDsForCommitMessage() { + return $this->reviewedBy; + } + + public function renderValueForCommitMessage($is_edit) { + if ($is_edit) { + return null; + } + + if (!$this->reviewedBy) { + return null; + } + + $names = array(); + foreach ($this->reviewedBy as $phid) { + $names[] = $this->getHandle($phid)->getName(); + } + + return implode(', ', $names); + } } diff --git a/src/applications/differential/field/specification/reviewedby/__init__.php b/src/applications/differential/field/specification/reviewedby/__init__.php index a87a459dbd..5240ff8da0 100644 --- a/src/applications/differential/field/specification/reviewedby/__init__.php +++ b/src/applications/differential/field/specification/reviewedby/__init__.php @@ -6,6 +6,8 @@ +phutil_require_module('phabricator', 'applications/differential/constants/action'); +phutil_require_module('phabricator', 'applications/differential/constants/revisionstatus'); phutil_require_module('phabricator', 'applications/differential/field/specification/base'); diff --git a/src/applications/differential/field/specification/reviewers/DifferentialReviewersFieldSpecification.php b/src/applications/differential/field/specification/reviewers/DifferentialReviewersFieldSpecification.php index 9450052e3e..f609494eb5 100644 --- a/src/applications/differential/field/specification/reviewers/DifferentialReviewersFieldSpecification.php +++ b/src/applications/differential/field/specification/reviewers/DifferentialReviewersFieldSpecification.php @@ -54,10 +54,13 @@ final class DifferentialReviewersFieldSpecification } public function shouldAppearOnEdit() { - $this->reviewers = $this->getReviewerPHIDs(); return true; } + protected function didSetRevision() { + $this->reviewers = $this->getReviewerPHIDs(); + } + public function getRequiredHandlePHIDsForRevisionEdit() { return $this->reviewers; } @@ -105,4 +108,25 @@ final class DifferentialReviewersFieldSpecification return $this; } + public function renderLabelForCommitMessage() { + return 'Reviewers'; + } + + public function getRequiredHandlePHIDsForCommitMessage() { + return $this->reviewers; + } + + public function renderValueForCommitMessage($is_edit) { + if (!$this->reviewers) { + return null; + } + + $names = array(); + foreach ($this->reviewers as $phid) { + $names[] = $this->getHandle($phid)->getName(); + } + + return implode(', ', $names); + } + } diff --git a/src/applications/differential/field/specification/revisionid/DifferentialRevisionIDFieldSpecification.php b/src/applications/differential/field/specification/revisionid/DifferentialRevisionIDFieldSpecification.php index e1f1cf30b3..02eade8f06 100644 --- a/src/applications/differential/field/specification/revisionid/DifferentialRevisionIDFieldSpecification.php +++ b/src/applications/differential/field/specification/revisionid/DifferentialRevisionIDFieldSpecification.php @@ -21,6 +21,10 @@ final class DifferentialRevisionIDFieldSpecification private $id; + protected function didSetRevision() { + $this->id = $this->getRevision()->getID(); + } + public function shouldAppearOnCommitMessage() { return true; } @@ -34,5 +38,13 @@ final class DifferentialRevisionIDFieldSpecification return $this; } + public function renderLabelForCommitMessage() { + return 'Differential Revision'; + } + + public function renderValueForCommitMessage($is_edit) { + return $this->id; + } + } diff --git a/src/applications/differential/field/specification/summary/DifferentialSummaryFieldSpecification.php b/src/applications/differential/field/specification/summary/DifferentialSummaryFieldSpecification.php index 46209c907f..a7e5ad50ed 100644 --- a/src/applications/differential/field/specification/summary/DifferentialSummaryFieldSpecification.php +++ b/src/applications/differential/field/specification/summary/DifferentialSummaryFieldSpecification.php @@ -22,10 +22,13 @@ final class DifferentialSummaryFieldSpecification private $summary = ''; public function shouldAppearOnEdit() { - $this->summary = $this->getRevision()->getSummary(); return true; } + protected function didSetRevision() { + $this->summary = $this->getRevision()->getSummary(); + } + public function setValueFromRequest(AphrontRequest $request) { $this->summary = $request->getStr('summary'); return $this; @@ -55,4 +58,16 @@ final class DifferentialSummaryFieldSpecification return $this; } + public function shouldOverwriteWhenCommitMessageIsEdited() { + return true; + } + + public function renderLabelForCommitMessage() { + return 'Summary'; + } + + public function renderValueForCommitMessage($is_edit) { + return $this->summary; + } + } diff --git a/src/applications/differential/field/specification/testplan/DifferentialTestPlanFieldSpecification.php b/src/applications/differential/field/specification/testplan/DifferentialTestPlanFieldSpecification.php index 8eed5dc248..14b46a7a9c 100644 --- a/src/applications/differential/field/specification/testplan/DifferentialTestPlanFieldSpecification.php +++ b/src/applications/differential/field/specification/testplan/DifferentialTestPlanFieldSpecification.php @@ -23,10 +23,13 @@ final class DifferentialTestPlanFieldSpecification private $error = true; public function shouldAppearOnEdit() { - $this->plan = $this->getRevision()->getTestPlan(); return true; } + protected function didSetRevision() { + $this->plan = $this->getRevision()->getTestPlan(); + } + public function setValueFromRequest(AphrontRequest $request) { $this->plan = $request->getStr('testplan'); $this->error = null; @@ -66,5 +69,17 @@ final class DifferentialTestPlanFieldSpecification return $this; } + public function shouldOverwriteWhenCommitMessageIsEdited() { + return true; + } + + public function renderLabelForCommitMessage() { + return 'Test Plan'; + } + + public function renderValueForCommitMessage($is_edit) { + return $this->plan; + } + } diff --git a/src/applications/differential/field/specification/title/DifferentialTitleFieldSpecification.php b/src/applications/differential/field/specification/title/DifferentialTitleFieldSpecification.php index 7b6bb94dc2..b4ba293935 100644 --- a/src/applications/differential/field/specification/title/DifferentialTitleFieldSpecification.php +++ b/src/applications/differential/field/specification/title/DifferentialTitleFieldSpecification.php @@ -23,10 +23,13 @@ final class DifferentialTitleFieldSpecification private $error = true; public function shouldAppearOnEdit() { - $this->title = $this->getRevision()->getTitle(); return true; } + protected function didSetRevision() { + $this->title = $this->getRevision()->getTitle(); + } + public function setValueFromRequest(AphrontRequest $request) { $this->title = $request->getStr('title'); $this->error = null; @@ -67,4 +70,16 @@ final class DifferentialTitleFieldSpecification return $this; } + public function shouldOverwriteWhenCommitMessageIsEdited() { + return true; + } + + public function renderLabelForCommitMessage() { + return 'Title'; + } + + public function renderValueForCommitMessage($is_edit) { + return $this->title; + } + }