[webkit-reviews] review granted: [Bug 26283] WebKit needs scripts to auto-commit patches from the commit queue : [Attachment 31696] Updated to respect the current working directory instead of using ../..

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 26283: WebKit needs scripts to auto-commit patches from the commit queue
https://bugs.webkit.org/show_bug.cgi?id=26283

Attachment 31696: Updated to respect the current working directory instead of
using ../..
https://bugs.webkit.org/attachment.cgi?id=31696&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Only a few trivial comments below.  Feel free to change and submit.



> diff --git a/WebKitTools/Scripts/bugzilla-tool
b/WebKitTools/Scripts/bugzilla-tool

> +def latest_changelog_entry(changelog_path):
...
> +	   changelog_date_line_regexp = re.compile('^(\d{4}-\d{2}-\d{2})' #
Consume the date.
> +					 + '\s+(.+)\s+' # Consume the name.
> +					 + '<([^<>]+)>$') # And finally the
email address.

You can remove this one since you already have it defined a few lines above
this.



> + def commit_message_for_this_commit(scm):
..
> +    # FIXME: We should sort and label the ChangeLog messages like
commit-log-editor does

Add "."



> + class PostDiffAsPatchToBug(Command):
...
> +	def execute(self, options, args, tool):
...
> +	    description = options.description if options.description else
"patch"

You could also do this if you find it more readable (I do):

  description = options.description || "patch"


> +	       log("Are you sure you want to attach %d patches to bug %s?" %
(len(commit_ids), bug_id))
> +	       # Should could --patches-limit option.

Fix comment "Should could"?



> +    def format_epilog(self, epilog):
> +	   if epilog:
> +	       return "\n" + epilog + "\n"
> +	   else:

Remove the else in this case.  (Following standard WebKit style).


> +    @staticmethod
> +    def split_args(args):
> +	   # Assume the first argument which doesn't start with '-' is the
command name.
> +	   command_index = 0
> +	   for arg in args:
> +	       if arg[0] != '-':
> +		   break
> +	       command_index += 1
> +
> +	   global_args = args[:command_index]
> +	   if command_index >= len(args):
> +	       return (global_args, None, [])
> +
> +	   command = args[command_index]
> +	   command_args = args[command_index + 1:]
> +	   return (global_args, command, command_args)

I really like the "else" statement in python for "while" and "for" statements.

Totally optional but consider

	command_index = 0
	for arg in args:
	    if arg[0] != '-':
		break
	    command_index += 1
	else:
	    return (args[:], None, [])

	global_args = args[:command_index]
	command = args[command_index]
	command_args = args[command_index + 1:]
	return (global_args, command, command_args)

btw, above I did the "args[:]" above just to get a new copy of the array (as
opposed to return a reference to args) to be consistent with the other code
path in the function.
 


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


> +# HACK: This should not depend on git for config storage

HACK or FIXME?


> +    reviewer_usernames_to_full_names = {

Would be nice to sort: ddkilzer and treat are out of place.


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

> +class SCM:
> +    # Subclasses must indicate if they support local commits
> +    # but the SCM baseclass will only call local_commits methods when this
is true

Add "."



>  I'm confused.  Are you suggesting that I should distribute the id=
differently
>  between the strings?

My mistake: I missed the "id=" when I was reading the code.  I was dealing with
a really small font at that moment.