Add a "list<regex>" config option and move regex config to it

Summary:
Fixes T3807. Several issues:

  - Currently, we split config of type `list<string>` on commas, which makes it impossible to enter a regex with a comma in it.
    - Split on newlines only.
  - Some of the examples are confusing (provided in JSON instead of the format you actually have to enter them).
    - Show examples in the same format you should enter text.
  - We didn't validate regexps.
    - Introduce `list<regex>` to validate regexes.

@hlau: Note that the old config format for the bugtraq stuff implied the delimiters on the regular expression. They are no longer implied. The examples show the correct format.

Test Plan: Viewed and edited affected config, hitting error and success cases.

Reviewers: btrahan

Reviewed By: btrahan

CC: hlau, aran

Maniphest Tasks: T3807

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

src/applications/config/controller/PhabricatorConfigEditController.php

@@ -282,7 +282,18 @@ final class PhabricatorConfigEditController
           $set_value = (string)$value;
           break;
         case 'list<string>':
-          $set_value = $request->getStrList('value');
+        case 'list<regex>':
+          $set_value = phutil_split_lines(
+            $request->getStr('value'),
+            $retain_endings = false);
+
+          foreach ($set_value as $key => $v) {
+            if (!strlen($v)) {
+              unset($set_value[$key]);
+            }
+          }
+          $set_value = array_values($set_value);
+
           break;
         case 'set':
           $set_value = array_fill_keys($request->getStrList('value'), true);
@@ -364,6 +375,7 @@ final class PhabricatorConfigEditController
         case 'bool':
           return $value ? 'true' : 'false';
         case 'list<string>':
+        case 'list<regex>':
           return implode("\n", nonempty($value, array()));
         case 'set':
           return implode("\n", nonempty(array_keys($value), array()));
@@ -424,6 +436,10 @@ final class PhabricatorConfigEditController
             ->setOptions($names);
           break;
         case 'list<string>':
+        case 'list<regex>':
+          $control = id(new AphrontFormTextAreaControl())
+            ->setCaption(pht('Separate values with newlines.'));
+          break;
         case 'set':
           $control = id(new AphrontFormTextAreaControl())
             ->setCaption(pht('Separate values with newlines or commas.'));

src/applications/config/option/PhabricatorApplicationConfigOptions.php

@@ -78,6 +78,34 @@ abstract class PhabricatorApplicationConfigOptions extends Phobject {
           }
         }
         break;
+      case 'list<regex>':
+        $valid = true;
+        if (!is_array($value)) {
+          throw new PhabricatorConfigValidationException(
+            pht(
+              "Option '%s' must be a list of regular expressions, but value ".
+              "is not an array.",
+              $option->getKey()));
+        }
+        if ($value && array_keys($value) != range(0, count($value) - 1)) {
+          throw new PhabricatorConfigValidationException(
+            pht(
+              "Option '%s' must be a list of regular expressions, but the ".
+              "value is a map with unnatural keys.",
+              $option->getKey()));
+        }
+        foreach ($value as $v) {
+          $ok = @preg_match($v, '');
+          if ($ok === false) {
+            throw new PhabricatorConfigValidationException(
+              pht(
+                "Option '%s' must be a list of regular expressions, but the ".
+                "value '%s' is not a valid regular expression.",
+                $option->getKey(),
+                $v));
+          }
+        }
+        break;
       case 'list<string>':
         $valid = true;
         if (!is_array($value)) {

src/applications/differential/config/PhabricatorDifferentialConfigOptions.php

@@ -40,7 +40,7 @@ final class PhabricatorDifferentialConfigOptions
             "PhutilRemarkupEngineBlockRule")),
       $this->newOption(
         'differential.whitespace-matters',
-        'list<string>',
+        'list<regex>',
         array(
           '/\.py$/',
           '/\.l?hs$/',
@@ -127,14 +127,14 @@ final class PhabricatorDifferentialConfigOptions
             "If you set this to true, users won't need to login to view ".
             "Differential revisions. Anonymous users will have read-only ".
             "access and won't be able to interact with the revisions.")),
-      $this->newOption('differential.generated-paths', 'list<string>', array())
+      $this->newOption('differential.generated-paths', 'list<regex>', array())
         ->setSummary(pht("File regexps to treat as automatically generated."))
         ->setDescription(
           pht(
             "List of file regexps that should be treated as if they are ".
             "generated by an automatic process, and thus get hidden by ".
             "default in differential."))
-        ->addExample('["/config\.h$/", "#/autobuilt/#"]', pht("Valid Setting")),
+        ->addExample("/config\.h$/\n#/autobuilt/#", pht("Valid Setting")),
       $this->newOption('differential.allow-self-accept', 'bool', false)
         ->setBoolOptions(
           array(

src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php

@@ -73,12 +73,12 @@ final class PhabricatorDiffusionConfigOptions
           'URL of external bug tracker used by Diffusion. %s will be '.
             'substituted by the bug ID.',
           '%BUGID%')),
-      $this->newOption('bugtraq.logregex', 'list<string>', array())
-        ->addExample(array('\B#([1-9]\d*)\b'), pht('Issue #123'))
+      $this->newOption('bugtraq.logregex', 'list<regex>', array())
+        ->addExample(array('/\B#([1-9]\d*)\b/'), pht('Issue #123'))
         ->addExample(
-          array('[Ii]ssues?:?(\s*,?\s*#\d+)+', '(\d+)'),
+          array('/[Ii]ssues?:?(\s*,?\s*#\d+)+/', '/(\d+)/'),
           pht('Issue #123, #456'))
-        ->addExample(array('(?<!#)\b(T[1-9]\d*)\b'), pht('Task T123'))
+        ->addExample(array('/(?<!#)\b(T[1-9]\d*)\b/'), pht('Task T123'))
         ->setDescription(pht(
           'Regular expression to link external bug tracker. See '.
             'http://tortoisesvn.net/docs/release/TortoiseSVN_en/'.

src/applications/repository/storage/PhabricatorRepository.php

@@ -717,13 +717,13 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
 
     $matches = null;
     $flags = PREG_SET_ORDER | PREG_OFFSET_CAPTURE;
-    preg_match_all('('.$bugtraq_re.')', $message, $matches, $flags);
+    preg_match_all($bugtraq_re, $message, $matches, $flags);
     foreach ($matches as $match) {
       list($all, $all_offset) = array_shift($match);
 
       if ($id_re != '') {
         // Match substrings with bug IDs
-        preg_match_all('('.$id_re.')', $all, $match, PREG_OFFSET_CAPTURE);
+        preg_match_all($id_re, $all, $match, PREG_OFFSET_CAPTURE);
         list(, $match) = $match;
       } else {
         $all_offset = 0;