Patchwork [5,of,6] extensions: prohibit registration of command without using @command

login
register
mail settings
Submitter Yuya Nishihara
Date May 13, 2017, 9:57 a.m.
Message ID <484b2808830a2a390d9e.1494669456@mimosa>
Download mbox | patch
Permalink /patch/20596/
State Accepted
Headers show

Comments

Yuya Nishihara - May 13, 2017, 9:57 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1494657710 -32400
#      Sat May 13 15:41:50 2017 +0900
# Node ID 484b2808830a2a390d9e1128c6e88d9249097fa6
# Parent  1d3639b4640fc0cb9d11e8263126f271917b9889
extensions: prohibit registration of command without using @command

Detect the problem earlier for better error indication. I'm tired of teaching
users that the mq extension is not guilty but the third-party extension is.

https://bitbucket.org/tortoisehg/thg/issues?q=%27norepo%27
Gregory Szorc - May 13, 2017, 5:55 p.m.
On Sat, May 13, 2017 at 2:57 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1494657710 -32400
> #      Sat May 13 15:41:50 2017 +0900
> # Node ID 484b2808830a2a390d9e1128c6e88d9249097fa6
> # Parent  1d3639b4640fc0cb9d11e8263126f271917b9889
> extensions: prohibit registration of command without using @command
>
> Detect the problem earlier for better error indication. I'm tired of
> teaching
> users that the mq extension is not guilty but the third-party extension is.
>
> https://bitbucket.org/tortoisehg/thg/issues?q=%27norepo%27


This should be marked as (API) to raise awareness to extension authors.


>
>
> diff --git a/mercurial/extensions.py b/mercurial/extensions.py
> --- a/mercurial/extensions.py
> +++ b/mercurial/extensions.py
> @@ -118,6 +118,19 @@ def _reportimporterror(ui, err, failed,
>      if ui.debugflag:
>          ui.traceback()
>
> +# attributes set by registrar.command
> +_cmdfuncattrs = ('norepo', 'optionalrepo', 'inferrepo')
> +
> +def _validatecmdtable(cmdtable):
> +    """Check if extension commands have required attributes"""
> +    for c, e in cmdtable.iteritems():
> +        f = e[0]
> +        missing = [a for a in _cmdfuncattrs if not util.safehasattr(f, a)]
> +        if not missing:
> +            continue
> +        raise error.ProgrammingError("missing attributes in command
> '%s': %s"
> +                                     % (c, ', '.join(missing)))
> +
>  def load(ui, name, path):
>      if name.startswith('hgext.') or name.startswith('hgext/'):
>          shortname = name[6:]
> @@ -139,6 +152,7 @@ def load(ui, name, path):
>          ui.warn(_('(third party extension %s requires version %s or newer
> '
>                    'of Mercurial; disabling)\n') % (shortname, minver))
>          return
> +    _validatecmdtable(getattr(mod, 'cmdtable', {}))
>
>      _extensions[shortname] = mod
>      _order.append(shortname)
> diff --git a/tests/test-extension.t b/tests/test-extension.t
> --- a/tests/test-extension.t
> +++ b/tests/test-extension.t
> @@ -1534,6 +1534,38 @@ disabling in command line overlays with
>
>    $ cd ..
>
> +Prohibit registration of commands that don't use @command (issue5137)
> +
> +  $ hg init deprecated
> +  $ cd deprecated
> +
> +  $ cat <<EOF > deprecatedcmd.py
> +  > def deprecatedcmd(repo, ui):
> +  >     pass
> +  > cmdtable = {
> +  >     'deprecatedcmd': (deprecatedcmd, [], ''),
> +  > }
> +  > EOF
> +  $ cat <<EOF > .hg/hgrc
> +  > [extensions]
> +  > deprecatedcmd = `pwd`/deprecatedcmd.py
> +  > mq = !
> +  > hgext.mq = !
> +  > hgext/mq = !
> +  > EOF
> +
> +  $ hg deprecatedcmd > /dev/null
> +  *** failed to import extension deprecatedcmd from $TESTTMP/deprecated/deprecatedcmd.py:
> missing attributes in command 'deprecatedcmd': norepo, optionalrepo,
> inferrepo
> +  hg: unknown command 'deprecatedcmd'
> +  [255]
>

If I were an extension author, this failure mode would leave me scratching
my head. Can we get a hint to use @command in the error message?


> +
> + the extension shouldn't be loaded at all so the mq works:
> +
> +  $ hg version --config extensions.mq= > /dev/null
> +  *** failed to import extension deprecatedcmd from $TESTTMP/deprecated/deprecatedcmd.py:
> missing attributes in command 'deprecatedcmd': norepo, optionalrepo,
> inferrepo
> +
> +  $ cd ..
> +
>  Test synopsis and docstring extending
>
>    $ hg init exthelp
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - May 14, 2017, 3:03 a.m.
On Sat, 13 May 2017 10:55:37 -0700, Gregory Szorc wrote:
> On Sat, May 13, 2017 at 2:57 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1494657710 -32400
> > #      Sat May 13 15:41:50 2017 +0900
> > # Node ID 484b2808830a2a390d9e1128c6e88d9249097fa6
> > # Parent  1d3639b4640fc0cb9d11e8263126f271917b9889
> > extensions: prohibit registration of command without using @command
> >
> > Detect the problem earlier for better error indication. I'm tired of
> > teaching
> > users that the mq extension is not guilty but the third-party extension is.
> >
> > https://bitbucket.org/tortoisehg/thg/issues?q=%27norepo%27
> 
> This should be marked as (API) to raise awareness to extension authors.

Good catch.

> > +  $ hg deprecatedcmd > /dev/null
> > +  *** failed to import extension deprecatedcmd from $TESTTMP/deprecated/deprecatedcmd.py:
> > missing attributes in command 'deprecatedcmd': norepo, optionalrepo,
> > inferrepo
> > +  hg: unknown command 'deprecatedcmd'
> > +  [255]
> >
> 
> If I were an extension author, this failure mode would leave me scratching
> my head. Can we get a hint to use @command in the error message?

Okay. I'll make V2.

Patch

diff --git a/mercurial/extensions.py b/mercurial/extensions.py
--- a/mercurial/extensions.py
+++ b/mercurial/extensions.py
@@ -118,6 +118,19 @@  def _reportimporterror(ui, err, failed, 
     if ui.debugflag:
         ui.traceback()
 
+# attributes set by registrar.command
+_cmdfuncattrs = ('norepo', 'optionalrepo', 'inferrepo')
+
+def _validatecmdtable(cmdtable):
+    """Check if extension commands have required attributes"""
+    for c, e in cmdtable.iteritems():
+        f = e[0]
+        missing = [a for a in _cmdfuncattrs if not util.safehasattr(f, a)]
+        if not missing:
+            continue
+        raise error.ProgrammingError("missing attributes in command '%s': %s"
+                                     % (c, ', '.join(missing)))
+
 def load(ui, name, path):
     if name.startswith('hgext.') or name.startswith('hgext/'):
         shortname = name[6:]
@@ -139,6 +152,7 @@  def load(ui, name, path):
         ui.warn(_('(third party extension %s requires version %s or newer '
                   'of Mercurial; disabling)\n') % (shortname, minver))
         return
+    _validatecmdtable(getattr(mod, 'cmdtable', {}))
 
     _extensions[shortname] = mod
     _order.append(shortname)
diff --git a/tests/test-extension.t b/tests/test-extension.t
--- a/tests/test-extension.t
+++ b/tests/test-extension.t
@@ -1534,6 +1534,38 @@  disabling in command line overlays with 
 
   $ cd ..
 
+Prohibit registration of commands that don't use @command (issue5137)
+
+  $ hg init deprecated
+  $ cd deprecated
+
+  $ cat <<EOF > deprecatedcmd.py
+  > def deprecatedcmd(repo, ui):
+  >     pass
+  > cmdtable = {
+  >     'deprecatedcmd': (deprecatedcmd, [], ''),
+  > }
+  > EOF
+  $ cat <<EOF > .hg/hgrc
+  > [extensions]
+  > deprecatedcmd = `pwd`/deprecatedcmd.py
+  > mq = !
+  > hgext.mq = !
+  > hgext/mq = !
+  > EOF
+
+  $ hg deprecatedcmd > /dev/null
+  *** failed to import extension deprecatedcmd from $TESTTMP/deprecated/deprecatedcmd.py: missing attributes in command 'deprecatedcmd': norepo, optionalrepo, inferrepo
+  hg: unknown command 'deprecatedcmd'
+  [255]
+
+ the extension shouldn't be loaded at all so the mq works:
+
+  $ hg version --config extensions.mq= > /dev/null
+  *** failed to import extension deprecatedcmd from $TESTTMP/deprecated/deprecatedcmd.py: missing attributes in command 'deprecatedcmd': norepo, optionalrepo, inferrepo
+
+  $ cd ..
+
 Test synopsis and docstring extending
 
   $ hg init exthelp