Patchwork [v4] command options: prohibit unicode defaults

login
register
mail settings
Submitter Christophe de Vienne
Date Sept. 1, 2017, 2:30 p.m.
Message ID <44e9719c499dae04eca3.1504276247@needle>
Download mbox | patch
Permalink /patch/23594/
State Accepted
Headers show

Comments

Christophe de Vienne - Sept. 1, 2017, 2:30 p.m.
# HG changeset patch
# User Christophe de Vienne <christophe@cdevienne.info>
# Date 1504023891 -7200
#      Tue Aug 29 18:24:51 2017 +0200
# Node ID 44e9719c499dae04eca36178ddfb2c5eddec6beb
# Parent  b2eb0aa445cbe7cadc8b7691c6266908adfc5057
command options: prohibit unicode defaults

If the default value of an option is a unicode string (something
than happen easily when using a 'from __future__ import unicode_literals'),
any value passed on the command line will be ignored because the fancyopts
module only checks for byte strings and not unicode strings.

Changing fancyopts behavior is easy but would make assumptions on how
the python3 port should be done, which is outside the scope of this patch.

The chosen approach is to stop an extension from being loaded when a unicode
default value is detected, with a hint for the developer.
Augie Fackler - Sept. 1, 2017, 7:21 p.m.
On Fri, Sep 01, 2017 at 04:30:47PM +0200, Christophe de Vienne wrote:
> # HG changeset patch
> # User Christophe de Vienne <christophe@cdevienne.info>
> # Date 1504023891 -7200
> #      Tue Aug 29 18:24:51 2017 +0200
> # Node ID 44e9719c499dae04eca36178ddfb2c5eddec6beb
> # Parent  b2eb0aa445cbe7cadc8b7691c6266908adfc5057
> command options: prohibit unicode defaults

queued, thanks

(Many thanks to yuya for the earlier rounds of review)
Yuya Nishihara - Sept. 2, 2017, 12:34 p.m.
On Fri, 01 Sep 2017 16:30:47 +0200, Christophe de Vienne wrote:
> # HG changeset patch
> # User Christophe de Vienne <christophe@cdevienne.info>
> # Date 1504023891 -7200
> #      Tue Aug 29 18:24:51 2017 +0200
> # Node ID 44e9719c499dae04eca36178ddfb2c5eddec6beb
> # Parent  b2eb0aa445cbe7cadc8b7691c6266908adfc5057
> command options: prohibit unicode defaults

> diff -r b2eb0aa445cb -r 44e9719c499d mercurial/extensions.py
> --- a/mercurial/extensions.py	Tue Aug 22 21:21:43 2017 -0400
> +++ b/mercurial/extensions.py	Tue Aug 29 18:24:51 2017 +0200
> @@ -133,6 +133,14 @@
>                            "registrar.command to register '%s'" % c, '4.6')
>          missing = [a for a in _cmdfuncattrs if not util.safehasattr(f, a)]
>          if not missing:
> +            for option in e[1]:
> +                default = option[2]
> +                if isinstance(default, type(u'')):
> +                    raise error.ProgrammingError(
> +                        "option '%s.%s' has a unicode default value"
> +                        % (c, option[1]),
> +                        "change the %s.%s default value to a "
> +                        "non-unicode string" % (c, option[1]))

The hint has to be passed as a keyword argument. Fixed in flight.

Patch

diff -r b2eb0aa445cb -r 44e9719c499d mercurial/extensions.py
--- a/mercurial/extensions.py	Tue Aug 22 21:21:43 2017 -0400
+++ b/mercurial/extensions.py	Tue Aug 29 18:24:51 2017 +0200
@@ -133,6 +133,14 @@ 
                           "registrar.command to register '%s'" % c, '4.6')
         missing = [a for a in _cmdfuncattrs if not util.safehasattr(f, a)]
         if not missing:
+            for option in e[1]:
+                default = option[2]
+                if isinstance(default, type(u'')):
+                    raise error.ProgrammingError(
+                        "option '%s.%s' has a unicode default value"
+                        % (c, option[1]),
+                        "change the %s.%s default value to a "
+                        "non-unicode string" % (c, option[1]))
             continue
         raise error.ProgrammingError(
             'missing attributes: %s' % ', '.join(missing),
diff -r b2eb0aa445cb -r 44e9719c499d tests/test-extension.t
--- a/tests/test-extension.t	Tue Aug 22 21:21:43 2017 -0400
+++ b/tests/test-extension.t	Tue Aug 29 18:24:51 2017 +0200
@@ -1709,3 +1709,25 @@ 
   $ hg --config extensions.nonregistrar=`pwd`/nonregistrar.py version > /dev/null
   devel-warn: cmdutil.command is deprecated, use registrar.command to register 'foo'
   (compatibility will be dropped after Mercurial-4.6, update your code.) * (glob)
+
+Prohibit the use of unicode strings as the default value of options
+
+  $ hg init $TESTTMP/opt-unicode-default
+
+  $ cat > $TESTTMP/test_unicode_default_value.py << EOF
+  > from mercurial import registrar
+  > cmdtable = {}
+  > command = registrar.command(cmdtable)
+  > @command('dummy', [('', 'opt', u'value', u'help')], 'ext [OPTIONS]')
+  > def ext(*args, **opts):
+  >     print(opts['opt'])
+  > EOF
+  $ cat > $TESTTMP/opt-unicode-default/.hg/hgrc << EOF
+  > [extensions]
+  > test_unicode_default_value = $TESTTMP/test_unicode_default_value.py
+  > EOF
+  $ hg -R $TESTTMP/opt-unicode-default dummy
+  *** failed to import extension test_unicode_default_value from $TESTTMP/test_unicode_default_value.py: ("option 'dummy.opt' has a unicode default value", 'change the dummy.opt default value to a non-unicode string')
+  hg: unknown command 'dummy'
+  (did you mean summary?)
+  [255]