[webkit-patch] Fix revert workflow

https://bugs.webkit.org/show_bug.cgi?id=240684
<rdar://93602151>

Reviewed by Dewei Zhu.

* Tools/Scripts/webkitpy/common/checkout/checkout.py:
(_changelog_data_for_revision): Use git commit message instead of ChangeLog.
(changelog_entries_for_revision): Deleted.
* Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:
(CheckoutTest.test_changelog_entries_for_revision): Deleted.
(CheckoutTest.test_commit_info_for_revision): Deleted.
(CheckoutTest.test_bug_id_for_revision): Deleted.
(CheckoutTest.test_suggested_reviewers): Deleted.
* Tools/Scripts/webkitpy/common/config/committers.py:
(Contributor.email): Match webkitscmpy's API.
* Tools/Scripts/webkitpy/tool/commands/download.py:
(AbstractRevertPrepCommand._prepare_state): Use email instead of bugzilla_email.
* Tools/Scripts/webkitpy/tool/steps/preparechangelogforrevert.py:
(PrepareChangeLogForRevert.run): Replace with a `git commit -a -m`.

Canonical link: https://commits.webkit.org/250777@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294521 268f45cc-cd09-0410-ab3c-d52691b4dbfc

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

@@ -31,9 +31,10 @@
 import sys
 
 from webkitcorepy import StringIO, string_utils
+from webkitscmpy import local
 
 from webkitpy.common.config import urls
-from webkitpy.common.checkout.changelog import ChangeLog, parse_bug_id_from_changelog
+from webkitpy.common.checkout.changelog import ChangeLog, ChangeLogEntry, parse_bug_id_from_changelog
 from webkitpy.common.checkout.commitinfo import CommitInfo
 from webkitpy.common.checkout.scm import CommitMessage
 from webkitpy.common.memoized import memoized
@@ -93,41 +94,19 @@ def _latest_entry_for_changelog_at_revision(self, changelog_path, revision):
         changelog_file = StringIO(changelog_contents.decode("utf-8", "ignore"))
         return ChangeLog.parse_latest_entry_from_file(changelog_file)
 
-    def changelog_entries_for_revision(self, revision, changed_files=None):
-        if not changed_files:
-            changed_files = self._scm.changed_files_for_revision(revision)
-        # FIXME: This gets confused if ChangeLog files are moved, as
-        # deletes are still "changed files" per changed_files_for_revision.
-        # 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
-
     def _changelog_data_for_revision(self, revision):
-        changed_files = self._scm.changed_files_for_revision(revision)
-        changelog_entries = self.changelog_entries_for_revision(revision, changed_files=changed_files)
-        # Assume for now that the first entry has everything we need:
-        # FIXME: This will throw an exception if there were no ChangeLogs.
-        if not len(changelog_entries):
-            return None
-        changelog_entry = changelog_entries[0]
+        repo = local.Scm.from_path(self._scm.checkout_root)
+        commit = repo.commit(revision=revision)
+
         return {
-            "bug_id": changelog_entry.bug_id(),
-            "author_name": changelog_entry.author_name(),
-            "author_email": changelog_entry.author_email(),
-            "author": changelog_entry.author(),
-            "bug_description": changelog_entry.bug_description(),
-            "reviewer_text": changelog_entry.reviewer_text(),
-            "reviewer": changelog_entry.reviewer(),
-            "contents": changelog_entry.contents(),
-            "changed_files": changed_files,
+            "bug_id": parse_bug_id_from_changelog(commit.message),
+            "author_name": commit.author.name,
+            "author_email": commit.author.email,
+            "author": commit.author,
+            "contents": commit.message,
+            "reviewer": ChangeLogEntry._parse_reviewer_text(commit.message)[0],
+            "bug_description": commit.message.splitlines()[0],
+            "changed_files": self._scm.changed_files_for_revision(revision),
         }
 
     @memoized

Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py

@@ -31,8 +31,6 @@
 import os
 import unittest
 
-from webkitcorepy import string_utils
-
 from webkitpy.common.checkout.checkout import Checkout
 from webkitpy.common.checkout.changelog import ChangeLogEntry
 from webkitpy.common.checkout.scm import CommitMessage, SCMDetector
@@ -44,7 +42,8 @@
 from webkitpy.common.system.executive_mock import MockExecutive
 from webkitpy.thirdparty.mock import Mock
 
-from webkitcorepy import OutputCapture
+from webkitcorepy import string_utils, OutputCapture
+from webkitscmpy import mocks
 
 
 _changelog1entry1 = u"""2010-03-25  Fr\u00e9d\u00e9ric Wang  <fred.wang@free.fr>
@@ -351,61 +350,6 @@ def mock_contents_at_revision(changelog_path, revision):
         entry = checkout._latest_entry_for_changelog_at_revision("foo", "bar")
         self.assertMultiLineEqual(entry.contents(), _changelog1entry1)  # Pylint is confused about this line, pylint: disable=E1101
 
-    # 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):
-        checkout = self._make_checkout()
-        checkout._scm.changed_files_for_revision = lambda revision: ['foo/ChangeLog', 'bar/ChangeLog']
-
-        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):
-        checkout = self._make_checkout()
-        checkout._scm.changed_files_for_revision = lambda revision: ['path/to/file', 'another/file']
-        checkout._scm.committer_email_for_revision = lambda revision, changed_files=None: "committer@example.com"
-        checkout.changelog_entries_for_revision = lambda revision, changed_files=None: [ChangeLogEntry(_changelog1entry1)]
-        commitinfo = checkout.commit_info_for_revision(4)
-        self.assertEqual(commitinfo.bug_id(), 36629)
-        self.assertEqual(commitinfo.bug_description(), "Unreviewed build fix to un-break webkit-patch land.")
-        self.assertEqual(commitinfo.author_name(), u"Fr\u00e9d\u00e9ric Wang")
-        self.assertEqual(commitinfo.author_email(), "fred.wang@free.fr")
-        self.assertIsNone(commitinfo.reviewer_text())
-        self.assertIsNone(commitinfo.reviewer())
-        self.assertEqual(commitinfo.committer_email(), "committer@example.com")
-        self.assertIsNone(commitinfo.committer())
-        self.assertEqual(commitinfo.to_json(), {
-            'bug_id': 36629,
-            'author_email': 'fred.wang@free.fr',
-            'changed_files': [
-                'path/to/file',
-                'another/file',
-            ],
-            'reviewer_text': None,
-            'author_name': u'Fr\u00e9d\u00e9ric Wang',
-            'bug_description': 'Unreviewed build fix to un-break webkit-patch land.',
-        })
-
-        checkout.changelog_entries_for_revision = lambda revision, changed_files=None: []
-        self.assertIsNone(checkout.commit_info_for_revision(1))
-
-    def test_bug_id_for_revision(self):
-        checkout = self._make_checkout()
-        checkout._scm.committer_email_for_revision = lambda revision: "committer@example.com"
-        checkout.changelog_entries_for_revision = lambda revision, changed_files=None: [ChangeLogEntry(_changelog1entry1)]
-        self.assertEqual(checkout.bug_id_for_revision(4), 36629)
-
     def test_bug_id_for_this_commit(self):
         checkout = self._make_checkout()
         checkout.commit_message_for_this_commit = lambda git_commit, changed_files=None, return_stderr=False: CommitMessage(ChangeLogEntry(_changelog1entry1).contents().splitlines())
@@ -418,30 +362,6 @@ def test_modified_changelogs(self):
         expected_changlogs = ["/foo/bar/ChangeLog", "/foo/bar/relative/path/ChangeLog"]
         self.assertEqual(checkout.modified_changelogs(git_commit=None), expected_changlogs)
 
-    def test_suggested_reviewers(self):
-        def mock_changelog_entries_for_revision(revision, changed_files=None):
-            if revision == 27:
-                return []
-            if revision % 2 == 0:
-                return [ChangeLogEntry(_changelog1entry1)]
-            return [ChangeLogEntry(_changelog1entry2)]
-
-        def mock_revisions_changing_file(path, limit=5):
-            if path.endswith('ChangeLog'):
-                return [3]
-            if path.endswith('file_with_empty_changelog'):
-                return [27]
-            return [4, 8]
-
-        checkout = self._make_checkout()
-        checkout._scm.checkout_root = '/foo/bar'
-        checkout._scm.changed_files = lambda git_commit: ['file1', 'file2', 'relative/path/ChangeLog', 'file_with_empty_changelog']
-        checkout._scm.revisions_changing_file = mock_revisions_changing_file
-        checkout.changelog_entries_for_revision = mock_changelog_entries_for_revision
-        reviewers = checkout.suggested_reviewers(git_commit=None)
-        reviewer_names = [reviewer.full_name for reviewer in reviewers]
-        self.assertEqual(reviewer_names, [u'Fr\u00e9d\u00e9ric Wang'])
-
     def test_apply_patch(self):
         checkout = self._make_checkout()
         checkout._executive = MockExecutive(should_log=True)

Tools/Scripts/webkitpy/common/config/committers.py

@@ -74,6 +74,10 @@ def bugzilla_email(self):
         # which might not be right.
         return self.emails[0]
 
+    @property
+    def email(self):
+        self.bugzilla_email()
+
     def __str__(self):
         return string_utils.encode(u'"{}" <{}>'.format(unicode(self.full_name), unicode(self.emails[0])), target_type=str)
 

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

@@ -333,9 +333,9 @@ def _prepare_state(self, options, args, tool):
                 # We use the earliest revision for the bug info
                 if revision == earliest_revision:
                     state["bug_blocked"] = commit_info.bug_id()
-                    cc_list = sorted([party.bugzilla_email()
+                    cc_list = sorted([party.email
                             for party in commit_info.responsible_parties()
-                            if party.bugzilla_email()])
+                            if getattr(party, 'email', None)])
                     # FIXME: We should used the list as the canonical representation.
                     state["bug_cc"] = ",".join(cc_list)
                 description_list.append(commit_info.bug_description())

Tools/Scripts/webkitpy/tool/steps/preparechangelogforrevert.py

@@ -54,14 +54,8 @@ def _message_for_revert(cls, revision_list, reason, description_list, reverted_b
 
     def run(self, state):
         reverted_bug_url_list = []
-        # This could move to prepare-ChangeLog by adding a --revert= option.
-        self._tool.executive.run_and_throw_if_fail(self._tool.deprecated_port().prepare_changelog_command(), cwd=self._tool.scm().checkout_root)
-        changelog_paths = self._tool.checkout().modified_changelogs(git_commit=None)
         revert_bug_url = self._tool.bugs.bug_url_for_bug_id(state["bug_id"]) if state["bug_id"] else None
         for bug_id in state["bug_id_list"]:
             reverted_bug_url_list.append(self._tool.bugs.bug_url_for_bug_id(bug_id))
         message = self._message_for_revert(state["revision_list"], state["reason"], state["description_list"], reverted_bug_url_list, revert_bug_url)
-        for changelog_path in changelog_paths:
-            # FIXME: Seems we should prepare the message outside of changelogs.py and then just pass in
-            # text that we want to use to replace the reviewed by line.
-            ChangeLog(changelog_path).update_with_unreviewed_message(message)
+        self._tool.executive.run_and_throw_if_fail(['git', 'commit', '-a', '-m', message], cwd=self._tool.scm().checkout_root)