Bug 26734 – bugzilla-tool land-and-update needs to fix reviewers in ChangeLogs

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

bugzilla-tool land-and-update needs to fix reviewers in ChangeLogs

Right now it just tries to commit and update the bug, but doesn't fix up the ChangeLogs to have a reviewer.

This command is meant for when you're landing your own patches which have been reviewed and are already in your tree (possibly with reviewer-suggested modifications).

It would be easy to grab the reviewer off the patch on the bug (assuming there is only one patch).  We can error-out if there is more than one patch I guess.

[Eric Seidel (no email)]

Ok.  I've changed "land-and-update" to the shorter "land-diff".

I've also made land-diff know how to build and test before actually committing.

I've also made land-diff's bug_id parameter optional (since sometimes you want to land changes which don't have a bug, but you do want to do want the automated goodness of reviewer setting and one more final build and test before landing.)

[Eric Seidel (no email)]

Created attachment 31962 [details]
First attempt

[David Levin]

Comment on attachment 31962 [details]
First attempt

A few minor things to fix.  Free to fix up and submit (unless you do larger changes and want another review of it).


> diff --git a/WebKitTools/Scripts/bugzilla-tool b/WebKitTools/Scripts/bugzilla-tool
> @@ -86,6 +87,11 @@ def latest_changelog_entry(changelog_path):
> +def set_reviewer_in_changelog(changelog_path, reviewer):
> +    # inplace=1 creates a backup file and re-directs stdout to the file
> +    for line in fileinput.FileInput(changelog_path, inplace=1):
> +       print line.replace("NOBODY (OOPS!)", reviewer),


Two comments:
1. Indent off by one space ("print line....")
2. Why is there a comma at the end of this line? (Can it be removed?)


>  class LandAndUpdateBug(Command):
>      def execute(self, options, args, tool):
> +        self.update_changelogs_with_reviewer(options.reviewer, bug_id, tool)
> +


It seems like it should update the date in the changelog as well. Could be a fix me for now.


> +        else:
> +            log(comment_text)


If the comment contains characters like \n, then I think it will mess up the output.  Maybe the log function should be updated to look like this

def log(string):
    print >> sys.stderr, "%s" % string

[Eric Seidel (no email)]

(In reply to comment #3)
> (From update of attachment 31962 [details] [review])
> A few minor things to fix.  Free to fix up and submit (unless you do larger
> changes and want another review of it).

Thanks!

> > diff --git a/WebKitTools/Scripts/bugzilla-tool b/WebKitTools/Scripts/bugzilla-tool
> > @@ -86,6 +87,11 @@ def latest_changelog_entry(changelog_path):
> > +def set_reviewer_in_changelog(changelog_path, reviewer):
> > +    # inplace=1 creates a backup file and re-directs stdout to the file
> > +    for line in fileinput.FileInput(changelog_path, inplace=1):
> > +       print line.replace("NOBODY (OOPS!)", reviewer),
>
> Two comments:
> 1. Indent off by one space ("print line....")
> 2. Why is there a comma at the end of this line? (Can it be removed?)

1.  Fixed
2.  To make print not add a newline when printing:
http://www.python.org/doc/2.4.4/ref/print.html
A "\n" character is written at the end, unless the print statement ends with a comma. This is the only action if the statement contains just the keyword print.
 
> >  class LandAndUpdateBug(Command):
> >      def execute(self, options, args, tool):
> > +        self.update_changelogs_with_reviewer(options.reviewer, bug_id, tool)
> > +
> 
> It seems like it should update the date in the changelog as well. Could be a
> fix me for now.

Yeah.  Will add a FIXME for now.
 
> > +        else:
> > +            log(comment_text)
> 
> If the comment contains characters like \n, then I think it will mess up the
> output.  Maybe the log function should be updated to look like this
> 
> def log(string):
>     print >> sys.stderr, "%s" % string

log("foo\nbar") seems to work just fine?

Thanks for the review!

[Eric Seidel (no email)]

Git got upset with itself.  But this landed as r45351.