Diff
Modified: trunk/Tools/ChangeLog (97884 => 97885)
--- trunk/Tools/ChangeLog 2011-10-19 22:31:55 UTC (rev 97884)
+++ trunk/Tools/ChangeLog 2011-10-19 22:33:39 UTC (rev 97885)
@@ -1,5 +1,26 @@
2011-10-19 David Levin <[email protected]>
+ watchlist: Should be more robust to bad regex.
+ https://bugs.webkit.org/show_bug.cgi?id=69486
+
+ Reviewed by Adam Barth.
+
+ * Scripts/webkitpy/common/config/watchlist: Change the instructions due to
+ watchlist being checked by check-webkit-style (bug 69487) and remove the .* from file
+ patterns since they are no longer anchored on the right hand side.
+ * Scripts/webkitpy/common/watchlist/amountchangedpattern.py: Change *pattern to take a compiled regex directly.
+ * Scripts/webkitpy/common/watchlist/amountchangedpattern_unittest.py: Ditto.
+ * Scripts/webkitpy/common/watchlist/changedlinepattern.py: Ditto.
+ * Scripts/webkitpy/common/watchlist/changedlinepattern_unittest.py: Ditto.
+ * Scripts/webkitpy/common/watchlist/filenamepattern.py: Ditto.
+ * Scripts/webkitpy/common/watchlist/filenamepattern_unittest.py: Ditto and change a test now that
+ filenames are no longer anchored on the right hand side.
+ * Scripts/webkitpy/common/watchlist/watchlistparser.py: Catch regex errors and log them as errors.
+ * Scripts/webkitpy/common/watchlist/watchlistparser_unittest.py: Add some tests to verify that bad regexes
+ don't kill everything.
+
+2011-10-19 David Levin <[email protected]>
+
watchlist: Add a stylecheck to do validity checks for the watchlist config.
https://bugs.webkit.org/show_bug.cgi?id=69487
Modified: trunk/Tools/Scripts/webkitpy/common/config/watchlist (97884 => 97885)
--- trunk/Tools/Scripts/webkitpy/common/config/watchlist 2011-10-19 22:31:55 UTC (rev 97884)
+++ trunk/Tools/Scripts/webkitpy/common/config/watchlist 2011-10-19 22:33:39 UTC (rev 97885)
@@ -3,7 +3,7 @@
# When editing this file, please run the following command to make sure you
# haven't introduced any syntax errors:
#
-# ./Tools/Scripts/test-webkitpy webkitpy.common.watchlist
+# ./Tools/Scripts/check-webkit-style
#
# If you want to test your regular expressions, you can edit various files and
# then try following command:
@@ -13,21 +13,21 @@
{
"DEFINITIONS": {
"ChromiumGraphics": {
- "filename": r"Source/WebCore/platform/graphics/chromium/.*",
+ "filename": r"Source/WebCore/platform/graphics/chromium/",
},
"ChromiumPublicApi": {
- "filename": r"Source/WebKit/chromium/public/.*"
+ "filename": r"Source/WebKit/chromium/public/"
},
"WebIDL": {
"filename": r"Source/WebCore/(?!inspector).*\.idl"
},
"ThreadingFiles": {
- "filename": r"Source/_javascript_Core/wtf/ThreadSpecific\..*"
- r"|Source/_javascript_Core/wtf/ThreadSafeRefCounted\..*"
- r"|Source/_javascript_Core/wtf/ThreadingPrimitives\..*"
- r"|Source/_javascript_Core/wtf/Threading\..*"
- r"|Source/WebCore/dom/CrossThreadTask\..*"
- r"|Source/WebCore/platform/CrossThreadCopier\..*",
+ "filename": r"Source/_javascript_Core/wtf/ThreadSpecific\."
+ r"|Source/_javascript_Core/wtf/ThreadSafeRefCounted\."
+ r"|Source/_javascript_Core/wtf/ThreadingPrimitives\."
+ r"|Source/_javascript_Core/wtf/Threading\."
+ r"|Source/WebCore/dom/CrossThreadTask\."
+ r"|Source/WebCore/platform/CrossThreadCopier\.",
},
"ThreadingUsage": {
# The intention of this regex is to detect places where people are using common threading mechanisms,
@@ -37,13 +37,13 @@
r"|createCallbackTask|crossThreadString|deprecatedTurnOffVerifier|threadsafeCopy)(?!\.(h|cpp))",
},
"WatchListScript": {
- "filename": r"Tools/Scripts/webkitpy/common/watchlist/.*",
+ "filename": r"Tools/Scripts/webkitpy/common/watchlist/",
},
"webkitpy": {
- "filename": r"Tools/Scripts/webkitpy/.*",
+ "filename": r"Tools/Scripts/webkitpy/",
},
"TestFailures": {
- "filename": r"Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/.*",
+ "filename": r"Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/",
},
"SecurityCritical": {
"more": r"[Ss]ecurityOrigin(?!\.(h|cpp))",
@@ -51,19 +51,19 @@
"filename": r"XSS|[Ss]ecurity",
},
"V8Bindings": {
- "filename": r"Source/WebCore/bindings/v8/.*",
+ "filename": r"Source/WebCore/bindings/v8/",
},
"BindingsScripts": {
- "filename": r"Source/WebCore/bindings/scripts/.*",
+ "filename": r"Source/WebCore/bindings/scripts/",
},
"FrameLoader": {
"more": r"FrameLoader\.(cpp|h)",
},
"Loader": {
- "filename": r"Source/WebCore/loader/.*",
+ "filename": r"Source/WebCore/loader/",
},
"StyleChecker": {
- "filename": r"Tools/Scripts/webkitpy/style/.*",
+ "filename": r"Tools/Scripts/webkitpy/style/",
},
},
"CC_RULES": {
@@ -81,7 +81,7 @@
"Loader": [ "[email protected]" ],
"SecurityCritical": [ "[email protected]" ],
"webkitpy": [ "[email protected]" ],
- "TestFailures": [ "[email protected]", "[email protected]" ],
+ "TestFailures": [ "[email protected]", "[email protected]" ],
},
"MESSAGE_RULES": {
"ChromiumPublicApi": [ "Please wait for approval from [email protected] before submitting "
Modified: trunk/Tools/Scripts/webkitpy/common/watchlist/amountchangedpattern.py (97884 => 97885)
--- trunk/Tools/Scripts/webkitpy/common/watchlist/amountchangedpattern.py 2011-10-19 22:31:55 UTC (rev 97884)
+++ trunk/Tools/Scripts/webkitpy/common/watchlist/amountchangedpattern.py 2011-10-19 22:33:39 UTC (rev 97885)
@@ -27,12 +27,9 @@
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-import re
-
-
class AmountChangedPattern:
- def __init__(self, regex, index_for_zero_value):
- self._regex = re.compile(regex)
+ def __init__(self, compile_regex, index_for_zero_value):
+ self._regex = compile_regex
self._index_for_zero_value = index_for_zero_value
def match(self, path, diff_file):
Modified: trunk/Tools/Scripts/webkitpy/common/watchlist/amountchangedpattern_unittest.py (97884 => 97885)
--- trunk/Tools/Scripts/webkitpy/common/watchlist/amountchangedpattern_unittest.py 2011-10-19 22:31:55 UTC (rev 97884)
+++ trunk/Tools/Scripts/webkitpy/common/watchlist/amountchangedpattern_unittest.py 2011-10-19 22:33:39 UTC (rev 97885)
@@ -30,7 +30,10 @@
'''Unit tests for amountchangedpattern.py.'''
+import re
import unittest
+
+
from webkitpy.common.watchlist.amountchangedpattern import AmountChangedPattern
@@ -47,18 +50,21 @@
(0, 3, 'both'),
)
+ def run_amount_changed_pattern_match(self, pattern, index_for_zero_value):
+ return AmountChangedPattern(re.compile(pattern), index_for_zero_value).match(None, self._DIFF_FILE)
+
def test_added_lines(self):
- self.assertTrue(AmountChangedPattern('hi', 0).match(None, self._DIFF_FILE))
- self.assertTrue(AmountChangedPattern('hi hi', 0).match(None, self._DIFF_FILE))
- self.assertFalse(AmountChangedPattern('other', 0).match(None, self._DIFF_FILE))
- self.assertFalse(AmountChangedPattern('both', 0).match(None, self._DIFF_FILE))
- self.assertFalse(AmountChangedPattern('bye', 0).match(None, self._DIFF_FILE))
- self.assertFalse(AmountChangedPattern('MatchesNothing', 0).match(None, self._DIFF_FILE))
+ self.assertTrue(self.run_amount_changed_pattern_match('hi', 0))
+ self.assertTrue(self.run_amount_changed_pattern_match('hi hi', 0))
+ self.assertFalse(self.run_amount_changed_pattern_match('other', 0))
+ self.assertFalse(self.run_amount_changed_pattern_match('both', 0))
+ self.assertFalse(self.run_amount_changed_pattern_match('bye', 0))
+ self.assertFalse(self.run_amount_changed_pattern_match('MatchesNothing', 0))
def test_removed_lines(self):
- self.assertFalse(AmountChangedPattern('hi', 1).match(None, self._DIFF_FILE))
- self.assertFalse(AmountChangedPattern('hi hi', 1).match(None, self._DIFF_FILE))
- self.assertFalse(AmountChangedPattern('other', 1).match(None, self._DIFF_FILE))
- self.assertFalse(AmountChangedPattern('both', 1).match(None, self._DIFF_FILE))
- self.assertTrue(AmountChangedPattern('bye', 1).match(None, self._DIFF_FILE))
- self.assertFalse(AmountChangedPattern('MatchesNothing', 1).match(None, self._DIFF_FILE))
+ self.assertFalse(self.run_amount_changed_pattern_match('hi', 1))
+ self.assertFalse(self.run_amount_changed_pattern_match('hi hi', 1))
+ self.assertFalse(self.run_amount_changed_pattern_match('other', 1))
+ self.assertFalse(self.run_amount_changed_pattern_match('both', 1))
+ self.assertTrue(self.run_amount_changed_pattern_match('bye', 1))
+ self.assertFalse(self.run_amount_changed_pattern_match('MatchesNothing', 1))
Modified: trunk/Tools/Scripts/webkitpy/common/watchlist/changedlinepattern.py (97884 => 97885)
--- trunk/Tools/Scripts/webkitpy/common/watchlist/changedlinepattern.py 2011-10-19 22:31:55 UTC (rev 97884)
+++ trunk/Tools/Scripts/webkitpy/common/watchlist/changedlinepattern.py 2011-10-19 22:33:39 UTC (rev 97885)
@@ -26,12 +26,10 @@
# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-import re
-
class ChangedLinePattern:
- def __init__(self, regex, index_for_zero_value):
- self._regex = re.compile(regex)
+ def __init__(self, compile_regex, index_for_zero_value):
+ self._regex = compile_regex
self._index_for_zero_value = index_for_zero_value
def match(self, path, diff_file):
Modified: trunk/Tools/Scripts/webkitpy/common/watchlist/changedlinepattern_unittest.py (97884 => 97885)
--- trunk/Tools/Scripts/webkitpy/common/watchlist/changedlinepattern_unittest.py 2011-10-19 22:31:55 UTC (rev 97884)
+++ trunk/Tools/Scripts/webkitpy/common/watchlist/changedlinepattern_unittest.py 2011-10-19 22:33:39 UTC (rev 97885)
@@ -28,7 +28,10 @@
'''Unit tests for changedlinepattern.py.'''
+import re
import unittest
+
+
from webkitpy.common.watchlist.changedlinepattern import ChangedLinePattern
@@ -45,18 +48,21 @@
(0, 3, 'both'),
)
+ def run_changed_line_pattern_match(self, pattern, index_for_zero_value):
+ return ChangedLinePattern(re.compile(pattern), index_for_zero_value).match(None, self._DIFF_FILE)
+
def test_added_lines(self):
- self.assertTrue(ChangedLinePattern('hi', 0).match(None, self._DIFF_FILE))
- self.assertTrue(ChangedLinePattern('h.', 0).match(None, self._DIFF_FILE))
- self.assertTrue(ChangedLinePattern('both', 0).match(None, self._DIFF_FILE))
- self.assertFalse(ChangedLinePattern('bye', 0).match(None, self._DIFF_FILE))
- self.assertFalse(ChangedLinePattern('y', 0).match(None, self._DIFF_FILE))
- self.assertFalse(ChangedLinePattern('other', 0).match(None, self._DIFF_FILE))
+ self.assertTrue(self.run_changed_line_pattern_match('hi', 0))
+ self.assertTrue(self.run_changed_line_pattern_match('h.', 0))
+ self.assertTrue(self.run_changed_line_pattern_match('both', 0))
+ self.assertFalse(self.run_changed_line_pattern_match('bye', 0))
+ self.assertFalse(self.run_changed_line_pattern_match('y', 0))
+ self.assertFalse(self.run_changed_line_pattern_match('other', 0))
def test_removed_lines(self):
- self.assertFalse(ChangedLinePattern('hi', 1).match(None, self._DIFF_FILE))
- self.assertFalse(ChangedLinePattern('h.', 1).match(None, self._DIFF_FILE))
- self.assertTrue(ChangedLinePattern('both', 1).match(None, self._DIFF_FILE))
- self.assertTrue(ChangedLinePattern('bye', 1).match(None, self._DIFF_FILE))
- self.assertTrue(ChangedLinePattern('y', 1).match(None, self._DIFF_FILE))
- self.assertFalse(ChangedLinePattern('other', 1).match(None, self._DIFF_FILE))
+ self.assertFalse(self.run_changed_line_pattern_match('hi', 1))
+ self.assertFalse(self.run_changed_line_pattern_match('h.', 1))
+ self.assertTrue(self.run_changed_line_pattern_match('both', 1))
+ self.assertTrue(self.run_changed_line_pattern_match('bye', 1))
+ self.assertTrue(self.run_changed_line_pattern_match('y', 1))
+ self.assertFalse(self.run_changed_line_pattern_match('other', 1))
Modified: trunk/Tools/Scripts/webkitpy/common/watchlist/filenamepattern.py (97884 => 97885)
--- trunk/Tools/Scripts/webkitpy/common/watchlist/filenamepattern.py 2011-10-19 22:31:55 UTC (rev 97884)
+++ trunk/Tools/Scripts/webkitpy/common/watchlist/filenamepattern.py 2011-10-19 22:33:39 UTC (rev 97885)
@@ -26,12 +26,10 @@
# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-import re
-
class FilenamePattern:
- def __init__(self, regex):
- self._regex = re.compile(regex + '$')
+ def __init__(self, compiled_regex):
+ self._regex = compiled_regex
def match(self, path, diff_file):
return self._regex.match(path)
Modified: trunk/Tools/Scripts/webkitpy/common/watchlist/filenamepattern_unittest.py (97884 => 97885)
--- trunk/Tools/Scripts/webkitpy/common/watchlist/filenamepattern_unittest.py 2011-10-19 22:31:55 UTC (rev 97884)
+++ trunk/Tools/Scripts/webkitpy/common/watchlist/filenamepattern_unittest.py 2011-10-19 22:33:39 UTC (rev 97885)
@@ -26,22 +26,25 @@
# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+import re
import unittest
+
+
from webkitpy.common.watchlist.filenamepattern import FilenamePattern
class FileNamePatternTest(unittest.TestCase):
def test_filename_pattern_literal(self):
- filename_pattern = FilenamePattern(r'MyFileName\.cpp')
+ filename_pattern = FilenamePattern(re.compile(r'MyFileName\.cpp'))
# Note the follow filenames are not regex.
self.assertTrue(filename_pattern.match('MyFileName.cpp', None))
- self.assertFalse(filename_pattern.match('MyFileName.cppa', None))
+ self.assertTrue(filename_pattern.match('MyFileName.cppa', None))
self.assertFalse(filename_pattern.match('aMyFileName.cpp', None))
self.assertFalse(filename_pattern.match('MyFileNamebcpp', None))
def test_filename_pattern_substring(self):
- filename_pattern = FilenamePattern(r'.*\\MyFileName\..*')
+ filename_pattern = FilenamePattern(re.compile(r'.*\\MyFileName\..*'))
# Note the follow filenames are not regex.
self.assertTrue(filename_pattern.match(r'\\MyFileName.cpp', None))
Modified: trunk/Tools/Scripts/webkitpy/common/watchlist/watchlistparser.py (97884 => 97885)
--- trunk/Tools/Scripts/webkitpy/common/watchlist/watchlistparser.py 2011-10-19 22:31:55 UTC (rev 97884)
+++ trunk/Tools/Scripts/webkitpy/common/watchlist/watchlistparser.py 2011-10-19 22:33:39 UTC (rev 97885)
@@ -57,10 +57,10 @@
}
self._definition_pattern_parsers = {
'filename': FilenamePattern,
- 'in_added_lines': (lambda regex: ChangedLinePattern(regex, 0)),
- 'in_deleted_lines': (lambda regex: ChangedLinePattern(regex, 1)),
- 'less': (lambda regex: AmountChangedPattern(regex, 1)),
- 'more': (lambda regex: AmountChangedPattern(regex, 0)),
+ 'in_added_lines': (lambda compiled_regex: ChangedLinePattern(compiled_regex, 0)),
+ 'in_deleted_lines': (lambda compiled_regex: ChangedLinePattern(compiled_regex, 1)),
+ 'less': (lambda compiled_regex: AmountChangedPattern(compiled_regex, 1)),
+ 'more': (lambda compiled_regex: AmountChangedPattern(compiled_regex, 0)),
}
def parse(self, watch_list_contents):
@@ -109,7 +109,13 @@
% (pattern_type, name))
continue
- pattern = pattern_parser(definition[pattern_type])
+ try:
+ compiled_regex = re.compile(definition[pattern_type])
+ except Exception, e:
+ self._log_error('The regex "%s" is invalid due to "%s".' % (definition[pattern_type], str(e)))
+ continue
+
+ pattern = pattern_parser(compiled_regex)
definitions[name].append(pattern)
if not definitions[name]:
self._log_error('The definition "%s" has no patterns, so it should be deleted.' % name)
Modified: trunk/Tools/Scripts/webkitpy/common/watchlist/watchlistparser_unittest.py (97884 => 97885)
--- trunk/Tools/Scripts/webkitpy/common/watchlist/watchlistparser_unittest.py 2011-10-19 22:31:55 UTC (rev 97884)
+++ trunk/Tools/Scripts/webkitpy/common/watchlist/watchlistparser_unittest.py 2011-10-19 22:33:39 UTC (rev 97885)
@@ -67,6 +67,40 @@
OutputCapture().assert_outputs(self, self._watch_list_parser.parse, args=[watch_list],
expected_logs='Invalid character "|" in definition "WatchList1|A".\n')
+ def test_bad_filename_regex(self):
+ watch_list = (
+ '{'
+ ' "DEFINITIONS": {'
+ ' "WatchList1": {'
+ ' "filename": r"*",'
+ ' "more": r"RefCounted",'
+ ' },'
+ ' },'
+ ' "CC_RULES": {'
+ ' "WatchList1": ["[email protected]"],'
+ ' },'
+ '}')
+
+ OutputCapture().assert_outputs(self, self._watch_list_parser.parse, args=[watch_list],
+ expected_logs='The regex "*" is invalid due to "nothing to repeat".\n')
+
+ def test_bad_more_regex(self):
+ watch_list = (
+ '{'
+ ' "DEFINITIONS": {'
+ ' "WatchList1": {'
+ ' "filename": r"aFileName\\.cpp",'
+ ' "more": r"*",'
+ ' },'
+ ' },'
+ ' "CC_RULES": {'
+ ' "WatchList1": ["[email protected]"],'
+ ' },'
+ '}')
+
+ OutputCapture().assert_outputs(self, self._watch_list_parser.parse, args=[watch_list],
+ expected_logs='The regex "*" is invalid due to "nothing to repeat".\n')
+
def test_bad_match_type(self):
watch_list = (
'{'
