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

@@ -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',

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');

src/applications/differential/editor/revision/DifferentialRevisionEditor.php

@@ -70,15 +70,36 @@ class DifferentialRevisionEditor {
 
     $revision = $this->revision;
 
-    $revision->setTitle((string)$fields['title']);
+    $aux_fields = DifferentialFieldSelector::newSelector()
-    $revision->setSummary((string)$fields['summary']);
+      ->getFieldSpecifications();
-    $revision->setTestPlan((string)$fields['testPlan']);
-    $revision->setBlameRevision((string)$fields['blameRevision']);
-    $revision->setRevertPlan((string)$fields['revertPlan']);
 
-    $this->setReviewers($fields['reviewerPHIDs']);
+    foreach ($aux_fields as $key => $aux_field) {
-    $this->setCCPHIDs($fields['ccPHIDs']);
+      $aux_field->setRevision($revision);
-    $this->setTasks($fields['tasks']);
+      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) {

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');

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(),
+
     );
   }
 

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');

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  )-------------------------------------------- */
 
 

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;
+  }
+
 }

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;
+  }
+
 }

src/applications/differential/field/specification/gitsvnid/DifferentialGitSVNIDFieldSpecification.php

@@ -0,0 +1,37 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class DifferentialGitSVNIDFieldSpecification
+  extends DifferentialFieldSpecification {
+
+  private $gitSVNID;
+
+  public function shouldAppearOnCommitMessage() {
+    return true;
+  }
+
+  public function getCommitMessageKey() {
+    return 'gitSVNID';
+  }
+
+  public function setValueFromParsedCommitMessage($value) {
+    $this->gitSVNID = $value;
+    return $this;
+  }
+
+}

src/applications/differential/field/specification/gitsvnid/__init__.php

@@ -0,0 +1,12 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/differential/field/specification/base');
+
+
+phutil_require_source('DifferentialGitSVNIDFieldSpecification.php');

src/applications/differential/field/specification/revertplan/DifferentialRevertPlanFieldSpecification.php

@@ -78,4 +78,17 @@ final class DifferentialRevertPlanFieldSpecification
     return $this->value;
   }
 
+  public function shouldAppearOnCommitMessage() {
+    return true;
+  }
+
+  public function getCommitMessageKey() {
+    return 'revertPlan';
+  }
+
+  public function setValueFromParsedCommitMessage($value) {
+    $this->value = $value;
+    return $this;
+  }
+
 }

src/applications/differential/field/specification/reviewedby/DifferentialReviewedByFieldSpecification.php

@@ -0,0 +1,38 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class DifferentialReviewedByFieldSpecification
+  extends DifferentialFieldSpecification {
+
+  private $reviewedBy;
+
+  public function shouldAppearOnCommitMessage() {
+    return true;
+  }
+
+  public function getCommitMessageKey() {
+    return 'reviewedByPHIDs';
+  }
+
+  public function setValueFromParsedCommitMessage($value) {
+    $this->reviewedBy = $value;
+    return $this;
+  }
+
+
+}

src/applications/differential/field/specification/reviewedby/__init__.php

@@ -0,0 +1,12 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/differential/field/specification/base');
+
+
+phutil_require_source('DifferentialReviewedByFieldSpecification.php');

src/applications/differential/field/specification/reviewers/DifferentialReviewersFieldSpecification.php

@@ -92,5 +92,17 @@ final class DifferentialReviewersFieldSpecification
     $editor->setReviewers($this->reviewers);
   }
 
+  public function shouldAppearOnCommitMessage() {
+    return true;
+  }
+
+  public function getCommitMessageKey() {
+    return 'reviewerPHIDs';
+  }
+
+  public function setValueFromParsedCommitMessage($value) {
+    $this->reviewers = nonempty($value, array());
+    return $this;
+  }
 
 }

src/applications/differential/field/specification/revisionid/DifferentialRevisionIDFieldSpecification.php

@@ -0,0 +1,38 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class DifferentialRevisionIDFieldSpecification
+  extends DifferentialFieldSpecification {
+
+  private $id;
+
+  public function shouldAppearOnCommitMessage() {
+    return true;
+  }
+
+  public function getCommitMessageKey() {
+    return 'revisionID';
+  }
+
+  public function setValueFromParsedCommitMessage($value) {
+    $this->id = $value;
+    return $this;
+  }
+
+
+}

src/applications/differential/field/specification/revisionid/__init__.php

@@ -0,0 +1,12 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/differential/field/specification/base');
+
+
+phutil_require_source('DifferentialRevisionIDFieldSpecification.php');

src/applications/differential/field/specification/summary/DifferentialSummaryFieldSpecification.php

@@ -42,4 +42,17 @@ final class DifferentialSummaryFieldSpecification
     $this->getRevision()->setSummary($this->summary);
   }
 
+  public function shouldAppearOnCommitMessage() {
+    return true;
+  }
+
+  public function getCommitMessageKey() {
+    return 'summary';
+  }
+
+  public function setValueFromParsedCommitMessage($value) {
+    $this->summary = $value;
+    return $this;
+  }
+
 }

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;
+  }
+
+
 }

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;
+  }
+
 }