1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-24 14:30:56 +01:00

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
This commit is contained in:
epriestley 2012-01-11 19:55:05 -08:00
parent 02fb5fea89
commit d84c0a5be5
3 changed files with 16 additions and 11 deletions

View file

@ -1,7 +1,7 @@
<?php <?php
/* /*
* Copyright 2011 Facebook, Inc. * Copyright 2012 Facebook, Inc.
* *
* Licensed under the Apache License, Version 2.0 (the "License"); * Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with 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()) { if (!$aux_field->shouldAppearOnCommitMessage()) {
unset($aux_fields[$key]); unset($aux_fields[$key]);
} }
$aux_field->setUser($request->getUser());
} }
$aux_fields = mpull($aux_fields, null, 'getCommitMessageKey'); $aux_fields = mpull($aux_fields, null, 'getCommitMessageKey');
@ -66,12 +67,24 @@ class ConduitAPI_differential_parsecommitmessage_Method
$field = $aux_fields[$field_key]; $field = $aux_fields[$field_key];
try { try {
$fields[$field_key] = $field->parseValueFromCommitMessage($field_value); $fields[$field_key] = $field->parseValueFromCommitMessage($field_value);
$field->setValueFromParsedCommitMessage($fields[$field_key]);
} catch (DifferentialFieldParseException $ex) { } catch (DifferentialFieldParseException $ex) {
$field_label = $field->renderLabelForCommitMessage(); $field_label = $field->renderLabelForCommitMessage();
$errors[] = "Error parsing field '{$field_label}': ".$ex->getMessage(); $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( return array(
'errors' => $errors, 'errors' => $errors,
'fields' => $fields, 'fields' => $fields,

View file

@ -1,7 +1,7 @@
<?php <?php
/* /*
* Copyright 2011 Facebook, Inc. * Copyright 2012 Facebook, Inc.
* *
* Licensed under the Apache License, Version 2.0 (the "License"); * Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License. * you may not use this file except in compliance with the License.
@ -51,10 +51,6 @@ final class DifferentialBlameRevisionFieldSpecification
->setValue($this->value); ->setValue($this->value);
} }
public function validateField() {
return;
}
public function shouldAppearOnRevisionView() { public function shouldAppearOnRevisionView() {
return true; return true;
} }

View file

@ -1,7 +1,7 @@
<?php <?php
/* /*
* Copyright 2011 Facebook, Inc. * Copyright 2012 Facebook, Inc.
* *
* Licensed under the Apache License, Version 2.0 (the "License"); * Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License. * you may not use this file except in compliance with the License.
@ -51,10 +51,6 @@ final class DifferentialRevertPlanFieldSpecification
->setValue($this->value); ->setValue($this->value);
} }
public function validateField() {
return;
}
public function shouldAppearOnRevisionView() { public function shouldAppearOnRevisionView() {
return true; return true;
} }