Bug 33336 – webkit-patch rollout should be able to do multi-revision rollouts

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

webkit-patch rollout should be able to do multi-revision rollouts

It should be able to accept more than one revision number on the command line. It should sort them into whatever order it needs, and then apply the series of inverse diffs, and generate the ChangeLog.

multi-revision rollouts aren't very common, but they're common enough that making the tools be able to do them would help.

[Csaba Osztrogonác]

Gábor, have you got a little time to investigate on this new feature?

[Gabor Rapcsanyi]

Created attachment 74544 [details]
proposed patch

With this patch we can use the rollout commands with more than one revision. We have to pass multiple revisions between quotes (eg. "12203 12204 12205").
It sorts the revisions and starts the rollout with the latest. For the bug information it uses the earliest revision from the list.
If it gets just one revision then it works as before.

[Adam Barth]

Comment on attachment 74544 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74544&action=review

We should have Eric look at this patch.  He's on vacation at the moment, so that might take a week.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:534
> +    def apply_reverse_diff(self, revision_list):
> +        for revision in revision_list:
> +            # '-c -revision' applies the inverse diff of 'revision'
> +            svn_merge_args = ['svn', 'merge', '--non-interactive', '-c', '-%s' % revision, self._repository_url()]
> +            log("WARNING: svn merge has been known to take more than 10 minutes to complete.  It is recommended you use git for rollouts.")
> +            log("Running '%s'" % " ".join(svn_merge_args))
> +            # FIXME: Should this use cwd=self.checkout_root?
> +            self.run(svn_merge_args)

Have you tested this?  The tricky part here is ChangeLogs.  We might need to deal with the ChangeLog in each iteration of the loop.

> WebKitTools/Scripts/webkitpy/tool/commands/download.py:291
> -    argument_names = "REVISION REASON"
> +    argument_names = "REVISION(S) REASON"

This isn't usually how we document multiple arguments.  Check out some of the other commands that take variable arguments for how we do that.

[Gabor Rapcsanyi]

(In reply to comment #3)
> (From update of attachment 74544 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74544&action=review
> 
> We should have Eric look at this patch.  He's on vacation at the moment, so that might take a week.
> 

Ok, than wait him :)

> > WebKitTools/Scripts/webkitpy/common/checkout/scm.py:534
> > +    def apply_reverse_diff(self, revision_list):
> > +        for revision in revision_list:
> > +            # '-c -revision' applies the inverse diff of 'revision'
> > +            svn_merge_args = ['svn', 'merge', '--non-interactive', '-c', '-%s' % revision, self._repository_url()]
> > +            log("WARNING: svn merge has been known to take more than 10 minutes to complete.  It is recommended you use git for rollouts.")
> > +            log("Running '%s'" % " ".join(svn_merge_args))
> > +            # FIXME: Should this use cwd=self.checkout_root?
> > +            self.run(svn_merge_args)
> 
> Have you tested this?  The tricky part here is ChangeLogs.  We might need to deal with the ChangeLog in each iteration of the loop.
> 

Yes I have tested it. As I know we want one ChangeLog for the rolled-out patches and if the patches affect more ChangeLogs we want the same changes in all.

> > WebKitTools/Scripts/webkitpy/tool/commands/download.py:291
> > -    argument_names = "REVISION REASON"
> > +    argument_names = "REVISION(S) REASON"
> 
> This isn't usually how we document multiple arguments.  Check out some of the other commands that take variable arguments for how we do that.

Ok, I will correct this.

[Eric Seidel (no email)]

Comment on attachment 74544 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74544&action=review

I think this needs another round and more new unit tests please.

> WebKitTools/Scripts/webkitpy/common/checkout/changelog.py:155
> +        message = "Unreviewed, rolling out %s.\n" % ', '.join(['r' + revision for revision in revision_list])

grammar.py's join_with_separators might be nicer.

> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:528
> +        for revision in revision_list:

I guess I would have added a apply_reverse_diffs on SCM which did the for loop for you.  That shares more code.

>>> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:534
>>> +            self.run(svn_merge_args)
>> 
>> Have you tested this?  The tricky part here is ChangeLogs.  We might need to deal with the ChangeLog in each iteration of the loop.
> 
> Yes I have tested it. As I know we want one ChangeLog for the rolled-out patches and if the patches affect more ChangeLogs we want the same changes in all.

The tricky part is reverting the ChangeLog parts of the diff, but there is a separt part of the code which does that iirc.

> WebKitTools/Scripts/webkitpy/tool/commands/download.py:306
> +        revision_list = sorted([revision for revision in args[0].split(' ') if revision.isdigit()], reverse=True)

Huh?  What's with the isdidit() stuff?  Can you test this please.

> WebKitTools/Scripts/webkitpy/tool/commands/download.py:308
> +        earliest_revision = revision_list[-1]

Why reverse sorted?  I would have reversed them in the SCM.apply_reverse_diffs function maybe?

> WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py:184
> +        self.assert_execute_outputs(CreateRollout(), ["852", "Reason"], options=self._default_options(), expected_stderr=expected_stderr)

Why the quotes?

[Eric Seidel (no email)]

Overall I think this is a fine patch, I'm just not confident it works right without more testing.  This is tricky code.  Breaking this would be bad as the sherriff bot is used for rollouts about once a day these days.

[Gabor Rapcsanyi]

Created attachment 76028 [details]
proposed_patch_v2

I corrected the mentioned issues and I made some unit tests for it.

[Eric Seidel (no email)]

Comment on attachment 76028 [details]
proposed_patch_v2

View in context: https://bugs.webkit.org/attachment.cgi?id=76028&action=review

Have you run test-webkitpy?  Have you don a real rollout with this yet? (You can do that by having your changes in one webkit checkout and operating on a differnet one.  webkit-patch is smart enough to operate on the cwd while pulling its .py files from paths relative to it's __file__ location.

> WebKitTools/Scripts/webkitpy/common/checkout/changelog.py:157
> +        for revision in revision_list:

We have to be careful when passing lists as strings count as lists.  If there is any chance someone might pass a string for the revision_list it will do strange things.

> WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py:65
> +        state = command._prepare_state(None, ["rev r122  125  123", "Reason"], None)

What's the 'rev' here?

> WebKitTools/Scripts/webkitpy/tool/commands/download.py:309
> +        # We use the earliest revision for the bug info
> +        earliest_revision = revision_list[0]
> +        commit_info = self._commit_info(earliest_revision)

I'm not sure we don't want to process each bug individually, but this might be ok for now.

[Eric Seidel (no email)]

How does this affect the prepare-rollout and rollout commands?  Could you link to an example bug created with prepare-rollout after this change?

[Gabor Rapcsanyi]

(In reply to comment #8)
> (From update of attachment 76028 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76028&action=review
> 
> Have you run test-webkitpy?  Have you don a real rollout with this yet? (You can do that by having your changes in one webkit checkout and operating on a differnet one.  webkit-patch is smart enough to operate on the cwd while pulling its .py files from paths relative to it's __file__ location.
> 

Yes I tested it and all unit tests passed and also I created some rollout patches locally and it worked.

> > WebKitTools/Scripts/webkitpy/common/checkout/changelog.py:157
> > +        for revision in revision_list:
> 
> We have to be careful when passing lists as strings count as lists.  If there is any chance someone might pass a string for the revision_list it will do strange things.
> 

The update_for_revert method will always get a revision_list from PrepareChangeLogForRevert so it can't happen.

> > WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py:65
> > +        state = command._prepare_state(None, ["rev r122  125  123", "Reason"], None)
> 
> What's the 'rev' here?

Here I just tested whether somebody tries to give wrong (not numerical) revision numbers. But the 'r122' is maybe enough to test this and we can drop the 'rev' string.

> 
> > WebKitTools/Scripts/webkitpy/tool/commands/download.py:309
> > +        # We use the earliest revision for the bug info
> > +        earliest_revision = revision_list[0]
> > +        commit_info = self._commit_info(earliest_revision)
> 
> I'm not sure we don't want to process each bug individually, but this might be ok for now.

The base conception was that all of the patches belong to one bug. If the patches belong to more bugs then we should make more than one rollout patch.

[Gabor Rapcsanyi]

(In reply to comment #9)
> How does this affect the prepare-rollout and rollout commands?  Could you link to an example bug created with prepare-rollout after this change?

I just tried it locally. Here is an example:

Commad:
WebKitTools/Scripts/webkit-patch prepare-rollout "73705 73703 73712" "Some reason ..." --dry-run --force-clean

Output:
Preparing rollout for bug 49658.
Cleaning working directory
Updating working directory
Running prepare-ChangeLog
  Running status to find changed, added, or removed files.
  Reviewing diff to determine which lines changed.
  Extracting affected function names from source files.
  Change author: Gabor Rapcsanyi <[e-mail]>.
  Editing the JavaScriptCore/ChangeLog file.
  Editing the WebCore/ChangeLog file.
  Editing the WebKitTools/ChangeLog file.
-- Please remember to include a detailed description in your ChangeLog entry. --
-- See <http://webkit.org/coding/contributing.html> for more info --


Created rollout patch:
https://gist.github.com/736251

[Eric Seidel (no email)]

Comment on attachment 76028 [details]
proposed_patch_v2

View in context: https://bugs.webkit.org/attachment.cgi?id=76028&action=review

>>> WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py:65
>>> +        state = command._prepare_state(None, ["rev r122  125  123", "Reason"], None)
>> 
>> What's the 'rev' here?
> 
> Here I just tested whether somebody tries to give wrong (not numerical) revision numbers. But the 'r122' is maybe enough to test this and we can drop the 'rev' string.

Who is responsible for erroring out when 'rev' is passed?  It seems that if we're *silently* ignoring bad args like 'r122' that will make it easy to make mistakes.

Seems we should error out in that case, no?

[Gabor Rapcsanyi]

(In reply to comment #12)
> (From update of attachment 76028 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76028&action=review
> 
> >>> WebKitTools/Scripts/webkitpy/tool/commands/download_unittest.py:65
> >>> +        state = command._prepare_state(None, ["rev r122  125  123", "Reason"], None)
> >> 
> >> What's the 'rev' here?
> > 
> > Here I just tested whether somebody tries to give wrong (not numerical) revision numbers. But the 'r122' is maybe enough to test this and we can drop the 'rev' string.
> 
> Who is responsible for erroring out when 'rev' is passed?  It seems that if we're *silently* ignoring bad args like 'r122' that will make it easy to make mistakes.
> 
> Seems we should error out in that case, no?

Yes maybe we should so I will correct this and change the unit test expectation.

[Gabor Rapcsanyi]

Created attachment 76415 [details]
proposed_patch_v3

[Eric Seidel (no email)]

Comment on attachment 76415 [details]
proposed_patch_v3

LGTM.

[WebKit Review Bot]

Comment on attachment 76415 [details]
proposed_patch_v3

Clearing flags on attachment: 76415

Committed r73951: <http://trac.webkit.org/changeset/73951>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.