Patchwork D2623: dispatch: adding config items for overriding flag defaults

login
register
mail settings
Submitter phabricator
Date March 3, 2018, 11:55 p.m.
Message ID <differential-rev-PHID-DREV-nmpyk5ghefhbml7lea2e-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/28884/
State New
Headers show

Comments

phabricator - March 3, 2018, 11:55 p.m.
rdamazio created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This introduces the new defaults format "commandname.default.optionname" which
  directly overrides the default of the option, instead of prepending the
  command-line option. This is meant to replace the [defaults] section which is
  already deprecated in a manner that's easier and safer to use than creating
  aliases.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2623

AFFECTED FILES
  mercurial/configitems.py
  mercurial/dispatch.py
  mercurial/ui.py
  tests/test-dispatch.t

CHANGE DETAILS




To: rdamazio, #hg-reviewers
Cc: mercurial-devel
phabricator - March 4, 2018, 12:10 a.m.
rdamazio added a comment.


  FYI this is a change I had previously sent to the list as 60b3222e01f96f91ece7eda9681a89bf3bb930a6, and Yuya reviewed . I just had never followed up on it.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2623

To: rdamazio, #hg-reviewers
Cc: mercurial-devel
phabricator - March 4, 2018, 2:21 p.m.
yuja requested changes to this revision.
yuja added a subscriber: dploch.
yuja added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> dispatch.py:624
> +            # parse the new default as the same type as the original.
> +            newdefault = ui.configtyped("commands", cfgitem, defaulttype, olddefault)
> +            if olddefault != newdefault:

Maybe this type conversion can be a `fancyopt.customopt` method since we've
refactored the default handling by https://phab.mercurial-scm.org/D2090?

  # no idea if _defaultopt() should be made public or the whole commands.default handling
  # should be moved to fancyopts
  x = fancyopts._defaultopt(olddefault)
  newdefault = x.configdefault(ui, cmd, optname, ...)

@dploch, any suggestions?

> ui.py:390
> +                if '.default.' in k:
> +                    del cfg['commands'][k]
>          if self.plain('revsetalias'):

Perhaps this is noop since `[commands]` is removed at all if `ui.plain()` returns True.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2623

To: rdamazio, #hg-reviewers, yuja
Cc: dploch, mercurial-devel
phabricator - March 4, 2018, 4:34 p.m.
rdamazio added inline comments.

INLINE COMMENTS

> yuja wrote in dispatch.py:624
> Maybe this type conversion can be a `fancyopt.customopt` method since we've
> refactored the default handling by https://phab.mercurial-scm.org/D2090?
> 
>   # no idea if _defaultopt() should be made public or the whole commands.default handling
>   # should be moved to fancyopts
>   x = fancyopts._defaultopt(olddefault)
>   newdefault = x.configdefault(ui, cmd, optname, ...)
> 
> @dploch, any suggestions?

The issue is that customopt (and all its children) assume the value type is already the correct one, and thus do not perform any conversion. Since we're parsing values from the config file, the conversion is desired to ensure they don't all end up as text - the config{bool,int,etc} methods called by configtyped perform the proper conversion. In most cases (all commands that declare default values) no conversio is needed since those already have the correct type.

> yuja wrote in ui.py:390
> Perhaps this is noop since `[commands]` is removed at all if `ui.plain()` returns True.

You're right, I had the plain logic inverted in my head. Removed.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2623

To: rdamazio, #hg-reviewers, yuja
Cc: dploch, mercurial-devel
phabricator - March 4, 2018, 5:15 p.m.
yuja added inline comments.

INLINE COMMENTS

> rdamazio wrote in dispatch.py:624
> The issue is that customopt (and all its children) assume the value type is already the correct one, and thus do not perform any conversion. Since we're parsing values from the config file, the conversion is desired to ensure they don't all end up as text - the config{bool,int,etc} methods called by configtyped perform the proper conversion. In most cases (all commands that declare default values) no conversio is needed since those already have the correct type.

IIUC, an extension author may implement its own customopt subclasses, and
put them into the command table, so we can't make ui.configtyped to
support all of them.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2623

To: rdamazio, #hg-reviewers, yuja
Cc: dploch, mercurial-devel
phabricator - March 4, 2018, 8:39 p.m.
rdamazio added inline comments.

INLINE COMMENTS

> yuja wrote in dispatch.py:624
> IIUC, an extension author may implement its own customopt subclasses, and
> put them into the command table, so we can't make ui.configtyped to
> support all of them.

Ah, makes sense. See if this addresses that case satisfactorily - it still has the caveat of not being able to "reset" container types, but that's true of the command line as well (if you have a list flag with a non-empty default, there's no way to remove that default item).

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2623

To: rdamazio, #hg-reviewers, yuja
Cc: dploch, mercurial-devel
phabricator - March 5, 2018, 11:29 p.m.
dploch added inline comments.

INLINE COMMENTS

> yuja wrote in dispatch.py:624
> Maybe this type conversion can be a `fancyopt.customopt` method since we've
> refactored the default handling by https://phab.mercurial-scm.org/D2090?
> 
>   # no idea if _defaultopt() should be made public or the whole commands.default handling
>   # should be moved to fancyopts
>   x = fancyopts._defaultopt(olddefault)
>   newdefault = x.configdefault(ui, cmd, optname, ...)
> 
> @dploch, any suggestions?

I think rdamazio's response is correct.  The type information for an opt exists only in the 'defaultvalue' slot in the tuple, so it needs to be extracted from there before reading the config.  I don't have a strong opinion as to where this code should go - 'ui' does seem marginally more appropriate than 'fancyopts', since it keeps 'fancyopts' low-level and not depending on 'ui'.

> dispatch.py:625-626
> +            # parse the new default as the same type as the original.
> +            newdefault = ui.configtyped("commands", cfgitem, defaulttype,
> +                                        olddefault)
> +            if isinstance(olddefault, fancyopts.customopt):

This doesn't handle callables properly.  I wonder if the something like the following would work instead:

oldopt = fancyopts._defaultopt(olddefault)
newdefault = old.opt.newstate(olddefault, ui.config("commands", cfgitem)
c[idx] = (opt[0], opt[1], fancyopts._withnewdefault(oldopt, newdefault), opt[3])

Where '_withnewdefault' is a wrapper customopt that just changes the default.

> dispatch.py:639-640
> +                # default will be appended to.
> +                olddefault.defaultvalue = olddefault.newstate(
> +                    olddefault.defaultvalue, newdefault, badvalue)
> +            elif olddefault != newdefault:

This makes me nervous.  What if someone re-uses a customopt instance in multiple commands?  i.e.:

DATE_FLAG = mypkg.dateopt()
...
('b', 'before', DATE_FLAG, '')
('a', 'after', 'DATE_FLAG', '')

Now, setting commands.defaults.before=2018-03-05 also silently changes the default for 'after'.  I suspect we need to introduce a wrapper class like what I suggest on lines 625-625, that delegates and leaves the original default unchanged.  And either way, we should probably clarify in the docs on customopts what expected use of the class is (i.e., should we just forbid reuse, is 'oldstate' safe to mutate, etc.)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2623

To: rdamazio, #hg-reviewers, yuja
Cc: dploch, mercurial-devel
phabricator - March 27, 2018, 6:30 a.m.
rdamazio added inline comments.

INLINE COMMENTS

> dploch wrote in dispatch.py:625-626
> This doesn't handle callables properly.  I wonder if the something like the following would work instead:
> 
> oldopt = fancyopts._defaultopt(olddefault)
> newdefault = old.opt.newstate(olddefault, ui.config("commands", cfgitem)
> c[idx] = (opt[0], opt[1], fancyopts._withnewdefault(oldopt, newdefault), opt[3])
> 
> Where '_withnewdefault' is a wrapper customopt that just changes the default.

I thought callables were meant to be used to generate the default default, not with overridden values?

> dploch wrote in dispatch.py:639-640
> This makes me nervous.  What if someone re-uses a customopt instance in multiple commands?  i.e.:
> 
> DATE_FLAG = mypkg.dateopt()
> ...
> ('b', 'before', DATE_FLAG, '')
> ('a', 'after', 'DATE_FLAG', '')
> 
> Now, setting commands.defaults.before=2018-03-05 also silently changes the default for 'after'.  I suspect we need to introduce a wrapper class like what I suggest on lines 625-625, that delegates and leaves the original default unchanged.  And either way, we should probably clarify in the docs on customopts what expected use of the class is (i.e., should we just forbid reuse, is 'oldstate' safe to mutate, etc.)

Nobody should use the same *instance* on multiple flags. Even with the current flags, if you use e.g. the same list on many, that'll cause problems with listopt.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2623

To: rdamazio, #hg-reviewers, yuja
Cc: dploch, mercurial-devel

Patch

diff --git a/tests/test-dispatch.t b/tests/test-dispatch.t
--- a/tests/test-dispatch.t
+++ b/tests/test-dispatch.t
@@ -8,8 +8,10 @@ 
   $ hg -v log -v x
 
   $ echo a > a
+  $ echo b > b
   $ hg ci -Ama
   adding a
+  adding b
 
 Missing arg:
 
@@ -52,10 +54,10 @@ 
 Parsing of early options should stop at "--":
 
   $ hg cat -- --config=hooks.pre-cat=false
-  --config=hooks.pre-cat=false: no such file in rev cb9a9f314b8b
+  --config=hooks.pre-cat=false: no such file in rev 0cd96de13884
   [1]
   $ hg cat -- --debugger
-  --debugger: no such file in rev cb9a9f314b8b
+  --debugger: no such file in rev 0cd96de13884
   [1]
 
 Unparsable form of early options:
@@ -155,31 +157,75 @@ 
   abort: pre-log hook exited with status 1
   [255]
   $ HGPLAIN=+strictflags hg --cwd .. -q -Ra log -b default
-  0:cb9a9f314b8b
+  0:0cd96de13884
   $ HGPLAIN=+strictflags hg --cwd .. -q --repository a log -b default
-  0:cb9a9f314b8b
+  0:0cd96de13884
   $ HGPLAIN=+strictflags hg --cwd .. -q --repo a log -b default
-  0:cb9a9f314b8b
+  0:0cd96de13884
 
 For compatibility reasons, HGPLAIN=+strictflags is not enabled by plain HGPLAIN:
 
   $ HGPLAIN= hg log --config='hooks.pre-log=false' -b default
   abort: pre-log hook exited with status 1
   [255]
   $ HGPLAINEXCEPT= hg log --cwd .. -q -Ra -b default
-  0:cb9a9f314b8b
+  0:0cd96de13884
 
 [defaults]
 
   $ hg cat a
   a
+  $ cp $HGRCPATH hgrc.bak
   $ cat >> $HGRCPATH <<EOF
   > [defaults]
   > cat = -r null
   > EOF
   $ 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.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
+  ? bad.py
+  ? bad.pyc
+  $ 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
+  ? bad.py
+  ? bad.pyc
+  $ hg revert a b c
 
   $ cd "$TESTTMP"
 
diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -379,10 +379,15 @@ 
                 del cfg['defaults'][k]
             for k, v in cfg.items('commands'):
                 del cfg['commands'][k]
-        # Don't remove aliases from the configuration if in the exceptionlist
+        # Don't remove specific sections from the configuration if in the
+        # exception list.
         if self.plain('alias'):
             for k, v in cfg.items('alias'):
                 del cfg['alias'][k]
+        if self.plain('commanddefaults'):
+            for k, v in cfg.items('commands'):
+                if '.default.' in k:
+		    del cfg['commands'][k]
         if self.plain('revsetalias'):
             for k, v in cfg.items('revsetalias'):
                 del cfg['revsetalias'][k]
diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -611,6 +611,21 @@ 
             args = pycompat.maplist(
                 util.expandpath, pycompat.shlexsplit(defaults)) + args
         c = list(entry[1])
+
+        # Apply new-style defaults from config file by actually changing the
+        # option defaults. We still let old-style defaults trump these (since
+        # those are added to the command line).
+        for idx, opt in enumerate(c):
+            optname = opt[1]
+            olddefault = opt[2]
+            defaulttype = type(olddefault)
+            cfgitem = "%s.default.%s" % (cmd, optname)
+            # parse the new default as the same type as the original.
+            newdefault = ui.configtyped("commands", cfgitem, defaulttype, olddefault)
+            if olddefault != newdefault:
+                # override the default in the flag declaration.
+                c[idx] = (opt[0], opt[1], newdefault, opt[3])
+
     else:
         cmd = None
         c = []
diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -184,6 +184,10 @@ 
 coreconfigitem('color', 'pagermode',
     default=dynamicdefault,
 )
+coreconfigitem('commands', '.*\.default\..*',
+    generic=True,
+    default=dynamicdefault,
+)
 coreconfigitem('commands', 'show.aliasprefix',
     default=list,
 )