Bug 56989 - webkit-patch should be more intelligent about whether a bug applies to a patch
Summary: webkit-patch should be more intelligent about whether a bug applies to a patch
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-23 20:34 PDT by Ojan Vafai
Modified: 2011-04-01 05:59 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch to improve parsing of bug IDs from changelogs (7.08 KB, patch)
2011-03-24 21:03 PDT, andrewf@chromium.org
ojan: review-
Details | Formatted Diff | Diff
Updated patch based on Ojan's feedback (7.58 KB, patch)
2011-03-24 22:21 PDT, andrewf@chromium.org
eric: review-
Details | Formatted Diff | Diff
Create and use parse_bug_id_from_changelog() (10.73 KB, patch)
2011-03-27 22:29 PDT, andrewf@chromium.org
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2011-03-23 20:34:14 PDT
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?
Comment 1 Adam Barth 2011-03-24 00:31:07 PDT
Sounds like a good idea.  There's a regexp in webkitpy you can try adjusting.
Comment 2 Eric Seidel (no email) 2011-03-24 07:25:30 PDT
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.
Comment 3 andrewf@chromium.org 2011-03-24 21:03:51 PDT
Created attachment 86875 [details]
Proposed patch to improve parsing of bug IDs from changelogs
Comment 4 Ojan Vafai 2011-03-24 21:14:59 PDT
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.
Comment 5 andrewf@chromium.org 2011-03-24 21:50:39 PDT
> > 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.
Comment 6 Ojan Vafai 2011-03-24 22:06:21 PDT
(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.
Comment 7 andrewf@chromium.org 2011-03-24 22:21:54 PDT
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
Comment 8 Ojan Vafai 2011-03-24 22:35:38 PDT
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?
Comment 9 Eric Seidel (no email) 2011-03-25 00:24:09 PDT
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?
Comment 10 Tony Chang 2011-03-25 09:54:08 PDT
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.
Comment 11 andrewf@chromium.org 2011-03-27 22:29:05 PDT
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).
Comment 12 WebKit Commit Bot 2011-03-28 22:56:29 PDT
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>
Comment 13 WebKit Commit Bot 2011-03-28 22:56:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Adam Roben (:aroben) 2011-03-31 14:23:20 PDT
(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.
Comment 15 Adam Roben (:aroben) 2011-03-31 14:25:16 PDT
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.
Comment 16 Eric Seidel (no email) 2011-03-31 20:50:13 PDT
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.
Comment 17 Eric Seidel (no email) 2011-03-31 20:50:49 PDT
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.
Comment 18 Adam Roben (:aroben) 2011-04-01 05:59:37 PDT
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.