* SUMMARY I use `prepare-ChangeLog -b ###` for all of my bugs. That adds a ChangeLog title like: prepare-ChangeLog should include radar number https://bugs.webkit.org/show_bug.cgi?id=152839 However a lot of bugzilla bugs also have <rdar://problem/####> links in the Bugzilla comments (e.g. webkit-bug-importer@group.apple.com). It would be great if `prepare-ChangeLog -b ####` detected this and created a title like: prepare-ChangeLog should include radar number https://bugs.webkit.org/show_bug.cgi?id=152839 <rdar://problem/########> * NOTES - A lamer but probably acceptable way to do this would be add a `prepare-ChangeLog -b ### -r ###` which at least removes the inconvenience of manually modifying every ChangeLog, but I still have to have found and copied the radar number myself.
<rdar://problem/24093563>
If prepare-changelog and its downstream webkitperl used the bugzilla JSON API, this would be pretty easy to do by looking for <rdar in a comment. I don't think it will be easy right now, since it's not listed in a well-defined field for scraping. I don't know if any other functionality needs to scrape comments HTML in bugzilla.
(In reply to comment #2) > If prepare-changelog and its downstream webkitperl used the bugzilla JSON > API, this would be pretty easy to do by looking for <rdar in a comment. I > don't think it will be easy right now, since it's not listed in a > well-defined field for scraping. I don't know if any other functionality > needs to scrape comments HTML in bugzilla. Currently `prepare-ChangeLog -b ###` will fetch the XML data for a bug: $ curl --insecure --silent 'https://bugs.webkit.org/show_bug.cgi?id=152839&ctype=xml&excludefield=attachmentdata' Which includes comments. Importer comments would be easy to recognize: > <long_desc isprivate="0"> > <commentid>1153467</commentid> > <who name="Radar WebKit Bug Importer">webkit-bug-importer</who> > <bug_when>2016-01-07 11:06:14 -0800</bug_when> > <thetext><rdar://problem/24093563>;</thetext> > </long_desc> I suppose I could hack in just searching for that single radar link comment. That will at least handle the importer pattern (which is all I really about).
Yep, I have this working, I'll clean it up.
Created attachment 268480 [details] [PATCH] Proposed Fix Test cases: > # Bad Bug ID > $ prepare-ChangeLog pco --git-index -b 99999999 > Running status to find changed, added, or removed files. > Reviewing diff to determine which lines changed. > Extracting affected function names from source files. > Change author: Joseph Pecoraro <pecoraro@apple.com>. > Bug 99999999 has no bug description. Maybe you set wrong bug ID? > The bug URL: https://bugs.webkit.org/show_bug.cgi?id=99999999 > > # Bug with Radar URL Comment > $ prepare-ChangeLog --git-index -b 152839 > Running status to find changed, added, or removed files. > Reviewing diff to determine which lines changed. > Extracting affected function names from source files. > Change author: Joseph Pecoraro <pecoraro@apple.com>. > Description from bug 152839: > "prepare-ChangeLog should include radar number". > Radar URL from bug 152839: > "<rdar://problem/24093563>". > Editing the ../Tools/ChangeLog file. > -- Please remember to include a detailed description in your ChangeLog entry. -- > -- See <http://webkit.org/coding/contributing.html> for more info -- > Opening the edited ChangeLog files. > > # Bug without Radar URL Comment > $ prepare-ChangeLog --git-index -b 152838 > Running status to find changed, added, or removed files. > Reviewing diff to determine which lines changed. > Extracting affected function names from source files. > Change author: Joseph Pecoraro <pecoraro@apple.com>. > Description from bug 152838: > "[mips] GPRInfo::toArgumentRegister missing". > Editing the ../Tools/ChangeLog file. > -- Please remember to include a detailed description in your ChangeLog entry. -- > -- See <http://webkit.org/coding/contributing.html> for more info -- > Opening the edited ChangeLog files.
Holy cow, Joe! This is fantastic!
Comment on attachment 268480 [details] [PATCH] Proposed Fix Very nice! r=me.
Comment on attachment 268480 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=268480&action=review > Tools/Scripts/prepare-ChangeLog:467 > + return "" if $bugXMLData !~ /<thetext>\s*(<rdar:\/\/problem\/\d+>)/; I would prefer to leave the <> out of the log. The JSC ChangeLog entries that have rdar links follow that format, and it looks nicer next to the http links.
Comment on attachment 268480 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=268480&action=review > Tools/Scripts/prepare-ChangeLog:463 > +sub fetchRadarURLFromBugXMLData($$) We have infrastructure to support writing a unit test for this function. You can use <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/extractLineRangeBeforeAndAfterChange.pl> as an example. > Tools/Scripts/prepare-ChangeLog:467 > + return "" if $bugXMLData !~ /<thetext>\s*(<rdar:\/\/problem\/\d+>)/; This is OK as-is. For your consideration, I suggest we chose another regular expression delimiter, say '|'. Then we can avoid escaping '/' characters: return "" if $bugXMLData !~ m|<thetext>\s*(<rdar://problem/\d+>)|; The benefit of using a different regular expression delimiter is that it makes the regular expression more straightforward to read.
(In reply to comment #9) > Comment on attachment 268480 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268480&action=review > > > Tools/Scripts/prepare-ChangeLog:463 > > +sub fetchRadarURLFromBugXMLData($$) > > We have infrastructure to support writing a unit test for this function. You > can use > <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/prepare- > ChangeLog_unittest/extractLineRangeBeforeAndAfterChange.pl> as an example. Look into this now. > > Tools/Scripts/prepare-ChangeLog:467 > > + return "" if $bugXMLData !~ /<thetext>\s*(<rdar:\/\/problem\/\d+>)/; > > This is OK as-is. For your consideration, I suggest we chose another regular > expression delimiter, say '|'. Then we can avoid escaping '/' characters: > > return "" if $bugXMLData !~ m|<thetext>\s*(<rdar://problem/\d+>)|; > > The benefit of using a different regular expression delimiter is that it > makes the regular expression more straightforward to read. I agree!
Created attachment 268490 [details] [PATCH] Proposed Fix With tests: > shell> ./Tools/Scripts/test-webkitperl > ... > Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/fetchRadarURLFromBugXMLData.pl ................. Variable "%attributeCache" is not available at (eval 12) line 1999. > Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/fetchRadarURLFromBugXMLData.pl ................. ok > ... > All tests successful. > Files=35, Tests=571, 3 wallclock secs ( 0.14 usr 0.08 sys + 2.32 cusr 0.42 csys = 2.96 CPU) > Result: PASS
Comment on attachment 268490 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=268490&action=review > Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/fetchRadarURLFromBugXMLData.pl:42 > +# But for these tests I will simplify the XML to just the <thetext> blocks. Hmm, I'll change "I" to "we".
Created attachment 268493 [details] [PATCH] Proposed Fix
Comment on attachment 268493 [details] [PATCH] Proposed Fix Thank you Joe for adding tests!
Comment on attachment 268493 [details] [PATCH] Proposed Fix Clearing cq while we debate if we should have <>s or not.
Relating bug 119766. Would be nice if the Bugzilla Bug URL was also the shorthand syntax.
(In reply to comment #16) > Relating bug 119766. Would be nice if the Bugzilla Bug URL was also the > shorthand syntax. Didn't Ryosuke point out that this broke some tool back when Dean originally tried to switch? (I can't find the reference at the moment).
Comment on attachment 268493 [details] [PATCH] Proposed Fix Clearing flags on attachment: 268493 Committed r194749: <http://trac.webkit.org/changeset/194749>
All reviewed patches have been landed. Closing bug.
(In reply to comment #17) > (In reply to comment #16) > > Relating bug 119766. Would be nice if the Bugzilla Bug URL was also the > > shorthand syntax. > > Didn't Ryosuke point out that this broke some tool back when Dean originally > tried to switch? (I can't find the reference at the moment). Quote from the linked bug: "There are just too many tools that rely on the old format. Using wekbit.org/b/<~> URL is good but wrapping them in <> and putting the URL and bug title isn't. It makes the long way too long for some bugs and wrapping it in < > breaks all sorts of tools." This sounds like it's trivial to fix, honestly. There are a lot of cases where we regex search such as var regexp = /\/show_bug.cgi\?id=(\d+)/; that could be changed to also search for \/b\/(\d+). I filed <https://webkit.org/b/152898> to track any blockers.