[webkit-reviews] review denied: [Bug 26715] bugzilla-tool should automate rollouts : [Attachment 32976] Begone ye tabs!

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jul 17 19:42:25 PDT 2009


David Levin <levin at chromium.org> has denied Eric Seidel <eric at webkit.org>'s
request for review:
Bug 26715: bugzilla-tool should automate rollouts
https://bugs.webkit.org/show_bug.cgi?id=26715

Attachment 32976: Begone ye tabs!
https://bugs.webkit.org/attachment.cgi?id=32976&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Just a few things to address.  I'm most concerned about the issue I mentioned
in update_changelog_for_revert.

Two general notes:
1. It was pointed out to me that python style is to have two blank lines
between top level functions: "Separate top-level function and class definitions
with two blank lines."
2. Many comments don't end with periods.  Please add them.

> diff --git a/WebKitTools/Scripts/bugzilla-tool
b/WebKitTools/Scripts/bugzilla-tool

> +def update_changelog_for_revert(changelog_path, revision):
> +    removing_boilerplate = False
> +    # inplace=1 creates a backup file and re-directs stdout to the file
> +    for line in fileinput.FileInput(changelog_path, inplace=1):
> +	   if re.search('Reviewed by', line):

I'm confused.  How does this stop after this boilerplate change log that was
just created?

It looks like it goes through the whole changelog removing all descriptions
along the way.


> +	       print line.replace("Reviewed by NOBODY (OOPS!)", "No review,
rolling out %s" % revision),

I like to prefix revisions with an r.
Consider: ..."No review, rolling out r%s"...


> +class RolloutCommit(Command):
...
> +    def create_changelogs_for_revert(scm, revision):
...
> +	   # This could move to prepare-ChangeLog by adding a --revert= option

That might be easier.  I think the current code has a bug as I noted above.


> diff --git a/WebKitTools/Scripts/modules/scm.py
b/WebKitTools/Scripts/modules/scm.py

> +    @classmethod
> +    def git_commit_from_svn_revision(cls, revision):
> +	   git_log_output = cls.run_command(['git', 'svn', 'log', '-r',
revision, '--oneline', '--show-commit'])
> +	   # Example: r45396 | b4954e7 | 2009-06-30  Antonio Gomes  
<antonio.gomes at openbossa.org>
> +	   matched_commit = re.search("^r(\d+) \| (\w+) \| .*$",
git_log_output, re.MULTILINE)

This is ok, but I always find code more readable when it uses named groups.


More information about the webkit-reviews mailing list