Bug 28291 - webkit-patch (or a pre-commit hook) needs to prevent bad ChangeLog changes
Summary: webkit-patch (or a pre-commit hook) needs to prevent bad ChangeLog changes
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: Eric Seidel (no email)
URL:
Keywords:
: 51561 (view as bug list)
Depends on:
Blocks: 38870
  Show dependency treegraph
 
Reported: 2009-08-13 20:58 PDT by Eric Seidel (no email)
Modified: 2010-12-24 10:22 PST (History)
4 users (show)

See Also:


Attachments
Patch (15.38 KB, patch)
2010-12-23 17:15 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (19.18 KB, patch)
2010-12-23 18:14 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch for landing (20.56 KB, patch)
2010-12-24 08:53 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch for landing (21.60 KB, patch)
2010-12-24 09:30 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-08-13 20:58:33 PDT
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.
Comment 1 Eric Seidel (no email) 2009-08-13 20:59:18 PDT
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.
Comment 2 Eric Seidel (no email) 2009-08-24 15:10:58 PDT
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.
Comment 3 Eric Seidel (no email) 2009-10-23 14:26:35 PDT
This should largely be fixed by bug 30683.  (Or rather, bug 30683 eliminates our primary source of bad ChangeLogs)
Comment 4 Eric Seidel (no email) 2010-01-19 16:42:45 PST
I feel like we have at least one dupe of this bug.
Comment 5 Eric Seidel (no email) 2010-12-23 14:47:38 PST
*** Bug 51561 has been marked as a duplicate of this bug. ***
Comment 6 Eric Seidel (no email) 2010-12-23 15:22:15 PST
This has too-long been a problem.  Writing up a patch now.
Comment 7 Eric Seidel (no email) 2010-12-23 17:15:33 PST
Created attachment 77387 [details]
Patch
Comment 8 Adam Barth 2010-12-23 17:25:16 PST
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?
Comment 9 David Levin 2010-12-23 17:29:40 PST
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?
Comment 10 Adam Barth 2010-12-23 17:34:29 PST
(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.  :)
Comment 11 Eric Seidel (no email) 2010-12-23 17:48:41 PST
(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. :)
Comment 12 Eric Seidel (no email) 2010-12-23 17:49:33 PST
(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). :)
Comment 13 Eric Seidel (no email) 2010-12-23 17:50:51 PST
(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. :)
Comment 14 David Levin 2010-12-23 17:54:55 PST
(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.)
Comment 15 Eric Seidel (no email) 2010-12-23 18:14:08 PST
Created attachment 77393 [details]
Patch
Comment 16 Eric Seidel (no email) 2010-12-23 18:15:04 PST
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.
Comment 17 Adam Barth 2010-12-23 18:25:58 PST
Comment on attachment 77393 [details]
Patch

Thanks!
Comment 18 WebKit Commit Bot 2010-12-24 05:10:15 PST
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
Comment 19 Eric Seidel (no email) 2010-12-24 08:53:42 PST
Created attachment 77417 [details]
Patch for landing
Comment 20 WebKit Commit Bot 2010-12-24 09:13:37 PST
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
Comment 21 Eric Seidel (no email) 2010-12-24 09:24:55 PST
Wow, well, that's subtle.  That's a bug in another step (not invalidating the cached diff).  Will fix.
Comment 22 Eric Seidel (no email) 2010-12-24 09:30:10 PST
Created attachment 77418 [details]
Patch for landing
Comment 23 WebKit Commit Bot 2010-12-24 10:19:06 PST
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: dumi@chromium.org)
The commit-queue is continuing to process your patch.
Comment 24 WebKit Commit Bot 2010-12-24 10:22:20 PST
Comment on attachment 77418 [details]
Patch for landing

Clearing flags on attachment: 77418

Committed r74639: <http://trac.webkit.org/changeset/74639>
Comment 25 WebKit Commit Bot 2010-12-24 10:22:30 PST
All reviewed patches have been landed.  Closing bug.