From b213d0f1e2dd325fe0ff97422d3a4a0da24e3278 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 11 Dec 2024 14:56:46 +0900 Subject: [PATCH 1/2] cli: set global args to config table without re-parsing as TOML This should be safer than constructing a parsable TOML form. --- cli/src/cli_util.rs | 21 +++++++++++++++------ lib/src/config.rs | 5 +++++ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index a202c681cd..b03aa5f29e 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -3276,19 +3276,28 @@ fn handle_early_args( ) .ignore_errors(true) .try_get_matches_from(args)?; - let mut args: EarlyArgs = EarlyArgs::from_arg_matches(&early_matches).unwrap(); + let args = EarlyArgs::from_arg_matches(&early_matches).unwrap(); + let old_layers_len = config.layers().len(); + config.extend_layers(parse_config_args(&args.config_toml)?); + + // Command arguments overrides any other configuration including the + // variables loaded from --config* arguments. + let mut layer = ConfigLayer::empty(ConfigSource::CommandArg); if let Some(choice) = args.color { - args.config_toml.push(format!(r#"ui.color="{choice}""#)); + layer.set_value("ui.color", choice.to_string()).unwrap(); } if args.quiet.unwrap_or_default() { - args.config_toml.push(r#"ui.quiet=true"#.to_string()); + layer.set_value("ui.quiet", true).unwrap(); } if args.no_pager.unwrap_or_default() { - args.config_toml.push(r#"ui.paginate="never""#.to_owned()); + layer.set_value("ui.paginate", "never").unwrap(); + } + if !layer.is_empty() { + config.add_layer(layer); } - if !args.config_toml.is_empty() { - config.extend_layers(parse_config_args(&args.config_toml)?); + + if config.layers().len() != old_layers_len { ui.reset(config)?; } Ok(()) diff --git a/lib/src/config.rs b/lib/src/config.rs index d6722a8a35..0d3b73e0fe 100644 --- a/lib/src/config.rs +++ b/lib/src/config.rs @@ -327,6 +327,11 @@ impl ConfigLayer { .try_collect() } + /// Returns true if the table has no configuration variables. + pub fn is_empty(&self) -> bool { + self.data.is_empty() + } + // Add .get_value(name) if needed. look_up_*() are low-level API. /// Looks up sub non-inline table by the `name` path. Returns `Some(table)` From 9293da88fdab37791e930bbe808b8522123d5de9 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 11 Dec 2024 12:33:59 +0900 Subject: [PATCH 2/2] cli: add --config-file=PATH argument This would be useful for scripting purpose. Maybe we can also replace the current --config-toml= use cases by --config-file= and simpler --config==. https://github.com/martinvonz/jj/issues/4926#issuecomment-2506672165 If we want to add more source variants (such as fd number), it might be better to add --config-from=:. In any case, we'll probably want --config==, and therefore, we'll need to merge more than one --config* arguments. --- CHANGELOG.md | 3 ++ cli/src/cli_util.rs | 88 +++++++++++++++++++++++++++++++- cli/src/config.rs | 19 +++++-- cli/tests/cli-reference@.md.snap | 1 + cli/tests/test_completion.rs | 1 + cli/tests/test_global_opts.rs | 69 +++++++++++++++++++++++++ docs/config.md | 12 +++-- lib/src/config.rs | 3 +- 8 files changed, 188 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6dbcb57d60..37bfed7bba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,9 @@ 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. + * Templates now support the `>=`, `>`, `<=`, and `<` relational operators for `Integer` types. diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index b03aa5f29e..7caf3395bc 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -157,6 +157,7 @@ use crate::complete; use crate::config::config_from_environment; use crate::config::parse_config_args; use crate::config::CommandNameAndArgs; +use crate::config::ConfigArg; use crate::config::ConfigEnv; use crate::diff_util; use crate::diff_util::DiffFormat; @@ -3102,6 +3103,26 @@ pub struct EarlyArgs { // cases, designed so that `--config ui.color=auto` works #[arg(long, value_name = "TOML", global = true)] pub config_toml: Vec, + /// Additional configuration files (can be repeated) + #[arg(long, value_name = "PATH", global = true, value_hint = clap::ValueHint::FilePath)] + pub config_file: Vec, +} + +impl EarlyArgs { + fn merged_config_args(&self, matches: &ArgMatches) -> Vec { + merge_args_with( + matches, + &[ + ("config_toml", &self.config_toml), + ("config_file", &self.config_file), + ], + |id, value| match id { + "config_toml" => ConfigArg::Toml(value.clone()), + "config_file" => ConfigArg::File(value.clone()), + _ => unreachable!("unexpected id {id:?}"), + }, + ) + } } /// Wrapper around revset expression argument. @@ -3143,6 +3164,29 @@ impl ValueParserFactory for RevisionArg { } } +/// Merges multiple clap args in order of appearance. +/// +/// The `id_values` is a list of `(id, values)` pairs, where `id` is the name of +/// the clap `Arg`, and `values` are the parsed values for that arg. The +/// `convert` function transforms each `(id, value)` pair to e.g. an enum. +/// +/// This is a workaround for . +pub fn merge_args_with<'k, 'v, T, U>( + matches: &ArgMatches, + id_values: &[(&'k str, &'v [T])], + mut convert: impl FnMut(&'k str, &'v T) -> U, +) -> Vec { + let mut pos_values: Vec<(usize, U)> = Vec::new(); + for (id, values) in id_values { + pos_values.extend(itertools::zip_eq( + matches.indices_of(id).into_iter().flatten(), + values.iter().map(|v| convert(id, v)), + )); + } + pos_values.sort_unstable_by_key(|&(pos, _)| pos); + pos_values.into_iter().map(|(_, value)| value).collect() +} + fn get_string_or_array( config: &StackedConfig, key: &'static str, @@ -3279,7 +3323,7 @@ fn handle_early_args( let args = EarlyArgs::from_arg_matches(&early_matches).unwrap(); let old_layers_len = config.layers().len(); - config.extend_layers(parse_config_args(&args.config_toml)?); + config.extend_layers(parse_config_args(&args.merged_config_args(&early_matches))?); // Command arguments overrides any other configuration including the // variables loaded from --config* arguments. @@ -3711,3 +3755,45 @@ fn format_template_aliases_hint(template_aliases: &TemplateAliasesMap) -> String ); hint } + +#[cfg(test)] +mod tests { + use clap::CommandFactory as _; + + use super::*; + + #[derive(clap::Parser, Clone, Debug)] + pub struct TestArgs { + #[arg(long)] + pub foo: Vec, + #[arg(long)] + pub bar: Vec, + #[arg(long)] + pub baz: bool, + } + + #[test] + fn test_merge_args_with() { + let command = TestArgs::command(); + let parse = |args: &[&str]| -> Vec<(&'static str, u32)> { + let matches = command.clone().try_get_matches_from(args).unwrap(); + let args = TestArgs::from_arg_matches(&matches).unwrap(); + merge_args_with( + &matches, + &[("foo", &args.foo), ("bar", &args.bar)], + |id, value| (id, *value), + ) + }; + + assert_eq!(parse(&["jj"]), vec![]); + assert_eq!(parse(&["jj", "--foo=1"]), vec![("foo", 1)]); + assert_eq!( + parse(&["jj", "--foo=1", "--bar=2"]), + vec![("foo", 1), ("bar", 2)] + ); + assert_eq!( + parse(&["jj", "--foo=1", "--baz", "--bar=2", "--foo", "3"]), + vec![("foo", 1), ("bar", 2), ("foo", 3)] + ); + } +} diff --git a/cli/src/config.rs b/cli/src/config.rs index 9ca8da9862..cacadcdcc1 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -319,7 +319,7 @@ impl ConfigEnv { /// 4. Repo config `.jj/repo/config.toml` /// 5. TODO: Workspace config `.jj/config.toml` /// 6. Override environment variables -/// 7. Command-line arguments `--config-toml` +/// 7. Command-line arguments `--config-toml`, `--config-file` /// /// This function sets up 1, 2, and 6. pub fn config_from_environment( @@ -401,15 +401,28 @@ fn env_overrides_layer() -> ConfigLayer { layer } +/// Configuration source/data provided as command-line argument. +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum ConfigArg { + /// `--config-toml=TOML` + Toml(String), + /// `--config-file=PATH` + File(String), +} + /// Parses `--config-toml` arguments. -pub fn parse_config_args(toml_strs: &[String]) -> Result, ConfigLoadError> { +pub fn parse_config_args(toml_strs: &[ConfigArg]) -> Result, 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. + let source = ConfigSource::CommandArg; toml_strs .iter() - .map(|text| ConfigLayer::parse(ConfigSource::CommandArg, text)) + .map(|arg| match arg { + ConfigArg::Toml(text) => ConfigLayer::parse(source, text), + ConfigArg::File(path) => ConfigLayer::load_from_file(source, path.into()), + }) .try_collect() } diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index b3083eb360..d666a72bee 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -196,6 +196,7 @@ 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-toml ` — Additional configuration options (can be repeated) +* `--config-file ` — Additional configuration files (can be repeated) diff --git a/cli/tests/test_completion.rs b/cli/tests/test_completion.rs index 778a729ea3..9cec9a0927 100644 --- a/cli/tests/test_completion.rs +++ b/cli/tests/test_completion.rs @@ -83,6 +83,7 @@ fn test_bookmark_names() { --quiet Silence non-primary command output --no-pager Disable the pager --config-toml Additional configuration options (can be repeated) + --config-file Additional configuration files (can be repeated) --help Print help (see more with '--help') "); diff --git a/cli/tests/test_global_opts.rs b/cli/tests/test_global_opts.rs index 503bf49541..06e3393815 100644 --- a/cli/tests/test_global_opts.rs +++ b/cli/tests/test_global_opts.rs @@ -14,6 +14,8 @@ use std::ffi::OsString; +use indoc::indoc; + use crate::common::get_stderr_string; use crate::common::strip_last_line; use crate::common::TestEnvironment; @@ -595,6 +597,72 @@ fn test_early_args() { ); } +#[test] +fn test_config_args() { + let test_env = TestEnvironment::default(); + let list_config = |args: &[&str]| { + test_env.jj_cmd_success( + test_env.env_root(), + &[&["config", "list", "--include-overridden", "test"], args].concat(), + ) + }; + + std::fs::write( + test_env.env_root().join("file1.toml"), + indoc! {" + test.key1 = 'file1' + test.key2 = 'file1' + "}, + ) + .unwrap(); + std::fs::write( + test_env.env_root().join("file2.toml"), + indoc! {" + test.key3 = 'file2' + "}, + ) + .unwrap(); + + let stdout = list_config(&["--config-toml=test.key1='arg1'"]); + insta::assert_snapshot!(stdout, @"test.key1 = 'arg1'"); + let stdout = list_config(&["--config-file=file1.toml"]); + insta::assert_snapshot!(stdout, @r" + test.key1 = 'file1' + test.key2 = 'file1' + "); + + // --config* arguments are processed in order of appearance + let stdout = list_config(&[ + "--config-toml=test.key1='arg1'", + "--config-file=file1.toml", + "--config-toml=test.key2='arg3'", + "--config-file=file2.toml", + ]); + 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-file=unknown.toml"], + ); + insta::with_settings!({ + filters => [("(?m)^([2-9]): .*", "$1: ")], + }, { + insta::assert_snapshot!(stderr, @r" + Config error: Failed to read configuration file + Caused by: + 1: Cannot access unknown.toml + 2: + For help, see https://martinvonz.github.io/jj/latest/config/. + "); + }); +} + #[test] fn test_invalid_config() { // Test that we get a reasonable error if the config is invalid (#55) @@ -708,6 +776,7 @@ fn test_help() { --quiet Silence non-primary command output --no-pager Disable the pager --config-toml Additional configuration options (can be repeated) + --config-file Additional configuration files (can be repeated) "); } diff --git a/docs/config.md b/docs/config.md index 3004eb8e93..a232986813 100644 --- a/docs/config.md +++ b/docs/config.md @@ -1203,9 +1203,9 @@ 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` 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-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 @@ -1221,3 +1221,9 @@ files with the config specified in `.jjconfig.toml`: ```shell jj --config-toml="$(cat extra-config.toml)" log ``` + +This is equivalent to + +```shell +jj --config-file=extra-config.toml log +``` diff --git a/lib/src/config.rs b/lib/src/config.rs index 0d3b73e0fe..7557feba61 100644 --- a/lib/src/config.rs +++ b/lib/src/config.rs @@ -292,7 +292,8 @@ impl ConfigLayer { Ok(Self::with_data(source, data.into_mut())) } - fn load_from_file(source: ConfigSource, path: PathBuf) -> Result { + /// Loads TOML file from the specified `path`. + pub fn load_from_file(source: ConfigSource, path: PathBuf) -> Result { let text = fs::read_to_string(&path) .context(&path) .map_err(ConfigLoadError::Read)?;