[webkit-reviews] review denied: [Bug 27119] bugzilla-tool: Add create-bug command : [Attachment 32609] Patch v3

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied David Kilzer (ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 27119: bugzilla-tool: Add create-bug command
https://bugs.webkit.org/show_bug.cgi?id=27119

Attachment 32609: Patch v3
https://bugs.webkit.org/attachment.cgi?id=32609&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
 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.