[webkit-reviews] review denied: [Bug 122654] webkitbot rollout ChangeLogs should be nicer : [Attachment 226314] Patch

[bugzilla-daemon at webkit.org]

Csaba Osztrogonác <ossy at webkit.org> has denied Éva Balázsfalvi
<evab.u-szeged at partner.samsung.com>'s request for review:
Bug 122654: webkitbot rollout ChangeLogs should be nicer
https://bugs.webkit.org/show_bug.cgi?id=122654

Attachment 226314: Patch
https://bugs.webkit.org/attachment.cgi?id=226314&action=review

------- Additional Comments from Csaba Osztrogonác <ossy at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=226314&action=review


Great work, I really like this new rollout changelog format.

The patch looks good to me in general, but I have some minor comments.

> Tools/ChangeLog:8
> +	   Added bug urls and descriptions of rolled out patches to the rollout
changelog if they are present in the original changelog. Additionally removed
the list of changed files and functions.

This line is too long, wrap it please.

> Tools/ChangeLog:11
> +	   (ChangeLog.update_with_unreviewed_message): Cut off the list of
modified files

It is a sentence, please close it with "." (not only here)

> Tools/Scripts/webkitpy/tool/steps/preparechangelogforrevert.py:48
> +	       if index < len(description_list) and index < len(bug_url_list):

Do we really need this check? 

I think the length of revision_list, description_list and bug_url_list must be
same always.
If they are same, we don't need this check. If they aren't same, it is a bug
which should be fixed.

> Tools/Scripts/webkitpy/tool/steps/preparechangelogforrevert.py:64
>	   bug_url = self._tool.bugs.bug_url_for_bug_id(state["bug_id"]) if
state["bug_id"] else None
> -	   message = self._message_for_revert(state["revision_list"],
state["reason"], bug_url)
> +	   for bug_id in state["bug_id_list"]:
> +	       bug_url_list.append(self._tool.bugs.bug_url_for_bug_id(bug_id))
> +	   message = self._message_for_revert(state["revision_list"],
state["reason"], state["description_list"], bug_url_list)

You missed to pass bug_url to _message_for_revert. Additionally
these names are a little bit confusing: bug_url and bug_url_list

I managed to understand that bug_url is the URL of the rollout bug (if there
is)
and bug_url_list contains the urls for rolled out patches. But it would be
great
to have better names, for example rollout_bug_url instead of bug_url and
reverted_bug_url_list instead of bug_url_list.