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.
Created attachment 31826 [details] Let land-patches take multiple bug ids --- 2 files changed, 95 insertions(+), 36 deletions(-)
Created attachment 31827 [details] Support local commits during apply-patches --- 4 files changed, 23 insertions(+), 4 deletions(-)
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]
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))
(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.
Created attachment 31832 [details] Patch updated and combined after David's review --- 4 files changed, 103 insertions(+), 38 deletions(-)
(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! ;)
fwiw, "if not count or count > 1:" Could also be written as "if count != 1:" Whatever you like better.
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