WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
27119
bugzilla-tool: Add create-bug command
https://bugs.webkit.org/show_bug.cgi?id=27119
Summary
bugzilla-tool: Add create-bug command
David Kilzer (:ddkilzer)
Reported
2009-07-09 07:54:29 PDT
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(-)
Attachments
Patch v1
(7.02 KB, patch)
2009-07-09 07:54 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Patch v2: More WIP
(7.84 KB, patch)
2009-07-10 18:39 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Patch v3
(9.15 KB, patch)
2009-07-10 22:04 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Patch v4
(8.38 KB, patch)
2009-07-28 17:41 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Patch v5
(8.51 KB, patch)
2009-07-28 21:47 PDT
,
David Kilzer (:ddkilzer)
levin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Sam Weinig
Comment 1
2009-07-09 21:20:21 PDT
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 2
2009-07-09 23:25:20 PDT
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)
Comment 3
2009-07-10 17:10:22 PDT
(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)
Comment 4
2009-07-10 18:39:23 PDT
Created
attachment 32600
[details]
Patch v2: More WIP
David Kilzer (:ddkilzer)
Comment 5
2009-07-10 18:41:39 PDT
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)
Comment 6
2009-07-10 22:04:33 PDT
Created
attachment 32609
[details]
Patch v3
David Kilzer (:ddkilzer)
Comment 7
2009-07-10 22:05:43 PDT
Comment on
attachment 32609
[details]
Patch v3 This implements create-bug for both svn and git.
David Kilzer (:ddkilzer)
Comment 8
2009-07-10 22:07:17 PDT
Bug 27173
was created with Patch v3 from an svn working directory.
Eric Seidel (no email)
Comment 9
2009-07-10 22:39:11 PDT
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)
Comment 10
2009-07-10 23:04:10 PDT
(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)
Comment 11
2009-07-10 23:28:57 PDT
(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)
Comment 12
2009-07-10 23:29:28 PDT
Dave Levin has much more python-fu than I do, btw.
Mark Rowe (bdash)
Comment 13
2009-07-10 23:39:21 PDT
(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)
Comment 14
2009-07-10 23:45:03 PDT
(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)
Comment 15
2009-07-28 17:41:30 PDT
Created
attachment 33683
[details]
Patch v4
David Kilzer (:ddkilzer)
Comment 16
2009-07-28 17:44:50 PDT
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 17
2009-07-28 17:54:36 PDT
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 18
2009-07-28 19:22:15 PDT
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)
Comment 19
2009-07-28 21:47:48 PDT
Created
attachment 33691
[details]
Patch v5
David Kilzer (:ddkilzer)
Comment 20
2009-07-28 21:57:53 PDT
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)
Comment 21
2009-07-28 22:12:31 PDT
(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 22
2009-07-28 22:40:28 PDT
Comment on
attachment 33691
[details]
Patch v5
> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog > +2009-07-28 David Kilzer <
ddkilzer@apple.com
> > + > + <
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)
Comment 23
2009-07-29 04:35:19 PDT
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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug