Patchwork command options: handle unicode defaults gracefully

login
register
mail settings
Submitter Christophe de Vienne
Date Aug. 29, 2017, 10:11 p.m.
Message ID <afba1cb303db6797e565.1504044678@needle>
Download mbox | patch
Permalink /patch/23483/
State Changes Requested
Headers show

Comments

Christophe de Vienne - Aug. 29, 2017, 10:11 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 afba1cb303db6797e565907c277fd4150e76c141
# Parent  b1f75d8e887a4c06e6b120807f3defc5c7b78d33
command options: handle unicode defaults gracefully

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 encode the default value to ascii when parsing
the command line, and issue a devel warning for the sake of the extension
developers.

If encoding to ascii fails, the command is aborted.
Augie Fackler - Aug. 30, 2017, 6:32 p.m.
On Wed, Aug 30, 2017 at 12:11:18AM +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 afba1cb303db6797e565907c277fd4150e76c141
> # Parent  b1f75d8e887a4c06e6b120807f3defc5c7b78d33
> command options: handle unicode defaults gracefully

I'm conflicted here. I feel like we should just abort entirely rather
than even trying to do something smart.

What inspired working on this, out of curiousity? Trying to do
something on Python 3?

>
> 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.

In what ways might we change fancyopts to try and do something better?
I'm curious.

>
> The chosen approach is to encode the default value to ascii when parsing
> the command line, and issue a devel warning for the sake of the extension
> developers.
>
> If encoding to ascii fails, the command is aborted.

Typically our development warnings have a time horizon: does it seem
reasonable to just hard abort starting after the 4.4 release?

>
> diff -r b1f75d8e887a -r afba1cb303db mercurial/dispatch.py
> --- a/mercurial/dispatch.py	Tue Aug 22 16:59:02 2017 -0400
> +++ b/mercurial/dispatch.py	Tue Aug 29 18:24:51 2017 +0200
> @@ -556,7 +556,23 @@
>          if defaults:
>              args = pycompat.maplist(
>                  util.expandpath, pycompat.shlexsplit(defaults)) + args
> -        c = list(entry[1])
> +        c = []
> +        for option in entry[1]:
> +            default = option[2]
> +            if not pycompat.ispy3 and isinstance(default, type(u'')):
> +                ui.develwarn(
> +                    "A unicode default value (%s) was passed to the "
> +                    "%s.%s option" % (repr(default), cmd, option[1]))
> +                try:
> +                    default = default.encode('ascii')
> +                except Exception:
> +                    raise error.Abort(
> +                        "Cannot encode %s.%s default value to ascii" %
> +                        (cmd, option[1]),
> +                        "Try changing the %s.%s default value to a "
> +                        "non-unicode string" % (cmd, option[1]))
> +                option = option[:2] + (default,) + option[3:]
> +            c.append(option)
>      else:
>          cmd = None
>          c = []
> diff -r b1f75d8e887a -r afba1cb303db tests/test-devel-warnings.t
> --- a/tests/test-devel-warnings.t	Tue Aug 22 16:59:02 2017 -0400
> +++ b/tests/test-devel-warnings.t	Tue Aug 29 18:24:51 2017 +0200
> @@ -250,3 +250,43 @@
>    devel-warn: config item requires an explicit default value: 'test.dynamic' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
>
>    $ cd ..
> +
> +Test warning on command options with unicode default value
> +
> +  $ hg init $TESTTMP/opt-unicode-default
> +
> +  $ cat > $TESTTMP/test_unicode_default_value.py << EOF
> +  > from mercurial import registrar
> +  > cmdtable = {}
> +  > command = registrar.command(cmdtable)
> +  > @command('ext', [('', '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 ext --opt newvalue
> +  devel-warn: A unicode default value (u'value') was passed to the ext.opt option at: */mercurial/dispatch.py:* (_dispatch) (glob)
> +  newvalue
> +
> +Test abort on command options with non-ascii unicode default value
> +
> +  $ cat > $TESTTMP/test_unicode_default_value_noascii.py << EOF
> +  > # encoding=utf-8
> +  > from mercurial import registrar
> +  > cmdtable = {}
> +  > command = registrar.command(cmdtable)
> +  > @command('ext', [('', 'opt', u'défaut', 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_noascii.py
> +  > EOF
> +  $ hg -R $TESTTMP/opt-unicode-default ext
> +  devel-warn: A unicode default value (u'd\xe9faut') was passed to the ext.opt option at: */mercurial/dispatch.py:* (_dispatch) (glob)
> +  abort: ('Cannot encode ext.opt default value to ascii', 'Try changing the ext.opt default value to a non-unicode string')
> +  [255]
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff -r b1f75d8e887a -r afba1cb303db mercurial/dispatch.py
--- a/mercurial/dispatch.py	Tue Aug 22 16:59:02 2017 -0400
+++ b/mercurial/dispatch.py	Tue Aug 29 18:24:51 2017 +0200
@@ -556,7 +556,23 @@ 
         if defaults:
             args = pycompat.maplist(
                 util.expandpath, pycompat.shlexsplit(defaults)) + args
-        c = list(entry[1])
+        c = []
+        for option in entry[1]:
+            default = option[2]
+            if not pycompat.ispy3 and isinstance(default, type(u'')):
+                ui.develwarn(
+                    "A unicode default value (%s) was passed to the "
+                    "%s.%s option" % (repr(default), cmd, option[1]))
+                try:
+                    default = default.encode('ascii')
+                except Exception:
+                    raise error.Abort(
+                        "Cannot encode %s.%s default value to ascii" %
+                        (cmd, option[1]),
+                        "Try changing the %s.%s default value to a "
+                        "non-unicode string" % (cmd, option[1]))
+                option = option[:2] + (default,) + option[3:]
+            c.append(option)
     else:
         cmd = None
         c = []
diff -r b1f75d8e887a -r afba1cb303db tests/test-devel-warnings.t
--- a/tests/test-devel-warnings.t	Tue Aug 22 16:59:02 2017 -0400
+++ b/tests/test-devel-warnings.t	Tue Aug 29 18:24:51 2017 +0200
@@ -250,3 +250,43 @@ 
   devel-warn: config item requires an explicit default value: 'test.dynamic' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
 
   $ cd ..
+
+Test warning on command options with unicode default value
+
+  $ hg init $TESTTMP/opt-unicode-default
+
+  $ cat > $TESTTMP/test_unicode_default_value.py << EOF
+  > from mercurial import registrar
+  > cmdtable = {}
+  > command = registrar.command(cmdtable)
+  > @command('ext', [('', '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 ext --opt newvalue
+  devel-warn: A unicode default value (u'value') was passed to the ext.opt option at: */mercurial/dispatch.py:* (_dispatch) (glob)
+  newvalue
+
+Test abort on command options with non-ascii unicode default value
+
+  $ cat > $TESTTMP/test_unicode_default_value_noascii.py << EOF
+  > # encoding=utf-8
+  > from mercurial import registrar
+  > cmdtable = {}
+  > command = registrar.command(cmdtable)
+  > @command('ext', [('', 'opt', u'défaut', 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_noascii.py
+  > EOF
+  $ hg -R $TESTTMP/opt-unicode-default ext
+  devel-warn: A unicode default value (u'd\xe9faut') was passed to the ext.opt option at: */mercurial/dispatch.py:* (_dispatch) (glob)
+  abort: ('Cannot encode ext.opt default value to ascii', 'Try changing the ext.opt default value to a non-unicode string')
+  [255]