Patchwork [v2] command options: abort on unicode defaults

login
register
mail settings
Submitter Christophe de Vienne
Date Aug. 31, 2017, 9:26 p.m.
Message ID <5bbb2d585a963985a917.1504214818@needle>
Download mbox | patch
Permalink /patch/23575/
State Superseded
Headers show

Comments

Christophe de Vienne - Aug. 31, 2017, 9:26 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 5bbb2d585a963985a9172325f98bb5367b53e438
# Parent  b2eb0aa445cbe7cadc8b7691c6266908adfc5057
command options: abort on 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 abord when a unicode default value is detected,
with a hint for the developer.
Jun Wu - Aug. 31, 2017, 9:50 p.m.
Excerpts from Christophe de Vienne's message of 2017-08-31 23:26:58 +0200:
> # HG changeset patch
> # User Christophe de Vienne <christophe@cdevienne.info>
> # Date 1504023891 -7200
> #      Tue Aug 29 18:24:51 2017 +0200
> # Node ID 5bbb2d585a963985a9172325f98bb5367b53e438
> # Parent  b2eb0aa445cbe7cadc8b7691c6266908adfc5057
> command options: abort on 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 abord when a unicode default value is detected,

"abort"

> with a hint for the developer.
> 
> diff -r b2eb0aa445cb -r 5bbb2d585a96 mercurial/dispatch.py
> --- a/mercurial/dispatch.py    Tue Aug 22 21:21:43 2017 -0400
> +++ b/mercurial/dispatch.py    Tue Aug 29 18:24:51 2017 +0200
> @@ -557,7 +557,15 @@
>          if defaults:
>              args = pycompat.maplist(
>                  util.expandpath, pycompat.shlexsplit(defaults)) + args
> -        c = list(entry[1])

Changing here means it only works for global opts. Is that intentional?

> +        for option in entry[1]:
> +            default = option[2]
> +            if not pycompat.ispy3 and isinstance(default, type(u'')):
> +                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]))

Maybe use error.ProgrammingError

> +        c = entry[1]
>      else:
>          cmd = None
>          c = []
> diff -r b2eb0aa445cb -r 5bbb2d585a96 tests/test-dispatch.t
> --- a/tests/test-dispatch.t    Tue Aug 22 21:21:43 2017 -0400
> +++ b/tests/test-dispatch.t    Tue Aug 29 18:24:51 2017 +0200
> @@ -60,3 +60,23 @@
>    [255]
>  
>  #endif
> +
> +Test abort 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
> +  abort: ('Cannot encode ext.opt default value to ascii', 'Try changing the ext.opt default value to a non-unicode string')
> +  [255]
Christophe de Vienne - Sept. 1, 2017, 7:31 a.m.
Le 31/08/2017 à 23:50, Jun Wu a écrit :
> Excerpts from Christophe de Vienne's message of 2017-08-31 23:26:58 +0200:
>> # HG changeset patch
>> # User Christophe de Vienne <christophe@cdevienne.info>
>> # Date 1504023891 -7200
>> #      Tue Aug 29 18:24:51 2017 +0200
>> # Node ID 5bbb2d585a963985a9172325f98bb5367b53e438
>> # Parent  b2eb0aa445cbe7cadc8b7691c6266908adfc5057
>> command options: abort on 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 abord when a unicode default value is detected,
> 
> "abort"

Thanks

> 
>> with a hint for the developer.
>>
>> diff -r b2eb0aa445cb -r 5bbb2d585a96 mercurial/dispatch.py
>> --- a/mercurial/dispatch.py    Tue Aug 22 21:21:43 2017 -0400
>> +++ b/mercurial/dispatch.py    Tue Aug 29 18:24:51 2017 +0200
>> @@ -557,7 +557,15 @@
>>          if defaults:
>>              args = pycompat.maplist(
>>                  util.expandpath, pycompat.shlexsplit(defaults)) + args
>> -        c = list(entry[1])
> 
> Changing here means it only works for global opts. Is that intentional?

no, and I realize I changed the line (I removed the list()), which I did
not intended.

That said, I am not sure what you mean by "it only works for global
opts". What are global opts?

> 
>> +        for option in entry[1]:
>> +            default = option[2]
>> +            if not pycompat.ispy3 and isinstance(default, type(u'')):
>> +                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]))
> 
> Maybe use error.ProgrammingError

ok
Christophe de Vienne - Sept. 1, 2017, 7:36 a.m.
Le 01/09/2017 à 09:31, Christophe de Vienne a écrit :
> 
> 
> Le 31/08/2017 à 23:50, Jun Wu a écrit :
>> Excerpts from Christophe de Vienne's message of 2017-08-31 23:26:58 +0200:
[...]
>>>
>>> diff -r b2eb0aa445cb -r 5bbb2d585a96 mercurial/dispatch.py
>>> --- a/mercurial/dispatch.py    Tue Aug 22 21:21:43 2017 -0400
>>> +++ b/mercurial/dispatch.py    Tue Aug 29 18:24:51 2017 +0200
>>> @@ -557,7 +557,15 @@
>>>          if defaults:
>>>              args = pycompat.maplist(
>>>                  util.expandpath, pycompat.shlexsplit(defaults)) + args
>>> -        c = list(entry[1])
>>
>> Changing here means it only works for global opts. Is that intentional?
> 
> no, and I realize I changed the line (I removed the list()), which I did
> not intended.
> 
> That said, I am not sure what you mean by "it only works for global
> opts". What are global opts?

Re-reading the source, it seems to be the other way around: the global
opts are not checked, only the ones of the command.

Patch

diff -r b2eb0aa445cb -r 5bbb2d585a96 mercurial/dispatch.py
--- a/mercurial/dispatch.py	Tue Aug 22 21:21:43 2017 -0400
+++ b/mercurial/dispatch.py	Tue Aug 29 18:24:51 2017 +0200
@@ -557,7 +557,15 @@ 
         if defaults:
             args = pycompat.maplist(
                 util.expandpath, pycompat.shlexsplit(defaults)) + args
-        c = list(entry[1])
+        for option in entry[1]:
+            default = option[2]
+            if not pycompat.ispy3 and isinstance(default, type(u'')):
+                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]))
+        c = entry[1]
     else:
         cmd = None
         c = []
diff -r b2eb0aa445cb -r 5bbb2d585a96 tests/test-dispatch.t
--- a/tests/test-dispatch.t	Tue Aug 22 21:21:43 2017 -0400
+++ b/tests/test-dispatch.t	Tue Aug 29 18:24:51 2017 +0200
@@ -60,3 +60,23 @@ 
   [255]
 
 #endif
+
+Test abort 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
+  abort: ('Cannot encode ext.opt default value to ascii', 'Try changing the ext.opt default value to a non-unicode string')
+  [255]