Skip to content
Permalink
Browse files
2010-10-28 Eric Seidel <eric@webkit.org>
        Reviewed by Adam Barth.

        webkit-patch upload calls changed_files more often than it should
        https://bugs.webkit.org/show_bug.cgi?id=48567

        Passing changed_files around everywhere isn't a very elegant solution
        but it's the one we have for the moment.  I think keeping an explicit
        cache on Checkout (or making StepState() a real class) is a better
        long-term option.

        Previously bug_id_for_this_commit was calling changed_files and the
        result was never getting cached on the state.  Now we're explicitly
        caching the result on the state and passing that to the bug_id_for_this_commit call.

        I looked into building unit tests for this.  Doing so would require
        using a real Checkout object with a MockSCM and overriding the appropriate
        calls on SCM to count how often we're stating the file system.
        That's a useful set of tests to build for a separate change.

        * Scripts/webkitpy/common/checkout/api.py:
        * Scripts/webkitpy/tool/commands/download.py:
        * Scripts/webkitpy/tool/commands/upload.py:
        * Scripts/webkitpy/tool/mocktool.py:

Canonical link: https://commits.webkit.org/61331@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@70820 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
eseidel committed Oct 28, 2010
1 parent ae87b51 commit 02a83d789b79e352371798f56698353e8ba0beee
Showing 7 changed files with 41 additions and 9 deletions.
@@ -1,3 +1,29 @@
2010-10-28 Eric Seidel <eric@webkit.org>

Reviewed by Adam Barth.

webkit-patch upload calls changed_files more often than it should
https://bugs.webkit.org/show_bug.cgi?id=48567

Passing changed_files around everywhere isn't a very elegant solution
but it's the one we have for the moment. I think keeping an explicit
cache on Checkout (or making StepState() a real class) is a better
long-term option.

Previously bug_id_for_this_commit was calling changed_files and the
result was never getting cached on the state. Now we're explicitly
caching the result on the state and passing that to the bug_id_for_this_commit call.

I looked into building unit tests for this. Doing so would require
using a real Checkout object with a MockSCM and overriding the appropriate
calls on SCM to count how often we're stating the file system.
That's a useful set of tests to build for a separate change.

* Scripts/webkitpy/common/checkout/api.py:
* Scripts/webkitpy/tool/commands/download.py:
* Scripts/webkitpy/tool/commands/upload.py:
* Scripts/webkitpy/tool/mocktool.py:

2010-10-28 Eric Seidel <eric@webkit.org>

Reviewed by Adam Barth.
@@ -1 +1,3 @@
# Required for Python to search this directory for module files

from api import Checkout
@@ -100,8 +100,8 @@ def modified_changelogs(self, git_commit, changed_files=None):
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)

def commit_message_for_this_commit(self, git_commit):
changelog_paths = self.modified_changelogs(git_commit)
def commit_message_for_this_commit(self, git_commit, changed_files=None):
changelog_paths = self.modified_changelogs(git_commit, changed_files)
if not len(changelog_paths):
raise ScriptError(message="Found no modified ChangeLogs, cannot create a commit message.\n"
"All changes require a ChangeLog. See:\n"
@@ -129,9 +129,9 @@ def suggested_reviewers(self, git_commit, changed_files=None):
reviewers.extend([commit_info.author() for commit_info in commit_infos if commit_info.author() and commit_info.author().can_review])
return sorted(set(reviewers))

def bug_id_for_this_commit(self, git_commit):
def bug_id_for_this_commit(self, git_commit, changed_files=None):
try:
return parse_bug_id(self.commit_message_for_this_commit(git_commit).message())
return parse_bug_id(self.commit_message_for_this_commit(git_commit, changed_files).message())
except ScriptError, e:
pass # We might not have ChangeLogs.

@@ -114,7 +114,7 @@ def tearDown(self):
# ChangeLog is difficult to mock at current.
def test_commit_message_for_this_commit(self):
checkout = Checkout(None)
checkout.modified_changelogs = lambda git_commit: ["ChangeLog1", "ChangeLog2"]
checkout.modified_changelogs = lambda git_commit, changed_files=None: ["ChangeLog1", "ChangeLog2"]
output = OutputCapture()
expected_stderr = "Parsing ChangeLog: ChangeLog1\nParsing ChangeLog: ChangeLog2\n"
commit_message = output.assert_outputs(self, checkout.commit_message_for_this_commit,
@@ -163,7 +163,7 @@ def test_bug_id_for_revision(self):
def test_bug_id_for_this_commit(self):
scm = Mock()
checkout = Checkout(scm)
checkout.commit_message_for_this_commit = lambda git_commit: CommitMessage(ChangeLogEntry(_changelog1entry1).contents().splitlines())
checkout.commit_message_for_this_commit = lambda git_commit, changed_files=None: CommitMessage(ChangeLogEntry(_changelog1entry1).contents().splitlines())
self.assertEqual(checkout.bug_id_for_this_commit(git_commit=None), 36629)

def test_modified_changelogs(self):
@@ -106,8 +106,10 @@ class Land(AbstractSequencedCommand):
If a bug id is provided, or one can be found in the ChangeLog land will update the bug after committing."""

def _prepare_state(self, options, args, tool):
changed_files = self._tool.scm().changed_files(options.git_commit)
return {
"bug_id": (args and args[0]) or tool.checkout().bug_id_for_this_commit(options.git_commit),
"changed_files": changed_files,
"bug_id": (args and args[0]) or tool.checkout().bug_id_for_this_commit(options.git_commit, changed_files),
}


@@ -152,7 +152,9 @@ def _bug_id(self, options, args, tool, state):
# Perfer a bug id passed as an argument over a bug url in the diff (i.e. ChangeLogs).
bug_id = args and args[0]
if not bug_id:
bug_id = tool.checkout().bug_id_for_this_commit(options.git_commit)
changed_files = self._tool.scm().changed_files(options.git_commit)
state["changed_files"] = changed_files
bug_id = tool.checkout().bug_id_for_this_commit(options.git_commit, changed_files)
return bug_id

def _prepare_state(self, options, args, tool):
@@ -475,7 +475,7 @@ def modified_changelogs(self, git_commit, changed_files=None):
# that LandDiff will try to actually read the patch from disk!
return []

def commit_message_for_this_commit(self, git_commit):
def commit_message_for_this_commit(self, git_commit, changed_files=None):
commit_message = Mock()
commit_message.message = lambda:"This is a fake commit message that is at least 50 characters."
return commit_message

0 comments on commit 02a83d7

Please sign in to comment.