cli: parse "jj config set" value a bit stricter, report syntax error

The same parsing function will be used for --config NAME=VALUE.

I don't think we'll add schema-based type inference anytime soon, so I moved
the value parsing to clap layer.

CHANGELOG.md

@@ -27,6 +27,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 * `jj config edit --user` now opens a file even if `$JJ_CONFIG` points to a
   directory. If there are multiple config files, the command will fail.
 
+* `jj config set` no longer accepts a bare string value that looks like a TOML
+  expression. For example, `jj config set NAME '[foo]'` must be quoted as `jj
+  config set NAME '"[foo]"'`.
+
 * The deprecated `[alias]` config section is no longer respected. Move command
   aliases to the `[aliases]` section.
 

cli/src/commands/config/set.rs

@@ -17,6 +17,7 @@ use std::io;
 use clap_complete::ArgValueCandidates;
 use jj_lib::commit::Commit;
 use jj_lib::config::ConfigNamePathBuf;
+use jj_lib::config::ConfigValue;
 use jj_lib::repo::Repo;
 use tracing::instrument;
 
@@ -26,7 +27,7 @@ use crate::cli_util::WorkspaceCommandHelper;
 use crate::command_error::user_error_with_message;
 use crate::command_error::CommandError;
 use crate::complete;
-use crate::config::parse_toml_value_or_bare_string;
+use crate::config::parse_value_or_bare_string;
 use crate::ui::Ui;
 
 /// Update config file to set the given option to a given value.
@@ -34,8 +35,13 @@ use crate::ui::Ui;
 pub struct ConfigSetArgs {
     #[arg(required = true, add = ArgValueCandidates::new(complete::leaf_config_keys))]
     name: ConfigNamePathBuf,
-    #[arg(required = true)]
-    value: String,
+    /// New value to set
+    ///
+    /// The value should be specified as a TOML expression. If string value
+    /// doesn't contain any TOML constructs (such as array notation), quotes can
+    /// be omitted.
+    #[arg(required = true, value_parser = parse_value_or_bare_string)]
+    value: ConfigValue,
     #[command(flatten)]
     level: ConfigLevelArgs,
 }
@@ -54,18 +60,15 @@ pub fn cmd_config_set(
 ) -> Result<(), CommandError> {
     let mut file = args.level.edit_config_file(command)?;
 
-    // TODO(#531): Infer types based on schema (w/ --type arg to override).
-    let value = parse_toml_value_or_bare_string(&args.value);
-
     // If the user is trying to change the author config, we should warn them that
     // it won't affect the working copy author
     if args.name == ConfigNamePathBuf::from_iter(vec!["user", "name"]) {
-        check_wc_author(ui, command, &value, AuthorChange::Name)?;
+        check_wc_author(ui, command, &args.value, AuthorChange::Name)?;
     } else if args.name == ConfigNamePathBuf::from_iter(vec!["user", "email"]) {
-        check_wc_author(ui, command, &value, AuthorChange::Email)?;
+        check_wc_author(ui, command, &args.value, AuthorChange::Email)?;
     };
 
-    file.set_value(&args.name, value)
+    file.set_value(&args.name, &args.value)
         .map_err(|err| user_error_with_message(format!("Failed to set {}", args.name), err))?;
     file.save()?;
     Ok(())

cli/src/config.rs

@@ -38,13 +38,36 @@ use tracing::instrument;
 pub const CONFIG_SCHEMA: &str = include_str!("config-schema.json");
 
 /// Parses a TOML value expression. Interprets the given value as string if it
-/// can't be parsed.
-pub fn parse_toml_value_or_bare_string(value_str: &str) -> toml_edit::Value {
+/// can't be parsed and doesn't look like a TOML expression.
+pub fn parse_value_or_bare_string(value_str: &str) -> Result<ConfigValue, toml_edit::TomlError> {
     match value_str.parse() {
-        Ok(value) => value,
-        // TODO: might be better to reject meta characters. A typo in TOML value
-        // expression shouldn't be silently converted to string.
-        _ => value_str.into(),
+        Ok(value) => Ok(value),
+        Err(_) if value_str.as_bytes().iter().copied().all(is_bare_char) => Ok(value_str.into()),
+        Err(err) => Err(err),
+    }
+}
+
+const fn is_bare_char(b: u8) -> bool {
+    match b {
+        // control chars (including tabs and newlines), which are unlikely to
+        // appear in command-line arguments
+        b'\x00'..=b'\x1f' | b'\x7f' => false,
+        // space and symbols that don't construct a TOML value
+        b' ' | b'!' | b'#' | b'$' | b'%' | b'&' | b'(' | b')' | b'*' | b'/' | b';' | b'<'
+        | b'>' | b'?' | b'@' | b'\\' | b'^' | b'_' | b'`' | b'|' | b'~' => true,
+        // there may be an error in integer, float, or date-time, but that's rare
+        b'+' | b'-' | b'.' | b':' => true,
+        // comma and equal don't construct a compound value by themselves, but
+        // they suggest that the value is an inline array or table
+        b',' | b'=' => false,
+        // unpaired quotes are often typo
+        b'"' | b'\'' => false,
+        // symbols that construct an inline array or table
+        b'[' | b']' | b'{' | b'}' => false,
+        // ASCII alphanumeric
+        b'0'..=b'9' | b'A'..=b'Z' | b'a'..=b'z' => true,
+        // non-ASCII
+        b'\x80'..=b'\xff' => true,
     }
 }
 
@@ -612,6 +635,38 @@ mod tests {
         settings
     }
 
+    #[test]
+    fn test_parse_value_or_bare_string() {
+        let parse = |s: &str| parse_value_or_bare_string(s);
+
+        // Value in TOML syntax
+        assert_eq!(parse("true").unwrap().as_bool(), Some(true));
+        assert_eq!(parse("42").unwrap().as_integer(), Some(42));
+        assert_eq!(parse("-1").unwrap().as_integer(), Some(-1));
+        assert_eq!(parse("'a'").unwrap().as_str(), Some("a"));
+        assert!(parse("[]").unwrap().is_array());
+        assert!(parse("{ a = 'b' }").unwrap().is_inline_table());
+
+        // Bare string
+        assert_eq!(parse("").unwrap().as_str(), Some(""));
+        assert_eq!(parse("John Doe").unwrap().as_str(), Some("John Doe"));
+        assert_eq!(
+            parse("<foo+bar@example.org>").unwrap().as_str(),
+            Some("<foo+bar@example.org>")
+        );
+        assert_eq!(parse("#ff00aa").unwrap().as_str(), Some("#ff00aa"));
+        assert_eq!(parse("all()").unwrap().as_str(), Some("all()"));
+        assert_eq!(parse("glob:*.*").unwrap().as_str(), Some("glob:*.*"));
+        assert_eq!(parse("柔術").unwrap().as_str(), Some("柔術"));
+
+        // Error in TOML value
+        assert!(parse("'foo").is_err());
+        assert!(parse("[0 1]").is_err());
+        assert!(parse("{ x = }").is_err());
+        assert!(parse("key = 'value'").is_err());
+        assert!(parse("[table]\nkey = 'value'").is_err());
+    }
+
     #[test]
     fn test_command_args() {
         let mut config = StackedConfig::empty();

cli/tests/cli-reference@.md.snap

@@ -607,7 +607,9 @@ Update config file to set the given option to a given value
 ###### **Arguments:**
 
 * `<NAME>`
-* `<VALUE>`
+* `<VALUE>` — New value to set
+
+   The value should be specified as a TOML expression. If string value doesn't contain any TOML constructs (such as array notation), quotes can be omitted.
 
 ###### **Options:**
 

cli/tests/test_config_command.rs

@@ -468,6 +468,22 @@ fn test_config_set_bad_opts() {
 
     For more information, try '--help'.
     "###);
+
+    let stderr = test_env.jj_cmd_cli_error(
+        test_env.env_root(),
+        &["config", "set", "--user", "x", "['typo'}"],
+    );
+    insta::assert_snapshot!(stderr, @r"
+    error: invalid value '['typo'}' for '<VALUE>': TOML parse error at line 1, column 8
+      |
+    1 | ['typo'}
+      |        ^
+    invalid array
+    expected `]`
+
+
+    For more information, try '--help'.
+    ");
 }
 
 #[test]