Patchwork [4,of,5,STABLE,RFC] dispatch: ignore --early-bool-option that might not be a flag (BC)

login
register
mail settings
Submitter Yuya Nishihara
Date Nov. 15, 2017, 12:54 p.m.
Message ID <c2c34cf080aefbd98332.1510750463@mimosa>
Download mbox | patch
Permalink /patch/25581/
State Accepted
Headers show

Comments

Yuya Nishihara - Nov. 15, 2017, 12:54 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1510321154 -32400
#      Fri Nov 10 22:39:14 2017 +0900
# Branch stable
# Node ID c2c34cf080aefbd983320bdaca111ebbd0826ff3
# Parent  5838a6130d07b588e4640542865e3c175a124bff
dispatch: ignore --early-bool-option that might not be a flag (BC)

The basic idea is if the preceding arg doesn't look like a flag taking a
value, the current arg can be parsed as a flag.

  hg ci -m --foo --config      # bad: --foo may be a flag
  hg ci -m --verbose --config  # good: --verbose known to take no value
                               # whether it is a value for -m or a flag

--debugger/--profile/--traceback are all for developers. They don't need to
be any fancy. So let's turn it to safer side.
Augie Fackler - Nov. 17, 2017, 10:41 p.m.
On Wed, Nov 15, 2017 at 09:54:23PM +0900, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1510321154 -32400
> #      Fri Nov 10 22:39:14 2017 +0900
> # Branch stable
> # Node ID c2c34cf080aefbd983320bdaca111ebbd0826ff3
> # Parent  5838a6130d07b588e4640542865e3c175a124bff
> dispatch: ignore --early-bool-option that might not be a flag (BC)

This one worries me less. Maybe we can land it?

>
> The basic idea is if the preceding arg doesn't look like a flag taking a
> value, the current arg can be parsed as a flag.
>
>   hg ci -m --foo --config      # bad: --foo may be a flag
>   hg ci -m --verbose --config  # good: --verbose known to take no value
>                                # whether it is a value for -m or a flag
>
> --debugger/--profile/--traceback are all for developers. They don't need to
> be any fancy. So let's turn it to safer side.
>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -106,6 +106,22 @@ globalopts = [
>  # TODO: perhaps --debugger should be included
>  earlyoptflags = ("--cwd", "-R", "--repository", "--repo", "--config")
>
> +# core options known to be always boolean type (i.e. takes no value)
> +corebooloptflags = {
> +    '-y', '--noninteractive',
> +    '-q', '--quiet',
> +    '-v', '--verbose',
> +    '--debug',
> +    '--debugger',
> +    '--traceback',
> +    '--time',
> +    '--profile',
> +    '--version',
> +    '-h', '--help',
> +    '--hidden',
> +    '--mq',  # mostly global as mqopt is inserted into all repo commands
> +}
> +
>  dryrunopts = cmdutil.dryrunopts
>  remoteopts = cmdutil.remoteopts
>  walkopts = cmdutil.walkopts
> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -644,6 +644,13 @@ def _parseconfig(ui, config):
>
>      return configs
>
> +def _argmaytakevalue(arg):
> +    """True if the given arg looks like an option flag taking the next item
> +    as its value"""
> +    return (arg.startswith('-')
> +            and not (arg.startswith('--') and '=' in arg)  # --opt=val
> +            and arg not in commands.corebooloptflags)
> +
>  def _earlygetopt(aliases, args, strip=True):
>      """Return list of values for an option (or aliases).
>
> @@ -751,17 +758,48 @@ def _earlyreqoptbool(req, name, aliases)
>
>      >>> req = request([b'x', b'--', b'--debugger'])
>      >>> _earlyreqoptbool(req, b'debugger', [b'--debugger'])
> +
> +    Options can't appear immediately after any option-like flag:
> +
> +    >>> req = request([b'x', b'-z', b'--debugger'])
> +    >>> _earlyreqoptbool(req, b'debugger', [b'--debugger'])
> +
> +    >>> req = request([b'x', b'-z', b'--zzz', b'--debugger'])
> +    >>> _earlyreqoptbool(req, b'debugger', [b'--debugger'])
> +
> +    unless the preceding flag is known to be a boolean:
> +
> +    >>> req = request([b'x', b'-v', b'--debugger'])
> +    >>> _earlyreqoptbool(req, b'debugger', [b'--debugger'])
> +    True
> +
> +    >>> req = request([b'x', b'--debug', b'--debugger'])
> +    >>> _earlyreqoptbool(req, b'debugger', [b'--debugger'])
> +    True
> +
> +    >>> req = request([b'x', b'-z', b'--debugger', b'--debugger'])
> +    >>> _earlyreqoptbool(req, b'debugger', [b'--debugger'])
> +    True
> +
> +    or it takes an immediate value:
> +
> +    >>> req = request([b'x', b'--foo=bar', b'--debugger'])
> +    >>> _earlyreqoptbool(req, b'debugger', [b'--debugger'])
> +    True
>      """
>      try:
>          argcount = req.args.index("--")
>      except ValueError:
>          argcount = len(req.args)
>      value = None
> +    inopt = False
>      pos = 0
>      while pos < argcount:
>          arg = req.args[pos]
> -        if arg in aliases:
> +        if arg in aliases and not inopt:
>              value = True
> +        else:
> +            inopt = _argmaytakevalue(arg)
>          pos += 1
>      req.earlyoptions[name] = value
>      return value
> @@ -897,7 +935,8 @@ def _dispatch(req):
>                  "option -R has to be separated from other options (e.g. not "
>                  "-qR) and --repository may only be abbreviated as --repo!"))
>          if options["debugger"] != req.earlyoptions["debugger"]:
> -            raise error.Abort(_("option --debugger may not be abbreviated!"))
> +            raise error.Abort(_("option --debugger may not be abbreviated "
> +                                "and should come very first!"))
>          # don't validate --profile/--traceback, which can be enabled from now
>
>          if options["encoding"]:
> diff --git a/tests/test-dispatch.t b/tests/test-dispatch.t
> --- a/tests/test-dispatch.t
> +++ b/tests/test-dispatch.t
> @@ -54,7 +54,12 @@ Parsing of early options should stop at
>  Unparsable form of early options:
>
>    $ hg cat --debugg
> -  abort: option --debugger may not be abbreviated!
> +  abort: option --debugger may not be abbreviated and should come very first!
> +  [255]
> +  $ hg log -T --debugger
> +  --debugger (no-eol)
> +  $ hg log -T -T --debugger
> +  abort: option --debugger may not be abbreviated and should come very first!
>    [255]
>
>  Parsing failure of early options should be detected before executing the
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Nov. 19, 2017, 3:07 a.m.
On Fri, 17 Nov 2017 17:41:04 -0500, Augie Fackler wrote:
> On Wed, Nov 15, 2017 at 09:54:23PM +0900, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1510321154 -32400
> > #      Fri Nov 10 22:39:14 2017 +0900
> > # Branch stable
> > # Node ID c2c34cf080aefbd983320bdaca111ebbd0826ff3
> > # Parent  5838a6130d07b588e4640542865e3c175a124bff
> > dispatch: ignore --early-bool-option that might not be a flag (BC)
> 
> This one worries me less. Maybe we can land it?

Please drop this, too. I think both bool/list options should follow the same
feature flag we'll soon introduce.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -106,6 +106,22 @@  globalopts = [
 # TODO: perhaps --debugger should be included
 earlyoptflags = ("--cwd", "-R", "--repository", "--repo", "--config")
 
+# core options known to be always boolean type (i.e. takes no value)
+corebooloptflags = {
+    '-y', '--noninteractive',
+    '-q', '--quiet',
+    '-v', '--verbose',
+    '--debug',
+    '--debugger',
+    '--traceback',
+    '--time',
+    '--profile',
+    '--version',
+    '-h', '--help',
+    '--hidden',
+    '--mq',  # mostly global as mqopt is inserted into all repo commands
+}
+
 dryrunopts = cmdutil.dryrunopts
 remoteopts = cmdutil.remoteopts
 walkopts = cmdutil.walkopts
diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -644,6 +644,13 @@  def _parseconfig(ui, config):
 
     return configs
 
+def _argmaytakevalue(arg):
+    """True if the given arg looks like an option flag taking the next item
+    as its value"""
+    return (arg.startswith('-')
+            and not (arg.startswith('--') and '=' in arg)  # --opt=val
+            and arg not in commands.corebooloptflags)
+
 def _earlygetopt(aliases, args, strip=True):
     """Return list of values for an option (or aliases).
 
@@ -751,17 +758,48 @@  def _earlyreqoptbool(req, name, aliases)
 
     >>> req = request([b'x', b'--', b'--debugger'])
     >>> _earlyreqoptbool(req, b'debugger', [b'--debugger'])
+
+    Options can't appear immediately after any option-like flag:
+
+    >>> req = request([b'x', b'-z', b'--debugger'])
+    >>> _earlyreqoptbool(req, b'debugger', [b'--debugger'])
+
+    >>> req = request([b'x', b'-z', b'--zzz', b'--debugger'])
+    >>> _earlyreqoptbool(req, b'debugger', [b'--debugger'])
+
+    unless the preceding flag is known to be a boolean:
+
+    >>> req = request([b'x', b'-v', b'--debugger'])
+    >>> _earlyreqoptbool(req, b'debugger', [b'--debugger'])
+    True
+
+    >>> req = request([b'x', b'--debug', b'--debugger'])
+    >>> _earlyreqoptbool(req, b'debugger', [b'--debugger'])
+    True
+
+    >>> req = request([b'x', b'-z', b'--debugger', b'--debugger'])
+    >>> _earlyreqoptbool(req, b'debugger', [b'--debugger'])
+    True
+
+    or it takes an immediate value:
+
+    >>> req = request([b'x', b'--foo=bar', b'--debugger'])
+    >>> _earlyreqoptbool(req, b'debugger', [b'--debugger'])
+    True
     """
     try:
         argcount = req.args.index("--")
     except ValueError:
         argcount = len(req.args)
     value = None
+    inopt = False
     pos = 0
     while pos < argcount:
         arg = req.args[pos]
-        if arg in aliases:
+        if arg in aliases and not inopt:
             value = True
+        else:
+            inopt = _argmaytakevalue(arg)
         pos += 1
     req.earlyoptions[name] = value
     return value
@@ -897,7 +935,8 @@  def _dispatch(req):
                 "option -R has to be separated from other options (e.g. not "
                 "-qR) and --repository may only be abbreviated as --repo!"))
         if options["debugger"] != req.earlyoptions["debugger"]:
-            raise error.Abort(_("option --debugger may not be abbreviated!"))
+            raise error.Abort(_("option --debugger may not be abbreviated "
+                                "and should come very first!"))
         # don't validate --profile/--traceback, which can be enabled from now
 
         if options["encoding"]:
diff --git a/tests/test-dispatch.t b/tests/test-dispatch.t
--- a/tests/test-dispatch.t
+++ b/tests/test-dispatch.t
@@ -54,7 +54,12 @@  Parsing of early options should stop at 
 Unparsable form of early options:
 
   $ hg cat --debugg
-  abort: option --debugger may not be abbreviated!
+  abort: option --debugger may not be abbreviated and should come very first!
+  [255]
+  $ hg log -T --debugger
+  --debugger (no-eol)
+  $ hg log -T -T --debugger
+  abort: option --debugger may not be abbreviated and should come very first!
   [255]
 
 Parsing failure of early options should be detected before executing the