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.
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.
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.
This should largely be fixed by bug 30683. (Or rather, bug 30683 eliminates our primary source of bad ChangeLogs)
I feel like we have at least one dupe of this bug.
*** Bug 51561 has been marked as a duplicate of this bug. ***
This has too-long been a problem. Writing up a patch now.
Created attachment 77387 [details] Patch
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?
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?
(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. :)
(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. :)
(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). :)
(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. :)
(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.)
Created attachment 77393 [details] Patch
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 on attachment 77393 [details] Patch Thanks!
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
Created attachment 77417 [details] Patch for landing
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
Wow, well, that's subtle. That's a bug in another step (not invalidating the cached diff). Will fix.
Created attachment 77418 [details] Patch for landing
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 on attachment 77418 [details] Patch for landing Clearing flags on attachment: 77418 Committed r74639: <http://trac.webkit.org/changeset/74639>
All reviewed patches have been landed. Closing bug.