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: https://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 3d2c8cc..cbe6ba6 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 6357982..29e43d3 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 a6ea756..5a5546c 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 020f339..1b478bf 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 3748a8f..ba23ab9 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 e12c8e2..6617b4f 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 64d9d05..f426f17 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 5525ea0..1c93d5b 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 9c16242..a165dd2 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 5aa6b51..859acbf 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 e46b790..2bf41b3 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 bdf729e..c2a27b9 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):