Skip to content

Commit

Permalink
cli: add simpler --config=NAME=VALUE argument
Browse files Browse the repository at this point in the history
This supersedes #3867. I'll probably replace all uses of --config-toml and
mark --config-toml as deprecated.
  • Loading branch information
yuja committed Dec 16, 2024
1 parent e7adcf0 commit d26d470
Show file tree
Hide file tree
Showing 8 changed files with 126 additions and 21 deletions.
4 changes: 2 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
`snapshot.max-new-file-size` config option. It will print a warning and large
files will be left untracked.

* New command option `--config-file=PATH` to load additional configuration from
files.
* New command options `--config=NAME=VALUE` and `--config-file=PATH` to set
string value without quoting and to load additional configuration from files.

* Templates now support the `>=`, `>`, `<=`, and `<` relational operators for
`Integer` types.
Expand Down
11 changes: 9 additions & 2 deletions cli/src/cli_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3099,8 +3099,13 @@ pub struct EarlyArgs {
// Option<bool>.
pub no_pager: Option<bool>,
/// Additional configuration options (can be repeated)
// TODO: Introduce a `--config` option with simpler syntax for simple
// cases, designed so that `--config ui.color=auto` works
///
/// The name should be specified as TOML dotted keys. The value should be
/// specified in either TOML syntax or as a string. If string value contains
/// TOML constructs (such as array notation), it must be quoted.
#[arg(long, value_name = "NAME=VALUE", global = true)]
pub config: Vec<String>,
/// Additional configuration options (can be repeated)
#[arg(long, value_name = "TOML", global = true)]
pub config_toml: Vec<String>,
/// Additional configuration files (can be repeated)
Expand All @@ -3113,10 +3118,12 @@ impl EarlyArgs {
merge_args_with(
matches,
&[
("config", &self.config),
("config_toml", &self.config_toml),
("config_file", &self.config_file),
],
|id, value| match id {
"config" => (ConfigArgKind::Item, value.as_ref()),
"config_toml" => (ConfigArgKind::Toml, value.as_ref()),
"config_file" => (ConfigArgKind::File, value.as_ref()),
_ => unreachable!("unexpected id {id:?}"),
Expand Down
1 change: 1 addition & 0 deletions cli/src/complete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,7 @@ fn get_jj_command() -> Result<(JjBuilder, UserSettings), CommandError> {
}
for (kind, value) in args.early_args.merged_config_args(&arg_matches) {
let arg = match kind {
ConfigArgKind::Item => format!("--config={value}"),
ConfigArgKind::Toml => format!("--config-toml={value}"),
ConfigArgKind::File => format!("--config-file={value}"),
};
Expand Down
86 changes: 79 additions & 7 deletions cli/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ use regex::Regex;
use thiserror::Error;
use tracing::instrument;

use crate::command_error::config_error;
use crate::command_error::config_error_with_message;
use crate::command_error::CommandError;

// TODO(#879): Consider generating entire schema dynamically vs. static file.
pub const CONFIG_SCHEMA: &str = include_str!("config-schema.json");

Expand Down Expand Up @@ -376,7 +380,7 @@ fn config_files_for(
/// 4. Repo config `.jj/repo/config.toml`
/// 5. TODO: Workspace config `.jj/config.toml`
/// 6. Override environment variables
/// 7. Command-line arguments `--config-toml`, `--config-file`
/// 7. Command-line arguments `--config`, `--config-toml`, `--config-file`
///
/// This function sets up 1, 2, and 6.
pub fn config_from_environment(
Expand Down Expand Up @@ -461,24 +465,34 @@ fn env_overrides_layer() -> ConfigLayer {
/// Configuration source/data type provided as command-line argument.
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub enum ConfigArgKind {
/// `--config=NAME=VALUE`
Item,
/// `--config-toml=TOML`
Toml,
/// `--config-file=PATH`
File,
}

/// Parses `--config-toml` arguments.
/// Parses `--config*` arguments.
pub fn parse_config_args(
toml_strs: &[(ConfigArgKind, &str)],
) -> Result<Vec<ConfigLayer>, ConfigLoadError> {
// It might look silly that a layer is constructed per argument, but
// --config-toml argument can contain a full TOML document, and it makes
// sense to preserve line numbers within the doc. If we add
// --config=KEY=VALUE, multiple values might be loaded into one layer.
) -> Result<Vec<ConfigLayer>, CommandError> {
let source = ConfigSource::CommandArg;
let mut layers = Vec::new();
for (kind, chunk) in &toml_strs.iter().chunk_by(|&(kind, _)| kind) {
match kind {
ConfigArgKind::Item => {
let mut layer = ConfigLayer::empty(source);
for (_, item) in chunk {
let (name, value) = parse_config_arg_item(item)?;
// Can fail depending on the argument order, but that
// wouldn't matter in practice.
layer.set_value(name, value).map_err(|err| {
config_error_with_message("--config argument cannot be set", err)
})?;
}
layers.push(layer);
}
ConfigArgKind::Toml => {
for (_, text) in chunk {
layers.push(ConfigLayer::parse(source, text)?);
Expand All @@ -494,6 +508,24 @@ pub fn parse_config_args(
Ok(layers)
}

/// Parses `NAME=VALUE` string.
fn parse_config_arg_item(item_str: &str) -> Result<(ConfigNamePathBuf, ConfigValue), CommandError> {
// split NAME=VALUE at the first parsable position
let split_candidates = item_str.as_bytes().iter().positions(|&b| b == b'=');
let Some((name, value_str)) = split_candidates
.map(|p| (&item_str[..p], &item_str[p + 1..]))
.map(|(name, value)| name.parse().map(|name| (name, value)))
.find_or_last(Result::is_ok)
.transpose()
.map_err(|err| config_error_with_message("--config name cannot be parsed", err))?
else {
return Err(config_error("--config must be specified as NAME=VALUE"));
};
let value = parse_value_or_bare_string(value_str)
.map_err(|err| config_error_with_message("--config value cannot be parsed", err))?;
Ok((name, value))
}

/// Command name and arguments specified by config.
#[derive(Clone, Debug, Eq, PartialEq, serde::Deserialize)]
#[serde(untagged)]
Expand Down Expand Up @@ -678,6 +710,46 @@ mod tests {
assert!(parse("[table]\nkey = 'value'").is_err());
}

#[test]
fn test_parse_config_arg_item() {
assert!(parse_config_arg_item("").is_err());
assert!(parse_config_arg_item("a").is_err());
assert!(parse_config_arg_item("=").is_err());
assert!(parse_config_arg_item("a=b=c").is_err());
// The value parser is sensitive to leading whitespaces, which seems
// good because the parsing falls back to a bare string.
assert!(parse_config_arg_item("a = 'b'").is_err());

let (name, value) = parse_config_arg_item("a=b").unwrap();
assert_eq!(name, ConfigNamePathBuf::from_iter(["a"]));
assert_eq!(value.as_str(), Some("b"));

let (name, value) = parse_config_arg_item("a=").unwrap();
assert_eq!(name, ConfigNamePathBuf::from_iter(["a"]));
assert_eq!(value.as_str(), Some(""));

let (name, value) = parse_config_arg_item("a= ").unwrap();
assert_eq!(name, ConfigNamePathBuf::from_iter(["a"]));
assert_eq!(value.as_str(), Some(" "));

let (name, value) = parse_config_arg_item("a.b=true").unwrap();
assert_eq!(name, ConfigNamePathBuf::from_iter(["a", "b"]));
assert_eq!(value.as_bool(), Some(true));

let (name, value) = parse_config_arg_item("a='b=c'").unwrap();
assert_eq!(name, ConfigNamePathBuf::from_iter(["a"]));
assert_eq!(value.as_str(), Some("b=c"));

let (name, value) = parse_config_arg_item("'a=b'=c").unwrap();
assert_eq!(name, ConfigNamePathBuf::from_iter(["a=b"]));
assert_eq!(value.as_str(), Some("c"));

let (name, value) = parse_config_arg_item("'a = b=c '={d = 'e=f'}").unwrap();
assert_eq!(name, ConfigNamePathBuf::from_iter(["a = b=c "]));
assert!(value.is_inline_table());
assert_eq!(value.to_string(), "{d = 'e=f'}");
}

#[test]
fn test_command_args() {
let mut config = StackedConfig::empty();
Expand Down
3 changes: 3 additions & 0 deletions cli/tests/[email protected]
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,9 @@ To get started, see the tutorial at https://martinvonz.github.io/jj/latest/tutor
Warnings and errors will still be printed.
* `--no-pager` — Disable the pager
* `--config <NAME=VALUE>` — Additional configuration options (can be repeated)
The name should be specified as TOML dotted keys. The value should be specified in either TOML syntax or as a string. If string value contains TOML constructs (such as array notation), it must be quoted.
* `--config-toml <TOML>` — Additional configuration options (can be repeated)
* `--config-file <PATH>` — Additional configuration files (can be repeated)
Expand Down
1 change: 1 addition & 0 deletions cli/tests/test_completion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ fn test_bookmark_names() {
--color When to colorize output (always, never, debug, auto)
--quiet Silence non-primary command output
--no-pager Disable the pager
--config Additional configuration options (can be repeated)
--config-toml Additional configuration options (can be repeated)
--config-file Additional configuration files (can be repeated)
--help Print help (see more with '--help')
Expand Down
26 changes: 23 additions & 3 deletions cli/tests/test_global_opts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -623,6 +623,8 @@ fn test_config_args() {
)
.unwrap();

let stdout = list_config(&["--config=test.key1=arg1"]);
insta::assert_snapshot!(stdout, @r#"test.key1 = "arg1""#);
let stdout = list_config(&["--config-toml=test.key1='arg1'"]);
insta::assert_snapshot!(stdout, @"test.key1 = 'arg1'");
let stdout = list_config(&["--config-file=file1.toml"]);
Expand All @@ -631,19 +633,36 @@ fn test_config_args() {
test.key2 = 'file1'
");

// --config items are inserted to a single layer internally
let stdout = list_config(&[
"--config=test.key1='arg1'",
"--config=test.key2.sub=true",
"--config=test.key1=arg3",
]);
insta::assert_snapshot!(stdout, @r#"
test.key1 = "arg3"
test.key2.sub = true
"#);

// --config* arguments are processed in order of appearance
let stdout = list_config(&[
"--config-toml=test.key1='arg1'",
"--config=test.key1=arg1",
"--config-file=file1.toml",
"--config-toml=test.key2='arg3'",
"--config-file=file2.toml",
]);
insta::assert_snapshot!(stdout, @r"
# test.key1 = 'arg1'
insta::assert_snapshot!(stdout, @r##"
# test.key1 = "arg1"
test.key1 = 'file1'
# test.key2 = 'file1'
test.key2 = 'arg3'
test.key3 = 'file2'
"##);

let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["config", "list", "--config=foo"]);
insta::assert_snapshot!(stderr, @r"
Config error: --config must be specified as NAME=VALUE
For help, see https://martinvonz.github.io/jj/latest/config/.
");

let stderr = test_env.jj_cmd_failure(
Expand Down Expand Up @@ -775,6 +794,7 @@ fn test_help() {
--color <WHEN> When to colorize output (always, never, debug, auto)
--quiet Silence non-primary command output
--no-pager Disable the pager
--config <NAME=VALUE> Additional configuration options (can be repeated)
--config-toml <TOML> Additional configuration options (can be repeated)
--config-file <PATH> Additional configuration files (can be repeated)
");
Expand Down
15 changes: 8 additions & 7 deletions docs/config.md
Original file line number Diff line number Diff line change
Expand Up @@ -1203,17 +1203,18 @@ env JJ_CONFIG=/dev/null jj log # Ignores any settings specified in the con

### Specifying config on the command-line

You can use one or more `--config-toml`/`--config-file` options on the command
line to specify additional configuration settings. This overrides settings
defined in config files or environment variables. For example,
You can use one or more `--config`/`--config-toml`/`--config-file` options on
the command line to specify additional configuration settings. This overrides
settings defined in config files or environment variables. For example,

```shell
jj --config-toml='ui.color="always"' --config-toml='ui.diff-editor="kdiff3"' split
jj --config=ui.color=always --config-toml='ui.diff-editor="kdiff3"' split
```

Config specified this way must be valid TOML. In particular, string values must
be surrounded by quotes. To pass these quotes to `jj`, most shells require
surrounding those quotes with single quotes as shown above.
Config specified by `--config-toml` must be valid TOML. In particular, string
values must be surrounded by quotes. To pass these quotes to `jj`, most shells
require surrounding those quotes with single quotes as shown above. On the other
hand, `--config` can accept a bare string value.

In `sh`-compatible shells, `--config-toml` can be used to merge entire TOML
files with the config specified in `.jjconfig.toml`:
Expand Down

0 comments on commit d26d470

Please sign in to comment.