Bug 26715

Once we have a way to do automated rollbacks, then we could eventually consider adding a web interface to do so. :) Never have a red tree again! :)

Created attachment 32173 [details] Work in progress

Created attachment 32174 [details] Working, but not ready for primetime

Created attachment 32974 [details] Updated patch for rollout support --- 4 files changed, 189 insertions(+), 51 deletions(-)

Created attachment 32976 [details] Begone ye tabs! --- 4 files changed, 189 insertions(+), 51 deletions(-)

Comment on attachment 32976 [details] Begone ye tabs! Just a few things to address. I'm most concerned about the issue I mentioned in update_changelog_for_revert. Two general notes: 1. It was pointed out to me that python style is to have two blank lines between top level functions: "Separate top-level function and class definitions with two blank lines." 2. Many comments don't end with periods. Please add them. > diff --git a/WebKitTools/Scripts/bugzilla-tool b/WebKitTools/Scripts/bugzilla-tool > +def update_changelog_for_revert(changelog_path, revision): > + removing_boilerplate = False > + # inplace=1 creates a backup file and re-directs stdout to the file > + for line in fileinput.FileInput(changelog_path, inplace=1): > + if re.search('Reviewed by', line): I'm confused. How does this stop after this boilerplate change log that was just created? It looks like it goes through the whole changelog removing all descriptions along the way. > + print line.replace("Reviewed by NOBODY (OOPS!)", "No review, rolling out %s" % revision), I like to prefix revisions with an r. Consider: ..."No review, rolling out r%s"... > +class RolloutCommit(Command): ... > + def create_changelogs_for_revert(scm, revision): ... > + # This could move to prepare-ChangeLog by adding a --revert= option That might be easier. I think the current code has a bug as I noted above. > diff --git a/WebKitTools/Scripts/modules/scm.py b/WebKitTools/Scripts/modules/scm.py > + @classmethod > + def git_commit_from_svn_revision(cls, revision): > + git_log_output = cls.run_command(['git', 'svn', 'log', '-r', revision, '--oneline', '--show-commit']) > + # Example: r45396 | b4954e7 | 2009-06-30 Antonio Gomes <antonio.gomes@openbossa.org> > + matched_commit = re.search("^r(\d+) \| (\w+) \| .*$", git_log_output, re.MULTILINE) This is ok, but I always find code more readable when it uses named groups.

btw, ignore this comment: > + # This could move to prepare-ChangeLog by adding a --revert= option That might be easier. I think the current code has a bug as I noted above. as I realized I was completely wrong about it being easier and I rather like it in these files :)

+ git_log_output = cls.run_command(['git', 'svn', 'log', '-r', revision, '--oneline', '--show-commit']) + # Example: r45396 | b4954e7 | 2009-06-30 Antonio Gomes <antonio.gomes@openbossa.org> + matched_commit = re.search("^r(\d+) \| (\w+) \| .*$", git_log_output, re.MULTILINE) git svn find-rev r45396

Created attachment 33930 [details] Updated, just needs testing with more rollouts --- 4 files changed, 186 insertions(+), 53 deletions(-)

Some testing code for later: def _parse_revision_to_commit_mappings(self, stderr): revision_to_commit_mapping = {} for line in stderr.split_lines(): match = line.match("^(?P<revision>r\d+) = (?P<commit_id>\w+)$") if match: revision_to_commit_mapping[match.group('revision')] = match.group('commit_id') return revision_to_commit_mapping

Created attachment 34639 [details] Updated to ToT --- 4 files changed, 123 insertions(+), 3 deletions(-)

Created attachment 35147 [details] Update rollout support --- 7 files changed, 373 insertions(+), 49 deletions(-)

Created attachment 39377 [details] Patch v1

Created attachment 39379 [details] Works well enough for others to start to use

Created attachment 39381 [details] Make ChangeLog less pessimistic

Comment on attachment 39381 [details] Make ChangeLog less pessimistic Would be nice to have some unittests for this change... We also discussed some alternate approaches if the ChangeLog conflicts are too problematic.

Created attachment 39397 [details] Add a bunch of testing code for abarth

Created attachment 39398 [details] Final patch with a bunch of extra testing

Note that I uploaded a diff from the reviewed patch which just contains the new testing changes as "Add a bunch of testing code for abarth". The final patch (including all the added tests) is up for review again.

Created attachment 39400 [details] Document all the extra testing in the ChangeLog

Comment on attachment 39400 [details] Document all the extra testing in the ChangeLog Thanks for the tests.

Comment on attachment 39400 [details] Document all the extra testing in the ChangeLog Rejecting patch 39400 from commit-queue. This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

Comment on attachment 39400 [details] Document all the extra testing in the ChangeLog Bitten by bug 28845. :( media/video-source-error.html -> timed out

Comment on attachment 39400 [details] Document all the extra testing in the ChangeLog Clearing flags on attachment: 39400 Committed r48303: <http://trac.webkit.org/changeset/48303>

All reviewed patches have been landed. Closing bug.

Committed r48304: <http://trac.webkit.org/changeset/48304>