[webkit-reviews] review denied: [Bug 30683] svn-apply's fixChangeLogPatch function seems broken : [Attachment 41746] Updated ChangeLog

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Eric Seidel
<eric at webkit.org>'s request for review:
Bug 30683: svn-apply's fixChangeLogPatch function seems broken
https://bugs.webkit.org/show_bug.cgi?id=30683

Attachment 41746: Updated ChangeLog
https://bugs.webkit.org/attachment.cgi?id=41746&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
I'm on vacation through Oct. 28, so I can't really give a detailed review until
then.

However, I see issues with this patch that require an r-:

- ChangeLog doesn't mention all the scripts affected.
- The new method in VCSUtils.pm has a debugging print() statement in it.
- The method Name added to the list of exported methods should be alphabetized.

- The method in VCSUtils.pm completely breaks the contract of the original
method--that the entire patch should be returned unaltered if the method can't
fix it.
- The method in VCSUtils.pm makes too many assumptions about the format of the
patch, e.g., that the patch's first line of code is the fourth line of the text
passed in.  While rare, there are some patches that don't include an "Index" or
"diff" preamble line.

I think the best approach to fix this is to move the unaltered method into
VCSUtils.pm first, then write a simple test harness in Perl that uses the
VCSUtils module to read a patch from stdin and write the altered result to
stdout.  Then this can be used to write test cases for different types of
ChangeLog patches, then the method can be fixed for the case that this bug was
filed for.