Bug 130676

Summary: Fix commit-log-editor bug revealed by r165447
Product: WebKit Reporter: Berta József <jberta.u-szeged>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, ossy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ossy: review-, ossy: commit-queue-
Patch none

Description Berta József 2014-03-24 09:44:53 PDT
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)
Comment 1 Berta József 2014-03-24 09:48:01 PDT
Created attachment 227661 [details]
Patch
Comment 2 Csaba Osztrogonác 2014-03-25 03:39:18 PDT
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?
Comment 3 Berta József 2014-04-14 04:30:06 PDT
Created attachment 229279 [details]
Patch
Comment 4 Csaba Osztrogonác 2014-04-14 04:35:21 PDT
Comment on attachment 229279 [details]
Patch

The comments are better and clear now, r=me.
Comment 5 WebKit Commit Bot 2014-04-14 05:24:50 PDT
Comment on attachment 229279 [details]
Patch

Clearing flags on attachment: 229279

Committed r167243: <http://trac.webkit.org/changeset/167243>
Comment 6 WebKit Commit Bot 2014-04-14 05:24:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Alexey Proskuryakov 2014-04-14 22:42:12 PDT
This patch broke webkitpy tests. Please run webkitpy tests when making changes, and please add tests for your fixes.

I'll land updated results.
Comment 8 Alexey Proskuryakov 2014-04-14 22:48:29 PDT
Update the result in <http://trac.webkit.org/r167296>.

I'm not sure if the new result is better, please take a look.
Comment 9 Csaba Osztrogonác 2014-04-16 03:14:32 PDT
(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