Bug 188235 – If there's a radar in the changelog, webkit-patch upload/create-bug should put the radar in the bug and set InRadar

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Lucas Forschler]

If there's a radar in the changelog, webkit-patch upload/create-bug should put the radar in the bug and set InRadar

[Lucas Forschler]

Created attachment 346291 [details]
Patch

[Lucas Forschler]

<rdar://problem/33732700>

[Andy Estes]

Comment on attachment 346291 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346291&action=review

What happens if I post multiple patches to the same bug? Will I get multiple comments with Radar links?

> Tools/Scripts/webkitpy/tool/steps/addradar.py:1
> +# Copyright (C) 2005, 2006, 2007, 2008, 2009, 2018 Apple Inc. All rights reserved.

This doesn't seem like the right copyright line for new code. It should just include 2018 (or was this copied from elsewhere?).

[Daniel Bates]

Comment on attachment 346291 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346291&action=review

> Tools/Scripts/webkitpy/tool/steps/addradar.py:35
> +        matcher = re.compile(r'(<?rdar:\/\/problems?\/)?(?P<radar_id>-?\d{7,})>?')

It is not necessary to escape the /. I take it you feel it is common error to write a radar id with less than 7 digits?

> Tools/Scripts/webkitpy/tool/steps/addradar.py:69
> +        changed_files = self.cached_lookup(state, "changed_files")
> +        for filename in changed_files:
> +            if not self._tool.checkout().is_path_to_changelog(filename):
> +                continue
> +            diff = self._tool.scm().diff_for_file(filename)
> +            radar_link = self._get_radar_link_from_diff(diff)
> +
> +            # Stop looking as soon as we get any radar link
> +            if radar_link:
> +                break

Can we take a similar as in class ValidateChangeLogs? We should also ensure we handle the case where the diff entry may be shifted down because lines at the top of the added entry are identical to lines in the previous entry [1].

[1] <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/VCSUtils_unittest/fixChangeLogPatchThenSetChangeLogDateAndReviewer.pl?rev=234401#L44> is an example where the diff for the added entry was shifted down such that it excluded the first and second lines of the added entry as they matched the top two lines in the previous entry. This example just illustrates the general problem of shifting.

> Tools/Scripts/webkitpy/tool/steps/addradar_unittest.py:88
> +        2018-07-30  Lucas Forschler  <[e-mail]>
> +
> +                Add Radar info to bugzilla when found in ChangeLog
> +                https://bugs.webkit.org/show_bug.cgi?id=188186
> +                <rdar://problem/1234567>
> +
> +                Reviewed by NOBODY (OOPS!).
> +
> +                * Scripts/webkit-patch:
> +                * Scripts/webkitpy/common/net/bugzilla/bugzilla.py:
> +                (Bugzilla.add_keyword_to_bug):
> +                * Scripts/webkitpy/tool/commands/upload.py:
> +                (Upload):
> +                * Scripts/webkitpy/tool/steps/__init__.py:
> +                * Scripts/webkitpy/tool/steps/addwkbi.py: Added.
> +                (AddWKBI):
> +                (AddWKBI.options):
> +                (AddWKBI.GetRadarLinkFromDiff):
> +                (AddWKBI.run):

These diffs, including this one, represent context. That is, they do not represent an added entry. A diff that represents an added entry should have lines prefixed by '+'.

[Daniel Bates]

Comment on attachment 346291 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346291&action=review

>> Tools/Scripts/webkitpy/tool/steps/addradar.py:69
>> +                break
> 
> Can we take a similar as in class ValidateChangeLogs? We should also ensure we handle the case where the diff entry may be shifted down because lines at the top of the added entry are identical to lines in the previous entry [1].
> 
> [1] <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/VCSUtils_unittest/fixChangeLogPatchThenSetChangeLogDateAndReviewer.pl?rev=234401#L44> is an example where the diff for the added entry was shifted down such that it excluded the first and second lines of the added entry as they matched the top two lines in the previous entry. This example just illustrates the general problem of shifting.

Actually, we will have a ChangeLog entry at the top of the file by the time this step runs as part of the Upload command. More specifically, we will have a ChangeLog entry at the top of the file once the PrepareChangeLog step runs to completion. We should take a similar approach as in PrepareChangeLog._ensure_bug_url() and use ChangeLog.latest_entry() for the top-most entry and teach ChangeLogEntry how to parse a radar URL.

[Lucas Forschler]

Created attachment 346420 [details]
Patch

[Lucas Forschler]

Created attachment 346422 [details]
Patch

[Lucas Forschler]

(In reply to Andy Estes from comment #3)
> Comment on attachment 346291 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346291&action=review
> 
> What happens if I post multiple patches to the same bug? Will I get multiple
> comments with Radar links?

this logic will only run when a 'new' bugzilla entry is created with 'webkit-patch upload'. if you are adding an attachment to an existing bug, this is a no-op.
> 
> > Tools/Scripts/webkitpy/tool/steps/addradar.py:1
> > +# Copyright (C) 2005, 2006, 2007, 2008, 2009, 2018 Apple Inc. All rights reserved.
> 
> This doesn't seem like the right copyright line for new code. It should just
> include 2018 (or was this copied from elsewhere?).

[Kocsen Chung]

Comment on attachment 346422 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346422&action=review

> Tools/ChangeLog:3
> +        If there's a radar in the changelog, webkit-patch upload/create-bug should put the radar in the bug and set InRadar

nit: ChangeLog
nit: Radar
(Capitalization)

> Tools/Scripts/webkitpy/common/checkout/changelog.py:69
> +    radar_id_regexp = r'(<?rdar://problems?/)?(?P<radar_id>-?\d{7,})>?'

The "rdar://problem" group is listed as optional, this means this would match on any integer with 7+ digits. 
I would assume we need the regex to be more confident if it detected a radar?

> Tools/Scripts/webkitpy/common/checkout/changelog.py:124
> +        if radar_id < 0:

When would this happen? 
I think the regex makes it impossible to have the id be negative. If that's the case we can get rid of this if statement.

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:857
> +        _log.info("Adding %s to the keyword list for bug %s" % (keyword, bug_id))

I'm unfamiliar with the rest of this file. Can we use .format() instead?

```python
"Adding {} to the keyword list for bug {}".format(keyword, bug_id)
```

[Aakash Jain]

r=me provided Kocsen and Dan's feedback is incorporated.

[Daniel Bates]

Comment on attachment 346422 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346422&action=review

> Tools/Scripts/webkitpy/common/checkout/changelog.py:117
> +    def _parse_radar_id(cls, text):

The matching in this function and the regex we are using seem too generous because they seem to search for a rdar-looking URL anywhere. I suggest we take a similar approach as in parse_bug_id_from_changelog() and restrict matching to a radar-like URL with zero or more leading whitespace from the start of a ChangeLog line. We should also add test cases for this.

> Tools/Scripts/webkitpy/tool/steps/addradar.py:40
> +        if state.get("created_new_bug") != True:

Remove the “!= True” and add “not” after “if”.

[Daniel Bates]

Have you tried running the regex on existing ChangeLogs and measuring the false positive rate?

[Daniel Bates]

And false negative rate?

[Lucas Forschler]

(In reply to Kocsen Chung from comment #9)
> Comment on attachment 346422 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346422&action=review
> 
> > Tools/ChangeLog:3
> > +        If there's a radar in the changelog, webkit-patch upload/create-bug should put the radar in the bug and set InRadar
> 
> nit: ChangeLog
> nit: Radar
> (Capitalization)
> 
fixed

> > Tools/Scripts/webkitpy/common/checkout/changelog.py:69
> > +    radar_id_regexp = r'(<?rdar://problems?/)?(?P<radar_id>-?\d{7,})>?'
> 
> The "rdar://problem" group is listed as optional, this means this would
> match on any integer with 7+ digits. 
> I would assume we need the regex to be more confident if it detected a radar?
> 
I'm using the same regex we use in our Internal repository (radar.py). It seems as though people think there may be a better one available. I'm happy to change it, but if we do, let's change it in both places.

> > Tools/Scripts/webkitpy/common/checkout/changelog.py:124
> > +        if radar_id < 0:
> 
> When would this happen? 
> I think the regex makes it impossible to have the id be negative. If that's
> the case we can get rid of this if statement.

quite possibly... this code was also borrowed from radar.py, so we should consider a change there as well.


> 
> > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:857
> > +        _log.info("Adding %s to the keyword list for bug %s" % (keyword, bug_id))
> 
> I'm unfamiliar with the rest of this file. Can we use .format() instead?

I also prefer .format, however this is the convention used in the rest of the tool. I'm open to using the .format notation, but feel it's better to stick with existing style.

> 
> ```python
> "Adding {} to the keyword list for bug {}".format(keyword, bug_id)
> ```

[Lucas Forschler]

(In reply to Daniel Bates from comment #11)
> Comment on attachment 346422 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346422&action=review
> 
> > Tools/Scripts/webkitpy/common/checkout/changelog.py:117
> > +    def _parse_radar_id(cls, text):
> 
> The matching in this function and the regex we are using seem too generous
> because they seem to search for a rdar-looking URL anywhere. I suggest we
> take a similar approach as in parse_bug_id_from_changelog() and restrict
> matching to a radar-like URL with zero or more leading whitespace from the
> start of a ChangeLog line. We should also add test cases for this.

This sounds reasonable, I will fix in the next iteration.

> 
> > Tools/Scripts/webkitpy/tool/steps/addradar.py:40
> > +        if state.get("created_new_bug") != True:
> 
> Remove the “!= True” and add “not” after “if”.
fixed in next patch. thanks!

[Lucas Forschler]

(In reply to Daniel Bates from comment #12)
> Have you tried running the regex on existing ChangeLogs and measuring the
> false positive rate?

After adding the ^\s* to the start of the regex, I see no false positives or negatives searching every ChangeLog in OpenSource. We no longer identify radar URLs inside the middle of a string as expected. However, the variations of rdar:// URLs inside ChangeLog files are relatively few. The unit tests I have created are significantly more thorough than anything found inside ChangeLogs.

[Lucas Forschler]

Created attachment 346711 [details]
Patch

[Lucas Forschler]

Committed r234663: <https://trac.webkit.org/changeset/234663>

[Alex Christensen]

Comment on attachment 346711 [details]
Patch

This has been requesting review for more than one year.  If this is still needed, please rebase and re-request review.