Changeset 74639 in webkit


Ignore:
Timestamp:
Dec 24, 2010 10:21:28 AM (13 years ago)
Author:
eric@webkit.org
Message:

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:
Location:
trunk/Tools
Files:
12 edited
2 copied

Legend:

Unmodified
Added
Removed

  • trunk/Tools/ChangeLog

    r74632 r74639  
     12010-12-24  Eric Seidel  <eric@webkit.org>
     2
     3        Reviewed by Adam Barth.
     4
     5        webkit-patch (or a pre-commit hook) needs to prevent bad ChangeLog changes
     6        https://bugs.webkit.org/show_bug.cgi?id=28291
     7
     8        This is a start.  At least now webkit-patch will prompt when your ChangeLog looks questionable.
     9        We could do more advanced things, like parsing the ChangeLog (with changelog.py) and comparing that
     10        to strings with find in the diff.
     11        Since non-interactive always returns the default, this should cause patches with bad changelogs to fail on the commit-queue.
     12
     13        * Scripts/webkitpy/common/checkout/api.py:
     14        * Scripts/webkitpy/common/checkout/diff_parser.py:
     15        * Scripts/webkitpy/tool/steps/abstractstep.py:
     16        * Scripts/webkitpy/tool/steps/cleanworkingdirectory.py:
     17        * Scripts/webkitpy/tool/steps/validatechangelogs.py: Copied from Tools/Scripts/webkitpy/tool/steps/validatereviewer.py.
     18        * Scripts/webkitpy/tool/steps/validatechangelogs_unittest.py: Copied from Tools/Scripts/webkitpy/tool/steps/cleanworkingdirectory.py.
     19        * Scripts/webkitpy/tool/steps/validatereviewer.py:
     20
    1212010-12-24  Dirk Pranke  <dpranke@chromium.org>
    222

  • trunk/Tools/Scripts/webkitpy/common/checkout/api.py

    r73951 r74639  
    4040
    4141
    42 # This class represents the WebKit-specific parts of the checkout (like
    43 # ChangeLogs).
     42# This class represents the WebKit-specific parts of the checkout (like ChangeLogs).
    4443# FIXME: Move a bunch of ChangeLog-specific processing from SCM to this object.
     44# NOTE: All paths returned from this class should be absolute.
    4545class Checkout(object):
    4646    def __init__(self, scm):
    4747        self._scm = scm
    4848
    49     def _is_path_to_changelog(self, path):
     49    def is_path_to_changelog(self, path):
    5050        return os.path.basename(path) == "ChangeLog"
    5151
    … …  
    6060    def changelog_entries_for_revision(self, revision):
    6161        changed_files = self._scm.changed_files_for_revision(revision)
    62         return [self._latest_entry_for_changelog_at_revision(path, revision) for path in changed_files if self._is_path_to_changelog(path)]
     62        return [self._latest_entry_for_changelog_at_revision(path, revision) for path in changed_files if self.is_path_to_changelog(path)]
    6363
    6464    @memoized
    … …  
    9797
    9898    def modified_changelogs(self, git_commit, changed_files=None):
    99         return self._modified_files_matching_predicate(git_commit, self._is_path_to_changelog, changed_files=changed_files)
     99        return self._modified_files_matching_predicate(git_commit, self.is_path_to_changelog, changed_files=changed_files)
    100100
    101101    def modified_non_changelogs(self, git_commit, changed_files=None):
    102         return self._modified_files_matching_predicate(git_commit, lambda path: not self._is_path_to_changelog(path), changed_files=changed_files)
     102        return self._modified_files_matching_predicate(git_commit, lambda path: not self.is_path_to_changelog(path), changed_files=changed_files)
    103103
    104104    def commit_message_for_this_commit(self, git_commit, changed_files=None):

  • trunk/Tools/Scripts/webkitpy/common/checkout/diff_parser.py

    r69744 r74639  
    119119
    120120
     121# If this is going to be called DiffParser, it should be a re-useable parser.
     122# Otherwise we should rename it to ParsedDiff or just Diff.
    121123class DiffParser(object):
    122124    """A parser for a patch file.
    … …  
    126128    """
    127129
    128     # FIXME: This function is way too long and needs to be broken up.
    129130    def __init__(self, diff_input):
    130131        """Parses a diff.
    … …  
    133134          diff_input: An iterable object.
    134135        """
     136        self.files = self._parse_into_diff_files(diff_input)
     137
     138    # FIXME: This function is way too long and needs to be broken up.
     139    def _parse_into_diff_files(self, diff_input):
     140        files = {}
    135141        state = _INITIAL_STATE
    136 
    137         self.files = {}
    138142        current_file = None
    139143        old_diff_line = None
    … …  
    149153                filename = file_declaration.group('FilePath')
    150154                current_file = DiffFile(filename)
    151                 self.files[filename] = current_file
     155                files[filename] = current_file
    152156                state = _DECLARED_FILE_PATH
    153157                continue
    … …  
    180184                    _log.error('Unexpected diff format when parsing a '
    181185                               'chunk: %r' % line)
     186        return files

  • trunk/Tools/Scripts/webkitpy/tool/commands/download.py

    r73951 r74639  
    9696        steps.UpdateChangeLogsWithReviewer,
    9797        steps.ValidateReviewer,
     98        steps.ValidateChangeLogs, # We do this after UpdateChangeLogsWithReviewer to avoid not having to cache the diff twice.
    9899        steps.Build,
    99100        steps.RunTests,
    … …  
    258259        steps.Update,
    259260        steps.ApplyPatch,
     261        steps.ValidateChangeLogs,
    260262        steps.ValidateReviewer,
    261263        steps.Build,

  • trunk/Tools/Scripts/webkitpy/tool/commands/download_unittest.py

    r74138 r74639  
    110110        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"
    111111        mock_tool = MockTool()
    112         mock_tool.scm().create_patch = Mock()
     112        mock_tool.scm().create_patch = Mock(return_value="Patch1\nMockPatch\n")
    113113        mock_tool.checkout().modified_changelogs = Mock(return_value=[])
    114114        self.assert_execute_outputs(Land(), [42], options=self._default_options(), expected_stderr=expected_stderr, tool=mock_tool)
    115115        # Make sure we're not calling expensive calls too often.
    116         self.assertEqual(mock_tool.scm().create_patch.call_count, 0)
     116        self.assertEqual(mock_tool.scm().create_patch.call_count, 1)
    117117        self.assertEqual(mock_tool.checkout().modified_changelogs.call_count, 1)
    118118

  • trunk/Tools/Scripts/webkitpy/tool/commands/upload.py

    r71502 r74639  
    197197    argument_names = "[BUGID]"
    198198    steps = [
     199        steps.ValidateChangeLogs,
    199200        steps.CheckStyle,
    200201        steps.ConfirmDiff,
    … …  
    216217    steps = [
    217218        steps.UpdateChangeLogsWithReviewer,
     219        steps.ValidateChangeLogs,
    218220        steps.ObsoletePatches,
    219221        steps.PostDiffForCommit,
    … …  
    242244    show_in_main_help = True
    243245    steps = [
     246        steps.ValidateChangeLogs,
    244247        steps.CheckStyle,
    245248        steps.PromptForBugOrTitle,

  • trunk/Tools/Scripts/webkitpy/tool/steps/__init__.py

    r70570 r74639  
    5757from webkitpy.tool.steps.updatechangelogswithreviewer import UpdateChangeLogsWithReviewer
    5858from webkitpy.tool.steps.update import Update
     59from webkitpy.tool.steps.validatechangelogs import ValidateChangeLogs
    5960from webkitpy.tool.steps.validatereviewer import ValidateReviewer

  • trunk/Tools/Scripts/webkitpy/tool/steps/abstractstep.py

    r70059 r74639  
    5353        "changed_files": lambda self, state: self._tool.scm().changed_files(self._options.git_commit),
    5454        "diff": lambda self, state: self._tool.scm().create_patch(self._options.git_commit, changed_files=self._changed_files(state)),
     55        # Absolute path to ChangeLog files.
    5556        "changelogs": lambda self, state: self._tool.checkout().modified_changelogs(self._options.git_commit, changed_files=self._changed_files(state)),
    5657    }

  • trunk/Tools/Scripts/webkitpy/tool/steps/cleanworkingdirectory.py

    r73733 r74639  
    4848        if not self._options.clean:
    4949            return
    50         # FIXME: This chdir should not be necessary.
     50        # FIXME: This chdir should not be necessary and can be removed as
     51        # soon as ensure_no_local_commits and ensure_clean_working_directory
     52        # are known to set the CWD to checkout_root when calling run_command.
    5153        os.chdir(self._tool.scm().checkout_root)
    5254        if not self._allow_local_commits:

  • trunk/Tools/Scripts/webkitpy/tool/steps/commit.py

    r73691 r74639  
    3737
    3838class Commit(AbstractStep):
    39     @classmethod
    40     def options(cls):
    41         return AbstractStep.options() + [
    42             Options.git_commit,
    43         ]
    44 
    4539    def _commit_warning(self, error):
    4640        working_directory_message = "" if error.working_directory_is_clean else " and working copy changes"

  • trunk/Tools/Scripts/webkitpy/tool/steps/updatechangelogswithreviewer.py

    r68496 r74639  
    6868            return
    6969
    70         os.chdir(self._tool.scm().checkout_root)
     70        # cached_lookup("changelogs") is always absolute paths.
    7171        for changelog_path in self.cached_lookup(state, "changelogs"):
    7272            ChangeLog(changelog_path).set_reviewer(reviewer)
     73
     74        # Tell the world that we just changed something on disk so that the cached diff is invalidated.
     75        self.did_modify_checkout(state)

  • trunk/Tools/Scripts/webkitpy/tool/steps/validatechangelogs.py

    r74637 r74639  
    11# Copyright (C) 2010 Google Inc. All rights reserved.
    2 # 
     2#
    33# Redistribution and use in source and binary forms, with or without
    44# modification, are permitted provided that the following conditions are
    55# met:
    6 # 
     6#
    77#     * Redistributions of source code must retain the above copyright
    88# notice, this list of conditions and the following disclaimer.
    … …  
    1414# contributors may be used to endorse or promote products derived from
    1515# this software without specific prior written permission.
    16 # 
     16#
    1717# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
    1818# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
    … …  
    2727# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
    2828
    29 import os
    30 
    3129from webkitpy.tool.steps.abstractstep import AbstractStep
    3230from webkitpy.tool.steps.options import Options
     31from webkitpy.common.checkout.diff_parser import DiffParser
     32from webkitpy.common.system.deprecated_logging import error, log
    3333
    3434
    35 class CleanWorkingDirectory(AbstractStep):
    36     def __init__(self, tool, options, allow_local_commits=False):
    37         AbstractStep.__init__(self, tool, options)
    38         self._allow_local_commits = allow_local_commits
    39 
    40     @classmethod
    41     def options(cls):
    42         return AbstractStep.options() + [
    43             Options.force_clean,
    44             Options.clean,
    45         ]
     35# This is closely related to the ValidateReviewer step and the CommitterValidator class.
     36# We may want to unify more of this code in one place.
     37class ValidateChangeLogs(AbstractStep):
     38    def _check_changelog_diff(self, diff_file):
     39        if not self._tool.checkout().is_path_to_changelog(diff_file.filename):
     40            return True
     41        # Each line is a tuple, the first value is the deleted line number
     42        # Date, reviewer, bug title, bug url, and empty lines could all be
     43        # identical in the most recent entries.  If the diff starts any
     44        # later than that, assume that the entry is wrong.
     45        if diff_file.lines[0][0] < 8:
     46            return True
     47        log("The diff to %s looks wrong.  Are you sure your ChangeLog entry is at the top of the file?" % (diff_file.filename))
     48        # FIXME: Do we need to make the file path absolute?
     49        self._tool.scm().diff_for_file(diff_file.filename)
     50        if self._tool.user.confirm("OK to continue?", default='n'):
     51            return True
     52        return False
    4653
    4754    def run(self, state):
    48         if not self._options.clean:
    49             return
    50         # FIXME: This chdir should not be necessary.
    51         os.chdir(self._tool.scm().checkout_root)
    52         if not self._allow_local_commits:
    53             self._tool.scm().ensure_no_local_commits(self._options.force_clean)
    54         self._tool.scm().ensure_clean_working_directory(force_clean=self._options.force_clean)
     55        parsed_diff = DiffParser(self.cached_lookup(state, "diff").splitlines())
     56        for filename, diff_file in parsed_diff.files.items():
     57            if not self._check_changelog_diff(diff_file):
     58                error("ChangeLog entry in %s is not at the top of the file." % diff_file.filename)

  • trunk/Tools/Scripts/webkitpy/tool/steps/validatechangelogs_unittest.py

    r74637 r74639  
    11# Copyright (C) 2010 Google Inc. All rights reserved.
    2 # 
     2#
    33# Redistribution and use in source and binary forms, with or without
    44# modification, are permitted provided that the following conditions are
    55# met:
    6 # 
     6#
    77#     * Redistributions of source code must retain the above copyright
    88# notice, this list of conditions and the following disclaimer.
    … …  
    1414# contributors may be used to endorse or promote products derived from
    1515# this software without specific prior written permission.
    16 # 
     16#
    1717# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
    1818# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
    … …  
    2727# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
    2828
    29 import os
     29import unittest
    3030
    31 from webkitpy.tool.steps.abstractstep import AbstractStep
    32 from webkitpy.tool.steps.options import Options
     31from webkitpy.common.system.outputcapture import OutputCapture
     32from webkitpy.thirdparty.mock import Mock
     33from webkitpy.tool.mocktool import MockOptions, MockTool
     34from webkitpy.tool.steps.validatechangelogs import ValidateChangeLogs
    3335
    3436
    35 class CleanWorkingDirectory(AbstractStep):
    36     def __init__(self, tool, options, allow_local_commits=False):
    37         AbstractStep.__init__(self, tool, options)
    38         self._allow_local_commits = allow_local_commits
     37class ValidateChangeLogsTest(unittest.TestCase):
    3938
    40     @classmethod
    41     def options(cls):
    42         return AbstractStep.options() + [
    43             Options.force_clean,
    44             Options.clean,
    45         ]
     39    def _assert_start_line_produces_output(self, start_line, should_prompt_user=False):
     40        tool = MockTool()
     41        tool._checkout.is_path_to_changelog = lambda path: True
     42        step = ValidateChangeLogs(tool, MockOptions(git_commit=None))
     43        diff_file = Mock()
     44        diff_file.filename = "mock/ChangeLog"
     45        diff_file.lines = [(start_line, start_line, "foo")]
     46        expected_stdout = expected_stderr = ""
     47        if should_prompt_user:
     48            expected_stdout = "OK to continue?\n"
     49            expected_stderr = "The diff to mock/ChangeLog looks wrong.  Are you sure your ChangeLog entry is at the top of the file?\n"
     50        OutputCapture().assert_outputs(self, step._check_changelog_diff, [diff_file], expected_stdout=expected_stdout, expected_stderr=expected_stderr)
    4651
    47     def run(self, state):
    48         if not self._options.clean:
    49             return
    50         # FIXME: This chdir should not be necessary.
    51         os.chdir(self._tool.scm().checkout_root)
    52         if not self._allow_local_commits:
    53             self._tool.scm().ensure_no_local_commits(self._options.force_clean)
    54         self._tool.scm().ensure_clean_working_directory(force_clean=self._options.force_clean)
     52    def test_check_changelog_diff(self):
     53        self._assert_start_line_produces_output(1, should_prompt_user=False)
     54        self._assert_start_line_produces_output(7, should_prompt_user=False)
     55        self._assert_start_line_produces_output(8, should_prompt_user=True)

  • trunk/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py

    r63004 r74639  
    3838# FIXME: Some of this logic should probably be unified with CommitterValidator?
    3939class ValidateReviewer(AbstractStep):
    40     @classmethod
    41     def options(cls):
    42         return AbstractStep.options() + [
    43             Options.git_commit,
    44         ]
    45 
    4640    # FIXME: This should probably move onto ChangeLogEntry
    4741    def _has_valid_reviewer(self, changelog_entry):
    … …  
    5953        if not self._options.non_interactive:
    6054            return
    61         # FIXME: We should figure out how to handle the current working
    62         #        directory issue more globally.
    63         os.chdir(self._tool.scm().checkout_root)
    6455        for changelog_path in self.cached_lookup(state, "changelogs"):
    6556            changelog_entry = ChangeLog(changelog_path).latest_entry()
Note: See TracChangeset for help on using the changeset viewer.