[Webkit-unassigned] [Bug 33336] webkit-patch rollout should be able to do multi-revision rollouts

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33336


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #74544|review?                     |review-
               Flag|                            |




--- Comment #5 from Eric Seidel <eric at webkit.org>  2010-11-30 23:44:41 PST ---
(From update of attachment 74544)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.