Bug 122654 – webkitbot rollout ChangeLogs should be nicer

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Alexey Proskuryakov]

webkitbot rollout ChangeLogs should be nicer:

1. There should be a title of rolled out patch, so that one could easily tell what was rolled out.

2. List of changed files and functions should be omitted - there are never per-function comments in an automatically prepared rollout, and searching ChangeLogs for function names doesn't need to show rollouts.

[Alexey Proskuryakov]

<rdar://problem/15208418>

[Éva Balázsfalvi]

Created attachment 226314 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 226314 [details]
Patch

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.

[Éva Balázsfalvi]

Created attachment 226327 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 226327 [details]
Patch

r=me

[Csaba Osztrogonác]

Comment on attachment 226327 [details]
Patch

Clearing flags on attachment: 226327

Committed r165447: <http://trac.webkit.org/changeset/165447>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #6)
> (From update of attachment 226327 [details])
> Clearing flags on attachment: 226327
> 
> Committed r165447: <http://trac.webkit.org/changeset/165447>

Please send a heads-up mail to webkit-dev too.