Bug 27119 – bugzilla-tool: Add create-bug command

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[David Kilzer (:ddkilzer)]

Created attachment 32518 [details]
Patch v1

Reviewed by NOBODY (OOPS!).

Initial pass at a "create-bug" command for bugzilla-tool.  This
version only works with local git commits since that was the
immediate need.

* Scripts/bugzilla-tool: Added CreateBugWithPatch class.
(CreateBugWithPatch.__init__): Added.
(CreateBugWithPatch.execute): Added.
(BugzillaTool.__init__): Added create-bug command.
* Scripts/modules/bugzilla.py:
(Bugzilla.prompt_for_component): Added.
(Bugzilla.create_bug_with_patch): Added.
---
 3 files changed, 101 insertions(+), 1 deletions(-)

[Sam Weinig]

Comment on attachment 32518 [details]
Patch v1

I think you should make this work with svn before it goes in as this is the primary tool used by this project.  Adding git support as well is okay, but it should not be the only way to do anything.

[Eric Seidel (no email)]

Comment on attachment 32518 [details]
Patch v1

I think it's entirely reasonable that the first run of create-bug works only with git. :)  Since at least 2 of the 3 known users of bugzilla-tool are git users. :)

maybe this would be "bug-from-commits" and "bug-from-diff" is the one svn users would use.  It's possible we could wrap them both into a single command, although currently post-diff and post-commits are separate because git users (at least myself) use both at different times.  post-diff is the only one which svn users use equivalently bug-from-diff would be for svn users so they don't have to create a patch file.

I really like the --cc option.  "gcl upload" (google's equivalent command) has a -r option to do that which is similarly awesome!

It seems we should name the option "COMMIT" instead of COMMITISH if only one commit is supported?

I thikn this should be in its own function:
480         commit_lines = [re.sub('^ {0,8}', '', line, 1) for line in commit_message.splitlines()]
 481 
 482         bug_title = commit_lines[0]
 483         comment_text = "\n".join(commit_lines[1:])
 484         comment_text += "\n---\n"
 485         comment_text += tool.scm().files_changed_summary_for_commit(commit_id)

(title, description) = title_and_description_for_bug(commit_text)
 maybe?

It seems this belongs in a local: (since you type it out twice)
self.browser.find_control('component').items

Aren't bug descriptions required by bugzilla?

I would probably break the create_bug function up into smaller pieces still.  I tend to be alergic to functions longer than about 30 lines. ;)

In general this looks great though!

So I'm not going to reverse Sam's review, but I don't think SVN support needs to be required in the first patch.  Certainly SVN support should follow, but bugzilla-tool hasn't even been announced on webkit-dev yet, so I don't really view it as a critical project infrastructure requiring of SVN support.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> (From update of attachment 32518 [details])

> maybe this would be "bug-from-commits" and "bug-from-diff" is the one svn users
> would use.  It's possible we could wrap them both into a single command,
> although currently post-diff and post-commits are separate because git users
> (at least myself) use both at different times.  post-diff is the only one which
> svn users use equivalently bug-from-diff would be for svn users so they don't
> have to create a patch file.

Yes, I should have named the command CreateBugWithCommit() instead of CreateBugWithPatch().  It would be nice if the create-bug command would work with both SCMs without having to create different commands for each.

> It seems we should name the option "COMMIT" instead of COMMITISH if only one
> commit is supported?

I wanted it to support multiple commits eventually, just not in the first pass.  I will change this for the first version and update it later.

> I thikn this should be in its own function:
> 480         commit_lines = [re.sub('^ {0,8}', '', line, 1) for line in
> commit_message.splitlines()]
>  481 
>  482         bug_title = commit_lines[0]
>  483         comment_text = "\n".join(commit_lines[1:])
>  484         comment_text += "\n---\n"
>  485         comment_text +=
> tool.scm().files_changed_summary_for_commit(commit_id)
> 
> (title, description) = title_and_description_for_bug(commit_text)
>  maybe?

I created a CommitMessage class that does this for you.  Haven't uploaded it quite yet.

> It seems this belongs in a local: (since you type it out twice)
> self.browser.find_control('component').items

Okay.

> Aren't bug descriptions required by bugzilla?

Please re-read the list of create_bug_with_patch() arguments.  Perhaps I should be using named parameters to make it clearer?  Or creating a dictionary and passing that in.  Yes, a bug[] dictionary would be nicer.

> I would probably break the create_bug function up into smaller pieces still.  I
> tend to be alergic to functions longer than about 30 lines. ;)

Yes, I could probably break out the error handling stuff into a separate method.

> In general this looks great though!

Thanks!

Note that I probably won't have time to work on this again until Tuesday.

[David Kilzer (:ddkilzer)]

Created attachment 32600 [details]
Patch v2: More WIP

[David Kilzer (:ddkilzer)]

Comment on attachment 32600 [details]
Patch v2: More WIP

Refactored a bit to address most issues in Comment #2 except the create_bug_with_patch() method signature.

Just have to add svn CreateBug.support to createBugFromPatch() now.

[David Kilzer (:ddkilzer)]

Created attachment 32609 [details]
Patch v3

[David Kilzer (:ddkilzer)]

Comment on attachment 32609 [details]
Patch v3

This implements create-bug for both svn and git.

[David Kilzer (:ddkilzer)]

Bug 27173 was created with Patch v3 from an svn working directory.

[Eric Seidel (no email)]

Comment on attachment 32609 [details]
Patch v3

 489         bug_id = tool.bugs.create_bug_with_patch(bug_title, comment_text, options.component, diff_file, "Patch v1", cc=options.cc, mark_for_review=options.review)

Is "Patch v1" intended there?

You don't have to forward declare in python AFAIK:
bug_title = ""
 476         comment_text = ""

It seems this wants to be its own function:
80             commit_message = tool.scm().commit_message_for_commit(commit_id)
 481             commit_lines = [re.sub('^ {0,8}', '', line, 1) for line in commit_message.splitlines()]
 482             bug_title = commit_lines[0]
 483             comment_text = "\n".join(commit_lines[1:]) + "\n"
 484             comment_text += "---\n"
 485             comment_text += tool.scm().files_changed_summary_for_commit(commit_id)


There seems to be come copy/paste from the previous section:
 503             commit_message = commit_message_for_this_commit(tool.scm())
 504             commit_lines = [re.sub('^ {0,8}', '', line, 1) for line in commit_message.splitlines()]
 505             bug_title = commit_lines[0]
 506             comment_text = "\n".join(commit_lines[1:]) + "\n"

AGain, no need for forward-declare:
498         bug_title = ""
 499         comment_text = ""

We shoudl just fix all these functions to detect a string and turn it into a file for Mechanize:
         diff = tool.scm().create_patch()
 509         diff_file = StringIO.StringIO(diff) # create_bug_with_patch expects a file-like object
 510         bug_id = tool.bugs.create_bug_with_patch(bug_title, comment_text, options.component, diff_file, "Patch v1", cc=options.cc, mark_for_review=options.review)

You can invert the order of the clauses with:
if len(args) == 0
and use
if len(args) instead.  Doesn't really matter.

r- for the copy/paste code.  Otherwise this seems fine in general.

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> (From update of attachment 32609 [details])
>  bug_id = tool.bugs.create_bug_with_patch(bug_title, comment_text, options.component, diff_file, "Patch v1", cc=options.cc, mark_for_review=options.review)
> 
> Is "Patch v1" intended there?

Absolutely.  It's a sane default for the first patch in the bug.  I want to do more in this area, specifically:

- Make bzt smart enough to increment "Patch vN" to "Patch vN+1" when new patches are attached.
- Give the user the ability to write their own patch description via command line for create-bug and post-commit and post-diff.  (And custom patch comments.)

But I wanted to get create-bug landed first.

> You don't have to forward declare in python AFAIK:
> bug_title = ""
> comment_text = ""

Umm, yes, but these variables will be out of scope if I don't declare them outside the if/else statement, right?

> It seems this wants to be its own function:
>  480             commit_message = tool.scm().commit_message_for_commit(commit_id)
>  481             commit_lines = [re.sub('^ {0,8}', '', line, 1) for line in
> commit_message.splitlines()]
>  482             bug_title = commit_lines[0]
>  483             comment_text = "\n".join(commit_lines[1:]) + "\n"
>  484             comment_text += "---\n"
>  485             comment_text +=
> tool.scm().files_changed_summary_for_commit(commit_id)

That code wants to be rewritten after Bug 27172 lands.  It's waiting for a review.

> There seems to be come copy/paste from the previous section:
>  503             commit_message = commit_message_for_this_commit(tool.scm())
>  504             commit_lines = [re.sub('^ {0,8}', '', line, 1) for line in
> commit_message.splitlines()]
>  505             bug_title = commit_lines[0]
>  506             comment_text = "\n".join(commit_lines[1:]) + "\n"

Yep.  I did that in the name of getting create-bug into the hands of the people.  I was going to clean this up with 27172 or this bug, which ever landed last.

> AGain, no need for forward-declare:
>  498         bug_title = ""
>  499         comment_text = ""

Can you explain how variable scoping works in Python?

> We shoudl just fix all these functions to detect a string and turn it into a
> file for Mechanize:
>  508         diff = tool.scm().create_patch()
>  509         diff_file = StringIO.StringIO(diff) # create_bug_with_patch
> expects a file-like object
>  510         bug_id = tool.bugs.create_bug_with_patch(bug_title, comment_text,
> options.component, diff_file, "Patch v1", cc=options.cc,
> mark_for_review=options.review)

How does one "detect a string" in Python?

> You can invert the order of the clauses with:
> if len(args) == 0
> and use
> if len(args) instead.  Doesn't really matter.

Okay.

> r- for the copy/paste code.  Otherwise this seems fine in general.

Further refinements will have to wait until Tuesday.  I'd appreciate answers to my questions in the meantime.  Thanks!

[Eric Seidel (no email)]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 32609 [details] [details])
> That code wants to be rewritten after Bug 27172 lands.  It's waiting for a
> review.

Done.  Feel free to email me directly if I miss your reviews in the queue for some reason.  The queue is a bit large to be able to see much in atm. ;)

> Can you explain how variable scoping works in Python?

No, and I probably am remembering it wrong. :(  Your sense that they should be "out of scope" makes sense.

> How does one "detect a string" in Python?

I think there are typeof and isinstance operators.  I'm pretty sure I detect a dict at one point in the code.

Hum... just found http://www.canonical.org/~kragen/isinstance/

> Further refinements will have to wait until Tuesday.  I'd appreciate answers to
> my questions in the meantime.  Thanks!

Thanks again!  Hopefully I've answered them.

[Eric Seidel (no email)]

Dave Levin has much more python-fu than I do, btw.

[Mark Rowe (bdash)]

(In reply to comment #11)
> > Can you explain how variable scoping works in Python?
> 
> No, and I probably am remembering it wrong. :(  Your sense that they should be
> "out of scope" makes sense.

Python is a little bit like JavaScript in that functions introduce a new scope, but the contents of "if", "while" and friends don't.  The only gotcha is that, unlike JavaScript, the variable name is not actually bound until the assignment statement executes.  If you access the variable before the assignment executes you'll be greeted with a name error.  This is especially important to keep in mind if only one side of an if/else assigns to a variable.

> > How does one "detect a string" in Python?
> 
> I think there are typeof and isinstance operators.  I'm pretty sure I detect a
> dict at one point in the code.

One typically doesn't do type detection in Python.  What are we trying to do here?  Avoid callers needing to wrap a string up in a StringIO object before passing to a function that takes a file-like object?

[Eric Seidel (no email)]

(In reply to comment #13)
> (In reply to comment #11)

Thank you for the scoping help.

> One typically doesn't do type detection in Python.  What are we trying to do
> here?  Avoid callers needing to wrap a string up in a StringIO object before
> passing to a function that takes a file-like object?

Yes.  Mechanize (or really ClientForm) requires a "file like object" (one with a .read()) be passed to to file upload controls.  We often end up with strings and thus wrap them with StringIO.  When there was just one callsite where I had to wrap first with StringIO it made sense to do it outside of the "post attachment" function.  Now there are a bunch of callsites and it seems that the Bugzilla class should handle either strings or file-like objects and be able to convert strings using StringIO when necessary.  (ideally ClientForm would do this for us, but alas it does not).

[David Kilzer (:ddkilzer)]

Created attachment 33683 [details]
Patch v4

[David Kilzer (:ddkilzer)]

Comment on attachment 33683 [details]
Patch v4

(In reply to comment #9)
> (From update of attachment 32609 [details])
>  489         bug_id = tool.bugs.create_bug_with_patch(bug_title, comment_text,
> options.component, diff_file, "Patch v1", cc=options.cc,
> mark_for_review=options.review)
> 
> Is "Patch v1" intended there?

Yes.  It's a sensible default until a switch is added later to provide a patch description.

> You don't have to forward declare in python AFAIK:
> bug_title = ""
>  476         comment_text = ""

Due to scoping rules in Python, this was required.

> It seems this wants to be its own function:
> 80             commit_message = tool.scm().commit_message_for_commit(commit_id)
>  481             commit_lines = [re.sub('^ {0,8}', '', line, 1) for line in
> commit_message.splitlines()]
>  482             bug_title = commit_lines[0]
>  483             comment_text = "\n".join(commit_lines[1:]) + "\n"
>  484             comment_text += "---\n"
>  485             comment_text +=
> tool.scm().files_changed_summary_for_commit(commit_id)
> 
> There seems to be come copy/paste from the previous section:
>  503             commit_message = commit_message_for_this_commit(tool.scm())
>  504             commit_lines = [re.sub('^ {0,8}', '', line, 1) for line in
> commit_message.splitlines()]
>  505             bug_title = commit_lines[0]
>  506             comment_text = "\n".join(commit_lines[1:]) + "\n"

These were updated to use CommitMessage methods from Bug 27172.

> AGain, no need for forward-declare:
> 498         bug_title = ""
>  499         comment_text = ""

They are needed.

> We shoudl just fix all these functions to detect a string and turn it into a
> file for Mechanize:
>          diff = tool.scm().create_patch()
>  509         diff_file = StringIO.StringIO(diff) # create_bug_with_patch
> expects a file-like object
>  510         bug_id = tool.bugs.create_bug_with_patch(bug_title, comment_text,
> options.component, diff_file, "Patch v1", cc=options.cc,
> mark_for_review=options.review)

Not done per Comment #11 and Comment #13.

> You can invert the order of the clauses with:
> if len(args) == 0
> and use
> if len(args) instead.  Doesn't really matter.

Done.

[David Kilzer (:ddkilzer)]

Comment on attachment 33683 [details]
Patch v4

> +            make_option("--no-prompt", action="store_true", dest="prompt", default=True, help="Do not prompt for bug title and comment; use commit log instead."),

This should be:  action="store_false"

> +            commit_message = tool.scm().commit_message_for_commit(commit_id)

This should be:  commit_message_for_local_commit(commit_id)

I will fix these when landing.

[David Levin]

Comment on attachment 33683 [details]
Patch v4

Biggest concern:

I don't understand the bootstrapping aspect to this. In order to create a well formed patch, I need a changelog with a bug link/title.  This tool creates a bug for a patch, but by definition, the patch must not be well formed because the bug doesn't exist yet.

It could modify the ChangeLog right after creating the bug (but that sounds complicated especially considering that you can given it a COMMITISH....)

On to the details.


> diff --git a/WebKitTools/Scripts/bugzilla-tool b/WebKitTools/Scripts/bugzilla-tool
> +class CreateBug(Command):
> +    def __init__(self):
> +        Command.__init__(self, 'Create a bug from local changes or local commits', '[COMMITISH]', options=options)

Nice to add period to this sentence.

> +
> +    def create_bug_from_commit(self, options, args, tool):
> +        commit_ids = tool.scm().commit_ids_from_range_arguments(args, cherry_pick=True)

This won't be very friendly to svn users if they happen to given an extra argument.  Maybe it should check that it is git and if not, error out with a friendly message.


> diff --git a/WebKitTools/Scripts/modules/bugzilla.py b/WebKitTools/Scripts/modules/bugzilla.py
> +    def check_create_bug_response_returning_bug_id_on_success(self, response_html):

If you don't expect external callers, then prefix it with a _

It is quite long. What about _get_create_bug_id(self, create_bug_response):
or something else (even) shorter?

> +    def create_bug_with_patch(self, bug_title, bug_description, component, patch_file_object, patch_description, cc, mark_for_review=False):
...
> +        if not component or component not in [item.name for item in component_items]:
> +            component = self.prompt_for_component([item.name for item in component_items])

This is fine.  I like this better:

        component_names = map(lambda item: item.name, component_items)
        if not component or component not in component_names:
            component = self.prompt_for_component(component_names)

You choose.

[David Kilzer (:ddkilzer)]

Created attachment 33691 [details]
Patch v5

[David Kilzer (:ddkilzer)]

Comment on attachment 33691 [details]
Patch v5

(In reply to comment #18)
> (From update of attachment 33683 [details])
> Biggest concern:
> 
> I don't understand the bootstrapping aspect to this. In order to create a well
> formed patch, I need a changelog with a bug link/title.  This tool creates a
> bug for a patch, but by definition, the patch must not be well formed because
> the bug doesn't exist yet.
> 
> It could modify the ChangeLog right after creating the bug (but that sounds
> complicated especially considering that you can given it a COMMITISH....)

I honestly don't understand what you're asking here.  This is the workflow I envision for git and svn:

Git
1. Modify code.
2. Run prepare-ChangeLog
3. Edit ChangeLogs.
4. Commit the patch.
5. Run bugzilla-tool create-bug.

Svn (or local changes to Git, if you roll that way)
1. Modify code.
2. Run prepare-ChangeLog
3. Edit ChangeLogs.
4. Run bugzilla-tool create-bug.

Either way you have to go back and add the bug number to the ChangeLog files currently.  This addresses the initial need for a way to create a Bugzilla bug for a patch, which Maciej and others have been asking for on webkit-dev.

After this create-bug patch lands, I plan to make it possible to substitute the bug number in the ChangeLog entry before you commit--similar to how the reviewer is added now.  I just didn't want to do it all at once and create a huge patch.

> > +        Command.__init__(self, 'Create a bug from local changes or local commits', '[COMMITISH]', options=options)
> 
> Nice to add period to this sentence.

Done.

> > +    def create_bug_from_commit(self, options, args, tool):
> > +        commit_ids = tool.scm().commit_ids_from_range_arguments(args, cherry_pick=True)
> 
> This won't be very friendly to svn users if they happen to given an extra
> argument.  Maybe it should check that it is git and if not, error out with a
> friendly message.

Fixed before create_bug_from_commit() is called.

> > +    def check_create_bug_response_returning_bug_id_on_success(self, response_html):
> 
> If you don't expect external callers, then prefix it with a _
> 
> It is quite long. What about _get_create_bug_id(self, create_bug_response):
> or something else (even) shorter?

I was trying to use a descriptive method name here, but I guess I got carried away.  Added "_" prefix and removed "_returning_bug_id_on_success" suffix.

> > +    def create_bug_with_patch(self, bug_title, bug_description, component, patch_file_object, patch_description, cc, mark_for_review=False):
> ...
> > +        if not component or component not in [item.name for item in component_items]:
> > +            component = self.prompt_for_component([item.name for item in component_items])
> 
> This is fine.  I like this better:
> 
>         component_names = map(lambda item: item.name, component_items)
>         if not component or component not in component_names:
>             component = self.prompt_for_component(component_names)
> 
> You choose.

I chose your way.  I'm new to python, so I haven't used this map/lambda construct.  Very concise, though.

[David Kilzer (:ddkilzer)]

(In reply to comment #18)
> (From update of attachment 33683 [details])
> Biggest concern:
> 
> I don't understand the bootstrapping aspect to this. In order to create a well
> formed patch, I need a changelog with a bug link/title.  This tool creates a
> bug for a patch, but by definition, the patch must not be well formed because
> the bug doesn't exist yet.

I see what you're asking.  Creating a bug for the sole purpose of reviewing a patch is a common practice when you're doing clean-up work or merging changes from a port back to the main repository.  You don't really need to create the bug until you're done with the patch, which this command provides.  It's a fairly common workflow that the Safari team uses.

[David Levin]

Comment on attachment 33691 [details]
Patch v5


> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> [phone]  David Kilzer  <[e-mail]>
> +
> +        <http://webkit.org/b/27119> bugzilla-tool: Add create-bug command
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Implement "create-bug" command for bugzilla-tool.

oooo new format :)


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

> +        if match:
> +            text_lines = BeautifulSoup(match.group('error_message')).findAll(text=True)
> +            error_message = "\n" + '\n'.join(["  " + line.strip() for line in text_lines if line.strip()])

fwiw, this statment is at the edge of the complexity boundary.  It is hard to define but when you start seeing "for in if" + other stuff, you're close.

I'd leave it as is though.  Just something to keep in mind in your future python writing sessions :)

[David Kilzer (:ddkilzer)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/bugzilla-tool
	M	WebKitTools/Scripts/modules/bugzilla.py
Committed r46538
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/modules/bugzilla.py
	M	WebKitTools/Scripts/bugzilla-tool
r46538 = 9c9cf704d95dadecc84ee834a62d67bf78f81e1c (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46538