Bug 152839 – prepare-ChangeLog should include radar number

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Joseph Pecoraro]

* 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. [e-mail]).

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.

[Radar WebKit Bug Importer]

<rdar://problem/24093563>

[Blaze Burg]

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.

[Joseph Pecoraro]

(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>&lt;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).

[Joseph Pecoraro]

Yep, I have this working, I'll clean it up.

[Joseph Pecoraro]

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 <[e-mail]>.
>   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 <[e-mail]>.
>   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 <[e-mail]>.
>   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.

[Brent Fulgham]

Holy cow, Joe! This is fantastic!

[Brent Fulgham]

Comment on attachment 268480 [details]
[PATCH] Proposed Fix

Very nice! r=me.

[Timothy Hatcher]

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*(&lt;rdar:\/\/problem\/\d+&gt;)/;

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.

[Daniel Bates]

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*(&lt;rdar:\/\/problem\/\d+&gt;)/;

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*(&lt;rdar://problem/\d+&gt;)|;

The benefit of using a different regular expression delimiter is that it makes the regular expression more straightforward to read.

[Joseph Pecoraro]

(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*(&lt;rdar:\/\/problem\/\d+&gt;)/;
> 
> 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*(&lt;rdar://problem/\d+&gt;)|;
> 
> The benefit of using a different regular expression delimiter is that it
> makes the regular expression more straightforward to read.

I agree!

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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".

[Joseph Pecoraro]

Created attachment 268493 [details]
[PATCH] Proposed Fix

[Daniel Bates]

Comment on attachment 268493 [details]
[PATCH] Proposed Fix

Thank you Joe for adding tests!

[Joseph Pecoraro]

Comment on attachment 268493 [details]
[PATCH] Proposed Fix

Clearing cq while we debate if we should have <>s or not.

[Joseph Pecoraro]

Relating bug 119766. Would be nice if the Bugzilla Bug URL was also the shorthand syntax.

[Brent Fulgham]

(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).

[WebKit Commit Bot]

Comment on attachment 268493 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 268493

Committed r194749: <http://trac.webkit.org/changeset/194749>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Blaze Burg]

(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.