Reduce laziness for "Mark Committed"

Summary:
  - Enforce proper workflow rules.
  - Fix a derp-bug with patches.

Test Plan:
  - Tried to mark a revision I didn't own.
  - Tried to mark a revision already marked committed.
  - Tried to mark a revision otherwise not accepted.
  - Verified daemon can override workflow rules and mark from arbitrary states.

Reviewers: btrahan, Makinde

Reviewed By: Makinde

CC: aran, epriestley

Maniphest Tasks: T948

Differential Revision: https://secure.phabricator.com/D1809

src/applications/differential/editor/comment/DifferentialCommentEditor.php

@@ -31,6 +31,8 @@ class DifferentialCommentEditor {
   private $parentMessageID;
   private $contentSource;
 
+  private $isDaemonWorkflow;
+
   public function __construct(
     DifferentialRevision $revision,
     $actor_phid,
@@ -88,6 +90,11 @@ class DifferentialCommentEditor {
     return $this;
   }
 
+  public function setIsDaemonWorkflow($is_daemon) {
+    $this->isDaemonWorkflow = $is_daemon;
+    return $this;
+  }
+
   public function save() {
     $revision = $this->revision;
     $action = $this->action;
@@ -321,12 +328,30 @@ class DifferentialCommentEditor {
 
       case DifferentialAction::ACTION_COMMIT:
 
-        // TODO: We allow this from any state because the daemons are
+        // NOTE: The daemons can mark things committed from any state. We treat
-        // considered authoritative. However, this technically means that anyone
+        // them as completely authoritative.
-        // can mark anything committed at any time with the right POST.
+
-        // Ideally we should set a flag on the editor to tell it whether it
+        if (!$this->isDaemonWorkflow) {
-        // should enforce the web workflow rules (actor must be author and
+          if (!$actor_is_author) {
-        // state must be 'accepted') or the daemon workflow rules (no rules).
+            throw new Exception(
+              "You can not mark a revision you don't own as committed.");
+          }
+
+          $status_committed = ArcanistDifferentialRevisionStatus::COMMITTED;
+          $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED;
+
+          if ($revision_status == $status_committed) {
+            throw new DifferentialActionHasNoEffectException(
+              "You can not mark this revision as committed because it has ".
+              "already been marked as committed.");
+          }
+
+          if ($revision_status != $status_accepted) {
+            throw new DifferentialActionHasNoEffectException(
+              "You can not mark this revision as committed because it is ".
+              "has not been accepted.");
+          }
+        }
 
         $revision
           ->setStatus(ArcanistDifferentialRevisionStatus::COMMITTED);

src/applications/differential/mail/reviewrequest/DifferentialReviewRequestMail.php

@@ -95,7 +95,8 @@ abstract class DifferentialReviewRequestMail extends DifferentialMail {
 
     if (PhabricatorEnv::getEnvConfig('metamta.differential.attach-patches')) {
 
-      $revision_id = $this->getRevision()->getID();
+      $revision = $this->getRevision();
+      $revision_id = $revision->getID();
 
       $diffs = $revision->loadDiffs();
       $diff_number = count($diffs);

src/applications/repository/worker/commitmessageparser/base/PhabricatorRepositoryCommitMessageParserWorker.php

@@ -105,6 +105,7 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
             $revision,
             $committer,
             DifferentialAction::ACTION_COMMIT);
+          $editor->setIsDaemonWorkflow(true);
           $editor->setMessage($message)->save();
         }
       }