2010-12-24 Eric Seidel <eric@webkit.org>
authoreric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 24 Dec 2010 18:21:28 +0000 (18:21 +0000)
committereric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 24 Dec 2010 18:21:28 +0000 (18:21 +0000)
        Reviewed by Adam Barth.

        webkit-patch (or a pre-commit hook) needs to prevent bad ChangeLog changes
        https://bugs.webkit.org/show_bug.cgi?id=28291

        This is a start.  At least now webkit-patch will prompt when your ChangeLog looks questionable.
        We could do more advanced things, like parsing the ChangeLog (with changelog.py) and comparing that
        to strings with find in the diff.
        Since non-interactive always returns the default, this should cause patches with bad changelogs to fail on the commit-queue.

        * Scripts/webkitpy/common/checkout/api.py:
        * Scripts/webkitpy/common/checkout/diff_parser.py:
        * Scripts/webkitpy/tool/steps/abstractstep.py:
        * Scripts/webkitpy/tool/steps/cleanworkingdirectory.py:
        * Scripts/webkitpy/tool/steps/validatechangelogs.py: Copied from Tools/Scripts/webkitpy/tool/steps/validatereviewer.py.
        * Scripts/webkitpy/tool/steps/validatechangelogs_unittest.py: Copied from Tools/Scripts/webkitpy/tool/steps/cleanworkingdirectory.py.
        * Scripts/webkitpy/tool/steps/validatereviewer.py:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@74639 268f45cc-cd09-0410-ab3c-d52691b4dbfc

14 files changed:
Tools/ChangeLog
Tools/Scripts/webkitpy/common/checkout/api.py
Tools/Scripts/webkitpy/common/checkout/diff_parser.py
Tools/Scripts/webkitpy/tool/commands/download.py
Tools/Scripts/webkitpy/tool/commands/download_unittest.py
Tools/Scripts/webkitpy/tool/commands/upload.py
Tools/Scripts/webkitpy/tool/steps/__init__.py
Tools/Scripts/webkitpy/tool/steps/abstractstep.py
Tools/Scripts/webkitpy/tool/steps/cleanworkingdirectory.py
Tools/Scripts/webkitpy/tool/steps/commit.py
Tools/Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py
Tools/Scripts/webkitpy/tool/steps/validatechangelogs.py [new file with mode: 0644]
Tools/Scripts/webkitpy/tool/steps/validatechangelogs_unittest.py [new file with mode: 0644]
Tools/Scripts/webkitpy/tool/steps/validatereviewer.py

index 3d2c8cc47c96d7bfee465620e2f3ef8e14efe913..cbe6ba627141d26f9f0828bc31e831e5f67f238f 100644 (file)
@@ -1,3 +1,23 @@
+2010-12-24  Eric Seidel  <eric@webkit.org>
+
+        Reviewed by Adam Barth.
+
+        webkit-patch (or a pre-commit hook) needs to prevent bad ChangeLog changes
+        https://bugs.webkit.org/show_bug.cgi?id=28291
+
+        This is a start.  At least now webkit-patch will prompt when your ChangeLog looks questionable.
+        We could do more advanced things, like parsing the ChangeLog (with changelog.py) and comparing that
+        to strings with find in the diff.
+        Since non-interactive always returns the default, this should cause patches with bad changelogs to fail on the commit-queue.
+
+        * Scripts/webkitpy/common/checkout/api.py:
+        * Scripts/webkitpy/common/checkout/diff_parser.py:
+        * Scripts/webkitpy/tool/steps/abstractstep.py:
+        * Scripts/webkitpy/tool/steps/cleanworkingdirectory.py:
+        * Scripts/webkitpy/tool/steps/validatechangelogs.py: Copied from Tools/Scripts/webkitpy/tool/steps/validatereviewer.py.
+        * Scripts/webkitpy/tool/steps/validatechangelogs_unittest.py: Copied from Tools/Scripts/webkitpy/tool/steps/cleanworkingdirectory.py.
+        * Scripts/webkitpy/tool/steps/validatereviewer.py:
+
 2010-12-24  Dirk Pranke  <dpranke@chromium.org>
 
         Reviewed by Kenneth Russell.
index 635798287424b6341ea702cff2c0d52e383649de..29e43d3d5c7e331ad75ddd088be005d621f3b112 100644 (file)
@@ -39,14 +39,14 @@ from webkitpy.common.system.executive import Executive, run_command, ScriptError
 from webkitpy.common.system.deprecated_logging import log
 
 
-# This class represents the WebKit-specific parts of the checkout (like
-# ChangeLogs).
+# This class represents the WebKit-specific parts of the checkout (like ChangeLogs).
 # FIXME: Move a bunch of ChangeLog-specific processing from SCM to this object.
+# NOTE: All paths returned from this class should be absolute.
 class Checkout(object):
     def __init__(self, scm):
         self._scm = scm
 
-    def _is_path_to_changelog(self, path):
+    def is_path_to_changelog(self, path):
         return os.path.basename(path) == "ChangeLog"
 
     def _latest_entry_for_changelog_at_revision(self, changelog_path, revision):
@@ -59,7 +59,7 @@ class Checkout(object):
 
     def changelog_entries_for_revision(self, revision):
         changed_files = self._scm.changed_files_for_revision(revision)
-        return [self._latest_entry_for_changelog_at_revision(path, revision) for path in changed_files if self._is_path_to_changelog(path)]
+        return [self._latest_entry_for_changelog_at_revision(path, revision) for path in changed_files if self.is_path_to_changelog(path)]
 
     @memoized
     def commit_info_for_revision(self, revision):
@@ -96,10 +96,10 @@ class Checkout(object):
         return [path for path in absolute_paths if predicate(path)]
 
     def modified_changelogs(self, git_commit, changed_files=None):
-        return self._modified_files_matching_predicate(git_commit, self._is_path_to_changelog, changed_files=changed_files)
+        return self._modified_files_matching_predicate(git_commit, self.is_path_to_changelog, changed_files=changed_files)
 
     def modified_non_changelogs(self, git_commit, changed_files=None):
-        return self._modified_files_matching_predicate(git_commit, lambda path: not self._is_path_to_changelog(path), changed_files=changed_files)
+        return self._modified_files_matching_predicate(git_commit, lambda path: not self.is_path_to_changelog(path), changed_files=changed_files)
 
     def commit_message_for_this_commit(self, git_commit, changed_files=None):
         changelog_paths = self.modified_changelogs(git_commit, changed_files)
index a6ea756026dc0d06ed2d116f6062714bd8710539..5a5546cde72a3a794f4bb376523d06ae9897407c 100644 (file)
@@ -118,6 +118,8 @@ class DiffFile(object):
         self.lines.append((deleted_line_number, new_line_number, line))
 
 
+# If this is going to be called DiffParser, it should be a re-useable parser.
+# Otherwise we should rename it to ParsedDiff or just Diff.
 class DiffParser(object):
     """A parser for a patch file.
 
@@ -125,16 +127,18 @@ class DiffParser(object):
     a DiffFile object.
     """
 
-    # FIXME: This function is way too long and needs to be broken up.
     def __init__(self, diff_input):
         """Parses a diff.
 
         Args:
           diff_input: An iterable object.
         """
-        state = _INITIAL_STATE
+        self.files = self._parse_into_diff_files(diff_input)
 
-        self.files = {}
+    # FIXME: This function is way too long and needs to be broken up.
+    def _parse_into_diff_files(self, diff_input):
+        files = {}
+        state = _INITIAL_STATE
         current_file = None
         old_diff_line = None
         new_diff_line = None
@@ -148,7 +152,7 @@ class DiffParser(object):
             if file_declaration:
                 filename = file_declaration.group('FilePath')
                 current_file = DiffFile(filename)
-                self.files[filename] = current_file
+                files[filename] = current_file
                 state = _DECLARED_FILE_PATH
                 continue
 
@@ -179,3 +183,4 @@ class DiffParser(object):
                 else:
                     _log.error('Unexpected diff format when parsing a '
                                'chunk: %r' % line)
+        return files
index 020f33976dc8a101dd986be40ce886726d5684ea..1b478bffaba649ecfd0ba717d7924732ba193f85 100644 (file)
@@ -95,6 +95,7 @@ class Land(AbstractSequencedCommand):
         steps.EnsureBuildersAreGreen,
         steps.UpdateChangeLogsWithReviewer,
         steps.ValidateReviewer,
+        steps.ValidateChangeLogs, # We do this after UpdateChangeLogsWithReviewer to avoid not having to cache the diff twice.
         steps.Build,
         steps.RunTests,
         steps.Commit,
@@ -257,6 +258,7 @@ class AbstractPatchLandingCommand(AbstractPatchSequencingCommand):
         steps.CleanWorkingDirectory,
         steps.Update,
         steps.ApplyPatch,
+        steps.ValidateChangeLogs,
         steps.ValidateReviewer,
         steps.Build,
         steps.RunTests,
index 3748a8f4139ff8933dd2aa1595722b084b9eae74..ba23ab98f52eb9c5e30be807b2645806fd8a8e67 100644 (file)
@@ -109,11 +109,11 @@ class DownloadCommandsTest(CommandsTest):
     def test_land_diff(self):
         expected_stderr = "Building WebKit\nRunning Python unit tests\nRunning Perl unit tests\nRunning JavaScriptCore tests\nRunning run-webkit-tests\nCommitted r49824: <http://trac.webkit.org/changeset/49824>\nUpdating bug 42\n"
         mock_tool = MockTool()
-        mock_tool.scm().create_patch = Mock()
+        mock_tool.scm().create_patch = Mock(return_value="Patch1\nMockPatch\n")
         mock_tool.checkout().modified_changelogs = Mock(return_value=[])
         self.assert_execute_outputs(Land(), [42], options=self._default_options(), expected_stderr=expected_stderr, tool=mock_tool)
         # Make sure we're not calling expensive calls too often.
-        self.assertEqual(mock_tool.scm().create_patch.call_count, 0)
+        self.assertEqual(mock_tool.scm().create_patch.call_count, 1)
         self.assertEqual(mock_tool.checkout().modified_changelogs.call_count, 1)
 
     def test_land_red_builders(self):
index e12c8e2e7ecd372aaaad6daf6275323fbd15188e..6617b4fe89e0267555f5816050d3daa38114ad44 100644 (file)
@@ -196,6 +196,7 @@ class Post(AbstractPatchUploadingCommand):
     help_text = "Attach the current working directory diff to a bug as a patch file"
     argument_names = "[BUGID]"
     steps = [
+        steps.ValidateChangeLogs,
         steps.CheckStyle,
         steps.ConfirmDiff,
         steps.ObsoletePatches,
@@ -215,6 +216,7 @@ class LandSafely(AbstractPatchUploadingCommand):
     show_in_main_help = True
     steps = [
         steps.UpdateChangeLogsWithReviewer,
+        steps.ValidateChangeLogs,
         steps.ObsoletePatches,
         steps.PostDiffForCommit,
     ]
@@ -241,6 +243,7 @@ class Upload(AbstractPatchUploadingCommand):
     argument_names = "[BUGID]"
     show_in_main_help = True
     steps = [
+        steps.ValidateChangeLogs,
         steps.CheckStyle,
         steps.PromptForBugOrTitle,
         steps.CreateBug,
index 64d9d059bda71db9cdd8928a123f2cf3b0010fd7..f426f177d5b6ccd7dcedd975bc3c5fe75df34fca 100644 (file)
@@ -56,4 +56,5 @@ from webkitpy.tool.steps.runtests import RunTests
 from webkitpy.tool.steps.suggestreviewers import SuggestReviewers
 from webkitpy.tool.steps.updatechangelogswithreviewer import UpdateChangeLogsWithReviewer
 from webkitpy.tool.steps.update import Update
+from webkitpy.tool.steps.validatechangelogs import ValidateChangeLogs
 from webkitpy.tool.steps.validatereviewer import ValidateReviewer
index 5525ea08b5681d81d85c0fb5434223bd02f01630..1c93d5b9f7d0e3bd331c510a0f24ce7fd9cac5b9 100644 (file)
@@ -52,6 +52,7 @@ class AbstractStep(object):
         "bug_title": lambda self, state: self._tool.bugs.fetch_bug(state["bug_id"]).title(),
         "changed_files": lambda self, state: self._tool.scm().changed_files(self._options.git_commit),
         "diff": lambda self, state: self._tool.scm().create_patch(self._options.git_commit, changed_files=self._changed_files(state)),
+        # Absolute path to ChangeLog files.
         "changelogs": lambda self, state: self._tool.checkout().modified_changelogs(self._options.git_commit, changed_files=self._changed_files(state)),
     }
 
index 9c16242eaa4df7ac777989e06589371b6f3c0134..a165dd21f98d089146aabaa93b0068fdddaedf48 100644 (file)
@@ -47,7 +47,9 @@ class CleanWorkingDirectory(AbstractStep):
     def run(self, state):
         if not self._options.clean:
             return
-        # FIXME: This chdir should not be necessary.
+        # FIXME: This chdir should not be necessary and can be removed as
+        # soon as ensure_no_local_commits and ensure_clean_working_directory
+        # are known to set the CWD to checkout_root when calling run_command.
         os.chdir(self._tool.scm().checkout_root)
         if not self._allow_local_commits:
             self._tool.scm().ensure_no_local_commits(self._options.force_clean)
index 5aa6b51de9503215895e42b8d4059ebccba9ac0c..859acbf8c258b2edb054a0aa6aba848bd93c1e23 100644 (file)
@@ -36,12 +36,6 @@ from webkitpy.tool.steps.options import Options
 
 
 class Commit(AbstractStep):
-    @classmethod
-    def options(cls):
-        return AbstractStep.options() + [
-            Options.git_commit,
-        ]
-
     def _commit_warning(self, error):
         working_directory_message = "" if error.working_directory_is_clean else " and working copy changes"
         return ('There are %s local commits%s. Everything will be committed as a single commit. '
index e46b7900ced9285e71d67849f65e2ef844e6687b..2bf41b3515c54355d7849b9d386507ab63a3b9a6 100644 (file)
@@ -67,6 +67,9 @@ class UpdateChangeLogsWithReviewer(AbstractStep):
             log("Failed to guess reviewer from bug %s and --reviewer= not provided.  Not updating ChangeLogs with reviewer." % bug_id)
             return
 
-        os.chdir(self._tool.scm().checkout_root)
+        # cached_lookup("changelogs") is always absolute paths.
         for changelog_path in self.cached_lookup(state, "changelogs"):
             ChangeLog(changelog_path).set_reviewer(reviewer)
+
+        # Tell the world that we just changed something on disk so that the cached diff is invalidated.
+        self.did_modify_checkout(state)
diff --git a/Tools/Scripts/webkitpy/tool/steps/validatechangelogs.py b/Tools/Scripts/webkitpy/tool/steps/validatechangelogs.py
new file mode 100644 (file)
index 0000000..e812f94
--- /dev/null
@@ -0,0 +1,58 @@
+# Copyright (C) 2010 Google Inc. All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+#     * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#     * Redistributions in binary form must reproduce the above
+# copyright notice, this list of conditions and the following disclaimer
+# in the documentation and/or other materials provided with the
+# distribution.
+#     * Neither the name of Google Inc. nor the names of its
+# contributors may be used to endorse or promote products derived from
+# this software without specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+from webkitpy.tool.steps.abstractstep import AbstractStep
+from webkitpy.tool.steps.options import Options
+from webkitpy.common.checkout.diff_parser import DiffParser
+from webkitpy.common.system.deprecated_logging import error, log
+
+
+# This is closely related to the ValidateReviewer step and the CommitterValidator class.
+# We may want to unify more of this code in one place.
+class ValidateChangeLogs(AbstractStep):
+    def _check_changelog_diff(self, diff_file):
+        if not self._tool.checkout().is_path_to_changelog(diff_file.filename):
+            return True
+        # Each line is a tuple, the first value is the deleted line number
+        # Date, reviewer, bug title, bug url, and empty lines could all be
+        # identical in the most recent entries.  If the diff starts any
+        # later than that, assume that the entry is wrong.
+        if diff_file.lines[0][0] < 8:
+            return True
+        log("The diff to %s looks wrong.  Are you sure your ChangeLog entry is at the top of the file?" % (diff_file.filename))
+        # FIXME: Do we need to make the file path absolute?
+        self._tool.scm().diff_for_file(diff_file.filename)
+        if self._tool.user.confirm("OK to continue?", default='n'):
+            return True
+        return False
+
+    def run(self, state):
+        parsed_diff = DiffParser(self.cached_lookup(state, "diff").splitlines())
+        for filename, diff_file in parsed_diff.files.items():
+            if not self._check_changelog_diff(diff_file):
+                error("ChangeLog entry in %s is not at the top of the file." % diff_file.filename)
diff --git a/Tools/Scripts/webkitpy/tool/steps/validatechangelogs_unittest.py b/Tools/Scripts/webkitpy/tool/steps/validatechangelogs_unittest.py
new file mode 100644 (file)
index 0000000..66db793
--- /dev/null
@@ -0,0 +1,55 @@
+# Copyright (C) 2010 Google Inc. All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+#     * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#     * Redistributions in binary form must reproduce the above
+# copyright notice, this list of conditions and the following disclaimer
+# in the documentation and/or other materials provided with the
+# distribution.
+#     * Neither the name of Google Inc. nor the names of its
+# contributors may be used to endorse or promote products derived from
+# this software without specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (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 unittest
+
+from webkitpy.common.system.outputcapture import OutputCapture
+from webkitpy.thirdparty.mock import Mock
+from webkitpy.tool.mocktool import MockOptions, MockTool
+from webkitpy.tool.steps.validatechangelogs import ValidateChangeLogs
+
+
+class ValidateChangeLogsTest(unittest.TestCase):
+
+    def _assert_start_line_produces_output(self, start_line, should_prompt_user=False):
+        tool = MockTool()
+        tool._checkout.is_path_to_changelog = lambda path: True
+        step = ValidateChangeLogs(tool, MockOptions(git_commit=None))
+        diff_file = Mock()
+        diff_file.filename = "mock/ChangeLog"
+        diff_file.lines = [(start_line, start_line, "foo")]
+        expected_stdout = expected_stderr = ""
+        if should_prompt_user:
+            expected_stdout = "OK to continue?\n"
+            expected_stderr = "The diff to mock/ChangeLog looks wrong.  Are you sure your ChangeLog entry is at the top of the file?\n"
+        OutputCapture().assert_outputs(self, step._check_changelog_diff, [diff_file], expected_stdout=expected_stdout, expected_stderr=expected_stderr)
+
+    def test_check_changelog_diff(self):
+        self._assert_start_line_produces_output(1, should_prompt_user=False)
+        self._assert_start_line_produces_output(7, should_prompt_user=False)
+        self._assert_start_line_produces_output(8, should_prompt_user=True)
index bdf729e4bfe2d0443b579188d59692cc066641d7..c2a27b9836c6ace294242d7107114285886c2344 100644 (file)
@@ -37,12 +37,6 @@ from webkitpy.common.system.deprecated_logging import error, log
 
 # FIXME: Some of this logic should probably be unified with CommitterValidator?
 class ValidateReviewer(AbstractStep):
-    @classmethod
-    def options(cls):
-        return AbstractStep.options() + [
-            Options.git_commit,
-        ]
-
     # FIXME: This should probably move onto ChangeLogEntry
     def _has_valid_reviewer(self, changelog_entry):
         if changelog_entry.reviewer():
@@ -58,9 +52,6 @@ class ValidateReviewer(AbstractStep):
         # this check is too draconian (and too poorly tested) to foist upon users.
         if not self._options.non_interactive:
             return
-        # FIXME: We should figure out how to handle the current working
-        #        directory issue more globally.
-        os.chdir(self._tool.scm().checkout_root)
         for changelog_path in self.cached_lookup(state, "changelogs"):
             changelog_entry = ChangeLog(changelog_path).latest_entry()
             if self._has_valid_reviewer(changelog_entry):