Bug 26927 - webkit-patch or pre-commit hook should validate reviewer lines before committing
Summary: webkit-patch or pre-commit hook should validate reviewer lines before committing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 32343
  Show dependency tree / graph
 
Reported: 2009-07-02 11:13 PDT by Eric Seidel (no email)
Modified: 2010-03-24 20:45 PDT (History)
3 users (show)

See Also:

Attachments
Patch (2.42 KB, patch)
2010-03-24 18:18 PDT, Adam Barth 		no flags Details | Formatted Diff | Diff
Patch (8.42 KB, patch)
2010-03-24 20:21 PDT, Adam Barth 		no flags Details | Formatted Diff | Diff
Patch (10.88 KB, patch)
2010-03-24 20:28 PDT, Adam Barth 		no flags Details | Formatted Diff | Diff
Patch (11.04 KB, patch)
2010-03-24 20:42 PDT, Adam Barth 		eric: review+
Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-07-02 11:13:46 PDT 
bugzilla-tool or pre-commit hook should validate reviewer lines before committing

This will avoid bad commit lines like:
http://trac.webkit.org/changeset/45463

We already have a list of authorized reviewers in svn (bugzilla.py for now, but eventually in reviewers.py) so validating a parsed out reviewer name against it is easy.
Comment 1 Kenneth Rohde Christiansen 2010-03-01 12:17:54 PST 
If I land a patch through the commit-queue that has been reviewed, it could fail if the ChangeLog says "Reviewed by NOBODY".

"Reviewed by NOBODY" is a valid line, but probably not the intention if the patch was actually reviewed.

Personally I always write "Unreviewed" and then some kind of explanation, like "Unreviewed Qt buildbot fix".
Comment 2 Eric Seidel (no email) 2010-03-24 16:25:11 PDT 
Another bad commit:
http://trac.webkit.org/changeset/56454
Comment 3 Adam Barth 2010-03-24 18:18:35 PDT 
Created attachment 51572 [details]
Patch
Comment 4 Eric Seidel (no email) 2010-03-24 19:24:14 PDT 
Comment on attachment 51572 [details]
Patch

You are a better man than I.  However, this is sub-optimal (and will break the case of the ChangeLog posted already having a valid reviewer).

Better would be to look at reviewer() on all the ChangeLogEntries right before we assemble the commit message.  We could even validate that that reviewer() matches the one we pulled off the bug, if the bug had one.
Comment 5 Adam Barth 2010-03-24 20:21:57 PDT 
Created attachment 51585 [details]
Patch
Comment 6 Adam Barth 2010-03-24 20:28:45 PDT 
Created attachment 51586 [details]
Patch
Comment 7 Eric Seidel (no email) 2010-03-24 20:35:50 PDT 
Comment on attachment 51586 [details]
Patch

+        os.chdir(self._tool.scm().checkout_root)

That needs a FIXME.

Can we say regexp?
+            if changelog_entry.contents().lower().find("unreviewed"):

re.match("unreviewed", changelog_entry.contents(), re.IGNORECASE)

You could/should even just compile the regexp as a class variable on ValidateReviewer.

+            error('%s neither lists a valid reviewer nor contains the string "Unreviewed".')
should note that the search is case insensitive.

I am saddened by the lack of the testzors.

Otherwise this looks great!
Comment 8 Adam Barth 2010-03-24 20:42:18 PDT 
Created attachment 51588 [details]
Patch
Comment 9 Eric Seidel (no email) 2010-03-24 20:44:38 PDT 
Comment on attachment 51588 [details]
Patch

I really dislike the lack of testing.  If you get this wrong, then "webkit-patch land" will be broken for people. :(

r+, assuming that you've tested this somehow or will...
Comment 10 Adam Barth 2010-03-24 20:45:22 PDT 
Committed r56478: <http://trac.webkit.org/changeset/56478>