From d84c0a5be59c7da805df35b011524d7a4fcf4bd0 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 11 Jan 2012 19:55:05 -0800
Subject: [PATCH] Validate all fields in differential.parsecommitmessage

Summary:
  - We currently run ##parseValueFromCommitMessage()## on all fields present in
the message, but not ##validateField()##.
  - This detects value errors (e.g., an invalid reviewer) but not higher-level
errors (e.g., a missing field).
  - This can break the stacked-commits Git mutable history workflow by
recognizing too many commit messages as valid ("multiple valid commit messages,
this is ambiguous").
  - This also gives you some errors ("Missing test plan") too late in "arc diff
--create" (after the diff has been built).

Test Plan:
  - Grepped for validateField() calls, removed a couple of calls that had the
same implementation as the base class.
  - Grepped for other calls to this to make sure I'm not stumbling into
unintended side effects, but it only runs from the diff workflow.
  - Ran "arc diff --create" with an invalid test plan, got a good error early in
the process.
  - Ran "arc diff master" with stacked local commits, got a correct selection of
the intended message.

Reviewers: cpiro, btrahan, jungejason

Reviewed By: cpiro

CC: aran, cpiro

Differential Revision: https://secure.phabricator.com/D1373
---
 ...API_differential_parsecommitmessage_Method.php | 15 ++++++++++++++-
 ...ifferentialBlameRevisionFieldSpecification.php |  6 +-----
 .../DifferentialRevertPlanFieldSpecification.php  |  6 +-----
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/src/applications/conduit/method/differential/parsecommitmessage/ConduitAPI_differential_parsecommitmessage_Method.php b/src/applications/conduit/method/differential/parsecommitmessage/ConduitAPI_differential_parsecommitmessage_Method.php
index cba392bbb8..02f5ca5f12 100644
--- a/src/applications/conduit/method/differential/parsecommitmessage/ConduitAPI_differential_parsecommitmessage_Method.php
+++ b/src/applications/conduit/method/differential/parsecommitmessage/ConduitAPI_differential_parsecommitmessage_Method.php
@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -51,6 +51,7 @@ class ConduitAPI_differential_parsecommitmessage_Method
       if (!$aux_field->shouldAppearOnCommitMessage()) {
         unset($aux_fields[$key]);
       }
+      $aux_field->setUser($request->getUser());
     }
 
     $aux_fields = mpull($aux_fields, null, 'getCommitMessageKey');
@@ -66,12 +67,24 @@ class ConduitAPI_differential_parsecommitmessage_Method
       $field = $aux_fields[$field_key];
       try {
         $fields[$field_key] = $field->parseValueFromCommitMessage($field_value);
+        $field->setValueFromParsedCommitMessage($fields[$field_key]);
       } catch (DifferentialFieldParseException $ex) {
         $field_label = $field->renderLabelForCommitMessage();
         $errors[] = "Error parsing field '{$field_label}': ".$ex->getMessage();
       }
     }
 
+    foreach ($aux_fields as $field_key => $aux_field) {
+      try {
+        $aux_field->validateField();
+      } catch (DifferentialFieldValidationException $ex) {
+        $field_label = $aux_field->renderLabelForCommitMessage();
+        $errors[] =
+          "Invalid or missing field '{$field_label}': ".
+          $ex->getMessage();
+      }
+    }
+
     return array(
       'errors' => $errors,
       'fields' => $fields,
diff --git a/src/applications/differential/field/specification/blamerev/DifferentialBlameRevisionFieldSpecification.php b/src/applications/differential/field/specification/blamerev/DifferentialBlameRevisionFieldSpecification.php
index 57d55e11e6..2821ede35b 100644
--- a/src/applications/differential/field/specification/blamerev/DifferentialBlameRevisionFieldSpecification.php
+++ b/src/applications/differential/field/specification/blamerev/DifferentialBlameRevisionFieldSpecification.php
@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -51,10 +51,6 @@ final class DifferentialBlameRevisionFieldSpecification
       ->setValue($this->value);
   }
 
-  public function validateField() {
-    return;
-  }
-
   public function shouldAppearOnRevisionView() {
     return true;
   }
diff --git a/src/applications/differential/field/specification/revertplan/DifferentialRevertPlanFieldSpecification.php b/src/applications/differential/field/specification/revertplan/DifferentialRevertPlanFieldSpecification.php
index 3f9cda19ac..9a7cacaf76 100644
--- a/src/applications/differential/field/specification/revertplan/DifferentialRevertPlanFieldSpecification.php
+++ b/src/applications/differential/field/specification/revertplan/DifferentialRevertPlanFieldSpecification.php
@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -51,10 +51,6 @@ final class DifferentialRevertPlanFieldSpecification
       ->setValue($this->value);
   }
 
-  public function validateField() {
-    return;
-  }
-
   public function shouldAppearOnRevisionView() {
     return true;
   }