This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2623

dispatch: adding config items for overriding flag defaults
Needs RevisionPublic

Authored by rdamazio on Mar 3 2018, 6:54 PM.

Details

Reviewers
yuja
baymax
Group Reviewers
hg-reviewers
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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

rdamazio created this revision.Mar 3 2018, 6:54 PM
rdamazio updated this revision to Diff 6523.Mar 3 2018, 6:58 PM
rdamazio added a comment.Mar 3 2018, 7:10 PM

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.

yuja added a subscriber: dploch.Mar 4 2018, 9:21 AM
yuja requested changes to this revision.
yuja added inline comments.
mercurial/dispatch.py
624

Maybe this type conversion can be a fancyopt.customopt method since we've
refactored the default handling by 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?

mercurial/ui.py
389

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

This revision now requires changes to proceed.Mar 4 2018, 9:21 AM
rdamazio updated this revision to Diff 6548.Mar 4 2018, 11:34 AM
rdamazio added inline comments.Mar 4 2018, 11:34 AM
mercurial/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.

mercurial/ui.py
389

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

yuja added inline comments.Mar 4 2018, 12:15 PM
mercurial/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.

rdamazio updated this revision to Diff 6609.Mar 4 2018, 3:36 PM
rdamazio added inline comments.Mar 4 2018, 3:39 PM
mercurial/dispatch.py
624

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

dploch added inline comments.Mar 5 2018, 6:29 PM
mercurial/dispatch.py
624

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

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.

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

rdamazio added inline comments.Mar 27 2018, 2:30 AM
mercurial/dispatch.py
625–626

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

639–640

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.

baymax requested changes to this revision.Jan 24 2020, 12:33 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Jan 24 2020, 12:33 PM

Revision Contents
Changeset List

PathPackages
Mmercurial/configitems.py (4 lines)
Mmercurial/dispatch.py (31 lines)
Mmercurial/ui.py (3 lines)
Mtests/test-dispatch.t (58 lines)

Diff 6609

mercurial/configitems.py

generic=True, generic=True,
) )
coreconfigitem('color', 'mode', coreconfigitem('color', 'mode',
default='auto', default='auto',
) )
coreconfigitem('color', 'pagermode', coreconfigitem('color', 'pagermode',
default=dynamicdefault, default=dynamicdefault,
) )
coreconfigitem('commands', '.*\.default\..*',
generic=True,
default=dynamicdefault,
)
coreconfigitem('commands', 'show.aliasprefix', coreconfigitem('commands', 'show.aliasprefix',
default=list, default=list,
) )
coreconfigitem('commands', 'status.relative', coreconfigitem('commands', 'status.relative',
default=False, default=False,
) )
coreconfigitem('commands', 'status.skipstates', coreconfigitem('commands', 'status.skipstates',
default=[], default=[],

mercurial/dispatch.py

ui.configbool("ui", "strict")) ui.configbool("ui", "strict"))
cmd = aliases[0] cmd = aliases[0]
args = aliasargs(entry[0], args) args = aliasargs(entry[0], args)
defaults = ui.config("defaults", cmd) defaults = ui.config("defaults", cmd)
if defaults: if defaults:
args = pycompat.maplist( args = pycompat.maplist(
util.expandpath, pycompat.shlexsplit(defaults)) + args util.expandpath, pycompat.shlexsplit(defaults)) + args
c = list(entry[1]) 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.
yujaUnsubmitted
Not Done

Maybe this type conversion can be a fancyopt.customopt method since we've
refactored the default handling by 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?

yuja: Maybe this type conversion can be a `fancyopt.customopt` method since we've refactored the…
rdamazioAuthorUnsubmitted
Not Done

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.

rdamazio: The issue is that customopt (and all its children) assume the value type is already the correct…
yujaUnsubmitted
Not Done

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.

yuja: IIUC, an extension author may implement its own customopt subclasses, and put them into the…
rdamazioAuthorUnsubmitted
Not Done

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

rdamazio: Ah, makes sense. See if this addresses that case satisfactorily - it still has the caveat of…
dplochUnsubmitted
Not Done

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

dploch: I think rdamazio's response is correct. The type information for an opt exists only in the…
newdefault = ui.configtyped("commands", cfgitem, defaulttype,
olddefault)
dplochUnsubmitted
Not Done

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.

dploch: This doesn't handle callables properly. I wonder if the something like the following would…
rdamazioAuthorUnsubmitted
Not Done

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

rdamazio: I thought callables were meant to be used to generate the default default, not with overridden…
if isinstance(olddefault, fancyopts.customopt):
def badvalue(s):
raise error.Abort(
_('invalid value %r for config option %s: %s') % (
newdefault, cfgitem, s))
# If it's a custom option, then configtyped must have parsed it
# as string - ask the customopt to parse its new default.
# Notice this does have limitations - since we have no way to
# start with a "clean" old state, this is not overriding the
# default, but rather adding to it - e.g. a non-empty list
# default will be appended to.
olddefault.defaultvalue = olddefault.newstate(
olddefault.defaultvalue, newdefault, badvalue)
dplochUnsubmitted
Not Done

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

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

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.

rdamazio: Nobody should use the same *instance* on multiple flags. Even with the current flags, if you…
elif olddefault != newdefault:
# override the default in the flag declaration.
c[idx] = (opt[0], opt[1], newdefault, opt[3])
else: else:
cmd = None cmd = None
c = [] c = []
# combine global options into local # combine global options into local
for o in commands.globalopts: for o in commands.globalopts:
c.append((o[0], o[1], options[o[1]], o[3])) c.append((o[0], o[1], options[o[1]], o[3]))

mercurial/ui.py

del cfg['commands'][k] del cfg['commands'][k]
# Don't remove aliases from the configuration if in the exceptionlist # Don't remove aliases from the configuration if in the exceptionlist
if self.plain('alias'): if self.plain('alias'):
for k, v in cfg.items('alias'): for k, v in cfg.items('alias'):
del cfg['alias'][k] del cfg['alias'][k]
if self.plain('revsetalias'): if self.plain('revsetalias'):
for k, v in cfg.items('revsetalias'): for k, v in cfg.items('revsetalias'):
del cfg['revsetalias'][k] del cfg['revsetalias'][k]
if self.plain('templatealias'): if self.plain('templatealias'):
yujaUnsubmitted
Not Done

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

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

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

rdamazio: You're right, I had the plain logic inverted in my head. Removed.
for k, v in cfg.items('templatealias'): for k, v in cfg.items('templatealias'):
del cfg['templatealias'][k] del cfg['templatealias'][k]
if trusted: if trusted:
self._tcfg.update(cfg) self._tcfg.update(cfg)
self._tcfg.update(self._ocfg) self._tcfg.update(self._ocfg)
self._ucfg.update(cfg) self._ucfg.update(cfg)
self._ucfg.update(self._ocfg) self._ucfg.update(self._ocfg)
for k, v in sub.items(): for k, v in sub.items():
uvalue = self._ucfg.get(section, '%s:%s' % (name, k)) uvalue = self._ucfg.get(section, '%s:%s' % (name, k))
if uvalue is not None and uvalue != v: if uvalue is not None and uvalue != v:
self.debug('ignoring untrusted configuration option ' self.debug('ignoring untrusted configuration option '
'%s:%s.%s = %s\n' % (section, name, k, uvalue)) '%s:%s.%s = %s\n' % (section, name, k, uvalue))
return main, sub return main, sub
def configtyped(self, section, name, itemtype, default=_unset, untrusted=False): def configtyped(self, section, name, itemtype, default=_unset,
untrusted=False):
"""Get a config item as the given type.""" """Get a config item as the given type."""
if itemtype is type(False) or itemtype is type(None): if itemtype is type(False) or itemtype is type(None):
return self.configbool(section, name, default, untrusted) return self.configbool(section, name, default, untrusted)
if itemtype is type(1): if itemtype is type(1):
return self.configint(section, name, default, untrusted) return self.configint(section, name, default, untrusted)
if itemtype is type([]): if itemtype is type([]):
return self.configlist(section, name, default, untrusted) return self.configlist(section, name, default, untrusted)
return self.config(section, name, default, untrusted) return self.config(section, name, default, untrusted)

tests/test-dispatch.t

test command parsing and dispatch test command parsing and dispatch
$ hg init a $ hg init a
$ cd a $ cd a
Redundant options used to crash (issue436): Redundant options used to crash (issue436):
$ hg -v log -v $ hg -v log -v
$ hg -v log -v x $ hg -v log -v x
$ echo a > a $ echo a > a
$ echo b > b
$ hg ci -Ama $ hg ci -Ama
adding a adding a
adding b
Missing arg: Missing arg:
$ hg cat $ hg cat
hg cat: invalid arguments hg cat: invalid arguments
hg cat [OPTION]... FILE... hg cat [OPTION]... FILE...
output the current or given revision of files output the current or given revision of files
[255] [255]
$ hg log -T -- $ hg log -T --
-- (no-eol) -- (no-eol)
$ hg log -T -- -k nomatch $ hg log -T -- -k nomatch
Parsing of early options should stop at "--": Parsing of early options should stop at "--":
$ hg cat -- --config=hooks.pre-cat=false $ 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] [1]
$ hg cat -- --debugger $ hg cat -- --debugger
--debugger: no such file in rev cb9a9f314b8b --debugger: no such file in rev 0cd96de13884
[1] [1]
Unparsable form of early options: Unparsable form of early options:
$ hg cat --debugg $ hg cat --debugg
abort: option --debugger may not be abbreviated! abort: option --debugger may not be abbreviated!
[255] [255]
$ HGPLAIN=+strictflags hg log -q -R . -b default $ HGPLAIN=+strictflags hg log -q -R . -b default
abort: option -R has to be separated from other options (e.g. not -qR) and --repository may only be abbreviated as --repo! abort: option -R has to be separated from other options (e.g. not -qR) and --repository may only be abbreviated as --repo!
[255] [255]
$ HGPLAIN=+strictflags hg --config='hooks.pre-log=false' log -b default $ HGPLAIN=+strictflags hg --config='hooks.pre-log=false' log -b default
abort: pre-log hook exited with status 1 abort: pre-log hook exited with status 1
[255] [255]
$ HGPLAIN=+strictflags hg --cwd .. -q -Ra log -b default $ HGPLAIN=+strictflags hg --cwd .. -q -Ra log -b default
0:cb9a9f314b8b 0:0cd96de13884
$ HGPLAIN=+strictflags hg --cwd .. -q --repository a log -b default $ HGPLAIN=+strictflags hg --cwd .. -q --repository a log -b default
0:cb9a9f314b8b 0:0cd96de13884
$ HGPLAIN=+strictflags hg --cwd .. -q --repo a log -b default $ 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: For compatibility reasons, HGPLAIN=+strictflags is not enabled by plain HGPLAIN:
$ HGPLAIN= hg log --config='hooks.pre-log=false' -b default $ HGPLAIN= hg log --config='hooks.pre-log=false' -b default
abort: pre-log hook exited with status 1 abort: pre-log hook exited with status 1
[255] [255]
$ HGPLAINEXCEPT= hg log --cwd .. -q -Ra -b default $ HGPLAINEXCEPT= hg log --cwd .. -q -Ra -b default
0:cb9a9f314b8b 0:0cd96de13884
[defaults] [defaults]
$ hg cat a $ hg cat a
a a
$ cp $HGRCPATH hgrc.bak
$ cat >> $HGRCPATH <<EOF $ cat >> $HGRCPATH <<EOF
> [defaults] > [defaults]
> cat = -r null > cat = -r null
> EOF > EOF
$ hg cat a $ hg cat a
a: no such file in rev 000000000000 a: no such file in rev 000000000000
[1] [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" $ cd "$TESTTMP"
OSError "No such file or directory" / "The system cannot find the path OSError "No such file or directory" / "The system cannot find the path
specified" should include filename even when it is empty specified" should include filename even when it is empty
$ hg -R a archive '' $ hg -R a archive ''
abort: *: '' (glob) abort: *: '' (glob)