Bug 28291 – webkit-patch (or a pre-commit hook) needs to prevent bad ChangeLog changes

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

bugzilla-tool (or a pre-commit hook) needs to prevent bad ChangeLog changes

Need to validate that if a ChangeLog is changed that it contains at a diff segment which starts at -1 (top of the file).

We could additionally validate the form of the ChangeLog (that it has a nice email address, name, etc.)

But mostly we need to prevent ever letting bugzilla-tool check in a bad ChangeLog entry.

[Eric Seidel (no email)]

This is separate from fixing bugzilla-tool to run resolve-ChangeLogs, as I think we should make sure that even if resolve-ChangeLogs ever fails us that we don't commit a bad ChangeLog.

[Eric Seidel (no email)]

I looked into adding a pre-commit hook.  I think the big annoyance will be in handling diffs to ChangeLogs where the added entry has the same couple lines with the previous entry.  I don't know of any way to get svn to spit out diffs which are greedy towards the end of the file instead of the beginning of the file, so we can't always check for diffs which start at -1.

[Eric Seidel (no email)]

This should largely be fixed by bug 30683.  (Or rather, bug 30683 eliminates our primary source of bad ChangeLogs)

[Eric Seidel (no email)]

I feel like we have at least one dupe of this bug.

[Eric Seidel (no email)]

*** Bug 51561 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

This has too-long been a problem.  Writing up a patch now.

[Eric Seidel (no email)]

Created attachment 77387 [details]
Patch

[Adam Barth]

Comment on attachment 77387 [details]
Patch

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

Did you want to add this step to any of the sequences?

> Tools/Scripts/webkitpy/tool/steps/checkchangelogs.py:37
> +# This is closely related to the ValidateReviewer step and the CommitterValidator class
> +# we may eventually want to unify more of this code in one place.
> +class CheckChangeLogs(AbstractStep):

Maybe call it ValidateChangeLogs then?

> Tools/Scripts/webkitpy/tool/steps/checkchangelogs.py:41
> +            Options.git_commit,

Isn't this option added to all steps by AbstractStep?

> Tools/Scripts/webkitpy/tool/steps/checkchangelogs.py:55
> +        # FIXME: Do we need to make the file path absolute?
> +        self._tool.scm().diff_for_file(diff_file.filename)

scm takes paths relative to the root of the repo.  Did you try this from a non-repo root CWD?

[David Levin]

Misc comments:

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

> Tools/Scripts/webkitpy/common/checkout/diff_parser.py:139
> +    # If this class keeps "Parser" in its name, this should be made public.

I don't understand why "parser" = "public".

> Tools/Scripts/webkitpy/tool/steps/checkchangelogs.py:35
> +# This is closely related to the ValidateReviewer step and the CommitterValidator class

Add . at end.

> Tools/Scripts/webkitpy/tool/steps/checkchangelogs.py:36
> +# we may eventually want to unify more of this code in one place.

s/we/We/

> Tools/Scripts/webkitpy/tool/steps/checkchangelogs.py:37
> +class CheckChangeLogs(AbstractStep):

Classes are typically named using nouns but this is a verb and sounds like a method.

ChangeLogsChecker or Verifier?

[Adam Barth]

(In reply to comment #9)
> View in context: https://bugs.webkit.org/attachment.cgi?id=77387&action=review
> 
> > Tools/Scripts/webkitpy/tool/steps/checkchangelogs.py:37
> > +class CheckChangeLogs(AbstractStep):
> 
> Classes are typically named using nouns but this is a verb and sounds like a method.
> 
> ChangeLogsChecker or Verifier?

Subclasses of AbstractStep are usually verbs in webkitpy.  For example, here's a list of steps:

1) Update the working copy.
2) Apply a patch.
3) Verify the ChangeLogs.

You could argue that we shouldn't be using classes for steps, but inheritance was too seductive for us.  :)

[Eric Seidel (no email)]

(In reply to comment #9)
> Misc comments:
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=77387&action=review
> 
> > Tools/Scripts/webkitpy/common/checkout/diff_parser.py:139
> > +    # If this class keeps "Parser" in its name, this should be made public.
> 
> I don't understand why "parser" = "public".

I"m noting that if it's a "Parser" it should be a long-lived object which can parse many files, instead of encapsulating a single diff as it does now.

Just going to remove the comment.

> > Tools/Scripts/webkitpy/tool/steps/checkchangelogs.py:35
> > +# This is closely related to the ValidateReviewer step and the CommitterValidator class
> 
> Add . at end.

Done.

> > Tools/Scripts/webkitpy/tool/steps/checkchangelogs.py:36
> > +# we may eventually want to unify more of this code in one place.
> 
> s/we/We/

Done.

> > Tools/Scripts/webkitpy/tool/steps/checkchangelogs.py:37
> > +class CheckChangeLogs(AbstractStep):
> 
> Classes are typically named using nouns but this is a verb and sounds like a method.
> 
> ChangeLogsChecker or Verifier?

Huh?  Steps are commonly verbs.  RunTests, Build, CreateBug. :)

[Eric Seidel (no email)]

(In reply to comment #11)
> (In reply to comment #9)
> > Classes are typically named using nouns but this is a verb and sounds like a method.
> > 
> > ChangeLogsChecker or Verifier?
> 
> Huh?  Steps are commonly verbs.  RunTests, Build, CreateBug. :)

Sorry for my tone, I thought I was replying to Adam (who lets me get away with a lot more cheek). :)

[Eric Seidel (no email)]

(In reply to comment #8)
> (From update of attachment 77387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77387&action=review
> 
> Did you want to add this step to any of the sequences?

Details, details.  Will do. :)

> > Tools/Scripts/webkitpy/tool/steps/checkchangelogs.py:37
> > +# This is closely related to the ValidateReviewer step and the CommitterValidator class
> > +# we may eventually want to unify more of this code in one place.
> > +class CheckChangeLogs(AbstractStep):
> 
> Maybe call it ValidateChangeLogs then?

Sure.  We have a CheckStyle step.  It's unclear which we should standardize on, but Validate sounds fine.

> > Tools/Scripts/webkitpy/tool/steps/checkchangelogs.py:41
> > +            Options.git_commit,
> 
> Isn't this option added to all steps by AbstractStep?

Woh, crazy.  Who knew.

> > Tools/Scripts/webkitpy/tool/steps/checkchangelogs.py:55
> > +        # FIXME: Do we need to make the file path absolute?
> > +        self._tool.scm().diff_for_file(diff_file.filename)
> 
> scm takes paths relative to the root of the repo.  Did you try this from a non-repo root CWD?

No, I have not.  Then again, I've not actually used it in a command yet. :)

[David Levin]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > Classes are typically named using nouns but this is a verb and sounds like a method.
> > > 
> > > ChangeLogsChecker or Verifier?
> > 
> > Huh?  Steps are commonly verbs.  RunTests, Build, CreateBug. :)
> 
> Sorry for my tone, I thought I was replying to Adam (who lets me get away with a lot more cheek). :)

I'm greatly offended and you will suffer my wrath!

ok not really. I thought it funny that you would think that I would know this :). I wasn't familiar with the pattern here. Adam explained it though.

The other stuff was super minor nits or curiosity. (I had started to review it and then saw Adam did the r+ so I just put i my comments anyway.)

[Eric Seidel (no email)]

Created attachment 77393 [details]
Patch

[Eric Seidel (no email)]

OK.  Deployed it to a bunch of places. :)  Seems to work at least for uploading to this bug!

Would love to have another round of review, plz.

[Adam Barth]

Comment on attachment 77393 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 77393 [details]
Patch

Rejecting attachment 77393 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
2.6/unittest.py", line 849, in createTests
    self.module)
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/unittest.py", line 613, in loadTestsFromNames
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/unittest.py", line 584, in loadTestsFromName
    parent, obj = obj, getattr(obj, part)
AttributeError: 'module' object has no attribute 'validatechangelogs_unittest'

Full output: http://queues.webkit.org/results/7213163

[Eric Seidel (no email)]

Created attachment 77417 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 77417 [details]
Patch for landing

Rejecting attachment 77417 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 77417]" exit_code: 1
Last 500 characters of output:
417&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=28291&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Updating working directory
Processing patch 77417 from bug 28291.
NOBODY (OOPS!) found in /mnt/git/webkit-commit-queue/Tools/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /mnt/git/webkit-commit-queue/Tools/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/7341048

[Eric Seidel (no email)]

Wow, well, that's subtle.  That's a bug in another step (not invalidating the cached diff).  Will fix.

[Eric Seidel (no email)]

Created attachment 77418 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 77418 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html bug 50995 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 77418 [details]
Patch for landing

Clearing flags on attachment: 77418

Committed r74639: <http://trac.webkit.org/changeset/74639>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.