Earlier today, in r82114, I checked in a rubber-stamped change to WebKitLibraries without a bug number in WebKitLibraries/ChangeLog. Then, I uploaded a patch for <https://bugs.webkit.org/show_bug.cgi?id=57195> at 2011-03-28 10:45 PST which also included a change to WebKitLibraries (with a bug number in the ChangeLog). However, check-webkit-style failed on WebKitLibraries/ChangeLog for the patch with: WebKitLibraries/ChangeLog:15: ChangeLog entry has no bug number [changelog/bugnumber] [5] I think it's confused by my earlier rubber-stamped ChangeLog entry.
Bummer I specifically got this added to the tests that went in with that change.
I'm also seeing this with my patch for https://bugs.webkit.org/show_bug.cgi?id=57262
Hmmm, I don't see the reason for the false positive at the moment. I need to test this on my "WebKit machine". :-( Unfortunally I don't have time to look at this before tomorrow. I'll take care of this issue.
I know what is going wrong with one of them: 1. The diff came out "funny" due to the entries in succession. When the checker logs the error for line 15 (for the following entry which has no bug number). the error filter allows the error to go through because this line is in the diff. 2. For bug 57262, the checker was correct that the ChangeLog entry starting at line 13 had no bug number but line 13 was not part of the patch for the error filter should have removed this. We can fix these by limiting the lines given to the main check function to be just the added lines. (Of course, we'll have to adjust the lines where the errors are reported if this approach is taken.)
(In reply to comment #4) > 2. For bug 57262, the checker was correct that the ChangeLog entry starting at line 13 had no bug number but line 13 was not part of the patch for the error filter should have removed this. Ah. For this one it is the same problem as #1. Even though the patch doesn't show line 13 as being part of the patch, the patch is applied locally and when the diff is done locally, then the line 1 is not part of the diff but line 13 is.
Created attachment 87279 [details] Patch
Comment on attachment 87279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87279&action=review LGTM > Tools/Scripts/webkitpy/style/checkers/changelog_unittest.py:-43 > - Hmmm, my style checker wanted this empty line before def. > Tools/Scripts/webkitpy/style/checkers/changelog_unittest.py:79 > + ' Example bug' > + '\n' \n into the same line > Tools/Scripts/webkitpy/style/checkers/changelog_unittest.py:106 > def test_no_error(self): Maybe you can add an empty one too
Comment on attachment 87279 [details] Patch We ahve code for fixing up ChagneLog diffs somewehre. Seems check-webkit-style would only want to scan the current ChangeLog and ignore the rest of hte diff. (I haven't really looked at your patch, mabye that's what it does)
Created attachment 87281 [details] Patch
(In reply to comment #8) > (From update of attachment 87279 [details]) > We ahve code for fixing up ChagneLog diffs somewehre. Seems check-webkit-style would only want to scan the current ChangeLog and ignore the rest of hte diff. (I haven't really looked at your patch, mabye that's what it does) Yes basically it does only look at the current ChangeLog. (In reply to comment #7) > (From update of attachment 87279 [details]) > > Tools/Scripts/webkitpy/style/checkers/changelog_unittest.py:-43 > > - > > Hmmm, my style checker wanted this empty line before def. It still does. It doesn't complain about my patch because the error gets filtered out (due to it being a deleted line) -- tricky case. Thanks for noticing. Addressed the other two comments also.
Comment on attachment 87281 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87281&action=review > Tools/Scripts/webkitpy/style/checkers/changelog_unittest.py:89 > + '2011-01-01 Patrick Gansterer <paroga@paroga.com>\n' > + '\n Example bug' > + '\n http://trac.webkit.org/changeset/12345\n') The other lines have the \n only at the end > Tools/Scripts/webkitpy/style/checkers/changelog_unittest.py:111 > + self.assert_no_error([], > + '2011-01-01 Patrick Gansterer <paroga@paroga.com>\n' > + '\n' > + ' Example bug\n' > + ' http://bugs.webkit.org/show_bug.cgi?id=12345\n') Thx, maybe we can add and "invalid" entry with emty range too.
Created attachment 87342 [details] Patch
(In reply to comment #11) > (From update of attachment 87281 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87281&action=review > > > Tools/Scripts/webkitpy/style/checkers/changelog_unittest.py:89 > > + '2011-01-01 Patrick Gansterer <paroga@paroga.com (:paroga) (c)>\n' > > + '\n Example bug' > > + '\n http://trac.webkit.org/changeset/12345\n') > > The other lines have the \n only at the end Fixed. > > > Tools/Scripts/webkitpy/style/checkers/changelog_unittest.py:111 > > + self.assert_no_error([], > > + '2011-01-01 Patrick Gansterer <paroga@paroga.com (:paroga) (c)>\n' > > + '\n' > > + ' Example bug\n' > > + ' http://bugs.webkit.org/show_bug.cgi?id=12345\n') > > Thx, maybe we can add and "invalid" entry with emty range too. I don't understand because I think the lines which start with self.assert_error(2, range(2, 5), 'changelog/bugnumber' already do this.
(In reply to comment #13) > I don't understand because I think the lines which start with > self.assert_error(2, range(2, 5), 'changelog/bugnumber' > already do this. I mean sth like the following: + self.assert_no_error([], + '2011-01-01 Patrick Gansterer <paroga@paroga.com>\n' + '\n' + ' Example ChangeLog entry out of range\n' + ' http://example.com/\n')
Created attachment 87381 [details] Patch
(In reply to comment #14) > (In reply to comment #13) > > I don't understand because I think the lines which start with > > self.assert_error(2, range(2, 5), 'changelog/bugnumber' > > already do this. > > I mean sth like the following: ok. done.
Comment on attachment 87381 [details] Patch Looks good.
Comment on attachment 87381 [details] Patch Clearing flags on attachment: 87381 Committed r82382: <http://trac.webkit.org/changeset/82382>
All reviewed patches have been landed. Closing bug.