config: Improve input checking in config command

Previously you could use config to set an invalid value, which would
prevent you from ever running config again to fix it.

- Run config before loading the parsed config into args. This ensures
that invalid config files won't prevent running config, and config itself can't
be configured anyway.

- Add a --delete option to config to delete keys

- Validate that the given config command and option are actually valid. if invalid,
suggest a list of valid options.

- Error out if the provided value is invalid for the given option, and also add
choices to the validity check.

Topic: improveconfig
Reviewers: aaron, brian-k

docs/config.md

@@ -4,7 +4,7 @@ revup config - Edit revup configuration files.
 
 # SYNOPSIS
 
-`revup config [--help] [--repo] <flag> <value>`
+`revup config [--help] [--repo] [--delete] <flag> <value>`
 
 # DESCRIPTION
 
@@ -40,6 +40,9 @@ revup will warn if attempting to specify them directly.
 : If specified, configuration value will be written to the file in the current
 repo. Otherwise, value will apply globally.
 
+**--delete, -d**
+: Delete the value with the given flag key.
+
 # EXAMPLES
 
 The default value for `revup upload --skip-confirm` is `false`. The user

revup/config.py

@@ -4,10 +4,87 @@
 import logging
 import os
 import re
+from argparse import _StoreAction, _StoreFalseAction, _StoreTrueAction
+from typing import Any, Dict, List, Optional
 
 from revup.types import RevupUsageException
 
 
+class RevupArgParser(argparse.ArgumentParser):
+    def __init__(self, *args: Any, **kwargs: Any) -> None:
+        super().__init__(*args, **kwargs)
+
+    def add_argument(self, *args: Any, **kwargs: Any) -> argparse.Action:
+        """
+        For each boolean store_true action, add a corresponding "no" store_false action
+        with the same target.
+        """
+        action = super().add_argument(*args, **kwargs)
+
+        if isinstance(action, _StoreTrueAction):
+            no_options = []
+            for option_string in action.option_strings:
+                if option_string.startswith("--"):
+                    no_options.append("--no-" + option_string[2:])
+                elif option_string.startswith("-"):
+                    no_options.append("-n" + option_string[1:])
+
+            if no_options:
+                action = _StoreFalseAction(
+                    no_options, action.dest, action.default, False, "autogenerated negation"
+                )
+                for op in no_options:
+                    self._option_string_actions[op] = action
+                self._actions.append(action)
+        elif isinstance(action, _StoreFalseAction):
+            # We assume all store false actions are the autogenerated one
+            raise RuntimeError("Direct store_false actions are not allowed")
+
+        return action
+
+    def set_defaults_from_config(self, conf: configparser.ConfigParser) -> None:
+        cmd = self.get_command()
+        for option, action in self.get_actions().items():
+            if conf.has_option(cmd, option):
+                self.set_option_default(option, action, conf.get(cmd, option))
+
+    def get_command(self) -> str:
+        return self.prog.split()[-1].replace("-", "_")
+
+    def get_actions(self) -> Dict[str, argparse.Action]:
+        ret: Dict[str, argparse.Action] = {}
+        for action in self._actions:
+            if not isinstance(action, (_StoreTrueAction, _StoreAction)):
+                # Ignore nonconfigurable actions (help, auto-generated negation)
+                continue
+
+            if len(action.option_strings) > 0 and action.option_strings[0].startswith("--"):
+                option = action.option_strings[0][2:].replace("-", "_")
+                ret[option] = action
+        return ret
+
+    def set_option_default(self, option: str, action: argparse.Action, value: str) -> None:
+        if isinstance(action, _StoreTrueAction):
+            override = value.lower()
+            if override in ("true", "false"):
+                action.default = override == "true"
+            else:
+                raise ValueError(
+                    f'"{override}" not a valid override for boolean flag {option}, must'
+                    ' be "true" or "false"'
+                )
+        elif isinstance(action, _StoreAction):
+            if action.choices and value not in action.choices:
+                raise ValueError(
+                    f"Value {value} for {self.get_command()}.{option} is not"
+                    f" one of the choices {action.choices}"
+                )
+
+            action.default = value
+        else:
+            raise RuntimeError("Unknown option type!: ", option, action)
+
+
 class Config:
     # Object containing configuration values. Populated by read(), and can then
     # be modified by set_value()
@@ -44,7 +121,13 @@ def write(self) -> None:
             self.config.write(f)
         self.dirty = False
 
-    def set_value(self, section: str, key: str, value: str) -> None:
+    def set_value(self, section: str, key: str, value: Optional[str]) -> None:
+        if value is None:
+            if self.config.has_option(section, key):
+                self.config.remove_option(section, key)
+                self.dirty = True
+            return
+
         if not self.config.has_section(section):
             self.config.add_section(section)
             self.dirty = True
@@ -57,7 +140,7 @@ def get_config(self) -> configparser.ConfigParser:
         return self.config
 
 
-def config_main(conf: Config, args: argparse.Namespace) -> int:
+def config_main(conf: Config, args: argparse.Namespace, all_parsers: List[RevupArgParser]) -> int:
     split_key = args.flag[0].replace("-", "_").split(".")
     if len(split_key) == 1:
         command = "revup"
@@ -68,34 +151,55 @@ def config_main(conf: Config, args: argparse.Namespace) -> int:
     else:
         raise RevupUsageException("Invalid flag argument (must be <key> or <command>.<key>)")
 
-    if not args.value:
-        value = getpass.getpass(f"Input value for {command}.{key}: ").strip()
-    else:
-        value = args.value
+    all_commands = {p.get_command(): p for p in all_parsers}
+    if command not in all_commands:
+        raise RevupUsageException(
+            f"Invalid command section {command}, choose from {list(all_commands.keys())}"
+        )
 
+    parser = all_commands[command]
+    actions = parser.get_actions()
+
+    if not args.delete:
+        if key not in actions:
+            raise RevupUsageException(
+                f"Invalid option key {key}, choose from {list(actions.keys())}"
+            )
+
+    if args.delete:
+        value = None
+        if args.value:
+            raise RevupUsageException("Can't provide a value when using --delete")
+    elif args.value:
+        value = args.value
         if command == "revup" and key == "github_oauth":
             logging.warning(
                 "Prefer to omit the value on command line when entering sensitive info. "
                 "You may want to clear your shell history."
             )
+    else:
+        value = getpass.getpass(f"Input value for {command}.{key}: ").strip()
 
     config_path = conf.repo_config_path if args.repo else conf.config_path
     this_config = Config(config_path)
     this_config.read()
 
-    if command == "revup" and key == "github_username":
-        # From https://www.npmjs.com/package/github-username-regex :
-        # Github username may only contain alphanumeric characters or hyphens.
-        # Github username cannot have multiple consecutive hyphens.
-        # Github username cannot begin or end with a hyphen.
-        # Maximum is 39 characters.
-        if not re.match(r"^[a-z\d](?:[a-z\d]|-(?=[a-z\d])){0,38}$", value, re.I):
-            raise ValueError(f"{args.value} is not a valid GitHub username")
-    elif command == "revup" and key == "github_oauth":
-        if not re.match(r"^[a-z\d_]+$", value, re.I):
-            raise ValueError("Input string is not a valid oauth")
-    elif command == "config":
-        raise RevupUsageException("Can't set defaults for the config command itself")
+    if value is not None:
+        # Check whether this value is allowed by the parser by attempting to set it
+        # (this may throw if the value is not allowed)
+        parser.set_option_default(key, actions[key], value)
+
+        if command == "revup" and key == "github_username":
+            # From https://www.npmjs.com/package/github-username-regex :
+            # Github username may only contain alphanumeric characters or hyphens.
+            # Github username cannot have multiple consecutive hyphens.
+            # Github username cannot begin or end with a hyphen.
+            # Maximum is 39 characters.
+            if not re.match(r"^[a-z\d](?:[a-z\d]|-(?=[a-z\d])){0,38}$", value, re.I):
+                raise ValueError(f"{value} is not a valid GitHub username")
+        elif command == "revup" and key == "github_oauth":
+            if not re.match(r"^[a-z\d_]+$", value, re.I):
+                raise ValueError("Input string is not a valid oauth")
 
     this_config.set_value(command, key, value)
     this_config.write()

revup/revup.py

@@ -1,19 +1,18 @@
 from __future__ import annotations
 
 import argparse
-import configparser
 import logging
 import os
 import stat
 import subprocess
 import sys
-from argparse import _StoreAction, _StoreFalseAction, _StoreTrueAction
 from builtins import FileNotFoundError
 from contextlib import asynccontextmanager
-from typing import Any, AsyncGenerator, Tuple
+from typing import Any, AsyncGenerator, List, Tuple
 
 import revup
 from revup import config, git, logs, shell
+from revup.config import RevupArgParser
 from revup.types import RevupUsageException
 
 REVUP_CONFIG_ENV_VAR = "REVUP_CONFIG_PATH"
@@ -38,55 +37,6 @@ def __call__(self, parser: Any, namespace: Any, values: Any, option_string: Any
         sys.exit(0)
 
 
-class RevupArgParser(argparse.ArgumentParser):
-    def __init__(self, *args: Any, **kwargs: Any) -> None:
-        super().__init__(*args, **kwargs)
-
-    def add_argument(self, *args: Any, **kwargs: Any) -> argparse.Action:
-        """
-        For each boolean store_true action, add a corresponding "no" store_false action
-        with the same target.
-        """
-        action = super().add_argument(*args, **kwargs)
-
-        if isinstance(action, _StoreTrueAction):
-            no_options = []
-            for option_string in action.option_strings:
-                if option_string.startswith("--"):
-                    no_options.append("--no-" + option_string[2:])
-                elif option_string.startswith("-"):
-                    no_options.append("-n" + option_string[1:])
-
-            if no_options:
-                action = _StoreFalseAction(
-                    no_options, action.dest, action.default, False, "autogenerated negation"
-                )
-                for op in no_options:
-                    self._option_string_actions[op] = action
-                self._actions.append(action)
-        return action
-
-    def set_defaults_from_config(self, conf: configparser.ConfigParser) -> None:
-        cmds = self.prog.split()
-        cmd = cmds[0] if len(cmds) == 1 else cmds[1]
-        for action in self._actions:
-            if len(action.option_strings) > 0 and action.option_strings[0].startswith("--"):
-                option = action.option_strings[0][2:].replace("-", "_")
-                if isinstance(action, _StoreTrueAction):
-                    if conf.has_option(cmd, option):
-                        override = conf.get(cmd, option).lower()
-                        if override in ("true", "false"):
-                            action.default = override == "true"
-                        else:
-                            raise ValueError(
-                                f'"{override}" not a valid override for boolean flag {option}, must'
-                                ' be "true" or "false"'
-                            )
-                elif isinstance(action, _StoreAction):
-                    if conf.has_option(cmd, option):
-                        action.default = conf.get(cmd, option)
-
-
 def make_toplevel_parser() -> RevupArgParser:
     revup_parser = RevupArgParser(add_help=False, prog="revup")
     revup_parser.add_argument("--help", "-h", action=HelpAction, nargs=0)
@@ -247,6 +197,18 @@ async def main() -> int:
         "config",
         add_help=False,
     )
+    toolkit_parser = subparsers.add_parser(
+        "toolkit", description="Test various subfunctionalities."
+    )
+
+    # Intentionally does not contain config or toolkit parsers since the those are not configurable
+    all_parsers: List[RevupArgParser] = [
+        revup_parser,
+        amend_parser,
+        cherry_pick_parser,
+        restack_parser,
+        upload_parser,
+    ]
 
     for p in [upload_parser, restack_parser, amend_parser]:
         # Some args are used by both upload and restack
@@ -301,10 +263,8 @@ async def main() -> int:
     config_parser.add_argument("flag", nargs=1)
     config_parser.add_argument("value", nargs="?")
     config_parser.add_argument("--repo", "-r", action="store_true")
+    config_parser.add_argument("--delete", "-d", action="store_true")
 
-    toolkit_parser = subparsers.add_parser(
-        "toolkit", description="Test various subfunctionalities."
-    )
     toolkit_subparsers = toolkit_parser.add_subparsers(dest="toolkit_cmd", required=True)
     detect_branch = toolkit_subparsers.add_parser(
         "detect-branch", description="Detect the base branch of the current branch."
@@ -368,10 +328,15 @@ async def main() -> int:
 
     # Do an initial parsing pass, which handles HelpAction
     args = revup_parser.parse_args()
-
     conf = await get_config()
 
-    for p in [revup_parser, amend_parser, cherry_pick_parser, restack_parser, upload_parser]:
+    # Run config before setting the config, in order to avoid the situation where a broken
+    # config prevents you from running config at all.
+    if args.cmd == "config":
+        logs.configure_logger(False, {})
+        return config.config_main(conf, args, all_parsers)
+
+    for p in all_parsers:
         assert isinstance(p, RevupArgParser)
         p.set_defaults_from_config(conf.get_config())
     args = revup_parser.parse_args()
@@ -383,9 +348,6 @@ async def main() -> int:
     )
     dump_args(args)
 
-    if args.cmd == "config":
-        return config.config_main(conf, args)
-
     git_ctx = await get_git(args)
 
     if args.cmd == "toolkit":