[webkit-reviews] review denied: [Bug 130676] Fix commit-log-editor bug revealed by r165447 : [Attachment 227661] Patch

[bugzilla-daemon at webkit.org]

Csaba Osztrogonác <ossy at webkit.org> has denied Berta József
<jberta.u-szeged at partner.samsung.com>'s request for review:
Bug 130676: Fix commit-log-editor bug revealed by r165447
https://bugs.webkit.org/show_bug.cgi?id=130676

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

------- Additional Comments from Csaba Osztrogonác <ossy at webkit.org>
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?