cli: config: check name format (#5094)

Currently we don't handle bad name well:

```
(dvc-3.8.3) ➜  dvc git:(config-bug) ✗ dvc config section
ERROR: unexpected error - not enough values to unpack (expected 2, got 1)
```

With this PR, we check name to conform to either `remote.name.option` or
`section.option`, so we'll give a more friendly:

```
(dvc-3.8.3) ➜  dvc git:(config-bug) ✗ dvc config section
ERROR: argument name: name argument should look like remote.name.option or section.option
usage: dvc config [-h] [--global | --system | --local] [-q | -v] [-u] [-l] [name] [value]

Get or set config options.
Documentation: <https://man.dvc.org/config>

positional arguments:
  name           Option name (remote.name.option or section.option).
  value          Option value.

optional arguments:
  -h, --help     show this help message and exit
  --global       Use global config.
  --system       Use system config.
  --local        Use local config.
  -q, --quiet    Be quiet.
  -v, --verbose  Be verbose.
  -u, --unset    Unset option.
  -l, --list     List all defined config values.
```

dvc/command/config.py

@@ -6,6 +6,25 @@ from dvc.config import Config, ConfigError
 
 logger = logging.getLogger(__name__)
 
+NAME_REGEX = r"^(?P<remote>remote\.)?(?P<section>[^\.]*)\.(?P<option>[^\.]*)$"
+
+
+def _name_type(value):
+    import re
+
+    match = re.match(NAME_REGEX, value)
+    if not match:
+        raise argparse.ArgumentTypeError(
+            "name argument should look like "
+            "remote.name.option or "
+            "section.option"
+        )
+    return (
+        bool(match.group("remote")),
+        match.group("section").lower(),
+        match.group("option").lower(),
+    )
+
 
 class CmdConfig(CmdBaseNoRepo):
     def __init__(self, args):
@@ -14,20 +33,24 @@ class CmdConfig(CmdBaseNoRepo):
         self.config = Config(validate=False)
 
     def run(self):
-        section, opt = self.args.name.lower().strip().split(".", 1)
+        remote, section, opt = self.args.name
 
         if self.args.value is None and not self.args.unset:
             conf = self.config.load_one(self.args.level)
-            self._check(conf, section, opt)
+            if remote:
+                conf = conf["remote"]
+            self._check(conf, remote, section, opt)
             logger.info(conf[section][opt])
             return 0
 
         with self.config.edit(self.args.level) as conf:
+            if remote:
+                conf = conf["remote"]
             if self.args.unset:
-                self._check(conf, section, opt)
+                self._check(conf, remote, section, opt)
                 del conf[section][opt]
             else:
-                self._check(conf, section)
+                self._check(conf, remote, section)
                 conf[section][opt] = self.args.value
 
         if self.args.name == "cache.type":
@@ -39,14 +62,15 @@ class CmdConfig(CmdBaseNoRepo):
 
         return 0
 
-    def _check(self, conf, section, opt=None):
+    def _check(self, conf, remote, section, opt=None):
+        name = "remote" if remote else "section"
         if section not in conf:
-            msg = "section {} doesn't exist"
-            raise ConfigError(msg.format(self.args.name))
+            raise ConfigError(f"{name} '{section}' doesn't exist")
 
         if opt and opt not in conf[section]:
-            msg = "option {} doesn't exist"
-            raise ConfigError(msg.format(self.args.name))
+            raise ConfigError(
+                f"option '{opt}' doesn't exist in {name} '{section}'"
+            )
 
 
 parent_config_parser = argparse.ArgumentParser(add_help=False)
@@ -92,6 +116,10 @@ def add_parser(subparsers, parent_parser):
         action="store_true",
         help="Unset option.",
     )
-    config_parser.add_argument("name", help="Option name.")
+    config_parser.add_argument(
+        "name",
+        type=_name_type,
+        help="Option name (remote.name.option or section.option).",
+    )
     config_parser.add_argument("value", nargs="?", help="Option value.")
     config_parser.set_defaults(func=CmdConfig)

tests/func/test_cli.py

@@ -141,14 +141,14 @@ class TestGCMultipleDvcRepos(TestDvc):
 
 class TestConfig(TestDvc):
     def test(self):
-        name = "param"
+        name = "section.option"
         value = "1"
 
         args = parse_args(["config", "-u", "--unset", name, value])
 
         self.assertIsInstance(args.func(args), CmdConfig)
         self.assertEqual(args.unset, True)
-        self.assertEqual(args.name, name)
+        self.assertEqual(args.name, (False, "section", "option"))
         self.assertEqual(args.value, value)
 
 

tests/func/test_config.py

@@ -145,3 +145,19 @@ def test_load_relative_paths(dvc, field, remote_url):
     assert cfg["remote"]["test"][field] == os.path.join(
         dvc_dir, "..", "file.txt"
     )
+
+
+def test_config_remote(tmp_dir, dvc, caplog):
+    (tmp_dir / ".dvc" / "config").write_text(
+        "['remote \"myremote\"']\n"
+        "  url = s3://bucket/path\n"
+        "  region = myregion\n"
+    )
+
+    caplog.clear()
+    assert main(["config", "remote.myremote.url"]) == 0
+    assert "s3://bucket/path" in caplog.text
+
+    caplog.clear()
+    assert main(["config", "remote.myremote.region"]) == 0
+    assert "myregion" in caplog.text

tests/unit/command/test_config.py

@@ -0,0 +1,11 @@
+import pytest
+
+from dvc.cli import DvcParserError, parse_args
+
+
+@pytest.mark.parametrize(
+    "name", ["way.too.long", "no_option", "remote.way.too.long"],
+)
+def test_config_bad_name(name):
+    with pytest.raises(DvcParserError):
+        parse_args(["config", name])