Patchwork [2,of,3] flags: allow specifying --boolean-flag=(true|false) on the command line (BC)

login
register
mail settings
Submitter Augie Fackler
Date Aug. 18, 2016, 4:13 p.m.
Message ID <9ec3aad0bf74ad1c25e8.1471536813@augie-macbookair2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/16356/
State Accepted
Headers show

Comments

Augie Fackler - Aug. 18, 2016, 4:13 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1471534453 14400
#      Thu Aug 18 11:34:13 2016 -0400
# Node ID 9ec3aad0bf74ad1c25e87bcd0f536896d1c355ca
# Parent  8ed7fdc716ffc95a4062875eafc2739697122fbc
flags: allow specifying --boolean-flag=(true|false) on the command line (BC)

This makes it much easier to enable some anti-foot-shooting features
(like update --check) by default, because now all boolean flags can be
explicitly disabled on the command line without having to use HGPLAIN
or similar.
Kyle Lippincott - Aug. 18, 2016, 7:52 p.m.
On Thu, Aug 18, 2016 at 9:13 AM, Augie Fackler <raf@durin42.com> wrote:

> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1471534453 14400
> #      Thu Aug 18 11:34:13 2016 -0400
> # Node ID 9ec3aad0bf74ad1c25e87bcd0f536896d1c355ca
> # Parent  8ed7fdc716ffc95a4062875eafc2739697122fbc
> flags: allow specifying --boolean-flag=(true|false) on the command line
> (BC)
>

Do we want to describe this (in this description) as supporting all of the
things parseable by parsebool?  Not sure how much it matters to be
pedantically correct in the descriptions.

Is there documentation that needs to be updated to mention this is now
possible?  (might be already done in another change in this series, but I
didn't see it)


> This makes it much easier to enable some anti-foot-shooting features
> (like update --check) by default, because now all boolean flags can be
> explicitly disabled on the command line without having to use HGPLAIN
> or similar.
>
> diff --git a/mercurial/fancyopts.py b/mercurial/fancyopts.py
> --- a/mercurial/fancyopts.py
> +++ b/mercurial/fancyopts.py
> @@ -10,7 +10,10 @@ from __future__ import absolute_import
>  import getopt
>
>  from .i18n import _
> -from . import error
> +from . import (
> +    error,
> +    util,
> +)
>
>  def gnugetopt(args, options, longoptions):
>      """Parse options mostly like getopt.gnu_getopt.
> @@ -91,6 +94,10 @@ def fancyopts(args, options, state, gnu=
>                  short += ':'
>              if oname:
>                  oname += '='
> +        else:
> +            prefix = '--' + oname + '='
> +            if any(a.startswith(prefix) for a in args):
> +                oname += '='
>          if short:
>              shortlist += short
>          if name:
> @@ -121,7 +128,10 @@ def fancyopts(args, options, state, gnu=
>          elif t is type([]):
>              state[name].append(val)
>          elif t is type(None) or t is type(False):
> -            state[name] = True
> +            if val:
> +                state[name] = util.parsebool(val)
> +            else:
> +                state[name] = True
>
>      # return unparsed args
>      return args
> diff --git a/tests/test-update-branches.t b/tests/test-update-branches.t
> --- a/tests/test-update-branches.t
> +++ b/tests/test-update-branches.t
> @@ -379,3 +379,14 @@ Test experimental revset support
>
>    $ hg log -r '_destupdate()'
>    2:bd10386d478c 2 (no-eol)
> +
> +Test that boolean flags allow --flag=false specification to override
> [defaults]
> +  $ cat >> $HGRCPATH <<EOF
> +  > [defaults]
> +  > update = --check
> +  > EOF
> +  $ hg co 2
> +  abort: uncommitted changes
> +  [255]
> +  $ hg co --check=no 2
> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Augie Fackler - Aug. 19, 2016, 1:02 p.m.
> On Aug 18, 2016, at 15:52, Kyle Lippincott <spectral@pewpew.net> wrote:
> 
> 
> 
> On Thu, Aug 18, 2016 at 9:13 AM, Augie Fackler <raf@durin42.com <mailto:raf@durin42.com>> wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com <mailto:augie@google.com>>
> # Date 1471534453 14400
> #      Thu Aug 18 11:34:13 2016 -0400
> # Node ID 9ec3aad0bf74ad1c25e87bcd0f536896d1c355ca
> # Parent  8ed7fdc716ffc95a4062875eafc2739697122fbc
> flags: allow specifying --boolean-flag=(true|false) on the command line (BC)
> 
> Do we want to describe this (in this description) as supporting all of the things parseable by parsebool?  Not sure how much it matters to be pedantically correct in the descriptions.

Probably should be in the commit message, yeah.

> Is there documentation that needs to be updated to mention this is now possible?  (might be already done in another change in this series, but I didn't see it)

I'm honestly not sure where to tuck this doc-wise, given that it matters for all boolean flags everywhere. Sigh. I'm very open to ideas...

> 
> 
> This makes it much easier to enable some anti-foot-shooting features
> (like update --check) by default, because now all boolean flags can be
> explicitly disabled on the command line without having to use HGPLAIN
> or similar.
> 
> diff --git a/mercurial/fancyopts.py b/mercurial/fancyopts.py
> --- a/mercurial/fancyopts.py
> +++ b/mercurial/fancyopts.py
> @@ -10,7 +10,10 @@ from __future__ import absolute_import
>  import getopt
> 
>  from .i18n import _
> -from . import error
> +from . import (
> +    error,
> +    util,
> +)
> 
>  def gnugetopt(args, options, longoptions):
>      """Parse options mostly like getopt.gnu_getopt.
> @@ -91,6 +94,10 @@ def fancyopts(args, options, state, gnu=
>                  short += ':'
>              if oname:
>                  oname += '='
> +        else:
> +            prefix = '--' + oname + '='
> +            if any(a.startswith(prefix) for a in args):
> +                oname += '='
>          if short:
>              shortlist += short
>          if name:
> @@ -121,7 +128,10 @@ def fancyopts(args, options, state, gnu=
>          elif t is type([]):
>              state[name].append(val)
>          elif t is type(None) or t is type(False):
> -            state[name] = True
> +            if val:
> +                state[name] = util.parsebool(val)
> +            else:
> +                state[name] = True
> 
>      # return unparsed args
>      return args
> diff --git a/tests/test-update-branches.t b/tests/test-update-branches.t
> --- a/tests/test-update-branches.t
> +++ b/tests/test-update-branches.t
> @@ -379,3 +379,14 @@ Test experimental revset support
> 
>    $ hg log -r '_destupdate()'
>    2:bd10386d478c 2 (no-eol)
> +
> +Test that boolean flags allow --flag=false specification to override [defaults]
> +  $ cat >> $HGRCPATH <<EOF
> +  > [defaults]
> +  > update = --check
> +  > EOF
> +  $ hg co 2
> +  abort: uncommitted changes
> +  [255]
> +  $ hg co --check=no 2
> +  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org <mailto:Mercurial-devel@mercurial-scm.org>
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel <https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel>
>
Yuya Nishihara - Aug. 22, 2016, 1:41 p.m.
On Thu, 18 Aug 2016 12:13:33 -0400, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1471534453 14400
> #      Thu Aug 18 11:34:13 2016 -0400
> # Node ID 9ec3aad0bf74ad1c25e87bcd0f536896d1c355ca
> # Parent  8ed7fdc716ffc95a4062875eafc2739697122fbc
> flags: allow specifying --boolean-flag=(true|false) on the command line (BC)
> 
> This makes it much easier to enable some anti-foot-shooting features
> (like update --check) by default, because now all boolean flags can be
> explicitly disabled on the command line without having to use HGPLAIN
> or similar.

I understand this will be somewhat useful, but I'm not a big fan of introducing
less common parsing rules.

> --- a/mercurial/fancyopts.py
> +++ b/mercurial/fancyopts.py
> @@ -10,7 +10,10 @@ from __future__ import absolute_import
>  import getopt
>  
>  from .i18n import _
> -from . import error
> +from . import (
> +    error,
> +    util,
> +)
>  
>  def gnugetopt(args, options, longoptions):
>      """Parse options mostly like getopt.gnu_getopt.
> @@ -91,6 +94,10 @@ def fancyopts(args, options, state, gnu=
>                  short += ':'
>              if oname:
>                  oname += '='
> +        else:
> +            prefix = '--' + oname + '='
> +            if any(a.startswith(prefix) for a in args):
> +                oname += '='

args may contain a value looks like --option=.

  $ hg up -t --check=false --check

> -            state[name] = True
> +            if val:
> +                state[name] = util.parsebool(val)

this should abort if val isn't a boolean literal.

> --- a/tests/test-update-branches.t
> +++ b/tests/test-update-branches.t
> @@ -379,3 +379,14 @@ Test experimental revset support
>  
>    $ hg log -r '_destupdate()'
>    2:bd10386d478c 2 (no-eol)
> +
> +Test that boolean flags allow --flag=false specification to override [defaults]
> +  $ cat >> $HGRCPATH <<EOF
> +  > [defaults]
> +  > update = --check
> +  > EOF
> +  $ hg co 2
> +  abort: uncommitted changes
> +  [255]
> +  $ hg co --check=no 2

Perhaps this would be parsed as --check '--check=no'.
Augie Fackler - Aug. 22, 2016, 2:26 p.m.
On Mon, Aug 22, 2016 at 9:41 AM, Yuya Nishihara <yuya@tcha.org> wrote:
>> +  abort: uncommitted changes
>> +  [255]
>> +  $ hg co --check=no 2
>
> Perhaps this would be parsed as --check '--check=no'.


Per an IRC conversation, I'm going to attempt to rework this to be
either --check or --no-check, which is more in line with some GNU
utilities and shouldn't require reimplementation of a big part of
getopt.

Patch

diff --git a/mercurial/fancyopts.py b/mercurial/fancyopts.py
--- a/mercurial/fancyopts.py
+++ b/mercurial/fancyopts.py
@@ -10,7 +10,10 @@  from __future__ import absolute_import
 import getopt
 
 from .i18n import _
-from . import error
+from . import (
+    error,
+    util,
+)
 
 def gnugetopt(args, options, longoptions):
     """Parse options mostly like getopt.gnu_getopt.
@@ -91,6 +94,10 @@  def fancyopts(args, options, state, gnu=
                 short += ':'
             if oname:
                 oname += '='
+        else:
+            prefix = '--' + oname + '='
+            if any(a.startswith(prefix) for a in args):
+                oname += '='
         if short:
             shortlist += short
         if name:
@@ -121,7 +128,10 @@  def fancyopts(args, options, state, gnu=
         elif t is type([]):
             state[name].append(val)
         elif t is type(None) or t is type(False):
-            state[name] = True
+            if val:
+                state[name] = util.parsebool(val)
+            else:
+                state[name] = True
 
     # return unparsed args
     return args
diff --git a/tests/test-update-branches.t b/tests/test-update-branches.t
--- a/tests/test-update-branches.t
+++ b/tests/test-update-branches.t
@@ -379,3 +379,14 @@  Test experimental revset support
 
   $ hg log -r '_destupdate()'
   2:bd10386d478c 2 (no-eol)
+
+Test that boolean flags allow --flag=false specification to override [defaults]
+  $ cat >> $HGRCPATH <<EOF
+  > [defaults]
+  > update = --check
+  > EOF
+  $ hg co 2
+  abort: uncommitted changes
+  [255]
+  $ hg co --check=no 2
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved