Patchwork [v2] fancyopts: making config defaults actually override defaults

login
register
mail settings
Submitter via Mercurial-devel
Date March 24, 2017, 7:32 a.m.
Message ID <60b3222e01f96f91ece7.1490340720@rdamazio.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/19625/
State Changes Requested
Headers show

Comments

via Mercurial-devel - March 24, 2017, 7:32 a.m.
# HG changeset patch
# User Rodrigo Damazio <rdamazio@google.com>
# Date 1490340211 25200
#      Fri Mar 24 00:23:31 2017 -0700
# Node ID 60b3222e01f96f91ece7eda9681a89bf3bb930a6
# Parent  df82f375fa005b9c71b463182e6b54aa47fa5999
fancyopts: making config defaults actually override defaults

This introduces the new defaults format "command.option" which directly
overrides the default of the option, instead of prepending the command-line
option.
Yuya Nishihara - April 1, 2017, 11:07 a.m.
On Fri, 24 Mar 2017 00:32:00 -0700, Rodrigo Damazio Bovendorp via Mercurial-devel wrote:
> # HG changeset patch
> # User Rodrigo Damazio <rdamazio@google.com>
> # Date 1490340211 25200
> #      Fri Mar 24 00:23:31 2017 -0700
> # Node ID 60b3222e01f96f91ece7eda9681a89bf3bb930a6
> # Parent  df82f375fa005b9c71b463182e6b54aa47fa5999
> fancyopts: making config defaults actually override defaults

Sorry for late review.

This looks generally good to me. I have a few comments inline.

>          c = list(entry[1])
> +
> +        # override new-style defaults from config file by actually changing the
> +        # option defaults.
> +        for idx, opt in enumerate(c):
> +            optname = opt[1]
> +            shortname = opt[0]
> +            defaulttype = type(opt[2])
> +            rawdefault = (
> +                ui.config("commands", "%s.default.%s" % (cmd, optname)) or
> +                ui.config("commands", "%s.default.%s" % (cmd, shortname)))

I prefer not supporting "default.<shortname>" because shortname doesn't look
like a config name. And if we do support it, an empty longname value should
supersede a shortname:

  ui.config(<longname>, default=ui.config(<shortname>))

> +            if rawdefault:
> +                # parse the new default using the type of the original default.
> +                default = fancyopts.parsevalue(optname, rawdefault, defaulttype,
> +                                               util.parsebool(rawdefault))

Can we use ui.configbool/list/int() instead of parsevalue() ? That will
provide a slightly better error message.
via Mercurial-devel - March 3, 2018, 11:33 p.m.
FYI I'm getting back to this old patch, but I'll send it through
phabricator this time.


On Sat, Apr 1, 2017 at 11:07 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Fri, 24 Mar 2017 00:32:00 -0700, Rodrigo Damazio Bovendorp via
> Mercurial-devel wrote:
> > # HG changeset patch
> > # User Rodrigo Damazio <rdamazio@google.com>
> > # Date 1490340211 25200
> > #      Fri Mar 24 00:23:31 2017 -0700
> > # Node ID 60b3222e01f96f91ece7eda9681a89bf3bb930a6
> > # Parent  df82f375fa005b9c71b463182e6b54aa47fa5999
> > fancyopts: making config defaults actually override defaults
>
> Sorry for late review.
>
> This looks generally good to me. I have a few comments inline.
>
> >          c = list(entry[1])
> > +
> > +        # override new-style defaults from config file by actually
> changing the
> > +        # option defaults.
> > +        for idx, opt in enumerate(c):
> > +            optname = opt[1]
> > +            shortname = opt[0]
> > +            defaulttype = type(opt[2])
> > +            rawdefault = (
> > +                ui.config("commands", "%s.default.%s" % (cmd, optname))
> or
> > +                ui.config("commands", "%s.default.%s" % (cmd,
> shortname)))
>
> I prefer not supporting "default.<shortname>" because shortname doesn't
> look
> like a config name. And if we do support it, an empty longname value should
> supersede a shortname:
>
>   ui.config(<longname>, default=ui.config(<shortname>))
>
> > +            if rawdefault:
> > +                # parse the new default using the type of the original
> default.
> > +                default = fancyopts.parsevalue(optname, rawdefault,
> defaulttype,
> > +
>  util.parsebool(rawdefault))
>
> Can we use ui.configbool/list/int() instead of parsevalue() ? That will
> provide a slightly better error message.
>

Patch

diff -r df82f375fa00 -r 60b3222e01f9 mercurial/dispatch.py
--- a/mercurial/dispatch.py	Tue Mar 21 23:30:13 2017 +0100
+++ b/mercurial/dispatch.py	Fri Mar 24 00:23:31 2017 -0700
@@ -473,11 +473,30 @@ 
                                          ui.configbool("ui", "strict"))
         cmd = aliases[0]
         args = aliasargs(entry[0], args)
+
+        # override old-style defaults from config file by prepending
+        # command-line flags
         defaults = ui.config("defaults", cmd)
         if defaults:
             args = pycompat.maplist(
                 util.expandpath, pycompat.shlexsplit(defaults)) + args
         c = list(entry[1])
+
+        # override new-style defaults from config file by actually changing the
+        # option defaults.
+        for idx, opt in enumerate(c):
+            optname = opt[1]
+            shortname = opt[0]
+            defaulttype = type(opt[2])
+            rawdefault = (
+                ui.config("commands", "%s.default.%s" % (cmd, optname)) or
+                ui.config("commands", "%s.default.%s" % (cmd, shortname)))
+            if rawdefault:
+                # parse the new default using the type of the original default.
+                default = fancyopts.parsevalue(optname, rawdefault, defaulttype,
+                                               util.parsebool(rawdefault))
+                c[idx] = (opt[0], opt[1], default, opt[3])
+
     else:
         cmd = None
         c = []
diff -r df82f375fa00 -r 60b3222e01f9 mercurial/fancyopts.py
--- a/mercurial/fancyopts.py	Tue Mar 21 23:30:13 2017 +0100
+++ b/mercurial/fancyopts.py	Fri Mar 24 00:23:31 2017 -0700
@@ -142,18 +142,27 @@ 
         t = type(obj)
         if callable(obj):
             state[name] = defmap[name](val)
-        elif t is type(1):
-            try:
-                state[name] = int(val)
-            except ValueError:
-                raise error.Abort(_('invalid value %r for option %s, '
-                                   'expected int') % (val, opt))
-        elif t is type(''):
-            state[name] = val
         elif t is type([]):
             state[name].append(val)
-        elif t is type(None) or t is type(False):
-            state[name] = boolval
+        else:
+            # non-callable single value.
+            state[name] = parsevalue(name, val, t, boolval)
 
     # return unparsed args
     return args
+
+def parsevalue(name, val, typ, boolval=True):
+    if typ is type(1):
+        try:
+            return int(val)
+        except ValueError:
+            raise error.Abort(_('invalid value %r for option %s, '
+                                'expected int') % (val, name))
+    elif typ is type(''):
+        return val
+    elif (typ is type(None) or typ is type(False)) and (
+          type(boolval) is type(False)):
+        return boolval
+    else:
+        raise error.Abort(_('invalid value %r for option %s, '
+                            'unknown type') % (val, name))
diff -r df82f375fa00 -r 60b3222e01f9 tests/test-dispatch.t
--- a/tests/test-dispatch.t	Tue Mar 21 23:30:13 2017 +0100
+++ b/tests/test-dispatch.t	Fri Mar 24 00:23:31 2017 -0700
@@ -8,8 +8,10 @@ 
   $ hg -v log -v x
 
   $ echo a > a
+  $ echo b > b
   $ hg ci -Ama
   adding a
+  adding b
 
 Missing arg:
 
@@ -34,6 +36,7 @@ 
 
   $ hg cat a
   a
+  $ cp $HGRCPATH hgrc.bak
   $ cat >> $HGRCPATH <<EOF
   > [defaults]
   > cat = -r null
@@ -41,6 +44,54 @@ 
   $ hg cat a
   a: no such file in rev 000000000000
   [1]
+  $ cp -f hgrc.bak $HGRCPATH
+
+new-style [commands] defaults and overrides
+
+  $ hg cat a
+  a
+  $ cat >> $HGRCPATH <<EOF
+  > [commands]
+  > cat.default.r = null
+  > EOF
+  $ hg cat a
+  a: no such file in rev 000000000000
+  [1]
+
+  $ cp -f hgrc.bak $HGRCPATH
+  $ cat >> $HGRCPATH <<EOF
+  > [commands]
+  > cat.default.rev = null
+  > EOF
+  $ hg cat a
+  a: no such file in rev 000000000000
+  [1]
+
+  $ mv -f hgrc.bak $HGRCPATH
+  $ echo foo >> a
+  $ hg rm b
+  $ echo bar > c
+  $ hg add c
+  $ hg status
+  M a
+  A c
+  R b
+  $ cat >> $HGRCPATH <<EOF
+  > [commands]
+  > status.default.removed = 1
+  > EOF
+  $ hg status
+  R b
+  $ hg status --modified
+  M a
+  R b
+  $ hg status --modified --no-removed
+  M a
+  $ hg status --no-removed
+  M a
+  A c
+  R b
+  $ hg revert a b c
 
   $ cd "$TESTTMP"