[Webkit-unassigned] [Bug 26715] bugzilla-tool should automate rollouts

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=26715


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #32976|review?                     |review-
               Flag|                            |




--- Comment #6 from David Levin <levin at chromium.org>  2009-07-17 19:42:25 PDT ---
(From update of attachment 32976)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.