hghooks: move ipcsync hook to modern hook class (bug 1454296) r?gps draft
authorbyron jones <glob@mozilla.com>
Wed, 18 Apr 2018 14:29:20 +0800
changeset 12245 7a3ffaac2d6f965270dae63102d4772047a5c742
parent 12244 8a77ed2182d1fe450eb16d502d16f3a2a25a0b2d
push id1919
push userbjones@mozilla.com
push dateWed, 02 May 2018 05:27:15 +0000
reviewersgps
bugs1454296
hghooks: move ipcsync hook to modern hook class (bug 1454296) r?gps MozReview-Commit-ID: KGzMlpGwHcI
hghooks/mozhghooks/check/prevent_sync_ipc_changes.py
hghooks/mozhghooks/extension.py
hghooks/mozhghooks/prevent_webidl_changes.py
hghooks/tests/test-prevent-sync-ipc-changes.t
new file mode 100644
--- /dev/null
+++ b/hghooks/mozhghooks/check/prevent_sync_ipc_changes.py
@@ -0,0 +1,100 @@
+# This software may be used and distributed according to the terms of the
+# GNU General Public License version 2 or any later version.
+
+from __future__ import absolute_import
+
+from mercurial import util
+from mercurial.node import short
+from mozautomation.commitparser import (
+    parse_requal_reviewers,
+    is_backout,
+)
+
+from ..checks import (
+    PreTxnChangegroupCheck,
+    print_banner
+)
+
+IPC_PEERS = [
+    dict(name='Andrew McCreight', nick=['mccr8'], email=['continuation@gmail.com']),
+    dict(name='Ben Kelly', nick=['bkelly'], email=['bkelly@mozilla.com', 'ben@wanderview.com']),
+    dict(name='Bill McCloskey', nick=['billm'], email=['billm@mozilla.com']),
+    dict(name='David Anderson', nick=['dvander'], email=['danderson@mozilla.com', 'dvander@alliedmods.net']),
+    dict(name='Jed David', nick=['jld'], email=['jld@mozilla.com']),
+    dict(name='Kan-Ru Chen', nick=['kanru'], email=['kchen@mozilla.com', 'kanru@kanru.info']),
+    dict(name='Nathan Froyd', nick=['froydnj'], email=['nfroyd@mozilla.com']),
+]
+
+MISSING_REVIEW = """
+Changeset %s alters sync-messages.ini without IPC peer review.
+
+Please, request review from either:
+"""
+for p in IPC_PEERS:
+    MISSING_REVIEW += "  - {} (:{})\n".format(p['name'], p['nick'][0])
+
+
+class SyncIPCCheck(PreTxnChangegroupCheck):
+    """Changes to ipc/ipdl/sync-messages.ini requires IPC peer review."""
+    @property
+    def name(self):
+        return 'ipcsync_check'
+
+    def relevant(self):
+        return self.repo_metadata['firefox_releasing']
+
+    def pre(self, node):
+        # Accept the entire push for code uplifts
+        changesets = list(self.repo.changelog.revs(self.repo[node].rev()))
+        self.is_uplift = 'a=release' in self.repo.changectx(
+            changesets[-1]).description().lower()
+
+    def check(self, ctx):
+        if self.is_uplift:
+            return True
+
+        # Ignore merge changesets
+        if len(ctx.parents()) > 1:
+            return True
+
+        # Ignore backouts
+        if is_backout(ctx.description()):
+            return True
+
+        # Ignore changes that don't touch sync-messages.ini
+        ipc_files = [f for f in ctx.files()
+                     if f == 'ipc/ipdl/sync-messages.ini']
+        if not ipc_files:
+            return True
+
+        # Allow patches authored by peers
+        if self._is_peer_email(util.email(ctx.user())):
+            return True
+
+        # Allow if reviewed by any peer
+        requal = list(parse_requal_reviewers(ctx.description()))
+        if any(self._is_peer_nick(nick) for nick in requal):
+            return True
+
+        # Reject
+        print_banner(self.ui, 'error', MISSING_REVIEW % short(ctx.node()))
+        return False
+
+    def post_check(self):
+        return True
+
+    @staticmethod
+    def _is_peer_email(email):
+        email = email.lower()
+        for peer in IPC_PEERS:
+            if email in peer['email']:
+                return True
+        return False
+
+    @staticmethod
+    def _is_peer_nick(nick):
+        nick = nick.lower()
+        for peer in IPC_PEERS:
+            if nick in peer['nick']:
+                return True
+        return False
--- a/hghooks/mozhghooks/extension.py
+++ b/hghooks/mozhghooks/extension.py
@@ -46,29 +46,31 @@ def get_check_classes(hook):
     # TODO come up with a mechanism for automatically discovering checks
     # so we don't have to enumerate them all.
     from mozhghooks.check import (
         advertise_upgrade,
         prevent_cross_channel_messages,
         prevent_ftl_changes,
         prevent_subrepos,
         prevent_symlinks,
+        prevent_sync_ipc_changes,
         prevent_webidl_changes,
         prevent_wptsync_changes,
         single_root,
         try_task_config_file,
     )
 
     # TODO check to hook mapping should also be automatically discovered.
     if hook == 'pretxnchangegroup':
         return (
             prevent_cross_channel_messages.XChannelMessageCheck,
             prevent_ftl_changes.FTLCheck,
             prevent_subrepos.PreventSubReposCheck,
             prevent_symlinks.PreventSymlinksCheck,
+            prevent_sync_ipc_changes.SyncIPCCheck,
             prevent_webidl_changes.WebIDLCheck,
             prevent_wptsync_changes.WPTSyncCheck,
             single_root.SingleRootCheck,
             try_task_config_file.TryConfigCheck,
         )
 
     elif hook == 'changegroup':
         return (
deleted file mode 100755
--- a/hghooks/mozhghooks/prevent_webidl_changes.py
+++ /dev/null
@@ -1,127 +0,0 @@
-#!/usr/bin/env python
-# Copyright (C) 2012 Mozilla Foundation
-#
-# This program is free software; you can redistribute it and/or
-# modify it under the terms of the GNU General Public License
-# as published by the Free Software Foundation; either version 2
-# of the License, or (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program; if not, write to the Free Software
-# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
-"""
-This hook is to prevent changes to files with strict review requirements in pushes
-without proper peer review.
-"""
-
-import re
-from mercurial.node import short
-from mercurial import util
-
-backoutMessage = [re.compile(x) for x in [
-    r'^(back(ing|ed)?\s+out|backout)',
-    r'^(revert(ed|ing)?)'
-]]
-
-def isBackout(message):
-    for r in backoutMessage:
-        if r.search(message):
-            return True
-    return False
-
-def hook(ui, repo, hooktype, node, source=None, **kwargs):
-    if source in ('pull', 'strip'):
-        return 0
-
-    IPC_peers = [
-        'billm',             # Bill McCloskey
-        'dvander',           # David Anderson
-        'jld',               # Jed Davis
-        'kanru',             # Kan-Ru Chen
-        'bkelly',            # Ben Kelly
-        'froydnj',           # Nathan Froyd
-        'mccr8',             # Andrew McCreight
-    ]
-    IPC_authors = [
-        'billm@mozilla.com',       # Bill McCloskey,
-        'danderson@mozilla.com',   # David Anderson
-        'dvander@alliedmods.net',  # David Anderson
-        'jld@mozilla.com',         # Jed Davis
-        'kchen@mozilla.com',       # Kan-Ru Chen
-        'kanru@kanru.info',        # Kan-Ru Chen
-        'bkelly@mozilla.com',      # Ben Kelly
-        'ben@wanderview.com',      # Ben Kelly
-        'nfroyd@mozilla.com',      # Nathan Froyd
-        'continuation@gmail.com',  # Andrew McCreight
-    ]
-
-    error = ""
-    note = ""
-    changesets = list(repo.changelog.revs(repo[node].rev()))
-    if 'a=release' in repo.changectx(changesets[-1]).description().lower():
-        # Accept the entire push for code uplifts.
-        return 0
-    # Loop through each changeset being added to the repository
-    for i in reversed(changesets):
-        c = repo.changectx(i)
-
-        if len(c.parents()) > 1:
-            # Skip merge changesets
-            continue
-
-        syncIPCReviewed = None
-
-        # Loop through each file for the current changeset
-        for file in c.files():
-            if file.startswith('servo/'):
-                ui.write('(%s modifies %s from Servo; not enforcing peer '
-                         'review)\n' % (short(c.node()), file))
-                continue
-
-            message = c.description().lower()
-            email = util.email(c.user()).lower()
-
-            # Ignore backouts
-            if isBackout(message):
-                continue
-
-            def search(authors, peers):
-              matches = re.findall('\Ws?r\s*=\s*(\w+(?:,\w+)*)', message)
-              for match in matches:
-                  if any(reviewer in peers for reviewer in match.split(',')):
-                      return True
-              # We allow peers to commit changes without any review
-              # requirements assuming that they have looked at the changes
-              # they're committing.
-              if any(peer == email for peer in authors):
-                  return True
-
-              return False
-
-            # Only check the IPDL sync-messages.ini here.
-            if file.endswith('ipc/ipdl/sync-messages.ini'):
-                if syncIPCReviewed is None:
-                    syncIPCReviewed = search(IPC_authors, IPC_peers)
-                if not syncIPCReviewed:
-                    error += "sync-messages.ini altered in changeset %s without IPC peer review\n" % (short(c.node()))
-                    note = "\nChanges to sync-messages.ini in this repo require review from a IPC peer in the form of r=...\nThis is to ensure that we behave responsibly by not adding sync IPC messages that cause performance issues needlessly. We appreciate your understanding..\n"
-
-        if syncIPCReviewed:
-            print ("You've received proper review from an IPC peer on the "
-                   "sync-messages.ini change(s) in commit %s, thanks for "
-                   "paying enough attention." % short(c.node()))
-    # Check if an error occured in any of the files that were changed
-    if error != "":
-        print "\n\n************************** ERROR ****************************"
-        ui.warn("\n" + error + "\n")
-        print note
-        print "*************************************************************\n\n"
-        # Reject the changesets
-        return 1
-    # Accept the changesets
-    return 0
--- a/hghooks/tests/test-prevent-sync-ipc-changes.t
+++ b/hghooks/tests/test-prevent-sync-ipc-changes.t
@@ -1,16 +1,14 @@
   $ . $TESTDIR/hghooks/tests/common.sh
 
   $ hg init server
+  $ configurehooks server
+  $ touch server/.hg/IS_FIREFOX_REPO
   $ cd server
-  $ cat >> .hg/hgrc << EOF
-  > [hooks]
-  > pretxnchangegroup.prevent_webidl = python:mozhghooks.prevent_webidl_changes.hook
-  > EOF
 
   $ echo "foo" > dummy
   $ hg commit -A -m 'original repo commit; r=baku'
   adding dummy
 
   $ cd ..
   $ hg clone server client
   updating to branch default
@@ -32,64 +30,65 @@ Editing the sync-messages.ini file witho
   $ hg push
   pushing to $TESTTMP/server
   searching for changes
   adding changesets
   adding manifests
   adding file changes
   added 1 changesets with 1 changes to 1 files
   
-  
-  ************************** ERROR ****************************
-  
-  sync-messages.ini altered in changeset 8fb3e82ba334 without IPC peer review
-  
+  ******************************** ERROR *********************************
+  Changeset 8fb3e82ba334 alters sync-messages.ini without IPC peer review.
   
-  Changes to sync-messages.ini in this repo require review from a IPC peer in the form of r=...
-  This is to ensure that we behave responsibly by not adding sync IPC messages that cause performance issues needlessly. We appreciate your understanding..
-  
-  *************************************************************
-  
+  Please, request review from either:
+    - Andrew McCreight (:mccr8)
+    - Ben Kelly (:bkelly)
+    - Bill McCloskey (:billm)
+    - David Anderson (:dvander)
+    - Jed David (:jld)
+    - Kan-Ru Chen (:kanru)
+    - Nathan Froyd (:froydnj)
+  ************************************************************************
   
   transaction abort!
   rollback completed
-  abort: pretxnchangegroup.prevent_webidl hook failed
+  abort: pretxnchangegroup.mozhooks hook failed
   [255]
 
 Editing the sync-messages.ini file without /IPC/ peer review should fail
 
   $ hg -q commit --amend -m 'Bug 123 - Add Bar; r=foobar'
   $ hg push
   pushing to $TESTTMP/server
   searching for changes
   adding changesets
   adding manifests
   adding file changes
   added 1 changesets with 1 changes to 1 files
   
-  
-  ************************** ERROR ****************************
-  
-  sync-messages.ini altered in changeset d970a5c85d15 without IPC peer review
-  
+  ******************************** ERROR *********************************
+  Changeset d970a5c85d15 alters sync-messages.ini without IPC peer review.
   
-  Changes to sync-messages.ini in this repo require review from a IPC peer in the form of r=...
-  This is to ensure that we behave responsibly by not adding sync IPC messages that cause performance issues needlessly. We appreciate your understanding..
-  
-  *************************************************************
-  
+  Please, request review from either:
+    - Andrew McCreight (:mccr8)
+    - Ben Kelly (:bkelly)
+    - Bill McCloskey (:billm)
+    - David Anderson (:dvander)
+    - Jed David (:jld)
+    - Kan-Ru Chen (:kanru)
+    - Nathan Froyd (:froydnj)
+  ************************************************************************
   
   transaction abort!
   rollback completed
-  abort: pretxnchangegroup.prevent_webidl hook failed
+  abort: pretxnchangegroup.mozhooks hook failed
   [255]
 
 Editing the sync-messages.ini file with /IPC/ peer review should pass
 
   $ hg -q commit --amend -m 'Bug 123 - Add Bar; r=billm'
   $ hg push
   pushing to $TESTTMP/server
   searching for changes
   adding changesets
   adding manifests
   adding file changes
   added 1 changesets with 1 changes to 1 files
-  You've received proper review from an IPC peer on the sync-messages.ini change(s) in commit 62716423067e, thanks for paying enough attention.