reviewboard: register config options (bug 1413628); r?glob draft
authorGregory Szorc <gps@mozilla.com>
Thu, 02 Nov 2017 10:38:53 -0700
changeset 11783 6249ecf9a76218415159c29f2293bea6f1aa3337
parent 11782 2d983d11f429f13bd78eedab1df91ed5cead68ce
child 11784 bc9e848e5282d511eeec1c35a30a0a8961837271
push id1816
push usergszorc@mozilla.com
push dateThu, 02 Nov 2017 19:46:54 +0000
reviewersglob
bugs1413628
reviewboard: register config options (bug 1413628); r?glob Mercurial 4.4 softly requires config options be registered via a central registrar. If they aren't, a devel warning is issued. The test runner enables all devel warnings by default (which is a good idea). This means we get new output and test failures for attempting to access a config option that isn't registered. When options are registered, Mercurial also issues a devel warning when the caller accessing the option specifies a default value. This is because the default value is defined as part of the option and doesn't need to be specified at the call site. Porting an extension to this mechanism is a bit annoying because old Mercurial versions need to specify a default value (since the option isn't registered) and new Mercurial versions would issue a warning if a default argument is specified. The workaround is to set the default value of "dynamicdefault." This special value means "the default is dynamic" and ui.config* functions won't issue a devel warning if a default value is specified. When support for older versions of Mercurial are dropped, the proper default value can be registered and the "default" argument can be dropped from ui.config* callers. This commit registers all config options used by the reviewboard extension. By doing so, most test failures in 4.4 due to new "devel-warn:" lines go away. This leaves a handful of legitimate test failures. MozReview-Commit-ID: KjS9MRuc2KD
hgext/reviewboard/client.py
hgext/reviewboard/hgrb/util.py
hgext/reviewboard/server.py
pylib/mozhg/mozhg/auth.py
--- a/hgext/reviewboard/client.py
+++ b/hgext/reviewboard/client.py
@@ -44,16 +44,23 @@ from mercurial import (
     obsolete,
     phases,
     registrar,
     scmutil,
     sshpeer,
     templatekw,
     util,
 )
+
+# TRACKING hg43
+try:
+    from mercurial import configitems
+except ImportError:
+    configitems = None
+
 from mercurial.i18n import _
 from mercurial.node import bin, hex
 
 OUR_DIR = os.path.normpath(os.path.dirname(__file__))
 execfile(os.path.join(OUR_DIR, '..', 'bootstrap.py'))
 
 with demandimport.deactivated():
     try:
@@ -71,16 +78,17 @@ from hgrb.util import (
 
 from mozautomation.commitparser import (
     parse_bugs,
     parse_commit_id,
 )
 from mozhg.auth import (
     getbugzillaauth,
     configureautobmoapikeyauth,
+    register_config_items,
 )
 from mozhg.rewrite import (
     newparents,
     replacechangesets,
 )
 
 testedwith = '4.1 4.2 4.3'
 minimumhgversion = '4.1'
@@ -90,16 +98,38 @@ cmdtable = {}
 
 # Mercurial 4.3 introduced registrar.command as a replacement for
 # cmdutil.command.
 if util.safehasattr(registrar, 'command'):
     command = registrar.command(cmdtable)
 else:
     command = cmdutil.command(cmdtable)
 
+# Mercurial 4.3 introduced the config registrar. 4.4 requires config
+# items to be registered to avoid a devel warning.
+if util.safehasattr(registrar, 'configitem'):
+    # Imports get evaluated in block scope. So the if above has no impact.
+
+
+    configtable = {}
+    configitem = registrar.configitem(configtable)
+
+    # Register bugzilla.* options via shared auth module.
+    register_config_items(configitem)
+
+    configitem('mozilla', 'ircnick',
+               default=configitems.dynamicdefault)
+    configitem('reviewboard', 'autopublish',
+               default=configitems.dynamicdefault)
+    configitem('reviewboard', 'deduce-reviewers',
+               default=configitems.dynamicdefault)
+    configitem('reviewboard', 'fakeids',
+               default=configitems.dynamicdefault)
+
+
 clientcapabilities = {
     'proto1',
     'listreviewdata',
     'listreviewrepos',
     'bzapikeys',
     'jsonproto',
     'commitid',
 }
@@ -681,17 +711,17 @@ def doreview(repo, ui, remote, nodes):
         # reviewboard.autopublish, prompt the user. Otherwise, publish
         # automatically or not based on this value.
         if ui.config('reviewboard', 'autopublish', None) is None:
             ui.write('\n')
             publish = ui.promptchoice(_('publish these review '
                                         'requests now (Yn)? '
                                         '$$ &Yes $$ &No')) == 0
         else:
-            publish = ui.configbool('reviewboard', 'autopublish')
+            publish = ui.configbool('reviewboard', 'autopublish', False)
 
         if publish:
             publishreviewrequests(ui, remote, bzauth, [newparentid])
         else:
             ui.status(_('(visit review url to publish these review '
                         'requests so others can see them)\n'))
 
 
--- a/hgext/reviewboard/hgrb/util.py
+++ b/hgext/reviewboard/hgrb/util.py
@@ -83,17 +83,17 @@ def genid(repo=None, fakeidpath=None):
     Base62 is used as the encoding mechanism because it is safe for both
     URLs and revsets. We could get base66 for URLs, but the characters
     -~. could conflict with revsets.
     """
     # Provide a backdoor to generate deterministic IDs. This is used for
     # testing purposes because tests want constant output. And since
     # commit IDs go into the commit and are part of the SHA-1, they need
     # to be deterministic.
-    if repo and repo.ui.configbool('reviewboard', 'fakeids'):
+    if repo and repo.ui.configbool('reviewboard', 'fakeids', False):
         fakeidpath = repo.vfs.join('genid')
 
     if fakeidpath:
         try:
             with open(fakeidpath, 'rb') as fh:
                 data = fh.read()
         except IOError as e:
             if e.errno != errno.ENOENT:
--- a/hgext/reviewboard/server.py
+++ b/hgext/reviewboard/server.py
@@ -27,16 +27,23 @@ from mercurial import (
     cmdutil,
     demandimport,
     extensions,
     hg,
     registrar,
     util,
     wireproto,
 )
+
+# TRACKING hg43
+try:
+    from mercurial import configitems
+except ImportError:
+    configitems = None
+
 from mercurial.i18n import _
 from mercurial.node import (
     hex,
     nullid,
 )
 from mercurial.hgweb import (
     webcommands,
 )
@@ -61,23 +68,36 @@ cmdtable = {}
 
 # Mercurial 4.3 introduced registrar.command as a replacement for
 # cmdutil.command.
 if util.safehasattr(registrar, 'command'):
     command = registrar.command(cmdtable)
 else:
     command = cmdutil.command(cmdtable)
 
-# Mercurial 4.3 started defining config items using a central registrar.
-# Mercurial 4.4 finished this transition.
-try:
-    from mercurial import configitems
-    have_config_registrar = True
-except ImportError:
-    have_config_registrar = False
+# Mercurial 4.4 requires config items to be registered.
+if util.safehasattr(registrar, 'configitem'):
+    configtable = {}
+    configitem = registrar.configitem(configtable)
+
+    configitem('bugzilla', 'url',
+               default=configitems.dynamicdefault)
+    configitem('reviewboard', 'isdiscoveryrepo',
+               default=configitems.dynamicdefault)
+    configitem('reviewboard', 'password',
+               default=configitems.dynamicdefault)
+    configitem('reviewboard', 'repobasepath',
+               default=configitems.dynamicdefault)
+    configitem('reviewboard', 'repoid',
+               default=configitems.dynamicdefault)
+    configitem('reviewboard', 'url',
+               default=configitems.dynamicdefault)
+    configitem('reviewboard', 'username',
+               default=configitems.dynamicdefault)
+
 
 # Capabilities the server requires in clients.
 #
 # Add to this and add a corresponding entry in the client extension to force
 # the client to pull latest code.
 requirecaps = set([
     # Client can speak protocol format 1.
     'proto1',
@@ -214,17 +234,17 @@ def wrappedwireprotoheads(orig, repo, pr
 
     1) A client will create a bundle before sending it to the server. This will
        take minutes on mozilla-central.
     2) Bundle2 aware HTTP clients don't like receiving a "pusherr" response from
        unbundle - they insist on receiving a bundle2 payload. However, the
        server doesn't know if the client supports bundle2 until it reads the
        header from the bundle submission!
     """
-    if not repo.ui.configbool('reviewboard', 'isdiscoveryrepo'):
+    if not repo.ui.configbool('reviewboard', 'isdiscoveryrepo', False):
         return orig(repo, proto, *args, **kwargs)
 
     return wireproto.ooberror(nopushdiscoveryrepos)
 
 
 def sendjsonresponse(req, obj):
     req.respond(HTTP_OK, 'application/json')
     return [json.dumps(obj, indent=2, sort_keys=True, encoding='utf-8')]
@@ -379,17 +399,17 @@ def reposetup(ui, repo):
         basepath = ui.config('reviewboard', 'repobasepath', None)
         if basepath and repo.root.startswith(basepath):
             active = True
 
     if not active:
         return
 
     if (not ui.configint('reviewboard', 'repoid', None) and
-            not ui.configbool('reviewboard', 'isdiscoveryrepo')):
+            not ui.configbool('reviewboard', 'isdiscoveryrepo', False)):
         raise util.Abort(_('Please set reviewboard.repoid to the numeric ID '
             'of the repository this repo is associated with.'))
 
     if not ui.config('reviewboard', 'url', None):
         raise util.Abort(_('Please set reviewboard.url to the URL of the '
             'Review Board instance to talk to.'))
 
     if not ui.config('reviewboard', 'username', None):
@@ -400,25 +420,25 @@ def reposetup(ui, repo):
         raise util.Abort(_('Please set reviewboard.password to the password '
             'for priveleged communications with Review Board.'))
 
     if not ui.config('bugzilla', 'url', None):
         raise util.Abort(_('Please set bugzilla.url to the URL of the '
             'Bugzilla instance to talk to.'))
 
     # TRACKING hg33+
-    if have_config_registrar:
+    if configitems:
         publish = ui.configbool('phases', 'publish')
     else:
         publish = ui.configbool('phases', 'publish', True)
 
     if publish:
         raise util.Abort(_('reviewboard server extension is only compatible '
             'with non-publishing repositories.'))
 
     ui.setconfig('hooks', 'changegroup.reviewboard', changegrouphook)
 
     # This shouldn't be needed to prevent pushes, as the "heads" wireproto
     # wrapping should kill them. However, this is defense in depth and will
     # kill all changegroup additions, even if the server operator is dumb.
-    if ui.configbool('reviewboard', 'isdiscoveryrepo'):
+    if ui.configbool('reviewboard', 'isdiscoveryrepo', False):
         ui.setconfig('hooks', 'pretxnchangegroup.disallowpush',
                      disallowpushhook)
--- a/pylib/mozhg/mozhg/auth.py
+++ b/pylib/mozhg/mozhg/auth.py
@@ -8,16 +8,41 @@ import platform
 import shutil
 import tempfile
 import urlparse
 
 from mercurial import config, util
 from mercurial.i18n import _
 
 
+def register_config_items(configitem):
+    """Registers config items with Mercurial's registrar.
+
+    The argument is a ``registrar.configitem`` instance.
+    """
+    from mercurial import configitems
+
+    configitem('bugzilla', 'username',
+               default=configitems.dynamicdefault)
+    configitem('bugzilla', 'apikey',
+               default=configitems.dynamicdefault)
+    configitem('bugzilla', 'password',
+               default=configitems.dynamicdefault)
+    configitem('bugzilla', 'userid',
+               default=configitems.dynamicdefault)
+    configitem('bugzilla', 'cookie',
+               default=configitems.dynamicdefault)
+    configitem('bugzilla', 'firefoxprofile',
+               default=configitems.dynamicdefault)
+    configitem('bugzilla', 'url',
+               default=configitems.dynamicdefault)
+    configitem('mozilla', 'trustedbmoapikeyservices',
+               default=configitems.dynamicdefault)
+
+
 class BugzillaAuth(object):
     """Holds Bugzilla authentication credentials."""
 
     def __init__(self, userid=None, cookie=None, username=None, password=None,
                  apikey=None):
         if apikey:
             self._type = 'apikey'
         elif userid:
@@ -51,21 +76,21 @@ def getbugzillaauth(ui, require=False, p
       4) login cookies from Firefox profiles
       5) prompt the user
 
     The ``bugzilla.firefoxprofile`` option is interpreted as a list of Firefox
     profiles from which data should be read. This overrides the default sort
     order.
     """
 
-    username = ui.config('bugzilla', 'username')
-    apikey = ui.config('bugzilla', 'apikey')
-    password = ui.config('bugzilla', 'password')
-    userid = ui.config('bugzilla', 'userid')
-    cookie = ui.config('bugzilla', 'cookie')
+    username = ui.config('bugzilla', 'username', None)
+    apikey = ui.config('bugzilla', 'apikey', None)
+    password = ui.config('bugzilla', 'password', None)
+    userid = ui.config('bugzilla', 'userid', None)
+    cookie = ui.config('bugzilla', 'cookie', None)
     profileorder = ui.configlist('bugzilla', 'firefoxprofile', [])
 
     if username and apikey:
         return BugzillaAuth(username=username, apikey=apikey)
 
     if userid and cookie:
         return BugzillaAuth(userid=userid, cookie=cookie)
 
@@ -293,18 +318,18 @@ def configureautobmoapikeyauth(ui):
     carries over the [bugzilla] entries to [auth] entries for trusted services.
     """
     services = ui.configlist('mozilla', 'trustedbmoapikeyservices',
                              TRUSTEDAPIKEYSERVICES)
     if not services:
         ui.debug('no trusted services to auto define credentials on\n')
         return
 
-    username = ui.config('bugzilla', 'username')
-    apikey = ui.config('bugzilla', 'apikey')
+    username = ui.config('bugzilla', 'username', None)
+    apikey = ui.config('bugzilla', 'apikey', None)
     if not username:
         ui.debug('Bugzilla username not defined; cannot define credentials\n')
         return
 
     for i, service in enumerate(services):
         ui.debug('automatically setting Bugzilla API Key auth %s\n' % service)
         key = 'autobmoapikey%d' % i
         ui.setconfig('auth', '%s.prefix' % key, service)