Bug 130676 – Fix commit-log-editor bug revealed by r165447

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Berta József]

After https://trac.webkit.org/changeset/165447 commit-log-editor generates wrong commit log for rollout changes when more then one changelog is affected, for example:
https://trac.webkit.org/changeset/165497 (landed manually)
https://trac.webkit.org/changeset/165581 (landed manually)
https://trac.webkit.org/changeset/165829 (landed by CQ)
https://trac.webkit.org/changeset/165933 (landed by CQ)

[Berta József]

Created attachment 227661 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 227661 [details]
Patch

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

It seems the fix works after testing on some changes, but 
r- for now because of inaccurate and misleading comments.

> Tools/ChangeLog:11
> +        (removeLongestCommonPrefixEndingInDoubleNewline): Change the longest prefix ending to \n,
> +        because after r165447 the block ending depends on who landed the patch.

r165447 didn't change the block ending. Does the block ending depends on 
who landed the patch? Checking the referred commit logs, it isn't true.

Otherwise the fix seems good on the referred commit logs,
but this can't be the proper comment why it works.

removeLongestCommonPrefixEndingInDoubleNewline
--> It should be renamed too, because it doesn't check double newline anymore.

> Tools/Scripts/commit-log-editor:307
> -            push @result, normalizeLineEndings("\n", $endl) if !$first;
> -            $first = 0;
> +            push @result, normalizeLineEndings("\n", $endl);

Could you explain why we don't need this extra new line for non-first blocks anymore?

[Berta József]

Created attachment 229279 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 229279 [details]
Patch

The comments are better and clear now, r=me.

[WebKit Commit Bot]

Comment on attachment 229279 [details]
Patch

Clearing flags on attachment: 229279

Committed r167243: <http://trac.webkit.org/changeset/167243>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This patch broke webkitpy tests. Please run webkitpy tests when making changes, and please add tests for your fixes.

I'll land updated results.

[Alexey Proskuryakov]

Update the result in <http://trac.webkit.org/r167296>.

I'm not sure if the new result is better, please take a look.

[Csaba Osztrogonác]

(In reply to comment #7)
> This patch broke webkitpy tests. Please run webkitpy tests when making changes, and please add tests for your fixes.
> 
> I'll land updated results.

Sorry, I haven't thought if a perl script can break a webkitpy unit test. :)

I filed a new bug report to fix this minor bug and to add more unit tests:
https://bugs.webkit.org/show_bug.cgi?id=131727