[webkit-reviews] review denied: [Bug 26703] bugzilla-tool apply-patches needs --local-commit and land-patches should support multiple bugs : [Attachment 31826] Let land-patches take multiple bug ids

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Eric Seidel <eric at webkit.org>'s
request for review:
Bug 26703: bugzilla-tool apply-patches needs --local-commit and land-patches
should support multiple bugs
https://bugs.webkit.org/show_bug.cgi?id=26703

Attachment 31826: Let land-patches take multiple bug ids
https://bugs.webkit.org/attachment.cgi?id=31826&action=review

------- Additional Comments from David Levin <levin at chromium.org>
A few things (most importantly the license) to address so r- for now.

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

> +# plural() is taken from
http://python.developpez.com/cours/DiveIntoPython/php/endiveintopython/dynamic_
functions/stage1.php

Unfortunately, this has a "python license" which is meant to be permissable (I
think) but doesn't meet our requirements and I'm fairly certain that
http://diveintopython.org/dynamic_functions/stage1.html is the official source.



> +    @staticmethod
> +    def apply_patches(patches, scm, commit_each):
> +	   for patch in patches:
> +	       scm.apply_patch(patch)
> +	       if commit_each:
> +		   scm.commit_locally_with_message(patch['name'])
> +

This looks like it should be a scm method.  I also wish "name" was "message"
since that how it seems to be used.



> +    @classmethod
> +    def build_and_commit(cls, scm, options):
> +	   if options.build:
> +	       cls.build_webkit()
> +	       if options.test:
> +		   cls.run_webkit_tests()

Is it possible to set test w/o build?  Should this error out if that is done?


> +    def execute(self, options, args, tool):
> +	   bugs_and_patches = []

This feels more like a dictionary to me.

  bugs_to_patches = {}

> +	   for bug_id in args:
> +	       patches = tool.bugs.fetch_reviewed_patches_from_bug(bug_id)
...
> +	       bugs_and_patches.append([bug_id, patches])
  bugs_to_patches[bug_id] = patches


> +	   for bug_and_patches in bugs_and_patches:

for bug_id in bugs_to_patches:

> +	       patches = bug_and_patches[1]

patches = bugs_to_patches[bug_id]