[webkit-reviews] review denied: [Bug 33336] webkit-patch rollout should be able to do multi-revision rollouts : [Attachment 74544] proposed patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Gabor Rapcsanyi
<rgabor at inf.u-szeged.hu>'s request for review:
Bug 33336: webkit-patch rollout should be able to do multi-revision rollouts
https://bugs.webkit.org/show_bug.cgi?id=33336

Attachment 74544: proposed patch
https://bugs.webkit.org/attachment.cgi?id=74544&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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?