Commit 6acfa901 authored by eric@webkit.org's avatar eric@webkit.org
Browse files

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:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@74639 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent 36fe29aa
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.
......@@ -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)
......
......@@ -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
......@@ -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,
......
......@@ -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):
......
......@@ -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,
......
......@@ -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
......@@ -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)),
}
......
......@@ -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)
......
......@@ -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. '
......
......@@ -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)
# 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)
# 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)
......@@ -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):
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment