Patchwork [2,of,7,v3,flags] fancyopts: disallow true as a boolean flag default (API)

login
register
mail settings
Submitter Augie Fackler
Date Sept. 14, 2016, 3:11 a.m.
Message ID <828f260114a3a55e246c.1473822678@augie-macbookair2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/16616/
State Changes Requested
Headers show

Comments

Augie Fackler - Sept. 14, 2016, 3:11 a.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1472584421 14400
#      Tue Aug 30 15:13:41 2016 -0400
# Node ID 828f260114a3a55e246cb5de434e75bdc782e4ad
# Parent  600be3c9acee0ec14bd19c032cc0480e4501fe8c
fancyopts: disallow true as a boolean flag default (API)

This was nonsense, as there's not been a way for the user to
un-specify the flag. Restricting this behavior will open the door to
some nice fit-and-finish functionality in a later patch, and shouldn't
break any third party extensions. This is marked as (API) since it
might break a third party extension, but given the fact that it was
silly before that's mostly a formality.

Due to how early parsing of global options works, we have to add some
extra coupling between fancyopts and dispatch so that we can provide a
"default" of True for already-parsed boolean flags when doing
command-level flag parsing.
Yuya Nishihara - Sept. 14, 2016, 3:03 p.m.
On Tue, 13 Sep 2016 23:11:18 -0400, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1472584421 14400
> #      Tue Aug 30 15:13:41 2016 -0400
> # Node ID 828f260114a3a55e246cb5de434e75bdc782e4ad
> # Parent  600be3c9acee0ec14bd19c032cc0480e4501fe8c
> fancyopts: disallow true as a boolean flag default (API)
> 
> This was nonsense, as there's not been a way for the user to
> un-specify the flag. Restricting this behavior will open the door to
> some nice fit-and-finish functionality in a later patch, and shouldn't
> break any third party extensions. This is marked as (API) since it
> might break a third party extension, but given the fact that it was
> silly before that's mostly a formality.

Yes, it was silly before, but now it isn't because we have --no-<opt>.
We can define ('status', default=True) in place of ('no-status', default=False).
Though I'm sure we'll need something to mark default-True flags in help, I
think that will still be simpler than ditching default=True and False.

For None/False/True tristate, I'd rather make it <empty>/False/True and
test 'key in opts' explicitly.
Augie Fackler - Sept. 14, 2016, 3:05 p.m.
> On Sep 14, 2016, at 11:03, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Tue, 13 Sep 2016 23:11:18 -0400, Augie Fackler wrote:
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1472584421 14400
>> #      Tue Aug 30 15:13:41 2016 -0400
>> # Node ID 828f260114a3a55e246cb5de434e75bdc782e4ad
>> # Parent  600be3c9acee0ec14bd19c032cc0480e4501fe8c
>> fancyopts: disallow true as a boolean flag default (API)
>> 
>> This was nonsense, as there's not been a way for the user to
>> un-specify the flag. Restricting this behavior will open the door to
>> some nice fit-and-finish functionality in a later patch, and shouldn't
>> break any third party extensions. This is marked as (API) since it
>> might break a third party extension, but given the fact that it was
>> silly before that's mostly a formality.
> 
> Yes, it was silly before, but now it isn't because we have --no-<opt>.
> We can define ('status', default=True) in place of ('no-status', default=False).
> Though I'm sure we'll need something to mark default-True flags in help, I
> think that will still be simpler than ditching default=True and False.
> 
> For None/False/True tristate, I'd rather make it <empty>/False/True and
> test 'key in opts' explicitly.

Aspirationally I agree, but that's going to be a much more invasive change than I'd really like to deal with right now (lots of commands *depend* on the default being falsy, and having an explicit "unset" value will require a fair amount of cleanup (diffopts and rebase both come to mind as code that will require love, and I'm sure there's a bunch more I didn't see.)
Yuya Nishihara - Sept. 14, 2016, 3:30 p.m.
On Wed, 14 Sep 2016 11:05:40 -0400, Augie Fackler wrote:
> > On Sep 14, 2016, at 11:03, Yuya Nishihara <yuya@tcha.org> wrote:
> > 
> > On Tue, 13 Sep 2016 23:11:18 -0400, Augie Fackler wrote:
> >> # HG changeset patch
> >> # User Augie Fackler <augie@google.com>
> >> # Date 1472584421 14400
> >> #      Tue Aug 30 15:13:41 2016 -0400
> >> # Node ID 828f260114a3a55e246cb5de434e75bdc782e4ad
> >> # Parent  600be3c9acee0ec14bd19c032cc0480e4501fe8c
> >> fancyopts: disallow true as a boolean flag default (API)
> >> 
> >> This was nonsense, as there's not been a way for the user to
> >> un-specify the flag. Restricting this behavior will open the door to
> >> some nice fit-and-finish functionality in a later patch, and shouldn't
> >> break any third party extensions. This is marked as (API) since it
> >> might break a third party extension, but given the fact that it was
> >> silly before that's mostly a formality.
> > 
> > Yes, it was silly before, but now it isn't because we have --no-<opt>.
> > We can define ('status', default=True) in place of ('no-status', default=False).
> > Though I'm sure we'll need something to mark default-True flags in help, I
> > think that will still be simpler than ditching default=True and False.
> > 
> > For None/False/True tristate, I'd rather make it <empty>/False/True and
> > test 'key in opts' explicitly.
> 
> Aspirationally I agree, but that's going to be a much more invasive change than I'd really like to deal with right now (lots of commands *depend* on the default being falsy, and having an explicit "unset" value will require a fair amount of cleanup (diffopts and rebase both come to mind as code that will require love, and I'm sure there's a bunch more I didn't see.)

But that can be changed incrementally. We need tristate for diffopts because
they are computed from command and config options, but many other commands take
None and False as the same value so they can remain to default to None/False.

I think --no-git is the most important stuff, which won't be difficult to
port to <unset> since opts are processed by patch.diff*opts().
Augie Fackler - Sept. 14, 2016, 3:52 p.m.
(sorry for the length of this reply)

> On Sep 14, 2016, at 11:30, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Wed, 14 Sep 2016 11:05:40 -0400, Augie Fackler wrote:
>>> On Sep 14, 2016, at 11:03, Yuya Nishihara <yuya@tcha.org> wrote:
>>> 
>>> On Tue, 13 Sep 2016 23:11:18 -0400, Augie Fackler wrote:
>>>> # HG changeset patch
>>>> # User Augie Fackler <augie@google.com>
>>>> # Date 1472584421 14400
>>>> #      Tue Aug 30 15:13:41 2016 -0400
>>>> # Node ID 828f260114a3a55e246cb5de434e75bdc782e4ad
>>>> # Parent  600be3c9acee0ec14bd19c032cc0480e4501fe8c
>>>> fancyopts: disallow true as a boolean flag default (API)

[...]

> But that can be changed incrementally. We need tristate for diffopts because
> they are computed from command and config options, but many other commands take
> None and False as the same value so they can remain to default to None/False.
> 
> I think --no-git is the most important stuff, which won't be difficult to
> port to <unset> since opts are processed by patch.diff*opts().

Is the idea something like:

fancyopts.py:

unsetbool = object()

in commands.py:

 diffopts = [
-     ('a', 'text', None, _('treat all files as text')),
+     ('a', 'text', fancyopts.unsetbool, _('treat all files as text')),
-     ('g', 'git', None, _('use git extended diff format')),
+     ('g', 'git', fancyopts.unsetbool, _('use git extended diff format')),
-     ('', 'nodates', None, _('omit dates from diff headers'))
+     ('', 'nodates', fancyopts.unsetbool, _('omit dates from diff headers'))
 ]

And then we could synthesize negated versions only for booleans that have fancyopts.unsetbool as the default? That'd be less generic (and so require a lot more cleanup work on a per-option basis), but maybe that's a good thing? Note that changing these entries in diffopts do stand to be at least a little bit risky for extensions (they may not be ready for the fancyopts.unsetbool part of the tai-state), but I don't think that's something we should worry about overmuch.

I could also see room for something like

class unsetbool(object):
  default = True
  def __init__(self, v):
    self._v = v
  def __bool__(self):
    return self._v

and then you'd have your default value be specified as fancyopts.unsetbool(True|False). I don't like this as well, but I can't put my finger on why (a previous round of this series between v2 and v3 actually went down this route and it got somewhat messy, but I was trying to be more generic and automatic about it).

Regardless, both of these new proposed approaches still leave us in the awkward place of having to figure out how to describe the default for a boolean flag in the help. 


If we go ahead with what I've got, that pretty well ties our hands in the future. I'd still rather do this[1] (it's already done, and means boolean flags are globally consistent behavior-wise) than have to go through and tweak the default value on nearly boolean flag in the codebase (and still have extensions get zero benefit). It also makes documentation simpler[0] - the default mode of a flag is the opposite of what shows up in help (so if the default of 'check' is false, then we show --check, but if the default is --check, we describe the flag as --no-check in the help output.)

How do people want to proceed?

AF

0: although still hard - even just notating that a flag can be negated without true-default as a possibility doesn't have an obvious correct answer yet. See patch 7.

1: I don't want to rush this through, even if it sort of sounds like that from this mail.
Yuya Nishihara - Sept. 14, 2016, 4:37 p.m.
On Wed, 14 Sep 2016 11:52:59 -0400, Augie Fackler wrote:
> > On Sep 14, 2016, at 11:30, Yuya Nishihara <yuya@tcha.org> wrote:
> Is the idea something like:
> 
> fancyopts.py:
> 
> unsetbool = object()
> 
> in commands.py:
> 
>  diffopts = [
> -     ('a', 'text', None, _('treat all files as text')),
> +     ('a', 'text', fancyopts.unsetbool, _('treat all files as text')),
> -     ('g', 'git', None, _('use git extended diff format')),
> +     ('g', 'git', fancyopts.unsetbool, _('use git extended diff format')),
> -     ('', 'nodates', None, _('omit dates from diff headers'))
> +     ('', 'nodates', fancyopts.unsetbool, _('omit dates from diff headers'))
>  ]

Yes.

> And then we could synthesize negated versions only for booleans that have
> fancyopts.unsetbool as the default?

No. My idea is to not set 'git' at all.

  default:  {}  # not {'git': None}
  --git:    {'git': True}
  --no-git: {'git': False}

and falls back to ui.configbool if 'git' not in opts.

My point is diffopts are special in that their defaults are configurable. Many
other boolean options have no such feature, so they can simply be default=None
(or False.)
Augie Fackler - Sept. 14, 2016, 9:51 p.m.
> On Sep 14, 2016, at 12:37, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Wed, 14 Sep 2016 11:52:59 -0400, Augie Fackler wrote:
>>> On Sep 14, 2016, at 11:30, Yuya Nishihara <yuya@tcha.org> wrote:
>> Is the idea something like:
>> 
>> fancyopts.py:
>> 
>> unsetbool = object()
>> 
>> in commands.py:
>> 
>> diffopts = [
>> -     ('a', 'text', None, _('treat all files as text')),
>> +     ('a', 'text', fancyopts.unsetbool, _('treat all files as text')),
>> -     ('g', 'git', None, _('use git extended diff format')),
>> +     ('g', 'git', fancyopts.unsetbool, _('use git extended diff format')),
>> -     ('', 'nodates', None, _('omit dates from diff headers'))
>> +     ('', 'nodates', fancyopts.unsetbool, _('omit dates from diff headers'))
>> ]
> 
> Yes.
> 
>> And then we could synthesize negated versions only for booleans that have
>> fancyopts.unsetbool as the default?
> 
> No. My idea is to not set 'git' at all.
> 
>  default:  {}  # not {'git': None}
>  --git:    {'git': True}
>  --no-git: {'git': False}
> 
> and falls back to ui.configbool if 'git' not in opts.
> 
> My point is diffopts are special in that their defaults are configurable. Many
> other boolean options have no such feature, so they can simply be default=None
> (or False.)

Ah, I see. I'll see about rolling a v4 that uses this approach so we can then not mangle true/false as a default.
Pierre-Yves David - Sept. 16, 2016, 1:48 p.m.
On 09/14/2016 05:11 AM, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1472584421 14400
> #      Tue Aug 30 15:13:41 2016 -0400
> # Node ID 828f260114a3a55e246cb5de434e75bdc782e4ad
> # Parent  600be3c9acee0ec14bd19c032cc0480e4501fe8c
> fancyopts: disallow true as a boolean flag default (API)
>
> This was nonsense, as there's not been a way for the user to
> un-specify the flag. Restricting this behavior will open the door to
> some nice fit-and-finish functionality in a later patch, and shouldn't
> break any third party extensions. This is marked as (API) since it
> might break a third party extension, but given the fact that it was
> silly before that's mostly a formality.

I really wish we had details on this as requested in the review of the 
previous version. Especially because I remember that forbidding 'True' 
as default was making other improvement hard so I'm not sure why we have 
to do this.

> Due to how early parsing of global options works, we have to add some
> extra coupling between fancyopts and dispatch so that we can provide a
> "default" of True for already-parsed boolean flags when doing
> command-level flag parsing.
>
> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -555,7 +555,10 @@ def _parse(ui, args):
>
>      # combine global options into local
>      for o in commands.globalopts:
> -        c.append((o[0], o[1], options[o[1]], o[3]))
> +        # Passing a tuple of length six through into the option parser
> +        # allows otherwise-illegal defaults to survive, which is how
> +        # we handle global options like --quiet.
> +        c.append((o[0], o[1], options[o[1]], o[3], '', True))
>
>      try:
>          args = fancyopts.fancyopts(args, c, cmdoptions, gnu=True)
> diff --git a/mercurial/fancyopts.py b/mercurial/fancyopts.py
> --- a/mercurial/fancyopts.py
> +++ b/mercurial/fancyopts.py
> @@ -79,10 +79,22 @@ def fancyopts(args, options, state, gnu=
>      alllong = set(o[1] for o in options)
>
>      for option in options:
> -        if len(option) == 5:
> +        boolok = False
> +        if len(option) == 6:
> +            # If the tuple is of length 6, then it's a global option
> +            # that was already parsed, so we're really just passing
> +            # its "default" through the second phase of flags parsing
> +            # (for command-level flags). As a result, we have to allow
> +            # defaults of True and not rewrite defaults of False.
> +            short, name, default, comment, dummy, dummy = option
> +            boolok = True
> +        elif len(option) == 5:
>              short, name, default, comment, dummy = option
>          else:
>              short, name, default, comment = option
> +        if default is True and not boolok:
> +            raise ValueError('fancyopts does not support default-true '
> +                             'boolean flags: %r' % name)

As pointed in a previous review, identity testing to boolean really 
raise red flag here. I see this as a significant sign of a bad idea and 
something we should avoid at all cost.

I see you have a longer reply about handling of the tri-state later in 
this thread, I'll reply with more details there.

Cheers,
Pierre-Yves David - Sept. 16, 2016, 1:48 p.m.
On 09/14/2016 06:37 PM, Yuya Nishihara wrote:
> On Wed, 14 Sep 2016 11:52:59 -0400, Augie Fackler wrote:
>>> On Sep 14, 2016, at 11:30, Yuya Nishihara <yuya@tcha.org> wrote:
>> Is the idea something like:
>>
>> fancyopts.py:
>>
>> unsetbool = object()
>>
>> in commands.py:
>>
>>  diffopts = [
>> -     ('a', 'text', None, _('treat all files as text')),
>> +     ('a', 'text', fancyopts.unsetbool, _('treat all files as text')),
>> -     ('g', 'git', None, _('use git extended diff format')),
>> +     ('g', 'git', fancyopts.unsetbool, _('use git extended diff format')),
>> -     ('', 'nodates', None, _('omit dates from diff headers'))
>> +     ('', 'nodates', fancyopts.unsetbool, _('omit dates from diff headers'))
>>  ]
>
> Yes.
>
>> And then we could synthesize negated versions only for booleans that have
>> fancyopts.unsetbool as the default?
>
> No. My idea is to not set 'git' at all.
>
>   default:  {}  # not {'git': None}
>   --git:    {'git': True}
>   --no-git: {'git': False}
>
> and falls back to ui.configbool if 'git' not in opts.
>
> My point is diffopts are special in that their defaults are configurable. Many
> other boolean options have no such feature, so they can simply be default=None
> (or False.)

So far, the option parsing logic have ensure options always exist and 
have some value in the 'opts' dictionary. Changing this would be a 
massive API breakage. I really don't think we should go this route.
Pierre-Yves David - Sept. 16, 2016, 2:06 p.m.
On 09/14/2016 05:52 PM, Augie Fackler wrote:
> (sorry for the length of this reply)
>
>> On Sep 14, 2016, at 11:30, Yuya Nishihara <yuya@tcha.org> wrote:
>>
>> On Wed, 14 Sep 2016 11:05:40 -0400, Augie Fackler wrote:
>>>> On Sep 14, 2016, at 11:03, Yuya Nishihara <yuya@tcha.org> wrote:
>>>>
>>>> On Tue, 13 Sep 2016 23:11:18 -0400, Augie Fackler wrote:
>>>>> # HG changeset patch
>>>>> # User Augie Fackler <augie@google.com>
>>>>> # Date 1472584421 14400
>>>>> #      Tue Aug 30 15:13:41 2016 -0400
>>>>> # Node ID 828f260114a3a55e246cb5de434e75bdc782e4ad
>>>>> # Parent  600be3c9acee0ec14bd19c032cc0480e4501fe8c
>>>>> fancyopts: disallow true as a boolean flag default (API)
>
> [...]
>
>> But that can be changed incrementally. We need tristate for diffopts because
>> they are computed from command and config options, but many other commands take
>> None and False as the same value so they can remain to default to None/False.
>>
>> I think --no-git is the most important stuff, which won't be difficult to
>> port to <unset> since opts are processed by patch.diff*opts().
>
> Is the idea something like:
>
> fancyopts.py:
>
> unsetbool = object()
>
> in commands.py:
>
>  diffopts = [
> -     ('a', 'text', None, _('treat all files as text')),
> +     ('a', 'text', fancyopts.unsetbool, _('treat all files as text')),
> -     ('g', 'git', None, _('use git extended diff format')),
> +     ('g', 'git', fancyopts.unsetbool, _('use git extended diff format')),
> -     ('', 'nodates', None, _('omit dates from diff headers'))
> +     ('', 'nodates', fancyopts.unsetbool, _('omit dates from diff headers'))
>  ]
>
> And then we could synthesize negated versions only for booleans that have fancyopts.unsetbool as the default? That'd be less generic (and so require a lot more cleanup work on a per-option basis), but maybe that's a good thing? Note that changing these entries in diffopts do stand to be at least a little bit risky for extensions (they may not be ready for the fancyopts.unsetbool part of the tai-state), but I don't think that's something we should worry about overmuch.
>
> I could also see room for something like
>
> class unsetbool(object):
>   default = True
>   def __init__(self, v):
>     self._v = v
>   def __bool__(self):
>     return self._v
>
> and then you'd have your default value be specified as fancyopts.unsetbool(True|False). I don't like this as well, but I can't put my finger on why (a previous round of this series between v2 and v3 actually went down this route and it got somewhat messy, but I was trying to be more generic and automatic about it).
>
> Regardless, both of these new proposed approaches still leave us in the awkward place of having to figure out how to describe the default for a boolean flag in the help.
>
>
> If we go ahead with what I've got, that pretty well ties our hands in the future. I'd still rather do this[1] (it's already done, and means boolean flags are globally consistent behavior-wise) than have to go through and tweak the default value on nearly boolean flag in the codebase (and still have extensions get zero benefit). It also makes documentation simpler[0] - the default mode of a flag is the opposite of what shows up in help (so if the default of 'check' is false, then we show --check, but if the default is --check, we describe the flag as --no-check in the help output.)
>
> How do people want to proceed?

I'm really opposed to having identity testing for True and False. This 
would not prevent us to use 'None' as 'default value' (I'm fine with 
identity testing with None) but that prevent 'default to True' setting 
which seems unfortunate. This is also less flexible for future evolution.

I think having a dedicated 'booleanobject' for the default value make 
senses here. It keeps all the existing code unchanged but for the place 
that are explicitly interested in checking for command line override. 
This is probably my preferred solution.

The could have a 'defaulttrue' and 'defaultfalse' value, with some 
'defaultvalue' attribute set to True for the code who care.

We could either update the existing definition to use these, of simply 
doing what you have done and have fancyopt to replace the value on the 
Fly when it see them (probably my preferred solution)

Supporting default to True seem useful even if mostly rare. Regarding 
documentation, when default is True, I think we could either:
- display: the --no-foobar version in the help
- add '(set by default)' to te help.

Using object for default value give use for flexibility. We could even 
imagine a future improvement were options have their config counterpart 
directly handled during option parsing, returning a slightly different 
object to keep track of the origin of that 'default' value. This would 
allow the help to report config based override to the user.

Cheers,
Yuya Nishihara - Sept. 16, 2016, 2:16 p.m.
On Fri, 16 Sep 2016 15:48:32 +0200, Pierre-Yves David wrote:
> On 09/14/2016 06:37 PM, Yuya Nishihara wrote:
> > On Wed, 14 Sep 2016 11:52:59 -0400, Augie Fackler wrote:
> >>> On Sep 14, 2016, at 11:30, Yuya Nishihara <yuya@tcha.org> wrote:
> >> Is the idea something like:
> >>
> >> fancyopts.py:
> >>
> >> unsetbool = object()
> >>
> >> in commands.py:
> >>
> >>  diffopts = [
> >> -     ('a', 'text', None, _('treat all files as text')),
> >> +     ('a', 'text', fancyopts.unsetbool, _('treat all files as text')),
> >> -     ('g', 'git', None, _('use git extended diff format')),
> >> +     ('g', 'git', fancyopts.unsetbool, _('use git extended diff format')),
> >> -     ('', 'nodates', None, _('omit dates from diff headers'))
> >> +     ('', 'nodates', fancyopts.unsetbool, _('omit dates from diff headers'))
> >>  ]
> >
> > Yes.
> >
> >> And then we could synthesize negated versions only for booleans that have
> >> fancyopts.unsetbool as the default?
> >
> > No. My idea is to not set 'git' at all.
> >
> >   default:  {}  # not {'git': None}
> >   --git:    {'git': True}
> >   --no-git: {'git': False}
> >
> > and falls back to ui.configbool if 'git' not in opts.
> >
> > My point is diffopts are special in that their defaults are configurable. Many
> > other boolean options have no such feature, so they can simply be default=None
> > (or False.)
> 
> So far, the option parsing logic have ensure options always exist and 
> have some value in the 'opts' dictionary. Changing this would be a 
> massive API breakage. I really don't think we should go this route.

That's why 'unsetbool' will be opt-in. We sometimes do opts.get() to support
internal function calls not through dispatch(), so I think it's okay to omit
explicitly-defined keys.

That said, I'm not opposed to the special defaulttrue/false objects.

Another crazy idea is to add 'bool_to_be_read_from_ui(section, name)' and pass
ui to fancyopts.

  ('g', 'git', bool_to_be_read_from_ui('diff', 'git'), ...)

Good thing is "help diff" can show --no-git if diff.git is set, but this would
be overly complicated.
Augie Fackler - Sept. 16, 2016, 2:21 p.m.
> On Sep 16, 2016, at 09:48, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> 
> 
> On 09/14/2016 05:11 AM, Augie Fackler wrote:
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1472584421 14400
>> #      Tue Aug 30 15:13:41 2016 -0400
>> # Node ID 828f260114a3a55e246cb5de434e75bdc782e4ad
>> # Parent  600be3c9acee0ec14bd19c032cc0480e4501fe8c
>> fancyopts: disallow true as a boolean flag default (API)
>> 
>> This was nonsense, as there's not been a way for the user to
>> un-specify the flag. Restricting this behavior will open the door to
>> some nice fit-and-finish functionality in a later patch, and shouldn't
>> break any third party extensions. This is marked as (API) since it
>> might break a third party extension, but given the fact that it was
>> silly before that's mostly a formality.
> 
> I really wish we had details on this as requested in the review of the previous version. Especially because I remember that forbidding 'True' as default was making other improvement hard so I'm not sure why we have to do this.

Err? I'm not sure what you're asking for now, and I definitely didn't understand it on the last go round.

>> Due to how early parsing of global options works, we have to add some
>> extra coupling between fancyopts and dispatch so that we can provide a
>> "default" of True for already-parsed boolean flags when doing
>> command-level flag parsing.
>> 
>> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
>> --- a/mercurial/dispatch.py
>> +++ b/mercurial/dispatch.py
>> @@ -555,7 +555,10 @@ def _parse(ui, args):
>> 
>>     # combine global options into local
>>     for o in commands.globalopts:
>> -        c.append((o[0], o[1], options[o[1]], o[3]))
>> +        # Passing a tuple of length six through into the option parser
>> +        # allows otherwise-illegal defaults to survive, which is how
>> +        # we handle global options like --quiet.
>> +        c.append((o[0], o[1], options[o[1]], o[3], '', True))
>> 
>>     try:
>>         args = fancyopts.fancyopts(args, c, cmdoptions, gnu=True)
>> diff --git a/mercurial/fancyopts.py b/mercurial/fancyopts.py
>> --- a/mercurial/fancyopts.py
>> +++ b/mercurial/fancyopts.py
>> @@ -79,10 +79,22 @@ def fancyopts(args, options, state, gnu=
>>     alllong = set(o[1] for o in options)
>> 
>>     for option in options:
>> -        if len(option) == 5:
>> +        boolok = False
>> +        if len(option) == 6:
>> +            # If the tuple is of length 6, then it's a global option
>> +            # that was already parsed, so we're really just passing
>> +            # its "default" through the second phase of flags parsing
>> +            # (for command-level flags). As a result, we have to allow
>> +            # defaults of True and not rewrite defaults of False.
>> +            short, name, default, comment, dummy, dummy = option
>> +            boolok = True
>> +        elif len(option) == 5:
>>             short, name, default, comment, dummy = option
>>         else:
>>             short, name, default, comment = option
>> +        if default is True and not boolok:
>> +            raise ValueError('fancyopts does not support default-true '
>> +                             'boolean flags: %r' % name)
> 
> As pointed in a previous review, identity testing to boolean really raise red flag here. I see this as a significant sign of a bad idea and something we should avoid at all cost.

Calm down - this isn't the one that was pointed out in the past, and I didn't notice this one as I was doing reworks for v3. I don't disagree with your opinion here.

It sounds like you should be mailing a check-code rule about 'is (True|False)' to forbid this pattern.

> I see you have a longer reply about handling of the tri-state later in this thread, I'll reply with more details there.
> 
> Cheers,
> 
> -- 
> Pierre-Yves David
Pierre-Yves David - Sept. 16, 2016, 2:22 p.m.
On 09/16/2016 04:16 PM, Yuya Nishihara wrote:
> On Fri, 16 Sep 2016 15:48:32 +0200, Pierre-Yves David wrote:
>> On 09/14/2016 06:37 PM, Yuya Nishihara wrote:
>>> On Wed, 14 Sep 2016 11:52:59 -0400, Augie Fackler wrote:
>>>>> On Sep 14, 2016, at 11:30, Yuya Nishihara <yuya@tcha.org> wrote:
>>>> Is the idea something like:
>>>>
>>>> fancyopts.py:
>>>>
>>>> unsetbool = object()
>>>>
>>>> in commands.py:
>>>>
>>>>  diffopts = [
>>>> -     ('a', 'text', None, _('treat all files as text')),
>>>> +     ('a', 'text', fancyopts.unsetbool, _('treat all files as text')),
>>>> -     ('g', 'git', None, _('use git extended diff format')),
>>>> +     ('g', 'git', fancyopts.unsetbool, _('use git extended diff format')),
>>>> -     ('', 'nodates', None, _('omit dates from diff headers'))
>>>> +     ('', 'nodates', fancyopts.unsetbool, _('omit dates from diff headers'))
>>>>  ]
>>>
>>> Yes.
>>>
>>>> And then we could synthesize negated versions only for booleans that have
>>>> fancyopts.unsetbool as the default?
>>>
>>> No. My idea is to not set 'git' at all.
>>>
>>>   default:  {}  # not {'git': None}
>>>   --git:    {'git': True}
>>>   --no-git: {'git': False}
>>>
>>> and falls back to ui.configbool if 'git' not in opts.
>>>
>>> My point is diffopts are special in that their defaults are configurable. Many
>>> other boolean options have no such feature, so they can simply be default=None
>>> (or False.)
>>
>> So far, the option parsing logic have ensure options always exist and
>> have some value in the 'opts' dictionary. Changing this would be a
>> massive API breakage. I really don't think we should go this route.
>
> That's why 'unsetbool' will be opt-in. We sometimes do opts.get() to support
> internal function calls not through dispatch(), so I think it's okay to omit
> explicitly-defined keys.
>
> That said, I'm not opposed to the special defaulttrue/false objects.
>
> Another crazy idea is to add 'bool_to_be_read_from_ui(section, name)' and pass
> ui to fancyopts.
>
>   ('g', 'git', bool_to_be_read_from_ui('diff', 'git'), ...)
>
> Good thing is "help diff" can show --no-git if diff.git is set, but this would
> be overly complicated.

marrying the command and config default option is definitly tempting, 
but I think we should keep this out of this first step for the sake of 
simplicity. In particular, we should probably have some centralised 
registry of config option in the process of making the advanced version 
happen.

Cheers.
Augie Fackler - Sept. 16, 2016, 2:33 p.m.
On Fri, Sep 16, 2016 at 10:29 AM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>>> On 09/14/2016 05:11 AM, Augie Fackler wrote:
>>>
>>>> # HG changeset patch
>>>> # User Augie Fackler <augie@google.com>
>>>> # Date 1472584421 14400
>>>> #      Tue Aug 30 15:13:41 2016 -0400
>>>> # Node ID 828f260114a3a55e246cb5de434e75bdc782e4ad
>>>> # Parent  600be3c9acee0ec14bd19c032cc0480e4501fe8c
>>>> fancyopts: disallow true as a boolean flag default (API)
>>>>
>>>> This was nonsense, as there's not been a way for the user to
>>>> un-specify the flag. Restricting this behavior will open the door to
>>>> some nice fit-and-finish functionality in a later patch, and shouldn't
>>>> break any third party extensions. This is marked as (API) since it
>>>> might break a third party extension, but given the fact that it was
>>>> silly before that's mostly a formality.
>>>>
>>>
>>> I really wish we had details on this as requested in the review of the
>>> previous version. Especially because I remember that forbidding 'True' as
>>> default was making other improvement hard so I'm not sure why we have to do
>>> this.
>>>
>>
>> Err? I'm not sure what you're asking for now, and I definitely didn't
>> understand it on the last go round.
>>
>
> Okay, let me clarified, My question here is:
>
> Why is this problematic to have default to 'True Value'
>
>
> You refered this as """These cases are a bit problematic, because we don't
> really have a way to specify default-true flags""" in
> https://www.mercurial-scm.org/pipermail/mercurial-devel/2016
> -September/087968.html
>
> I'm seeing another avatar of this question when you says "Restricting this
> behavior will open the door to some nice fit-and-finish functionality in a
> later patch" in the current thread. I would like details on this "nice
> fit-and-finish" features.
>
> Does this clarify my question ?



Sure. The answer (which I can fold into the log message if you think it's
important) is that because of how opts make it into diffopts, we can't rely
on opts to not be filled in for a value that was unspecified on the command
line. Because of that, we need to find a backwards compatible way to thread
defaults through such that they can be identified as defaults. The easy way
out is to use None for default and False for explicitly-false.

I'm open to other solutions on the tri-state, but it's a significant piece
of work that I don't really want to take on.
Augie Fackler - Sept. 16, 2016, 2:48 p.m.
On Thu, Sep 15, 2016 at 01:37:26AM +0900, Yuya Nishihara wrote:
> On Wed, 14 Sep 2016 11:52:59 -0400, Augie Fackler wrote:
> > > On Sep 14, 2016, at 11:30, Yuya Nishihara <yuya@tcha.org> wrote:
> > Is the idea something like:
> >
> > fancyopts.py:
> >
> > unsetbool = object()
> >
> > in commands.py:
> >
> >  diffopts = [
> > -     ('a', 'text', None, _('treat all files as text')),
> > +     ('a', 'text', fancyopts.unsetbool, _('treat all files as text')),
> > -     ('g', 'git', None, _('use git extended diff format')),
> > +     ('g', 'git', fancyopts.unsetbool, _('use git extended diff format')),
> > -     ('', 'nodates', None, _('omit dates from diff headers'))
> > +     ('', 'nodates', fancyopts.unsetbool, _('omit dates from diff headers'))
> >  ]
>
> Yes.
>
> > And then we could synthesize negated versions only for booleans that have
> > fancyopts.unsetbool as the default?
>
> No. My idea is to not set 'git' at all.
>
>   default:  {}  # not {'git': None}
>   --git:    {'git': True}
>   --no-git: {'git': False}
>
> and falls back to ui.configbool if 'git' not in opts.
>
> My point is diffopts are special in that their defaults are configurable. Many
> other boolean options have no such feature, so they can simply be default=None
> (or False.)

Yeah. I'm running out of energy on this topic, and this doesn't look
like it's an easy refactor to me. Can we map out a path that works
with the existing True/False/None tri-state for the time being, and
consider coming back to this later?

I like the idea of command line flags that can tie in to config
options (this would actually be a huge win for hgsubversion, we've got
some nasty code there to handle it), but it feels like an annoying
refactor that is mostly orthogonal to my actual goals here.
Pierre-Yves David - Sept. 16, 2016, 2:58 p.m.
On 09/16/2016 04:33 PM, Augie Fackler wrote:
>
> On Fri, Sep 16, 2016 at 10:29 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>
>
>             On 09/14/2016 05:11 AM, Augie Fackler wrote:
>
>                 # HG changeset patch
>                 # User Augie Fackler <augie@google.com
>                 <mailto:augie@google.com>>
>                 # Date 1472584421 14400
>                 #      Tue Aug 30 15:13:41 2016 -0400
>                 # Node ID 828f260114a3a55e246cb5de434e75bdc782e4ad
>                 # Parent  600be3c9acee0ec14bd19c032cc0480e4501fe8c
>                 fancyopts: disallow true as a boolean flag default (API)
>
>                 This was nonsense, as there's not been a way for the user to
>                 un-specify the flag. Restricting this behavior will open
>                 the door to
>                 some nice fit-and-finish functionality in a later patch,
>                 and shouldn't
>                 break any third party extensions. This is marked as
>                 (API) since it
>                 might break a third party extension, but given the fact
>                 that it was
>                 silly before that's mostly a formality.
>
>
>             I really wish we had details on this as requested in the
>             review of the previous version. Especially because I
>             remember that forbidding 'True' as default was making other
>             improvement hard so I'm not sure why we have to do this.
>
>
>         Err? I'm not sure what you're asking for now, and I definitely
>         didn't understand it on the last go round.
>
>
>     Okay, let me clarified, My question here is:
>
>     Why is this problematic to have default to 'True Value'
>
>
>     You refered this as """These cases are a bit problematic, because we
>     don't really have a way to specify default-true flags""" in
>     https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-September/087968.html
>     <https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-September/087968.html>
>
>     I'm seeing another avatar of this question when you says
>     "Restricting this behavior will open the door to some nice
>     fit-and-finish functionality in a later patch" in the current
>     thread. I would like details on this "nice fit-and-finish" features.
>
>     Does this clarify my question ?
>
>
>
> Sure. The answer (which I can fold into the log message if you think
> it's important) is that because of how opts make it into diffopts, we
> can't rely on opts to not be filled in for a value that was unspecified
> on the command line. Because of that, we need to find a backwards
> compatible way to thread defaults through such that they can be
> identified as defaults. The easy way out is to use None for default and
> False for explicitly-false.

Yes please, fold it into the commit message. This this have been 
confusing me for the last versions this will probably be useful to other.

> I'm open to other solutions on the tri-state, but it's a significant
> piece of work that I don't really want to take on.

Getting the handle of tri-state right in a way to do not install a 
future complex rework for the 'default' to True case seems important to 
me. The amount of work involved does not appear too daunting to me.

Cheers,

Patch

diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -555,7 +555,10 @@  def _parse(ui, args):
 
     # combine global options into local
     for o in commands.globalopts:
-        c.append((o[0], o[1], options[o[1]], o[3]))
+        # Passing a tuple of length six through into the option parser
+        # allows otherwise-illegal defaults to survive, which is how
+        # we handle global options like --quiet.
+        c.append((o[0], o[1], options[o[1]], o[3], '', True))
 
     try:
         args = fancyopts.fancyopts(args, c, cmdoptions, gnu=True)
diff --git a/mercurial/fancyopts.py b/mercurial/fancyopts.py
--- a/mercurial/fancyopts.py
+++ b/mercurial/fancyopts.py
@@ -79,10 +79,22 @@  def fancyopts(args, options, state, gnu=
     alllong = set(o[1] for o in options)
 
     for option in options:
-        if len(option) == 5:
+        boolok = False
+        if len(option) == 6:
+            # If the tuple is of length 6, then it's a global option
+            # that was already parsed, so we're really just passing
+            # its "default" through the second phase of flags parsing
+            # (for command-level flags). As a result, we have to allow
+            # defaults of True and not rewrite defaults of False.
+            short, name, default, comment, dummy, dummy = option
+            boolok = True
+        elif len(option) == 5:
             short, name, default, comment, dummy = option
         else:
             short, name, default, comment = option
+        if default is True and not boolok:
+            raise ValueError('fancyopts does not support default-true '
+                             'boolean flags: %r' % name)
         # convert opts to getopt format
         oname = name
         name = name.replace('-', '_')