Bug 26703 – bugzilla-tool apply-patches needs --local-commit and land-patches should support multiple bugs

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

[Eric Seidel (no email)]

bugzilla-tool apply-patches needs --local-commit and land-patches should support multiple bugs

Dave Levin asked about this feature when he first tried bugzilla-tool.  Basically it makes apply-patches into a "git am" equivalent, since now it can turn patches directly into local commits.

bugzilla-tool land-patches should also take multiple bug ids.  Makes it possible to land multiple bugs at once with a single command.  This is very useful for our current commit queue, since we can't just land the whole queue until we straighten out what r+ means.  But we can pre-screen a few bugs and then send the script off to build, test and land the pre-screened bugs.

[Eric Seidel (no email)]

Created attachment 31826 [details]
Let land-patches take multiple bug ids

---
 2 files changed, 95 insertions(+), 36 deletions(-)

[Eric Seidel (no email)]

Created attachment 31827 [details]
Support local commits during apply-patches

---
 4 files changed, 23 insertions(+), 4 deletions(-)

[David Levin]

Comment on attachment 31826 [details]
Let land-patches take multiple bug ids

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]

[David Levin]

Comment on attachment 31827 [details]
Support local commits during apply-patches

My real concern is the commit message so r- for that.


> diff --git a/WebKitTools/Scripts/bugzilla-tool b/WebKitTools/Scripts/bugzilla-tool
>          for patch in patches:
>              tool.scm().apply_patch(patch)
> +            if options.local_commit:
> +                tool.scm().commit_locally_with_message(patch['name'])

As discussed, the message given here doesn't seem like the right thing.


> diff --git a/WebKitTools/Scripts/modules/scm.py b/WebKitTools/Scripts/modules/scm.py
> index e226962..7c9593f 100644
> --- a/WebKitTools/Scripts/modules/scm.py
> +++ b/WebKitTools/Scripts/modules/scm.py
> @@ -65,14 +65,14 @@ class SCM:
>          output = process.communicate(input)[0].rstrip()
>          exit_code = process.wait()
>          if raise_on_failure and exit_code:
> -            raise ScriptError("Failed to run " + command)
> +            raise ScriptError("Failed to run \"%s\"  exit_code: %d  cwd: %s" % (command, exit_code, cwd))

Consider:
            raise ScriptError('Failed to run "%s"  exit_code: %d  cwd: %s' % (command, exit_code, cwd))

[Eric Seidel (no email)]

(In reply to comment #3)
> (From update of attachment 31826 [details] [review])
> 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.

I emailed the author about using it under BSD license.  I've written a dumber implementation myself for now.

> > +    @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.

Perhaps.  Right now I'm going to leave it on that class.

> > +    @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?

No, it's not possible.  It's also not possible to specifically enable testing.  Testing defaults to on, you can disable it by either --no-build or --no-test.  If someone only passes --no-build and not --no-test it doesn't make sense to error.  Maybe some day we'll support explicit --no-build and --test, and then we could have an error.

> > +    def execute(self, options, args, tool):
> > +        bugs_and_patches = []
> 
> This feels more like a dictionary to me.

Fixed.

[Eric Seidel (no email)]

Created attachment 31832 [details]
Patch updated and combined after David's review

---
 4 files changed, 103 insertions(+), 38 deletions(-)

[Eric Seidel (no email)]

(In reply to comment #4)
> (From update of attachment 31827 [details] [review])
> > +                tool.scm().commit_locally_with_message(patch['name'])
> 
> As discussed, the message given here doesn't seem like the right thing.

commit_message = commit_message_for_this_commit(scm)
scm.commit_locally_with_message(commit_message or patch['name'])

Which should "do the right thing" for both patches which have ChangeLogs and those which don't.

> Consider:
>             raise ScriptError('Failed to run "%s"  exit_code: %d  cwd: %s' %
> (command, exit_code, cwd))

Considered.  And changed! ;)

[David Levin]

fwiw,
  "if not count or count > 1:"

Could also be written as
  "if count != 1:"

Whatever you like better.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/bugzilla-tool
	M	WebKitTools/Scripts/modules/bugzilla.py
	M	WebKitTools/Scripts/modules/scm.py
Committed r45160