Changeset 75769 in webkit

Timestamp:

Jan 13, 2011 9:40:18 PM (13 years ago)

Author:

eric@webkit.org

Message:

2011-01-13 Eric Seidel <​eric@webkit.org>

Reviewed by David Levin.

webkit-patch suggest-reviewers dies when ChangeLogs are missing
​https://bugs.webkit.org/show_bug.cgi?id=49158

This is not the most elegant, but it is a very safe fix to this bug.
One advantage of catching ScriptError like this instead of adding a
new added_or_modified_files or fixing all changed_files callers
to use a more specific change_files variant, is that we catch
all kinds of ScriptErrors which might cause our (non-essential)
suggest-reviewers code to fail out. This should make passing
--suggest-reviewers to webkit-patch upload much more robust
and may even make it possible for us to make it default.

The root of the problem here is that SCM.changed_files includes
deleted ChangeLog paths (from moves, etc) which then when we ask
SVN/Git for the contents of the file at that revision, the command
errors out and Executive.run_command raises a ScriptError.

In the future we might fix this differently by making all current
callers of chagned_files use a more specific method for requesting
what types of changes they're interested in (adds, modifies, deletes, etc.)

• Scripts/webkitpy/common/checkout/api.py:
• Scripts/webkitpy/common/checkout/api_unittest.py:

Location:

trunk/Tools

Files:

• ChangeLog (modified) (1 diff)
• Scripts/webkitpy/common/checkout/api.py (modified) (1 diff)
• Scripts/webkitpy/common/checkout/api_unittest.py (modified) (2 diffs)

trunk/Tools/ChangeLog

@@ +1 @@
+2011-01-13  Eric Seidel  <eric@webkit.org>
+
+        Reviewed by David Levin.
+
+        webkit-patch suggest-reviewers dies when ChangeLogs are missing
+        https://bugs.webkit.org/show_bug.cgi?id=49158
+
+        This is not the most elegant, but it is a very safe fix to this bug.
+        One advantage of catching ScriptError like this instead of adding a
+        new added_or_modified_files or fixing all changed_files callers
+        to use a more specific change_files variant, is that we catch
+        all kinds of ScriptErrors which might cause our (non-essential)
+        suggest-reviewers code to fail out.  This should make passing
+        --suggest-reviewers to webkit-patch upload much more robust
+        and may even make it possible for us to make it default.
+
+        The root of the problem here is that SCM.changed_files includes
+        deleted ChangeLog paths (from moves, etc) which then when we ask
+        SVN/Git for the contents of the file at that revision, the command
+        errors out and Executive.run_command raises a ScriptError.
+
+        In the future we might fix this differently by making all current
+        callers of chagned_files use a more specific method for requesting
+        what types of changes they're interested in (adds, modifies, deletes, etc.)
+
+        * Scripts/webkitpy/common/checkout/api.py:
+        * Scripts/webkitpy/common/checkout/api_unittest.py:
+
 2011-01-13  Dan Bernstein  <mitz@apple.com>
 

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

@@ -63 +63 @@
         # FIXME: This gets confused if ChangeLog files are moved, as
         # deletes are still "changed files" per changed_files_for_revision.
-        return [self._latest_entry_for_changelog_at_revision(path, revision) for path in changed_files if self.is_path_to_changelog(path)]
+        # FIXME: For now we hack around this by caching any exceptions
+        # which result from having deleted files included the changed_files list.
+        changelog_entries = []
+        for path in changed_files:
+            if not self.is_path_to_changelog(path):
+                continue
+            try:
+                changelog_entries.append(self._latest_entry_for_changelog_at_revision(path, revision))
+            except ScriptError:
+                pass
+        return changelog_entries
 
     @memoized

trunk/Tools/Scripts/webkitpy/common/checkout/api_unittest.py

@@ -39 +39 @@
 from webkitpy.common.checkout.scm import detect_scm_system, CommitMessage
 from webkitpy.common.system.outputcapture import OutputCapture
+from webkitpy.common.system.executive import ScriptError
 from webkitpy.thirdparty.mock import Mock
 
@@ -139 +140 @@
         self.assertEqual(entry.contents(), _changelog1entry1)
 
+    # FIXME: This tests a hack around our current changed_files handling.
+    # Right now changelog_entries_for_revision tries to fetch deleted files
+    # from revisions, resulting in a ScriptError exception.  Test that we
+    # recover from those and still return the other ChangeLog entries.
+    def test_changelog_entries_for_revision(self):
+        scm = Mock()
+        scm.changed_files_for_revision = lambda revision: ['foo/ChangeLog', 'bar/ChangeLog']
+        checkout = Checkout(scm)
+
+        def mock_latest_entry_for_changelog_at_revision(path, revision):
+            if path == "foo/ChangeLog":
+                return 'foo'
+            raise ScriptError()
+
+        checkout._latest_entry_for_changelog_at_revision = mock_latest_entry_for_changelog_at_revision
+
+        # Even though fetching one of the entries failed, the other should succeed.
+        entries = checkout.changelog_entries_for_revision(1)
+        self.assertEqual(len(entries), 1)
+        self.assertEqual(entries[0], 'foo')
+
     def test_commit_info_for_revision(self):
         scm = Mock()