From ec0d91a3ff354730ba109495d223d2283c399eda Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 14 Aug 2011 18:52:09 -0700 Subject: [PATCH] Drive revision update from Conduit via custom fields Summary: When we create or update a revision, we use a parsed commit message dictionary to edit its fields. Drive consumption of the dictionary through custom fields instead of hardcoding. This requires adding some fields which don't really do anything right now to cover fields which appear only in the commit message. Test Plan: "arc diff"'d this revision against localhost, "arc diff"'d again to update. Reviewed By: jungejason Reviewers: jungejason, tuomaspelkonen, aran CC: aran, jungejason Differential Revision: 811 --- src/__phutil_library_map__.php | 6 ++ ...API_differential_updaterevision_Method.php | 3 + .../revision/DifferentialRevisionEditor.php | 37 +++++++-- .../differential/editor/revision/__init__.php | 1 + .../DifferentialDefaultFieldSelector.php | 10 +++ .../field/selector/default/__init__.php | 5 ++ .../base/DifferentialFieldSpecification.php | 76 +++++++++++++++++++ ...rentialBlameRevisionFieldSpecification.php | 13 ++++ .../ccs/DifferentialCCsFieldSpecification.php | 13 ++++ ...DifferentialGitSVNIDFieldSpecification.php | 37 +++++++++ .../field/specification/gitsvnid/__init__.php | 12 +++ ...fferentialRevertPlanFieldSpecification.php | 13 ++++ ...fferentialReviewedByFieldSpecification.php | 38 ++++++++++ .../specification/reviewedby/__init__.php | 12 +++ ...ifferentialReviewersFieldSpecification.php | 12 +++ ...fferentialRevisionIDFieldSpecification.php | 38 ++++++++++ .../specification/revisionid/__init__.php | 12 +++ .../DifferentialSummaryFieldSpecification.php | 13 ++++ ...DifferentialTestPlanFieldSpecification.php | 14 ++++ .../DifferentialTitleFieldSpecification.php | 13 ++++ 20 files changed, 370 insertions(+), 8 deletions(-) create mode 100644 src/applications/differential/field/specification/gitsvnid/DifferentialGitSVNIDFieldSpecification.php create mode 100644 src/applications/differential/field/specification/gitsvnid/__init__.php create mode 100644 src/applications/differential/field/specification/reviewedby/DifferentialReviewedByFieldSpecification.php create mode 100644 src/applications/differential/field/specification/reviewedby/__init__.php create mode 100644 src/applications/differential/field/specification/revisionid/DifferentialRevisionIDFieldSpecification.php create mode 100644 src/applications/differential/field/specification/revisionid/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 2bfa0b596a..527a3ffaa3 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -174,6 +174,7 @@ phutil_register_library_map(array( 'DifferentialFieldSpecification' => 'applications/differential/field/specification/base', 'DifferentialFieldSpecificationIncompleteException' => 'applications/differential/field/exception/incomplete', 'DifferentialFieldValidationException' => 'applications/differential/field/exception/validation', + 'DifferentialGitSVNIDFieldSpecification' => 'applications/differential/field/specification/gitsvnid', 'DifferentialHostFieldSpecification' => 'applications/differential/field/specification/host', 'DifferentialHunk' => 'applications/differential/storage/hunk', 'DifferentialInlineComment' => 'applications/differential/storage/inlinecomment', @@ -191,6 +192,7 @@ phutil_register_library_map(array( 'DifferentialReplyHandler' => 'applications/differential/replyhandler', 'DifferentialRevertPlanFieldSpecification' => 'applications/differential/field/specification/revertplan', 'DifferentialReviewRequestMail' => 'applications/differential/mail/reviewrequest', + 'DifferentialReviewedByFieldSpecification' => 'applications/differential/field/specification/reviewedby', 'DifferentialReviewersFieldSpecification' => 'applications/differential/field/specification/reviewers', 'DifferentialRevision' => 'applications/differential/storage/revision', 'DifferentialRevisionCommentListView' => 'applications/differential/view/revisioncommentlist', @@ -200,6 +202,7 @@ phutil_register_library_map(array( 'DifferentialRevisionDetailView' => 'applications/differential/view/revisiondetail', 'DifferentialRevisionEditController' => 'applications/differential/controller/revisionedit', 'DifferentialRevisionEditor' => 'applications/differential/editor/revision', + 'DifferentialRevisionIDFieldSpecification' => 'applications/differential/field/specification/revisionid', 'DifferentialRevisionListController' => 'applications/differential/controller/revisionlist', 'DifferentialRevisionListData' => 'applications/differential/data/revisionlist', 'DifferentialRevisionStatus' => 'applications/differential/constants/revisionstatus', @@ -822,6 +825,7 @@ phutil_register_library_map(array( 'DifferentialDiffViewController' => 'DifferentialController', 'DifferentialExceptionMail' => 'DifferentialMail', 'DifferentialExportPatchFieldSpecification' => 'DifferentialFieldSpecification', + 'DifferentialGitSVNIDFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialHostFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialHunk' => 'DifferentialDAO', 'DifferentialInlineComment' => 'DifferentialDAO', @@ -837,12 +841,14 @@ phutil_register_library_map(array( 'DifferentialReplyHandler' => 'PhabricatorMailReplyHandler', 'DifferentialRevertPlanFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialReviewRequestMail' => 'DifferentialMail', + 'DifferentialReviewedByFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialReviewersFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialRevision' => 'DifferentialDAO', 'DifferentialRevisionCommentListView' => 'AphrontView', 'DifferentialRevisionCommentView' => 'AphrontView', 'DifferentialRevisionDetailView' => 'AphrontView', 'DifferentialRevisionEditController' => 'DifferentialController', + 'DifferentialRevisionIDFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialRevisionListController' => 'DifferentialController', 'DifferentialRevisionStatusFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialRevisionUpdateHistoryView' => 'AphrontView', diff --git a/src/applications/conduit/method/differential/updaterevision/ConduitAPI_differential_updaterevision_Method.php b/src/applications/conduit/method/differential/updaterevision/ConduitAPI_differential_updaterevision_Method.php index 633761b4ed..428d5ee97b 100644 --- a/src/applications/conduit/method/differential/updaterevision/ConduitAPI_differential_updaterevision_Method.php +++ b/src/applications/conduit/method/differential/updaterevision/ConduitAPI_differential_updaterevision_Method.php @@ -54,6 +54,9 @@ class ConduitAPI_differential_updaterevision_Method extends ConduitAPIMethod { } $revision = id(new DifferentialRevision())->load($request->getValue('id')); + if (!$revision) { + throw new ConduitException('ERR_BAD_REVISION'); + } if ($request->getUser()->getPHID() !== $revision->getAuthorPHID()) { throw new ConduitException('ERR_WRONG_USER'); diff --git a/src/applications/differential/editor/revision/DifferentialRevisionEditor.php b/src/applications/differential/editor/revision/DifferentialRevisionEditor.php index b5bb7075b4..c71f66d55c 100644 --- a/src/applications/differential/editor/revision/DifferentialRevisionEditor.php +++ b/src/applications/differential/editor/revision/DifferentialRevisionEditor.php @@ -70,15 +70,36 @@ class DifferentialRevisionEditor { $revision = $this->revision; - $revision->setTitle((string)$fields['title']); - $revision->setSummary((string)$fields['summary']); - $revision->setTestPlan((string)$fields['testPlan']); - $revision->setBlameRevision((string)$fields['blameRevision']); - $revision->setRevertPlan((string)$fields['revertPlan']); + $aux_fields = DifferentialFieldSelector::newSelector() + ->getFieldSpecifications(); - $this->setReviewers($fields['reviewerPHIDs']); - $this->setCCPHIDs($fields['ccPHIDs']); - $this->setTasks($fields['tasks']); + foreach ($aux_fields as $key => $aux_field) { + $aux_field->setRevision($revision); + if (!$aux_field->shouldAppearOnCommitMessage()) { + unset($aux_fields[$key]); + } + } + + $aux_fields = mpull($aux_fields, null, 'getCommitMessageKey'); + + foreach ($fields as $field => $value) { + + if ($field == 'tasks') { + // TODO: Deprecate once this can be fully supported with custom fields. + // This is just to prevent a backcompat break for Facebook. + $this->setTasks($value); + continue; + } + + if (empty($aux_fields[$field])) { + throw new Exception( + "Parsed commit message contains unrecognized field '{$field}'."); + } + $aux_fields[$field]->setValueFromParsedCommitMessage($value); + } + + $aux_fields = array_values($aux_fields); + $this->setAuxiliaryFields($aux_fields); } public function setAuxiliaryFields(array $auxiliary_fields) { diff --git a/src/applications/differential/editor/revision/__init__.php b/src/applications/differential/editor/revision/__init__.php index 1b6a0cc33b..231afd8eff 100644 --- a/src/applications/differential/editor/revision/__init__.php +++ b/src/applications/differential/editor/revision/__init__.php @@ -8,6 +8,7 @@ phutil_require_module('phabricator', 'applications/differential/constants/action'); phutil_require_module('phabricator', 'applications/differential/constants/revisionstatus'); +phutil_require_module('phabricator', 'applications/differential/field/selector/base'); phutil_require_module('phabricator', 'applications/differential/mail/ccwelcome'); phutil_require_module('phabricator', 'applications/differential/mail/newdiff'); phutil_require_module('phabricator', 'applications/differential/storage/auxiliaryfield'); diff --git a/src/applications/differential/field/selector/default/DifferentialDefaultFieldSelector.php b/src/applications/differential/field/selector/default/DifferentialDefaultFieldSelector.php index 6249b62d30..c8dd382fe0 100644 --- a/src/applications/differential/field/selector/default/DifferentialDefaultFieldSelector.php +++ b/src/applications/differential/field/selector/default/DifferentialDefaultFieldSelector.php @@ -27,6 +27,7 @@ final class DifferentialDefaultFieldSelector new DifferentialRevisionStatusFieldSpecification(), new DifferentialAuthorFieldSpecification(), new DifferentialReviewersFieldSpecification(), + new DifferentialReviewedByFieldSpecification(), new DifferentialCCsFieldSpecification(), new DifferentialUnitFieldSpecification(), new DifferentialLintFieldSpecification(), @@ -39,6 +40,15 @@ final class DifferentialDefaultFieldSelector new DifferentialArcanistProjectFieldSpecification(), new DifferentialApplyPatchFieldSpecification(), new DifferentialExportPatchFieldSpecification(), + new DifferentialRevisionIDFieldSpecification(), + new DifferentialGitSVNIDFieldSpecification(), + + // TODO: Remove these from the default set, we need them temporarily + // because the commit message dictionary always contains their keys + // right now. + new DifferentialBlameRevisionFieldSpecification(), + new DifferentialRevertPlanFieldSpecification(), + ); } diff --git a/src/applications/differential/field/selector/default/__init__.php b/src/applications/differential/field/selector/default/__init__.php index 7c02e0fc83..80ad649b78 100644 --- a/src/applications/differential/field/selector/default/__init__.php +++ b/src/applications/differential/field/selector/default/__init__.php @@ -10,16 +10,21 @@ phutil_require_module('phabricator', 'applications/differential/field/selector/b phutil_require_module('phabricator', 'applications/differential/field/specification/applypatch'); phutil_require_module('phabricator', 'applications/differential/field/specification/arcanistproject'); phutil_require_module('phabricator', 'applications/differential/field/specification/author'); +phutil_require_module('phabricator', 'applications/differential/field/specification/blamerev'); phutil_require_module('phabricator', 'applications/differential/field/specification/ccs'); phutil_require_module('phabricator', 'applications/differential/field/specification/commits'); phutil_require_module('phabricator', 'applications/differential/field/specification/dependencies'); phutil_require_module('phabricator', 'applications/differential/field/specification/exportpatch'); +phutil_require_module('phabricator', 'applications/differential/field/specification/gitsvnid'); phutil_require_module('phabricator', 'applications/differential/field/specification/host'); phutil_require_module('phabricator', 'applications/differential/field/specification/lines'); phutil_require_module('phabricator', 'applications/differential/field/specification/lint'); phutil_require_module('phabricator', 'applications/differential/field/specification/maniphesttasks'); phutil_require_module('phabricator', 'applications/differential/field/specification/path'); +phutil_require_module('phabricator', 'applications/differential/field/specification/revertplan'); +phutil_require_module('phabricator', 'applications/differential/field/specification/reviewedby'); phutil_require_module('phabricator', 'applications/differential/field/specification/reviewers'); +phutil_require_module('phabricator', 'applications/differential/field/specification/revisionid'); phutil_require_module('phabricator', 'applications/differential/field/specification/revisionstatus'); phutil_require_module('phabricator', 'applications/differential/field/specification/summary'); phutil_require_module('phabricator', 'applications/differential/field/specification/testplan'); diff --git a/src/applications/differential/field/specification/base/DifferentialFieldSpecification.php b/src/applications/differential/field/specification/base/DifferentialFieldSpecification.php index 0c2d98dadc..96b5103000 100644 --- a/src/applications/differential/field/specification/base/DifferentialFieldSpecification.php +++ b/src/applications/differential/field/specification/base/DifferentialFieldSpecification.php @@ -28,6 +28,7 @@ * @task edit Extending the Revision Edit Interface * @task view Extending the Revision View Interface * @task conduit Extending the Conduit View Interface + * @task commit Extending Commit Messages * @task load Loading Additional Data * @task context Contextual Data */ @@ -177,6 +178,16 @@ abstract class DifferentialFieldSpecification { } /** + * Hook for applying revision changes via the editor. Normally, you should + * not implement this, but a number of builtin fields use the revision object + * itself as storage. If you need to do something similar for whatever reason, + * this method gives you an opportunity to interact with the editor or + * revision before changes are saved (for example, you can write the field's + * value into some property of the revision). + * + * @param DifferentialRevisionEditor Active editor which is applying changes + * to the revision. + * @return void * @task edit */ public function willWriteRevision(DifferentialRevisionEditor $editor) { @@ -184,6 +195,14 @@ abstract class DifferentialFieldSpecification { } /** + * Hook after an edit operation has completed. This allows you to update + * link tables or do other write operations which should happen after the + * revision is saved. Normally you don't need to implement this. + * + * + * @param DifferentialRevisionEditor Active editor which has just applied + * changes to the revision. + * @return void * @task edit */ public function didWriteRevision(DifferentialRevisionEditor $editor) { @@ -273,6 +292,63 @@ abstract class DifferentialFieldSpecification { } +/* -( Extending Commit Messages )------------------------------------------ */ + + + /** + * Determine if this field should appear in commit messages. You should return + * true if this field participates in any part of the commit message workflow, + * even if it is not rendered by default. + * + * If you implement this method, you must implement + * @{method:getCommitMessageKey} and + * @{method:setValueFromParsedCommitMessage}. + * + * @return bool True if this field appears in commit messages in any capacity. + * @task commit + */ + public function shouldAppearOnCommitMessage() { + return false; + } + + /** + * Key which identifies this field in parsed commit messages. Commit messages + * exist in two forms: raw textual commit messages and parsed dictionaries of + * fields. This method must return a unique string which identifies this field + * in dictionaries. Principally, this dictionary is shipped to and from arc + * over Conduit. Keys should be appropriate property names, like "testPlan" + * (not "Test Plan") and must be globally unique. + * + * You must implement this method if you return true from + * @{method:shouldAppearOnCommitMessage}. + * + * @return string Key which identifies the field in dictionaries. + * @task commit + */ + public function getCommitMessageKey() { + throw new DifferentialFieldSpecificationIncompleteException($this); + } + + /** + * Set this field's value from a value in a parsed commit message dictionary. + * Afterward, this field will go through the normal write workflows and the + * change will be permanently stored via either the storage mechanisms (if + * your field implements them), revision write hooks (if your field implements + * them) or discarded (if your field implements neither, e.g. is just a + * display field). + * + * You must implement this method if you return true from + * @{method:shouldAppearOnCommitMessage}. + * + * @param mixed Field value from a parsed commit message dictionary. + * @return this + * @task commit + */ + public function setValueFromParsedCommitMessage($value) { + throw new DifferentialFieldSpecificationIncompleteException($this); + } + + /* -( Loading Additional Data )-------------------------------------------- */ diff --git a/src/applications/differential/field/specification/blamerev/DifferentialBlameRevisionFieldSpecification.php b/src/applications/differential/field/specification/blamerev/DifferentialBlameRevisionFieldSpecification.php index 6c877c2024..c058590cee 100644 --- a/src/applications/differential/field/specification/blamerev/DifferentialBlameRevisionFieldSpecification.php +++ b/src/applications/differential/field/specification/blamerev/DifferentialBlameRevisionFieldSpecification.php @@ -78,4 +78,17 @@ final class DifferentialBlameRevisionFieldSpecification return $this->value; } + public function shouldAppearOnCommitMessage() { + return true; + } + + public function getCommitMessageKey() { + return 'blameRevision'; + } + + public function setValueFromParsedCommitMessage($value) { + $this->value = $value; + return $this; + } + } diff --git a/src/applications/differential/field/specification/ccs/DifferentialCCsFieldSpecification.php b/src/applications/differential/field/specification/ccs/DifferentialCCsFieldSpecification.php index 672dcab5fd..6bf514fb7f 100644 --- a/src/applications/differential/field/specification/ccs/DifferentialCCsFieldSpecification.php +++ b/src/applications/differential/field/specification/ccs/DifferentialCCsFieldSpecification.php @@ -82,4 +82,17 @@ final class DifferentialCCsFieldSpecification $editor->setCCPHIDs($this->ccs); } + public function shouldAppearOnCommitMessage() { + return true; + } + + public function getCommitMessageKey() { + return 'ccPHIDs'; + } + + public function setValueFromParsedCommitMessage($value) { + $this->value = nonempty($value, array()); + return $this; + } + } diff --git a/src/applications/differential/field/specification/gitsvnid/DifferentialGitSVNIDFieldSpecification.php b/src/applications/differential/field/specification/gitsvnid/DifferentialGitSVNIDFieldSpecification.php new file mode 100644 index 0000000000..c908946303 --- /dev/null +++ b/src/applications/differential/field/specification/gitsvnid/DifferentialGitSVNIDFieldSpecification.php @@ -0,0 +1,37 @@ +gitSVNID = $value; + return $this; + } + +} diff --git a/src/applications/differential/field/specification/gitsvnid/__init__.php b/src/applications/differential/field/specification/gitsvnid/__init__.php new file mode 100644 index 0000000000..b380586817 --- /dev/null +++ b/src/applications/differential/field/specification/gitsvnid/__init__.php @@ -0,0 +1,12 @@ +value; } + public function shouldAppearOnCommitMessage() { + return true; + } + + public function getCommitMessageKey() { + return 'revertPlan'; + } + + public function setValueFromParsedCommitMessage($value) { + $this->value = $value; + return $this; + } + } diff --git a/src/applications/differential/field/specification/reviewedby/DifferentialReviewedByFieldSpecification.php b/src/applications/differential/field/specification/reviewedby/DifferentialReviewedByFieldSpecification.php new file mode 100644 index 0000000000..f5904e2ee2 --- /dev/null +++ b/src/applications/differential/field/specification/reviewedby/DifferentialReviewedByFieldSpecification.php @@ -0,0 +1,38 @@ +reviewedBy = $value; + return $this; + } + + +} diff --git a/src/applications/differential/field/specification/reviewedby/__init__.php b/src/applications/differential/field/specification/reviewedby/__init__.php new file mode 100644 index 0000000000..a87a459dbd --- /dev/null +++ b/src/applications/differential/field/specification/reviewedby/__init__.php @@ -0,0 +1,12 @@ +setReviewers($this->reviewers); } + public function shouldAppearOnCommitMessage() { + return true; + } + + public function getCommitMessageKey() { + return 'reviewerPHIDs'; + } + + public function setValueFromParsedCommitMessage($value) { + $this->reviewers = nonempty($value, array()); + return $this; + } } diff --git a/src/applications/differential/field/specification/revisionid/DifferentialRevisionIDFieldSpecification.php b/src/applications/differential/field/specification/revisionid/DifferentialRevisionIDFieldSpecification.php new file mode 100644 index 0000000000..e1f1cf30b3 --- /dev/null +++ b/src/applications/differential/field/specification/revisionid/DifferentialRevisionIDFieldSpecification.php @@ -0,0 +1,38 @@ +id = $value; + return $this; + } + + +} diff --git a/src/applications/differential/field/specification/revisionid/__init__.php b/src/applications/differential/field/specification/revisionid/__init__.php new file mode 100644 index 0000000000..1b3dc16ecc --- /dev/null +++ b/src/applications/differential/field/specification/revisionid/__init__.php @@ -0,0 +1,12 @@ +getRevision()->setSummary($this->summary); } + public function shouldAppearOnCommitMessage() { + return true; + } + + public function getCommitMessageKey() { + return 'summary'; + } + + public function setValueFromParsedCommitMessage($value) { + $this->summary = $value; + return $this; + } + } diff --git a/src/applications/differential/field/specification/testplan/DifferentialTestPlanFieldSpecification.php b/src/applications/differential/field/specification/testplan/DifferentialTestPlanFieldSpecification.php index 6d4d3bcb6d..8eed5dc248 100644 --- a/src/applications/differential/field/specification/testplan/DifferentialTestPlanFieldSpecification.php +++ b/src/applications/differential/field/specification/testplan/DifferentialTestPlanFieldSpecification.php @@ -53,4 +53,18 @@ final class DifferentialTestPlanFieldSpecification } } + public function shouldAppearOnCommitMessage() { + return true; + } + + public function getCommitMessageKey() { + return 'testPlan'; + } + + public function setValueFromParsedCommitMessage($value) { + $this->plan = $value; + return $this; + } + + } diff --git a/src/applications/differential/field/specification/title/DifferentialTitleFieldSpecification.php b/src/applications/differential/field/specification/title/DifferentialTitleFieldSpecification.php index e62bc0626d..7b6bb94dc2 100644 --- a/src/applications/differential/field/specification/title/DifferentialTitleFieldSpecification.php +++ b/src/applications/differential/field/specification/title/DifferentialTitleFieldSpecification.php @@ -54,4 +54,17 @@ final class DifferentialTitleFieldSpecification } } + public function shouldAppearOnCommitMessage() { + return true; + } + + public function getCommitMessageKey() { + return 'title'; + } + + public function setValueFromParsedCommitMessage($value) { + $this->title = $value; + return $this; + } + }