Fix greediness in Apache License linter.

Summary: The multi-line comment regexp was potentially too greedy. See
"greedy.lint-test".
	- Made it less greedy.
	- Added test coverage.
	- Fixed an issue with the Apache license getting applied with too much
	  whitespace against C files.

Test Plan: Ran unit tests.

Reviewers: aran

CC:

Differential Revision: 36

src/__phutil_library_map__.php

@@ -10,6 +10,7 @@ phutil_register_library_map(array(
   array(
     'ArcanistAmendWorkflow' => 'workflow/amend',
     'ArcanistApacheLicenseLinter' => 'lint/linter/apachelicense',
+    'ArcanistApacheLicenseLinterTestCase' => 'lint/linter/apachelicense/__tests__',
     'ArcanistBaseUnitTestEngine' => 'unit/engine/base',
     'ArcanistBaseWorkflow' => 'workflow/base',
     'ArcanistBundle' => 'parser/bundle',
@@ -43,6 +44,7 @@ phutil_register_library_map(array(
     'ArcanistLintSeverity' => 'lint/severity',
     'ArcanistLintWorkflow' => 'workflow/lint',
     'ArcanistLinter' => 'lint/linter/base',
+    'ArcanistLinterTestCase' => 'lint/linter/base/test',
     'ArcanistListWorkflow' => 'workflow/list',
     'ArcanistMarkCommittedWorkflow' => 'workflow/mark-committed',
     'ArcanistNoEffectException' => 'exception/usage/noeffect',
@@ -77,6 +79,7 @@ phutil_register_library_map(array(
   array(
     'ArcanistAmendWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistApacheLicenseLinter' => 'ArcanistLicenseLinter',
+    'ArcanistApacheLicenseLinterTestCase' => 'ArcanistLinterTestCase',
     'ArcanistCommitWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistCoverWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistDiffParserTestCase' => 'ArcanistPhutilTestCase',
@@ -89,6 +92,7 @@ phutil_register_library_map(array(
     'ArcanistHelpWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistLicenseLinter' => 'ArcanistLinter',
     'ArcanistLintWorkflow' => 'ArcanistBaseWorkflow',
+    'ArcanistLinterTestCase' => 'ArcanistPhutilTestCase',
     'ArcanistListWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistMarkCommittedWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistNoEffectException' => 'ArcanistUsageException',
@@ -103,7 +107,7 @@ phutil_register_library_map(array(
     'ArcanistUnitWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistUserAbortException' => 'ArcanistUsageException',
     'ArcanistXHPASTLinter' => 'ArcanistLinter',
-    'ArcanistXHPASTLinterTestCase' => 'ArcanistPhutilTestCase',
+    'ArcanistXHPASTLinterTestCase' => 'ArcanistLinterTestCase',
     'PhutilLintEngine' => 'ArcanistLintEngine',
     'PhutilUnitTestEngine' => 'ArcanistBaseUnitTestEngine',
     'PhutilUnitTestEngineTestCase' => 'ArcanistPhutilTestCase',

src/lint/linter/apachelicense/ArcanistApacheLicenseLinter.php

@@ -50,7 +50,14 @@ EOLICENSE;
     $maybe_php_or_script = '(#![^\n]+?[\n])?(<[?]php\s+?)?';
     return array(
       "@^{$maybe_php_or_script}//[^\n]*Copyright[^\n]*[\n]\s*@i",
-      "@^{$maybe_php_or_script}/[*].*?Copyright.*?[*]/\s*@is",
+
+      // We need to be careful about matching after "/*", since otherwise we'll
+      // end up in trouble on code like this, and consume the entire thing:
+      //
+      //  /* a */
+      //  copyright();
+      //  /* b */
+      "@^{$maybe_php_or_script}/[*](?:[^*]|[*][^/])*?Copyright.*?[*]/\s*@is",
       "@^{$maybe_php_or_script}\s*@",
     );
   }

src/lint/linter/apachelicense/__tests__/ArcanistApacheLicenseLinterTestCase.php

@@ -0,0 +1,26 @@
+<?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 ArcanistApacheLicenseLinterTestCase extends ArcanistLinterTestCase {
+
+  public function testApacheLicenseLint() {
+    $linter = new ArcanistApacheLicenseLinter();
+    return $this->executeTestsInDirectory(dirname(__FILE__).'/data/', $linter);
+  }
+
+}

src/lint/linter/apachelicense/__tests__/__init__.php

@@ -0,0 +1,13 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('arcanist', 'lint/linter/apachelicense');
+phutil_require_module('arcanist', 'lint/linter/base/test');
+
+
+phutil_require_source('ArcanistApacheLicenseLinterTestCase.php');

src/lint/linter/apachelicense/__tests__/data/c-basic.lint-test

@@ -0,0 +1,29 @@
+#include <stdio>
+
+int main(int argv, char **argv) {
+  return 0;
+}
+~~~~~~~~~~
+error:1:1
+~~~~~~~~~~
+/*
+ * 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.
+ */
+
+#include <stdio>
+
+int main(int argv, char **argv) {
+  return 0;
+}

src/lint/linter/apachelicense/__tests__/data/greedy.lint-test

@@ -0,0 +1,37 @@
+<?php
+
+/* Some Comment */
+
+function Copyright(Copyright $copyright) {
+  return new Copyright($copyright);
+}
+
+/* More Comments */
+~~~~~~~~~~
+error:1:1
+~~~~~~~~~~
+<?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.
+ */
+
+/* Some Comment */
+
+function Copyright(Copyright $copyright) {
+  return new Copyright($copyright);
+}
+
+/* More Comments */

src/lint/linter/apachelicense/__tests__/data/php-basic.lint-test

@@ -0,0 +1,25 @@
+<?php
+
+do_stuff();
+~~~~~~~~~~
+error:1:1
+~~~~~~~~~~
+<?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.
+ */
+
+do_stuff();

src/lint/linter/apachelicense/__tests__/data/php-script.lint-test

@@ -0,0 +1,27 @@
+#!/usr/bin/php
+<?php
+
+do_stuff();
+~~~~~~~~~~
+error:1:1
+~~~~~~~~~~
+#!/usr/bin/php
+<?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.
+ */
+
+do_stuff();

src/lint/linter/apachelicense/__tests__/data/php-update-mess.lint-test

@@ -0,0 +1,29 @@
+<?php
+
+/*****COPYRIGHT
+  COPYRIGHT // ************** // COPYRIGHT
+**/
+
+do_stuff();
+~~~~~~~~~~
+error:1:1
+~~~~~~~~~~
+<?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.
+ */
+
+do_stuff();

src/lint/linter/apachelicense/__tests__/data/php-update-multi.lint-test

@@ -0,0 +1,27 @@
+<?php
+
+/* Copyright 1901 Facebook */
+
+do_stuff();
+~~~~~~~~~~
+error:1:1
+~~~~~~~~~~
+<?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.
+ */
+
+do_stuff();

src/lint/linter/apachelicense/__tests__/data/php-update-single.lint-test

@@ -0,0 +1,27 @@
+<?php
+
+/* Copyright 1901 Facebook */
+
+do_stuff();
+~~~~~~~~~~
+error:1:1
+~~~~~~~~~~
+<?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.
+ */
+
+do_stuff();

src/lint/linter/base/test/ArcanistLinterTestCase.php

@@ -0,0 +1,172 @@
+<?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 ArcanistLinterTestCase extends ArcanistPhutilTestCase {
+
+  public function executeTestsInDirectory($root, $linter) {
+    foreach (Filesystem::listDirectory($root, $hidden = false) as $file) {
+      $this->lintFile($root.$file, $linter);
+    }
+  }
+
+  private function lintFile($file, $linter) {
+    $linter = clone $linter;
+
+    $working_copy = ArcanistWorkingCopyIdentity::newFromPath(__FILE__);
+
+    $contents = Filesystem::readFile($file);
+    $contents = explode("~~~~~~~~~~\n", $contents);
+    if (count($contents) < 2) {
+      throw new Exception(
+        "Expected '~~~~~~~~~~' separating test case and results.");
+    }
+
+    list ($data, $expect, $xform, $config) = array_merge(
+      $contents,
+      array(null, null));
+
+    $basename = basename($file);
+
+    if ($config) {
+      $config = json_decode($config, true);
+      if (!is_array($config)) {
+        throw new Exception(
+          "Invalid configuration in test '{$basename}', not valid JSON.");
+      }
+    } else {
+      $config = array();
+    }
+
+    /* TODO: ?
+    validate_parameter_list(
+      $config,
+      array(
+      ),
+      array(
+        'project' => true,
+        'path' => true,
+        'hook' => true,
+      ));
+    */
+
+    $exception = null;
+    $after_lint = null;
+    $messages = null;
+    $exception_message = false;
+    $caught_exception = false;
+    try {
+
+      $path = idx($config, 'path', 'lint/'.$basename.'.php');
+
+      $engine = new UnitTestableArcanistLintEngine();
+      $engine->setWorkingCopy($working_copy);
+      $engine->setPaths(array($path));
+
+//  TODO: restore this
+//      $engine->setCommitHookMode(idx($config, 'hook', false));
+
+      $linter->addPath($path);
+      $linter->addData($path, $data);
+
+      $engine->addLinter($linter);
+      $engine->addFileData($path, $data);
+
+      $results = $engine->run();
+      $this->assertEqual(
+        1,
+        count($results),
+        'Expect one result returned by linter.');
+
+      $result = reset($results);
+      $patcher = ArcanistLintPatcher::newFromArcanistLintResult($result);
+      $after_lint = $patcher->getModifiedFileContent();
+
+    } catch (ArcanistPhutilTestTerminatedException $ex) {
+      throw $ex;
+    } catch (Exception $exception) {
+      $caught_exception = true;
+      $exception_message = $exception->getMessage()."\n\n".
+                           $exception->getTraceAsString();
+    }
+
+    switch ($basename) {
+      default:
+        $this->assertEqual(false, $caught_exception, $exception_message);
+        $this->compareLint($basename, $expect, $result);
+        $this->compareTransform($xform, $after_lint);
+        break;
+    }
+  }
+
+  private function compareLint($file, $expect, $result) {
+    $seen = array();
+    $raised = array();
+    foreach ($result->getMessages() as $message) {
+      $sev = $message->getSeverity();
+      $line = $message->getLine();
+      $char = $message->getChar();
+      $code = $message->getCode();
+      $name = $message->getName();
+      $seen[] = $sev.":".$line.":".$char;
+      $raised[] = "  {$sev} at line {$line}, char {$char}: {$code} {$name}";
+    }
+    $expect = trim($expect);
+    if ($expect) {
+      $expect = explode("\n", $expect);
+    } else {
+      $expect = array();
+    }
+    foreach ($expect as $key => $expected) {
+      $expect[$key] = reset(explode(' ', $expected));
+    }
+
+    $expect = array_fill_keys($expect, true);
+    $seen   = array_fill_keys($seen, true);
+
+    if (!$raised) {
+      $raised = array("No messages.");
+    }
+    $raised = "Actually raised:\n".implode("\n", $raised);
+
+    foreach (array_diff_key($expect, $seen) as $missing => $ignored) {
+      list($sev, $line, $char) = explode(':', $missing);
+      $this->assertFailure(
+        "In '{$file}', ".
+        "expected lint to raise {$sev} on line {$line} at char {$char}, ".
+        "but no {$sev} was raised. {$raised}");
+    }
+
+    foreach (array_diff_key($seen, $expect) as $surprising => $ignored) {
+      list($sev, $line, $char) = explode(':', $surprising);
+      $this->assertFailure(
+        "In '{$file}', ".
+        "lint raised {$sev} on line {$line} at char {$char}, ".
+        "but nothing was expected. {$raised}");
+    }
+  }
+
+  private function compareTransform($expected, $actual) {
+    if (!strlen($expected)) {
+      return;
+    }
+    $this->assertEqual(
+      $expected,
+      $actual,
+      "File as patched by lint did not match the expected patched file.");
+  }
+}

src/lint/linter/base/test/__init__.php

@@ -0,0 +1,18 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('arcanist', 'lint/engine/test');
+phutil_require_module('arcanist', 'lint/patcher');
+phutil_require_module('arcanist', 'unit/engine/phutil/testcase');
+phutil_require_module('arcanist', 'workingcopyidentity');
+
+phutil_require_module('phutil', 'filesystem');
+phutil_require_module('phutil', 'utils');
+
+
+phutil_require_source('ArcanistLinterTestCase.php');

src/lint/linter/license/ArcanistLicenseLinter.php

@@ -68,7 +68,7 @@ abstract class ArcanistLicenseLinter extends ArcanistLinter {
             self::LINT_NO_LICENSE_HEADER,
             'This file has a missing or out of date license header.',
             $matches[0],
-            $expect);
+            ltrim($expect));
         }
         break;
       }

src/lint/linter/xhpast/__tests__/ArcanistXHPASTLinterTestCase.php

@@ -16,157 +16,11 @@
  * limitations under the License.
  */
 
-class ArcanistXHPASTLinterTestCase extends ArcanistPhutilTestCase {
+class ArcanistXHPASTLinterTestCase extends ArcanistLinterTestCase {
 
   public function testXHPASTLint() {
-    $root = realpath(dirname(__FILE__)).'/data/';
-    foreach (Filesystem::listDirectory($root, $hidden = false) as $file) {
-      $this->lintFile($root.$file);
-    }
+    $linter = new ArcanistXHPASTLinter();
+    return $this->executeTestsInDirectory(dirname(__FILE__).'/data/', $linter);
   }
 
-  private function lintFile($file) {
-    $working_copy = ArcanistWorkingCopyIdentity::newFromPath(__FILE__);
-
-    $contents = Filesystem::readFile($file);
-    $contents = explode("~~~~~~~~~~\n", $contents);
-    if (count($contents) < 2) {
-      throw new Exception(
-        "Expected '~~~~~~~~~~' separating test case and results.");
-    }
-
-    list ($data, $expect, $xform, $config) = array_merge(
-      $contents,
-      array(null, null));
-
-    $basename = basename($file);
-
-    if ($config) {
-      $config = json_decode($config, true);
-      if (!is_array($config)) {
-        throw new Exception(
-          "Invalid configuration in test '{$basename}', not valid JSON.");
-      }
-    } else {
-      $config = array();
-    }
-
-    /* TODO: ?
-    validate_parameter_list(
-      $config,
-      array(
-      ),
-      array(
-        'project' => true,
-        'path' => true,
-        'hook' => true,
-      ));
-    */
-
-    $exception = null;
-    $after_lint = null;
-    $messages = null;
-    $exception_message = false;
-    $caught_exception = false;
-    try {
-
-      $path = idx($config, 'path', 'lint/'.$basename.'.php');
-
-      $engine = new UnitTestableArcanistLintEngine();
-      $engine->setWorkingCopy($working_copy);
-      $engine->setPaths(array($path));
-
-//  TODO: restore this
-//      $engine->setCommitHookMode(idx($config, 'hook', false));
-
-      $linter = new ArcanistXHPASTLinter();
-      $linter->addPath($path);
-      $linter->addData($path, $data);
-
-      $engine->addLinter($linter);
-      $engine->addFileData($path, $data);
-
-      $results = $engine->run();
-      $this->assertEqual(
-        1,
-        count($results),
-        'Expect one result returned by linter.');
-
-      $result = reset($results);
-      $patcher = ArcanistLintPatcher::newFromArcanistLintResult($result);
-      $after_lint = $patcher->getModifiedFileContent();
-
-    } catch (ArcanistPhutilTestTerminatedException $ex) {
-      throw $ex;
-    } catch (Exception $exception) {
-      $caught_exception = true;
-      $exception_message = $exception->getMessage()."\n\n".
-                           $exception->getTraceAsString();
-    }
-
-    switch ($basename) {
-      default:
-        $this->assertEqual(false, $caught_exception, $exception_message);
-        $this->compareLint($basename, $expect, $result);
-        $this->compareTransform($xform, $after_lint);
-        break;
-    }
-  }
-
-  private function compareLint($file, $expect, $result) {
-    $seen = array();
-    $raised = array();
-    foreach ($result->getMessages() as $message) {
-      $sev = $message->getSeverity();
-      $line = $message->getLine();
-      $char = $message->getChar();
-      $code = $message->getCode();
-      $name = $message->getName();
-      $seen[] = $sev.":".$line.":".$char;
-      $raised[] = "  {$sev} at line {$line}, char {$char}: {$code} {$name}";
-    }
-    $expect = trim($expect);
-    if ($expect) {
-      $expect = explode("\n", $expect);
-    } else {
-      $expect = array();
-    }
-    foreach ($expect as $key => $expected) {
-      $expect[$key] = reset(explode(' ', $expected));
-    }
-
-    $expect = array_fill_keys($expect, true);
-    $seen   = array_fill_keys($seen, true);
-
-    if (!$raised) {
-      $raised = array("No messages.");
-    }
-    $raised = "Actually raised:\n".implode("\n", $raised);
-
-    foreach (array_diff_key($expect, $seen) as $missing => $ignored) {
-      list($sev, $line, $char) = explode(':', $missing);
-      $this->assertFailure(
-        "In '{$file}', ".
-        "expected lint to raise {$sev} on line {$line} at char {$char}, ".
-        "but no {$sev} was raised. {$raised}");
-    }
-
-    foreach (array_diff_key($seen, $expect) as $surprising => $ignored) {
-      list($sev, $line, $char) = explode(':', $surprising);
-      $this->assertFailure(
-        "In '{$file}', ".
-        "lint raised {$sev} on line {$line} at char {$char}, ".
-        "but nothing was expected. {$raised}");
-    }
-  }
-
-  private function compareTransform($expected, $actual) {
-    if (!strlen($expected)) {
-      return;
-    }
-    $this->assertEqual(
-      $expected,
-      $actual,
-      "File as patched by lint did not match the expected patched file.");
-  }
 }

src/lint/linter/xhpast/__tests__/__init__.php

@@ -6,14 +6,8 @@
 
 
 
-phutil_require_module('arcanist', 'lint/engine/test');
+phutil_require_module('arcanist', 'lint/linter/base/test');
 phutil_require_module('arcanist', 'lint/linter/xhpast');
-phutil_require_module('arcanist', 'lint/patcher');
-phutil_require_module('arcanist', 'unit/engine/phutil/testcase');
-phutil_require_module('arcanist', 'workingcopyidentity');
-
-phutil_require_module('phutil', 'filesystem');
-phutil_require_module('phutil', 'utils');
 
 
 phutil_require_source('ArcanistXHPASTLinterTestCase.php');