Changeset 84549 in webkit

Timestamp:

Apr 21, 2011 3:00:54 PM (13 years ago)

Author:

abarth@webkit.org

Message:

2011-04-21 Adam Barth <​abarth@webkit.org>

Reviewed by Eric Seidel.

ValidateChangeLogs doesn't work on SVN
​https://bugs.webkit.org/show_bug.cgi?id=59115

svn-create-patch lies about the diff in the working copy by moving
ChangeLog entries to the top of the diff. That's fine on most cases,
but causes problems for ValidateChangeLogs, which is trying validate
the where the ChangeLog entry appears.

I haven't added a test for this change because I couldn't figure out
how to write one. The issue is more of an integration issue, which
we're not really set up to test in our unit testing framework. If this
patch had worked around the output from svn-create-patch, then I could
have tested that we behave correctly on sample svn-create-patch output,
but, in this case, I've removed the dependency on svn-create-patch. I
could test that we behave correctly on "svn diff" output, but we
already have those tests.

• Scripts/webkitpy/tool/commands/download_unittest.py:
• Scripts/webkitpy/tool/mocktool.py:
• Scripts/webkitpy/tool/steps/validatechangelogs.py:

Location:

trunk/Tools

Files:

• ChangeLog (modified) (1 diff)
• Scripts/webkitpy/tool/commands/download_unittest.py (modified) (1 diff)
• Scripts/webkitpy/tool/mocktool.py (modified) (1 diff)
• Scripts/webkitpy/tool/steps/validatechangelogs.py (modified) (1 diff)

trunk/Tools/ChangeLog

@@ +1 @@
+2011-04-21  Adam Barth  <abarth@webkit.org>
+
+        Reviewed by Eric Seidel.
+
+        ValidateChangeLogs doesn't work on SVN
+        https://bugs.webkit.org/show_bug.cgi?id=59115
+
+        svn-create-patch lies about the diff in the working copy by moving
+        ChangeLog entries to the top of the diff.  That's fine on most cases,
+        but causes problems for ValidateChangeLogs, which is trying validate
+        the where the ChangeLog entry appears.
+
+        I haven't added a test for this change because I couldn't figure out
+        how to write one.  The issue is more of an integration issue, which
+        we're not really set up to test in our unit testing framework.  If this
+        patch had worked around the output from svn-create-patch, then I could
+        have tested that we behave correctly on sample svn-create-patch output,
+        but, in this case, I've removed the dependency on svn-create-patch.  I
+        could test that we behave correctly on "svn diff" output, but we
+        already have those tests.
+
+        * Scripts/webkitpy/tool/commands/download_unittest.py:
+        * Scripts/webkitpy/tool/mocktool.py:
+        * Scripts/webkitpy/tool/steps/validatechangelogs.py:
+
 2011-04-21  Tony Chang  <tony@chromium.org>
 

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

@@ -114 +114 @@
         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, 1)
+        self.assertEqual(mock_tool.scm().create_patch.call_count, 0)
         self.assertEqual(mock_tool.checkout().modified_changelogs.call_count, 1)
 

trunk/Tools/Scripts/webkitpy/tool/mocktool.py

@@ -536 +536 @@
         })
 
+    def is_path_to_changelog(self, path):
+        return os.path.basename(path) == "ChangeLog"
+
     def bug_id_for_revision(self, svn_revision):
         return 12345

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

@@ -56 +56 @@
 
     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)
+        changed_files = self.cached_lookup(state, "changed_files")
+        for filename in changed_files:
+            if not self._tool.checkout().is_path_to_changelog(filename):
+                continue
+            # Diff ChangeLogs directly because svn-create-patch will move
+            # ChangeLog entries to the # top automatically, defeating our
+            # validation here.
+            # FIXME: Should we diff all the ChangeLogs at once?
+            diff = self._tool.scm().diff_for_file(filename)
+            parsed_diff = DiffParser(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)