Patchwork [5,of,5] commands: always register special command properties in registrar (API)

login
register
mail settings
Submitter Gregory Szorc
Date Jan. 2, 2016, 7:30 p.m.
Message ID <3ed88f67769061aec1a6.1451763054@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/12482/
State Rejected
Delegated to: Yuya Nishihara
Headers show

Comments

Gregory Szorc - Jan. 2, 2016, 7:30 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1451761491 28800
#      Sat Jan 02 11:04:51 2016 -0800
# Node ID 3ed88f67769061aec1a6de827f739c3f65463952
# Parent  f77ea53bf73191d7fa373dddb0fbd03f3cb61b12
commands: always register special command properties in registrar (API)

The previous patch to start the refactor of this mechanism
intentionally did not modify dispatch.py. This patch starts the process
of teaching dispatch.py to be registrar aware.

We still end up setting values in commands.py. But, we no longer do this
from @command and commands.py no longer needs to know about registrar.

This is marked as API incompatible because extensions examining
commands.{norepo,optionalrepo,inferrepo} will no longer work since this
variable is no longer populated at extension load time. This is
demonstrated by the simple change to mq.py.
Yuya Nishihara - Jan. 3, 2016, 1:51 a.m.
On Sat, 02 Jan 2016 11:30:54 -0800, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1451761491 28800
> #      Sat Jan 02 11:04:51 2016 -0800
> # Node ID 3ed88f67769061aec1a6de827f739c3f65463952
> # Parent  f77ea53bf73191d7fa373dddb0fbd03f3cb61b12
> commands: always register special command properties in registrar (API)
> 
> The previous patch to start the refactor of this mechanism
> intentionally did not modify dispatch.py. This patch starts the process
> of teaching dispatch.py to be registrar aware.
> 
> We still end up setting values in commands.py. But, we no longer do this
> from @command and commands.py no longer needs to know about registrar.

Uh, I have experimental patches that store command properties in function
object.

https://bitbucket.org/yuja/hg-work/commits/7012d4af63af

Pros:

 - no circular import
 - no change to global variables at _checkshellalias()

Cons:

 - these attributes must be copied to a wrapper function

> This is marked as API incompatible because extensions examining
> commands.{norepo,optionalrepo,inferrepo} will no longer work since this
> variable is no longer populated at extension load time. This is
> demonstrated by the simple change to mq.py.

If we can drop commands.{norepo,optionalrepo,inferrepo}, API compatibility
issues will be more discoverable.
Gregory Szorc - Jan. 3, 2016, 2:11 a.m.
On Sat, Jan 2, 2016 at 5:51 PM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Sat, 02 Jan 2016 11:30:54 -0800, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1451761491 28800
> > #      Sat Jan 02 11:04:51 2016 -0800
> > # Node ID 3ed88f67769061aec1a6de827f739c3f65463952
> > # Parent  f77ea53bf73191d7fa373dddb0fbd03f3cb61b12
> > commands: always register special command properties in registrar (API)
> >
> > The previous patch to start the refactor of this mechanism
> > intentionally did not modify dispatch.py. This patch starts the process
> > of teaching dispatch.py to be registrar aware.
> >
> > We still end up setting values in commands.py. But, we no longer do this
> > from @command and commands.py no longer needs to know about registrar.
>
> Uh, I have experimental patches that store command properties in function
> object.
>
> https://bitbucket.org/yuja/hg-work/commits/7012d4af63af
>
> Pros:
>
>  - no circular import
>  - no change to global variables at _checkshellalias()
>
> Cons:
>
>  - these attributes must be copied to a wrapper function
>

Storing these properties in the function object is a much better approach.

The impetus behind these patches was getting cmdutil and commands converted
to absolute_import so the Python 3 porting could be unblocked. I concede
that porting commands.{norepo,optionalrepo,inferrepo} is sub-optimal.

The Python 3 work is low priority. If you want to convert these to function
object attributes, I'm fine with dropping this series and picking up
whenever your changes land.


>
> > This is marked as API incompatible because extensions examining
> > commands.{norepo,optionalrepo,inferrepo} will no longer work since this
> > variable is no longer populated at extension load time. This is
> > demonstrated by the simple change to mq.py.
>
> If we can drop commands.{norepo,optionalrepo,inferrepo}, API compatibility
> issues will be more discoverable.
>

I'd like to drop them too. But I think it may have significant implications
on legacy extensions. Perhaps this is a mpm decision?
Yuya Nishihara - Jan. 3, 2016, 3:17 p.m.
On Sat, 2 Jan 2016 18:11:07 -0800, Gregory Szorc wrote:
> On Sat, Jan 2, 2016 at 5:51 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Sat, 02 Jan 2016 11:30:54 -0800, Gregory Szorc wrote:
> > > # HG changeset patch
> > > # User Gregory Szorc <gregory.szorc@gmail.com>
> > > # Date 1451761491 28800
> > > #      Sat Jan 02 11:04:51 2016 -0800
> > > # Node ID 3ed88f67769061aec1a6de827f739c3f65463952
> > > # Parent  f77ea53bf73191d7fa373dddb0fbd03f3cb61b12
> > > commands: always register special command properties in registrar (API)
> > >
> > > The previous patch to start the refactor of this mechanism
> > > intentionally did not modify dispatch.py. This patch starts the process
> > > of teaching dispatch.py to be registrar aware.
> > >
> > > We still end up setting values in commands.py. But, we no longer do this
> > > from @command and commands.py no longer needs to know about registrar.
> >
> > Uh, I have experimental patches that store command properties in function
> > object.
> >
> > https://bitbucket.org/yuja/hg-work/commits/7012d4af63af
> >
> > Pros:
> >
> >  - no circular import
> >  - no change to global variables at _checkshellalias()
> >
> > Cons:
> >
> >  - these attributes must be copied to a wrapper function
> >
> 
> Storing these properties in the function object is a much better approach.
> 
> The impetus behind these patches was getting cmdutil and commands converted
> to absolute_import so the Python 3 porting could be unblocked. I concede
> that porting commands.{norepo,optionalrepo,inferrepo} is sub-optimal.
> 
> The Python 3 work is low priority. If you want to convert these to function
> object attributes, I'm fine with dropping this series and picking up
> whenever your changes land.

Thanks, I'll finish my series and patchbomb it soon.

> > > This is marked as API incompatible because extensions examining
> > > commands.{norepo,optionalrepo,inferrepo} will no longer work since this
> > > variable is no longer populated at extension load time. This is
> > > demonstrated by the simple change to mq.py.
> >
> > If we can drop commands.{norepo,optionalrepo,inferrepo}, API compatibility
> > issues will be more discoverable.
> >
> 
> I'd like to drop them too. But I think it may have significant implications
> on legacy extensions. Perhaps this is a mpm decision?

CC-ing mpm. :-)
Matt Mackall - Jan. 13, 2016, 8:35 p.m.
On Mon, 2016-01-04 at 00:17 +0900, Yuya Nishihara wrote:
> On Sat, 2 Jan 2016 18:11:07 -0800, Gregory Szorc wrote:
> > On Sat, Jan 2, 2016 at 5:51 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> > > On Sat, 02 Jan 2016 11:30:54 -0800, Gregory Szorc wrote:
> > > > # HG changeset patch
> > > > # User Gregory Szorc <gregory.szorc@gmail.com>
> > > > # Date 1451761491 28800
> > > > #      Sat Jan 02 11:04:51 2016 -0800
> > > > # Node ID 3ed88f67769061aec1a6de827f739c3f65463952
> > > > # Parent  f77ea53bf73191d7fa373dddb0fbd03f3cb61b12
> > > > commands: always register special command properties in registrar (API)
> > > > 
> > > > The previous patch to start the refactor of this mechanism
> > > > intentionally did not modify dispatch.py. This patch starts the process
> > > > of teaching dispatch.py to be registrar aware.
> > > > 
> > > > We still end up setting values in commands.py. But, we no longer do this
> > > > from @command and commands.py no longer needs to know about registrar.
> > > 
> > > Uh, I have experimental patches that store command properties in function
> > > object.
> > > 
> > > https://bitbucket.org/yuja/hg-work/commits/7012d4af63af
> > > 
> > > Pros:
> > > 
> > >  - no circular import
> > >  - no change to global variables at _checkshellalias()
> > > 
> > > Cons:
> > > 
> > >  - these attributes must be copied to a wrapper function
> > > 
> > 
> > Storing these properties in the function object is a much better approach.
> > 
> > The impetus behind these patches was getting cmdutil and commands converted
> > to absolute_import so the Python 3 porting could be unblocked. I concede
> > that porting commands.{norepo,optionalrepo,inferrepo} is sub-optimal.
> > 
> > The Python 3 work is low priority. If you want to convert these to function
> > object attributes, I'm fine with dropping this series and picking up
> > whenever your changes land.
> 
> Thanks, I'll finish my series and patchbomb it soon.
> 
> > > > This is marked as API incompatible because extensions examining
> > > > commands.{norepo,optionalrepo,inferrepo} will no longer work since this
> > > > variable is no longer populated at extension load time. This is
> > > > demonstrated by the simple change to mq.py.
> > > 
> > > If we can drop commands.{norepo,optionalrepo,inferrepo}, API compatibility
> > > issues will be more discoverable.
> > > 
> > 
> > I'd like to drop them too. But I think it may have significant implications
> > on legacy extensions. Perhaps this is a mpm decision?
> 
> CC-ing mpm. :-)

I love to break extensions, but perhaps we shouldn't do it so wholesale so close
to the freeze.
Yuya Nishihara - Jan. 14, 2016, 2:36 p.m.
On Wed, 13 Jan 2016 14:35:36 -0600, Matt Mackall wrote:
> On Mon, 2016-01-04 at 00:17 +0900, Yuya Nishihara wrote:
> > On Sat, 2 Jan 2016 18:11:07 -0800, Gregory Szorc wrote:
> > > On Sat, Jan 2, 2016 at 5:51 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> > > > > This is marked as API incompatible because extensions examining
> > > > > commands.{norepo,optionalrepo,inferrepo} will no longer work since this
> > > > > variable is no longer populated at extension load time. This is
> > > > > demonstrated by the simple change to mq.py.
> > > > 
> > > > If we can drop commands.{norepo,optionalrepo,inferrepo}, API compatibility
> > > > issues will be more discoverable.
> > > > 
> > > 
> > > I'd like to drop them too. But I think it may have significant implications
> > > on legacy extensions. Perhaps this is a mpm decision?
> > 
> > CC-ing mpm. :-)
> 
> I love to break extensions, but perhaps we shouldn't do it so wholesale so close
> to the freeze.

Ok, I'll patchbomb it next month. Thanks.
Gregory Szorc - Feb. 28, 2016, 6:06 a.m.
On Thu, Jan 14, 2016 at 6:36 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Wed, 13 Jan 2016 14:35:36 -0600, Matt Mackall wrote:
> > On Mon, 2016-01-04 at 00:17 +0900, Yuya Nishihara wrote:
> > > On Sat, 2 Jan 2016 18:11:07 -0800, Gregory Szorc wrote:
> > > > On Sat, Jan 2, 2016 at 5:51 PM, Yuya Nishihara <yuya@tcha.org>
> wrote:
> > > > > > This is marked as API incompatible because extensions examining
> > > > > > commands.{norepo,optionalrepo,inferrepo} will no longer work
> since this
> > > > > > variable is no longer populated at extension load time. This is
> > > > > > demonstrated by the simple change to mq.py.
> > > > >
> > > > > If we can drop commands.{norepo,optionalrepo,inferrepo}, API
> compatibility
> > > > > issues will be more discoverable.
> > > > >
> > > >
> > > > I'd like to drop them too. But I think it may have significant
> implications
> > > > on legacy extensions. Perhaps this is a mpm decision?
> > >
> > > CC-ing mpm. :-)
> >
> > I love to break extensions, but perhaps we shouldn't do it so wholesale
> so close
> > to the freeze.
>
> Ok, I'll patchbomb it next month. Thanks.
>

Yuya: What is the state of your patches? I was looking at Python 3
compatibility again today and commands.py and cmdutil.py not using
absolute_import is blocking some porting work, which a number of people
have expressed interest in working on. I think addressing command
registration is beyond the skills of a new contributor. So I'd like to get
these 2 modules sorted out so people can start hacking on the long tail of
Python 3 work in mercurial.*.
Yuya Nishihara - Feb. 28, 2016, 7:05 a.m.
On Sat, 27 Feb 2016 22:06:14 -0800, Gregory Szorc wrote:
> On Thu, Jan 14, 2016 at 6:36 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Wed, 13 Jan 2016 14:35:36 -0600, Matt Mackall wrote:
> > > On Mon, 2016-01-04 at 00:17 +0900, Yuya Nishihara wrote:
> > > > On Sat, 2 Jan 2016 18:11:07 -0800, Gregory Szorc wrote:
> > > > > On Sat, Jan 2, 2016 at 5:51 PM, Yuya Nishihara <yuya@tcha.org>
> > wrote:
> > > > > > > This is marked as API incompatible because extensions examining
> > > > > > > commands.{norepo,optionalrepo,inferrepo} will no longer work
> > since this
> > > > > > > variable is no longer populated at extension load time. This is
> > > > > > > demonstrated by the simple change to mq.py.
> > > > > >
> > > > > > If we can drop commands.{norepo,optionalrepo,inferrepo}, API
> > compatibility
> > > > > > issues will be more discoverable.
> > > > > >
> > > > >
> > > > > I'd like to drop them too. But I think it may have significant
> > implications
> > > > > on legacy extensions. Perhaps this is a mpm decision?
> > > >
> > > > CC-ing mpm. :-)
> > >
> > > I love to break extensions, but perhaps we shouldn't do it so wholesale
> > so close
> > > to the freeze.
> >
> > Ok, I'll patchbomb it next month. Thanks.
> 
> Yuya: What is the state of your patches? I was looking at Python 3
> compatibility again today and commands.py and cmdutil.py not using
> absolute_import is blocking some porting work, which a number of people
> have expressed interest in working on. I think addressing command
> registration is beyond the skills of a new contributor. So I'd like to get
> these 2 modules sorted out so people can start hacking on the long tail of
> Python 3 work in mercurial.*.

Flushed. Sorry, they were pushed near the bottom of my unsent stack as they
weren't a blocker of my work. :-)

Patch

diff --git a/hgext/mq.py b/hgext/mq.py
--- a/hgext/mq.py
+++ b/hgext/mq.py
@@ -65,16 +65,17 @@  in the strip extension.
 from mercurial.i18n import _
 from mercurial.node import bin, hex, short, nullid, nullrev
 from mercurial.lock import release
 from mercurial import commands, cmdutil, hg, scmutil, util, revset
 from mercurial import extensions, error, phases
 from mercurial import patch as patchmod
 from mercurial import lock as lockmod
 from mercurial import localrepo
+from mercurial import registrar
 from mercurial import subrepo
 import os, re, errno, shutil
 
 seriesopts = [('s', 'summary', None, _('print first line of patch header'))]
 
 cmdtable = {}
 command = cmdutil.command(cmdtable)
 # Note for extension authors: ONLY specify testedwith = 'internal' for
@@ -3577,22 +3578,20 @@  def extsetup(ui):
     mqopt = [('', 'mq', None, _("operate on patch repository"))]
 
     extensions.wrapcommand(commands.table, 'import', mqimport)
     cmdutil.summaryhooks.add('mq', summaryhook)
 
     entry = extensions.wrapcommand(commands.table, 'init', mqinit)
     entry[1].extend(mqopt)
 
-    nowrap = set(commands.norepo.split(" "))
-
     def dotable(cmdtable):
         for cmd in cmdtable.keys():
             cmd = cmdutil.parsealiases(cmd)[0]
-            if cmd in nowrap:
+            if cmd in registrar.commands['norepo']:
                 continue
             entry = extensions.wrapcommand(cmdtable, cmd, mqcommand)
             entry[1].extend(mqopt)
 
     dotable(commands.table)
 
     for extname, extmodule in extensions.extensions():
         if extmodule.__file__ != __file__:
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -3279,21 +3279,16 @@  def _performrevert(repo, parents, ctx, a
         normal(f)
 
     copied = copies.pathcopies(repo[parent], ctx)
 
     for f in actions['add'][0] + actions['undelete'][0] + actions['revert'][0]:
         if f in copied:
             repo.dirstate.copy(copied[f], f)
 
-# Whether commands should be registered with the registrar module.
-# TODO remove this once dispatch and extension loading are aware of
-# the registrar.
-commanduseregistrar = True
-
 def command(table):
     """Returns a function object to be used as a decorator for making commands.
 
     This function receives a command table as its argument. The table should
     be a dict.
 
     The returned function can be used as a decorator for adding commands
     to that command table. This function accepts multiple arguments to define
@@ -3323,36 +3318,26 @@  def command(table):
             inferrepo=False):
         def decorator(func):
             if synopsis:
                 table[name] = func, list(options), synopsis
             else:
                 table[name] = func, list(options)
 
             if norepo or optionalrepo or inferrepo:
-                aliases = parsealiases(name)
-                from . import commands
+                aliases = set(parsealiases(name))
 
             if norepo:
-                if commanduseregistrar:
-                    registrar.commands['norepo'] |= set(aliases)
-                else:
-                    commands.norepo += ' %s' % ' '.join(aliases)
+                registrar.commands['norepo'] |= aliases
 
             if optionalrepo:
-                if commanduseregistrar:
-                    registrar.commands['optionalrepo'] |= set(aliases)
-                else:
-                    commands.optionalrepo += ' %s' % ' '.join(aliases)
+                registrar.commands['optionalrepo'] |= aliases
 
             if inferrepo:
-                if commanduseregistrar:
-                    registrar.commands['inferrepo'] |= set(aliases)
-                else:
-                    commands.inferrepo += ' %s' % ' '.join(aliases)
+                registrar.commands['inferrepo'] |= aliases
 
             return func
         return decorator
 
     return cmd
 
 # a list of (ui, repo, otherpeer, opts, missing) functions called by
 # commands.outgoing.  "missing" is "missing" of the result of
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -54,17 +54,16 @@  from . import (
     localrepo,
     lock as lockmod,
     merge as mergemod,
     minirst,
     obsolete,
     patch,
     phases,
     pvec,
-    registrar,
     repair,
     revlog,
     revset,
     scmutil,
     setdiscovery,
     simplemerge,
     sshserver,
     streamclone,
@@ -7046,28 +7045,21 @@  def version_(ui):
         for name, module in extensions.extensions():
             names.append(name)
             vers.append(extensions.moduleversion(module))
         if names:
             maxnamelen = max(len(n) for n in names)
             for i, name in enumerate(names):
                 ui.write("  %-*s  %s\n" % (maxnamelen, name, vers[i]))
 
-# TODO make registrar source of truth and pull values from these variables into
-# registrar.
-
 # Space delimited list of commands that don't require local repositories.
 # This should be populated by passing norepo=True into the @command decorator.
-norepo = ' '.join(registrar.commands['norepo'])
+norepo = ''
 
 # Space delimited list of commands that optionally require local repositories.
 # This should be populated by passing optionalrepo=True into the @command
 # decorator.
-optionalrepo = ' '.join(registrar.commands['optionalrepo'])
+optionalrepo = ''
 
 # Space delimited list of commands that will examine arguments looking for
 # a repository. This should be populated by passing inferrepo=True into the
 # @command decorator.
-inferrepo = ' '.join(registrar.commands['inferrepo'])
-
-# Dispatch isn't yet aware of the registrar. So have the decorator register to
-# variables in this module for now.
-cmdutil.commanduseregistrar = False
+inferrepo = ''
diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -28,16 +28,17 @@  from . import (
     commands,
     demandimport,
     encoding,
     error,
     extensions,
     fancyopts,
     hg,
     hook,
+    registrar,
     ui as uimod,
     util,
 )
 
 class request(object):
     def __init__(self, args, ui=None, repo=None, fin=None, fout=None,
                  ferr=None):
         self.args = args
@@ -782,16 +783,25 @@  def _dispatch(req):
         overrides = [cmd for cmd in cmdtable if cmd in commands.table]
         if overrides:
             ui.warn(_("extension '%s' overrides commands: %s\n")
                     % (name, " ".join(overrides)))
         commands.table.update(cmdtable)
         _loaded.add(name)
 
     # (reposetup is handled in hg.repository)
+    norepo = set(commands.norepo.split())
+    norepo |= registrar.commands['norepo']
+    commands.norepo = ' '.join(norepo)
+    optionalrepo = set(commands.optionalrepo.split())
+    optionalrepo |= registrar.commands['optionalrepo']
+    commands.optionalrepo = ' '.join(optionalrepo)
+    inferrepo = set(commands.inferrepo.split())
+    inferrepo |= registrar.commands['inferrepo']
+    commands.inferrepo = ' '.join(inferrepo)
 
     addaliases(lui, commands.table)
 
     if not lui.configbool("ui", "strict"):
         # All aliases and commands are completely defined, now.
         # Check abbreviation/ambiguity of shell alias again, because shell
         # alias may cause failure of "_parse" (see issue4355)
         shellaliasfn = _checkshellalias(lui, ui, args, precheck=False)