Make prepare-ChangeLog -g <commit> generate a more standard ChangeLog entry. Before this change, |prepare-ChangeLog -g HEAD| generated something like: <git commit title> <git commit long description> Need a short description (OOPS!) Need the bug URL (OOPS!) Reviewed by NOBODY (OOPS!). No new tests (OOPS!). This change converts that in: <git commit title> Need the bug URL (OOPS!) Reviewed by NOBODY (OOPS!). <git commit long description> No new tests (OOPS!). This bit me the first time I tried to submit a patch to WebKit, and still I have to manually edit the ChangeLog every time I use the script. This generates a more convenient and standard ChangeLog entry.
Created attachment 314708 [details] Patch
Created attachment 314709 [details] Patch
Comment on attachment 314709 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314709&action=review > Tools/Scripts/prepare-ChangeLog:684 > + if (!$bugDescription && $description) { > + my @lines = split(/\n/, $description); > + $bugDescription = shift @lines; > + $bugDescription =~ s/^\s*//g; > + if (scalar @lines > 0 and $lines[0] =~ /^\s*$/) { > + shift @lines; > + } > + $description = join("\n", @lines) > + } Hmmm. On the one hand, this seems like a great change. On the other hand, this code looks like gobbledygook. First, congratulate yourself on having learned enough Perl to do this. Next, try to find a reviewer who understands Perl too. I'm just not comfortable giving r+ when I have no clue how to read the code.
Comment on attachment 314709 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314709&action=review Seems fine. Since non-perl programmers can’t read the code to strip off the first line anyway, I would consider doing it with a single regular expression like this: ($bugDescription, $description) = ($description =~ /^(?:\s*(.*)\n)?(?:\s*\n)*(.*)/) if !$bugDescription && $description; Could write that all on one line, too, if you prefer. Note that my version strips multiple blank lines, not just one. Could use a "?" instead of a "*" to strip only one. > Tools/Scripts/prepare-ChangeLog:677 > + my @lines = split(/\n/, $description); I think it’s more efficient to split on "\n" rather than /\n/ but not sure. Of course, if you use my alternative version you won’t need that. > Tools/Scripts/prepare-ChangeLog:679 > + $bugDescription =~ s/^\s*//g; No need for the "g" here; this only applies once, not multiple times. Also, I would have used + rather than * myself.
(In reply to Darin Adler from comment #4) > Comment on attachment 314709 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=314709&action=review > > Seems fine. > > Since non-perl programmers can’t read the code to strip off the first line > anyway, I would consider doing it with a single regular expression like this: > > ($bugDescription, $description) > = ($description =~ /^(?:\s*(.*)\n)?(?:\s*\n)*(.*)/) > if !$bugDescription && $description; > > Could write that all on one line, too, if you prefer. Note that my version > strips multiple blank lines, not just one. Could use a "?" instead of a "*" > to strip only one. Multiple sounds fine (though I don't think it's common). I had it to tweak it slightly so it grabbed the multiline description entirely and not only the first line of it, but your suggestion is much more compact :) Thanks for the review!
Created attachment 315219 [details] Patch
Comment on attachment 315219 [details] Patch Attachment 315219 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4106398 New failing tests: imported/w3c/IndexedDB-private-browsing/idbfactory_open.html
Created attachment 315230 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 315219 [details] Patch Clearing flags on attachment: 315219 Committed r219406: <http://trac.webkit.org/changeset/219406>
All reviewed patches have been landed. Closing bug.