Bug 57250 – check-webkit-style confused by two ChangeLog entries in a row from same user

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

[Jeff Miller]

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.

[David Levin]

Bummer I specifically got this added to the tests that went in with that change.

[Jeff Miller]

I'm also seeing this with my patch for https://bugs.webkit.org/show_bug.cgi?id=57262

[Patrick R. Gansterer]

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.

[David Levin]

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

[David Levin]

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

[David Levin]

Created attachment 87279 [details]
Patch

[Patrick R. Gansterer]

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

[Eric Seidel (no email)]

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)

[David Levin]

Created attachment 87281 [details]
Patch

[David Levin]

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

[Patrick R. Gansterer]

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

[David Levin]

Created attachment 87342 [details]
Patch

[David Levin]

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

[Patrick R. Gansterer]

(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  <[e-mail]>\n'
+                             '\n'
+                             '        Example ChangeLog entry out of range\n'
+                             '        http://example.com/\n')

[David Levin]

Created attachment 87381 [details]
Patch

[David Levin]

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

[Shinichiro Hamaji]

Comment on attachment 87381 [details]
Patch

Looks good.

[WebKit Commit Bot]

Comment on attachment 87381 [details]
Patch

Clearing flags on attachment: 87381

Committed r82382: <http://trac.webkit.org/changeset/82382>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.