Bug 56989 – webkit-patch should be more intelligent about whether a bug applies to a patch

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Ojan Vafai]

http://trac.webkit.org/changeset/81850 closed bug 56988, but that was obviously not my intention. Maybe we should expect a bug on a line by itself?

[Adam Barth]

Sounds like a good idea.  There's a regexp in webkitpy you can try adjusting.

[Eric Seidel (no email)]

We're trying to add semantic meaning where there is none.  We have conventions for ChangeLogs, but not everyone follows them, so webkit-patch makes mistakes.

[[e-mail]]

Created attachment 86875 [details]
Proposed patch to improve parsing of bug IDs from changelogs

[Ojan Vafai]

Comment on attachment 86875 [details]
Proposed patch to improve parsing of bug IDs from changelogs

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

The regexp and tests look good. Just not sure about the need for an argument.

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:53
> +def parse_bug_id(message, show_bug_url_on_own_line=False):

I don't see why you need this argument. As best I can tell, all the paces where this is used are to get the bug ID out of a changelog/commit entry and they should all have the same format. Is there a case that breaks if you just always restrict to searching for the bug url on it's own line?

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:59
> +    if show_bug_url_on_own_line == False:

If you invert the order here, you can just check "if show_bug_url_on_own_line". Also, general preferred style is to use "if bool" or "if not bool" instead of comparing to True/False.

[[e-mail]]

> > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:53
> > +def parse_bug_id(message, show_bug_url_on_own_line=False):
> 
> I don't see why you need this argument. As best I can tell, all the paces where this is used are to get the bug ID out of a changelog/commit entry and they should all have the same format. Is there a case that breaks if you just always restrict to searching for the bug url on it's own line?

Currently sheriff_unittest.py creates a string "Created bug https://bugs.webkit.org/..." which is used as an input to parse_bug_id().  

I could alter this particular test, but my concern is potentially breaking anything that may be lacking robust unittests that assumes that a non-start-of-line match of a bug URL is handled by parse_bug_id()
 
> > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:59
> > +    if show_bug_url_on_own_line == False:
> 
> If you invert the order here, you can just check "if show_bug_url_on_own_line". Also, general preferred style is to use "if bool" or "if not bool" instead of comparing to True/False.

Fixed, thanks, I'll upload another patch if we agree that an argument is or isn't needed.

[Ojan Vafai]

(In reply to comment #5)
> > > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:53
> > > +def parse_bug_id(message, show_bug_url_on_own_line=False):
> > 
> > I don't see why you need this argument. As best I can tell, all the paces where this is used are to get the bug ID out of a changelog/commit entry and they should all have the same format. Is there a case that breaks if you just always restrict to searching for the bug url on it's own line?
> 
> Currently sheriff_unittest.py creates a string "Created bug https://bugs.webkit.org/..." which is used as an input to parse_bug_id().  
> 
> I could alter this particular test, but my concern is potentially breaking anything that may be lacking robust unittests that assumes that a non-start-of-line match of a bug URL is handled by parse_bug_id()

Oh. I see. Yeah, create_bug uses that output. I'd prefer for the default to be True here since the sheriffbot case is the exception.

[[e-mail]]

Created attachment 86882 [details]
Updated patch based on Ojan's feedback

- Made the regex used by parse_bug_id() when show_bug_url_on_own_line is True more robust by optionally permitting &ctype=xml in the URL whilst still checking for end of line
- Made show_bug_url_on_own_line True by default, and explicitly set value to false in calling functions where needed

[Ojan Vafai]

Comment on attachment 86882 [details]
Updated patch based on Ojan's feedback

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

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:61
> +            "^\s*" + Bugzilla.bug_server_regex + "show_bug\.cgi\?id=(?P<bug_id>\d+)(&ctype=xml)?$",

Interesting. If we're going to handle this, we should probably also handle the case where ctype comes before the id?

[Eric Seidel (no email)]

Comment on attachment 86882 [details]
Updated patch based on Ojan's feedback

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

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:53
> +def parse_bug_id(message, show_bug_url_on_own_line=True):

What does "show_bug_url_on_own_line" mean? 

I also wonder if this shouldn't ust be a separate function.  Why is it OK to find http://webkit.org/b/ links not on their own line?

[Tony Chang]

Comment on attachment 86882 [details]
Updated patch based on Ojan's feedback

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

>> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:61
>> +            "^\s*" + Bugzilla.bug_server_regex + "show_bug\.cgi\?id=(?P<bug_id>\d+)(&ctype=xml)?$",
> 
> Interesting. If we're going to handle this, we should probably also handle the case where ctype comes before the id?

Drive-by-nit: These string literals should be prefixed with 'r' to make it clear you don't want backslashes to be escaped.

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:65
> +            Bugzilla.bug_server_regex + "show_bug\.cgi\?id=(?P<bug_id>\d+)",

This string too.

[[e-mail]]

Created attachment 87100 [details]
Create and use parse_bug_id_from_changelog()

Create parse_bug_id_from_changelog() which is stricter about looking for a URL to a bug than parse_bug_id (which will match anywhere in the input message).  
parse_bug_id_from_changelog() will parse out a bug ID from a ChangeLog that has been created with prepare-ChangeLog (ie, the URL to the bug is on a line by itself).

[WebKit Commit Bot]

Comment on attachment 87100 [details]
Create and use parse_bug_id_from_changelog()

Clearing flags on attachment: 87100

Committed r82196: <http://trac.webkit.org/changeset/82196>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Roben (:aroben)]

(In reply to comment #0)
> http://trac.webkit.org/changeset/81850 closed bug 56988, but that was obviously not my intention. Maybe we should expect a bug on a line by itself?

r81850 was clearly a patch that was landed due to bug 56988. I think it makes sense that webkit-patch would have associated the two. The fact that you didn't want to close the bug is a separate issue; we have --no-close for that.

The new behavior is so strict that it doesn't find the bug URLs in my commits, so I filed bug 57579 about it.

[Adam Roben (:aroben)]

I guess what I'm saying is that it seems like a bug that webkit-patch no longer associates r81850 and bug 56988, *and* the new behavior doesn't work well with my non-standard-but-long-in-use ChangeLog format. Hence bug 57579.

[Eric Seidel (no email)]

There are tools which create these entries (including the bug link) for you.  If both you and Ojan used them, than neither of these bugs woudl exist. :)

But then again, part of why i wrote the parser so flexibly in the first place was to be able to read historical change log entries.  I think we should just roll out ojan's change.

[Eric Seidel (no email)]

It's never useful for the changelog code to *not* return a bug number.  If that's the wrong number.  Than that's OK.  It's better to have a number than not IMO.

[Adam Roben (:aroben)]

As I think I've made clear above, I don't think we've yet discussed a case where webkit-patch chose the wrong number. The only thing that was wrong was that the bug was closed instead of just being updated.