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

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

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

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');
+    $revision->loadRelationships();
-    $mode = $edit
-      ? DifferentialCommitMessageData::MODE_EDIT
-      : DifferentialCommitMessageData::MODE_AMEND;
 
-    $message_data = new DifferentialCommitMessageData($revision, $mode);
+    $is_edit = $request->getValue('edit');
-    $message_data->prepare();
 
-    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) {
-
-        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);
+        $aux_field = idx($aux_fields, $field);
-          } else {
+        if (!$aux_field) {
-            $overwrite = true;
+          if ($field == 'tasks') {
-            static $overwrite_map = array(
+            // TODO: Remove, backcompat for Facebook.
-              'reviewerPHIDs' => 'Reviewers',
+            continue;
-              '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);
           }
+          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[] = '<<Enter Revision Title>>';
+        } 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);
   }

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

src/applications/differential/data/commitmessage/DifferentialCommitMessageData.php

@@ -1,196 +0,0 @@
-<?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.
- */
-
-class DifferentialCommitMessageData {
-
-  protected $revision;
-  protected $fields = array();
-  protected $mode;
-  protected $comments;
-
-  const MODE_EDIT   = 'edit';
-  const MODE_AMEND  = 'amend';
-
-  public function __construct(DifferentialRevision $revision, $mode) {
-    $this->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;
-  }
-}

src/applications/differential/data/commitmessage/DifferentialCommitMessageField.php

@@ -1,49 +0,0 @@
-<?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.
- */
-
-class DifferentialCommitMessageField {
-
-  // true if the field should be displayed on a single line
-  protected $oneLine;
-  protected $value;
-
-  public function __construct($name, $value) {
-    $this->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;
-  }
-}

src/applications/differential/data/commitmessage/DifferentialCommitMessageModifier.php

@@ -1,39 +0,0 @@
-<?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.
- */
-
-abstract class DifferentialCommitMessageModifier {
-  protected $revision;
-
-  /**
-  * @param  DifferentialRevision The revision to generate fields for
-  */
-  public function __construct(DifferentialRevision $revision) {
-    $this->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);
-}

src/applications/differential/data/commitmessage/__init__.php

@@ -1,19 +0,0 @@
-<?php
-/**
- * This file is automatically generated. Lint this module to rebuild it.
- * @generated
- */
-
-
-
-phutil_require_module('phabricator', 'applications/differential/constants/action');
-phutil_require_module('phabricator', 'applications/differential/storage/comment');
-phutil_require_module('phabricator', 'applications/phid/handle/data');
-phutil_require_module('phabricator', 'infrastructure/env');
-
-phutil_require_module('phutil', 'utils');
-
-
-phutil_require_source('DifferentialCommitMessageData.php');
-phutil_require_source('DifferentialCommitMessageField.php');
-phutil_require_source('DifferentialCommitMessageModifier.php');

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

@@ -69,6 +69,7 @@ class DifferentialRevisionEditor {
   public function copyFieldsFromConduit(array $fields) {
 
     $revision = $this->revision;
+    $revision->loadRelationships();
 
     $aux_fields = DifferentialFieldSelector::newSelector()
       ->getFieldSpecifications();

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

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

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

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

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

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

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

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

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

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

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

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