diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index a93aa71045..d2819686fa 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -3078,7 +3078,10 @@ impl ValueParserFactory for RevisionArg { } } -fn get_string_or_array(config: &config::Config, key: &str) -> Result, ConfigError> { +fn get_string_or_array( + config: &StackedConfig, + key: &'static str, +) -> Result, ConfigError> { config .get(key) .map(|string| vec![string]) @@ -3087,7 +3090,7 @@ fn get_string_or_array(config: &config::Config, key: &str) -> Result fn resolve_default_command( ui: &Ui, - config: &config::Config, + config: &StackedConfig, app: &Command, mut string_args: Vec, ) -> Result, CommandError> { @@ -3130,7 +3133,7 @@ fn resolve_default_command( fn resolve_aliases( ui: &Ui, - config: &config::Config, + config: &StackedConfig, app: &Command, mut string_args: Vec, ) -> Result, CommandError> { @@ -3240,7 +3243,7 @@ fn handle_early_args( } if !args.config_toml.is_empty() { config.add_layer(parse_config_args(&args.config_toml)?); - ui.reset(&config.merge())?; + ui.reset(config)?; } Ok(()) } @@ -3248,7 +3251,7 @@ fn handle_early_args( fn handle_shell_completion( ui: &Ui, app: &Command, - config: &config::Config, + config: &StackedConfig, cwd: &Path, ) -> Result<(), CommandError> { let mut args = vec![]; @@ -3296,7 +3299,7 @@ pub fn expand_args( ui: &Ui, app: &Command, args_os: impl IntoIterator, - config: &config::Config, + config: &StackedConfig, ) -> Result, CommandError> { let mut string_args: Vec = vec![]; for arg_os in args_os { @@ -3509,11 +3512,7 @@ impl CliRunner { } #[instrument(skip_all)] - fn run_internal( - self, - ui: &mut Ui, - mut stacked_config: StackedConfig, - ) -> Result<(), CommandError> { + fn run_internal(self, ui: &mut Ui, mut config: StackedConfig) -> Result<(), CommandError> { // `cwd` is canonicalized for consistency with `Workspace::workspace_root()` and // to easily compute relative paths between them. let cwd = env::current_dir() @@ -3532,12 +3531,11 @@ impl CliRunner { .workspace_loader_factory .create(find_workspace_dir(&cwd)) .map_err(|err| map_workspace_load_error(err, None)); - config_env.reload_user_config(&mut stacked_config)?; + config_env.reload_user_config(&mut config)?; if let Ok(loader) = &maybe_cwd_workspace_loader { config_env.reset_repo_path(loader.repo_path()); - config_env.reload_repo_config(&mut stacked_config)?; + config_env.reload_repo_config(&mut config)?; } - let config = stacked_config.merge(); ui.reset(&config).map_err(|e| { let user_config_path = config_env.existing_user_config_path(); let repo_config_path = config_env.existing_repo_config_path(); @@ -3559,9 +3557,9 @@ impl CliRunner { &self.app, &self.tracing_subscription, &string_args, - &mut stacked_config, + &mut config, ) - .map_err(|err| map_clap_cli_error(err, ui, &stacked_config))?; + .map_err(|err| map_clap_cli_error(err, ui, &config))?; for process_global_args_fn in self.process_global_args_fns { process_global_args_fn(ui, &matches)?; } @@ -3573,14 +3571,13 @@ impl CliRunner { .create(&cwd.join(path)) .map_err(|err| map_workspace_load_error(err, Some(path)))?; config_env.reset_repo_path(loader.repo_path()); - config_env.reload_repo_config(&mut stacked_config)?; + config_env.reload_repo_config(&mut config)?; Ok(loader) } else { maybe_cwd_workspace_loader }; // Apply workspace configs and --config-toml arguments. - let config = stacked_config.merge(); ui.reset(&config)?; // If -R is specified, check if the expanded arguments differ. Aliases @@ -3595,7 +3592,7 @@ impl CliRunner { } } - let settings = UserSettings::from_config(stacked_config); + let settings = UserSettings::from_config(config); let command_helper_data = CommandHelperData { app: self.app, cwd, @@ -3631,7 +3628,7 @@ impl CliRunner { .build() .unwrap(); let stacked_config = config_from_environment(config); - let mut ui = Ui::with_config(&stacked_config.merge()) + let mut ui = Ui::with_config(&stacked_config) .expect("default config should be valid, env vars are stringly typed"); let result = self.run_internal(&mut ui, stacked_config); let exit_code = handle_command_result(&mut ui, result); diff --git a/cli/src/commands/config/list.rs b/cli/src/commands/config/list.rs index 6e8542ec67..76862fecc6 100644 --- a/cli/src/commands/config/list.rs +++ b/cli/src/commands/config/list.rs @@ -79,7 +79,7 @@ pub fn cmd_config_list( let mut formatter = ui.stdout_formatter(); let name_path = args.name.clone().unwrap_or_else(ConfigNamePathBuf::root); let mut wrote_values = false; - for annotated in resolved_config_values(command.settings().config(), &name_path)? { + for annotated in resolved_config_values(command.settings().config(), &name_path) { // Remove overridden values. if annotated.is_overridden && !args.include_overridden { continue; diff --git a/cli/src/complete.rs b/cli/src/complete.rs index bad1e81504..a9d8264d93 100644 --- a/cli/src/complete.rs +++ b/cli/src/complete.rs @@ -17,9 +17,9 @@ use std::io::BufRead; use clap::builder::StyledStr; use clap::FromArgMatches as _; use clap_complete::CompletionCandidate; -use config::Config; use itertools::Itertools; use jj_lib::config::ConfigNamePathBuf; +use jj_lib::settings::UserSettings; use jj_lib::workspace::DefaultWorkspaceLoaderFactory; use jj_lib::workspace::WorkspaceLoaderFactory as _; @@ -101,7 +101,7 @@ pub fn tracked_bookmarks() -> Vec { } pub fn untracked_bookmarks() -> Vec { - with_jj(|jj, config| { + with_jj(|jj, settings| { let output = jj .build() .arg("bookmark") @@ -118,7 +118,7 @@ pub fn untracked_bookmarks() -> Vec { .output() .map_err(user_error)?; - let prefix = config.get::("git.push-bookmark-prefix").ok(); + let prefix = settings.get_string("git.push-bookmark-prefix").ok(); Ok(String::from_utf8_lossy(&output.stdout) .lines() @@ -139,7 +139,7 @@ pub fn untracked_bookmarks() -> Vec { } pub fn bookmarks() -> Vec { - with_jj(|jj, config| { + with_jj(|jj, settings| { let output = jj .build() .arg("bookmark") @@ -156,7 +156,7 @@ pub fn bookmarks() -> Vec { .map_err(user_error)?; let stdout = String::from_utf8_lossy(&output.stdout); - let prefix = config.get::("git.push-bookmark-prefix").ok(); + let prefix = settings.get_string("git.push-bookmark-prefix").ok(); Ok((&stdout .lines() @@ -204,8 +204,8 @@ pub fn git_remotes() -> Vec { } pub fn aliases() -> Vec { - with_jj(|_, config| { - Ok(config + with_jj(|_, settings| { + Ok(settings .get_table("aliases")? .into_keys() // This is opinionated, but many people probably have several @@ -219,7 +219,7 @@ pub fn aliases() -> Vec { } fn revisions(revisions: Option<&str>) -> Vec { - with_jj(|jj, config| { + with_jj(|jj, settings| { // display order const LOCAL_BOOKMARK_MINE: usize = 0; const LOCAL_BOOKMARK: usize = 1; @@ -232,7 +232,7 @@ fn revisions(revisions: Option<&str>) -> Vec { // bookmarks - let prefix = config.get::("git.push-bookmark-prefix").ok(); + let prefix = settings.get_string("git.push-bookmark-prefix").ok(); let mut cmd = jj.build(); cmd.arg("bookmark") @@ -298,8 +298,8 @@ fn revisions(revisions: Option<&str>) -> Vec { let revisions = revisions .map(String::from) - .or_else(|| config.get_string("revsets.short-prefixes").ok()) - .or_else(|| config.get_string("revsets.log").ok()) + .or_else(|| settings.get_string("revsets.short-prefixes").ok()) + .or_else(|| settings.get_string("revsets.log").ok()) .unwrap_or_default(); let output = jj @@ -643,10 +643,10 @@ pub fn interdiff_files(current: &std::ffi::OsStr) -> Vec { /// In case of errors, print them and early return an empty vector. fn with_jj(completion_fn: F) -> Vec where - F: FnOnce(JjBuilder, &Config) -> Result, CommandError>, + F: FnOnce(JjBuilder, &UserSettings) -> Result, CommandError>, { get_jj_command() - .and_then(|(jj, config)| completion_fn(jj, &config)) + .and_then(|(jj, settings)| completion_fn(jj, &settings)) .unwrap_or_else(|e| { eprintln!("{}", e.error); Vec::new() @@ -662,7 +662,7 @@ where /// give completion code access to custom backends. Shelling out was chosen as /// the preferred method, because it's more maintainable and the performance /// requirements of completions aren't very high. -fn get_jj_command() -> Result<(JjBuilder, Config), CommandError> { +fn get_jj_command() -> Result<(JjBuilder, UserSettings), CommandError> { let current_exe = std::env::current_exe().map_err(user_error)?; let mut cmd_args = Vec::::new(); @@ -676,23 +676,18 @@ fn get_jj_command() -> Result<(JjBuilder, Config), CommandError> { // child process. This shouldn't fail, since none of the global args are // required. let app = crate::commands::default_app(); - let config = config::Config::builder() - .add_source(default_config()) - .build() - .expect("default config should be valid"); - let mut stacked_config = config_from_environment(config); - let ui = Ui::with_config(&stacked_config.merge()).expect("default config should be valid"); + let mut config = config_from_environment(default_config()); + let ui = Ui::with_config(&config).expect("default config should be valid"); let cwd = std::env::current_dir() .and_then(|cwd| cwd.canonicalize()) .map_err(user_error)?; let mut config_env = ConfigEnv::from_environment()?; let maybe_cwd_workspace_loader = DefaultWorkspaceLoaderFactory.create(find_workspace_dir(&cwd)); - let _ = config_env.reload_user_config(&mut stacked_config); + let _ = config_env.reload_user_config(&mut config); if let Ok(loader) = &maybe_cwd_workspace_loader { config_env.reset_repo_path(loader.repo_path()); - let _ = config_env.reload_repo_config(&mut stacked_config); + let _ = config_env.reload_repo_config(&mut config); } - let mut config = stacked_config.merge(); // skip 2 because of the clap_complete prelude: jj -- jj let args = std::env::args_os().skip(2); let args = expand_args(&ui, &app, args, &config)?; @@ -708,8 +703,7 @@ fn get_jj_command() -> Result<(JjBuilder, Config), CommandError> { // Try to update repo-specific config on a best-effort basis. if let Ok(loader) = DefaultWorkspaceLoaderFactory.create(&cwd.join(&repository)) { config_env.reset_repo_path(loader.repo_path()); - let _ = config_env.reload_repo_config(&mut stacked_config); - config = stacked_config.merge(); + let _ = config_env.reload_repo_config(&mut config); } cmd_args.push("--repository".into()); cmd_args.push(repository); @@ -750,8 +744,9 @@ fn get_jj_command() -> Result<(JjBuilder, Config), CommandError> { cmd: current_exe, args: cmd_args, }; + let settings = UserSettings::from_config(config); - Ok((builder, config)) + Ok((builder, settings)) } /// A helper struct to allow completion functions to call jj multiple times with diff --git a/cli/src/config.rs b/cli/src/config.rs index 42eb179248..426409b598 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -28,7 +28,6 @@ use jj_lib::config::ConfigNamePathBuf; use jj_lib::config::ConfigSource; use jj_lib::config::ConfigValue; use jj_lib::config::StackedConfig; -use jj_lib::settings::ConfigResultExt as _; use regex::Captures; use regex::Regex; use thiserror::Error; @@ -107,16 +106,19 @@ pub struct AnnotatedValue { pub fn resolved_config_values( stacked_config: &StackedConfig, filter_prefix: &ConfigNamePathBuf, -) -> Result, ConfigError> { +) -> Vec { // Collect annotated values from each config. let mut config_vals = vec![]; for layer in stacked_config.layers() { - let Some(top_value) = filter_prefix.lookup_value(&layer.data).optional()? else { + // TODO: Err(item) means all descendant paths are overridden by the + // current layer. For example, the default ui.pager. should be + // marked as overridden if user had ui.pager = [...] set. + let Ok(Some(top_item)) = layer.look_up_item(filter_prefix) else { continue; }; - let mut config_stack = vec![(filter_prefix.clone(), &top_value)]; - while let Some((name, value)) = config_stack.pop() { - match &value.kind { + let mut config_stack = vec![(filter_prefix.clone(), top_item)]; + while let Some((name, item)) = config_stack.pop() { + match &item.kind { config::ValueKind::Table(table) => { // TODO: Remove sorting when config crate maintains deterministic ordering. for (k, v) in table.iter().sorted_by_key(|(k, _)| *k).rev() { @@ -128,7 +130,7 @@ pub fn resolved_config_values( _ => { config_vals.push(AnnotatedValue { name, - value: value.to_owned(), + value: item.to_owned(), source: layer.source, // Note: Value updated below. is_overridden: false, @@ -145,7 +147,7 @@ pub fn resolved_config_values( val.is_overridden = !names_found.insert(&val.name); } - Ok(config_vals) + config_vals } #[derive(Clone, Debug)] @@ -685,29 +687,28 @@ impl TryFrom> for NonEmptyCommandArgsVec { mod tests { use anyhow::anyhow; use assert_matches::assert_matches; + use indoc::indoc; use maplit::hashmap; use super::*; #[test] fn test_command_args() { - let config = config::Config::builder() - .set_override("empty_array", Vec::::new()) - .unwrap() - .set_override("empty_string", "") - .unwrap() - .set_override("array", vec!["emacs", "-nw"]) - .unwrap() - .set_override("string", "emacs -nw") - .unwrap() - .set_override("structured.env.KEY1", "value1") - .unwrap() - .set_override("structured.env.KEY2", "value2") - .unwrap() - .set_override("structured.command", vec!["emacs", "-nw"]) - .unwrap() - .build() - .unwrap(); + let mut config = StackedConfig::empty(); + config.add_layer( + ConfigLayer::parse( + ConfigSource::User, + indoc! {" + empty_array = [] + empty_string = '' + array = ['emacs', '-nw'] + string = 'emacs -nw' + structured.env = { KEY1 = 'value1', KEY2 = 'value2' } + structured.command = ['emacs', '-nw'] + "}, + ) + .unwrap(), + ); assert!(config.get::("empty_array").is_err()); @@ -757,7 +758,7 @@ mod tests { fn test_resolved_config_values_empty() { let config = StackedConfig::empty(); assert_eq!( - resolved_config_values(&config, &ConfigNamePathBuf::root()).unwrap(), + resolved_config_values(&config, &ConfigNamePathBuf::root()), [] ); } @@ -784,7 +785,7 @@ mod tests { config.add_layer(ConfigLayer::with_data(ConfigSource::Repo, repo_config)); // Note: "email" is alphabetized, before "name" from same layer. insta::assert_debug_snapshot!( - resolved_config_values(&config, &ConfigNamePathBuf::root()).unwrap(), + resolved_config_values(&config, &ConfigNamePathBuf::root()), @r#" [ AnnotatedValue { @@ -924,8 +925,7 @@ mod tests { config.add_layer(ConfigLayer::with_data(ConfigSource::User, user_config)); config.add_layer(ConfigLayer::with_data(ConfigSource::Repo, repo_config)); insta::assert_debug_snapshot!( - resolved_config_values(&config, &ConfigNamePathBuf::from_iter(["test-table1"])) - .unwrap(), + resolved_config_values(&config, &ConfigNamePathBuf::from_iter(["test-table1"])), @r#" [ AnnotatedValue { diff --git a/cli/src/formatter.rs b/cli/src/formatter.rs index e19286d969..9b88a8fa37 100644 --- a/cli/src/formatter.rs +++ b/cli/src/formatter.rs @@ -30,6 +30,7 @@ use crossterm::style::SetBackgroundColor; use crossterm::style::SetForegroundColor; use itertools::Itertools; use jj_lib::config::ConfigError; +use jj_lib::config::StackedConfig; // Lets the caller label strings and translates the labels to colors pub trait Formatter: Write { @@ -159,7 +160,7 @@ impl FormatterFactory { FormatterFactory { kind } } - pub fn color(config: &config::Config, debug: bool) -> Result { + pub fn color(config: &StackedConfig, debug: bool) -> Result { let rules = Arc::new(rules_from_config(config)?); let kind = FormatterFactoryKind::Color { rules, debug }; Ok(FormatterFactory { kind }) @@ -296,11 +297,7 @@ impl ColorFormatter { } } - pub fn for_config( - output: W, - config: &config::Config, - debug: bool, - ) -> Result { + pub fn for_config(output: W, config: &StackedConfig, debug: bool) -> Result { let rules = rules_from_config(config)?; Ok(Self::new(output, Arc::new(rules), debug)) } @@ -404,7 +401,7 @@ impl ColorFormatter { } } -fn rules_from_config(config: &config::Config) -> Result { +fn rules_from_config(config: &StackedConfig) -> Result { let mut result = vec![]; let table = config.get_table("colors")?; for (key, value) in table { @@ -704,13 +701,17 @@ fn write_sanitized(output: &mut impl Write, buf: &[u8]) -> Result<(), Error> { mod tests { use std::str; + use indoc::indoc; + use jj_lib::config::ConfigLayer; + use jj_lib::config::ConfigSource; + use jj_lib::config::StackedConfig; + use super::*; - fn config_from_string(text: &str) -> config::Config { - config::Config::builder() - .add_source(config::File::from_str(text, config::FileFormat::Toml)) - .build() - .unwrap() + fn config_from_string(text: &str) -> StackedConfig { + let mut config = StackedConfig::empty(); + config.add_layer(ConfigLayer::parse(ConfigSource::User, text).unwrap()); + config } #[test] @@ -745,93 +746,82 @@ mod tests { #[test] fn test_color_formatter_color_codes() { // Test the color code for each color. - let colors = [ - "black", - "red", - "green", - "yellow", - "blue", - "magenta", - "cyan", - "white", - "bright black", - "bright red", - "bright green", - "bright yellow", - "bright blue", - "bright magenta", - "bright cyan", - "bright white", - ]; - let mut config_builder = config::Config::builder(); - for color in colors { - // Use the color name as the label. - config_builder = config_builder - .set_override(format!("colors.{}", color.replace(' ', "-")), color) - .unwrap(); - } + // Use the color name as the label. + let config = config_from_string(indoc! {" + [colors] + black = 'black' + red = 'red' + green = 'green' + yellow = 'yellow' + blue = 'blue' + magenta = 'magenta' + cyan = 'cyan' + white = 'white' + bright-black = 'bright black' + bright-red = 'bright red' + bright-green = 'bright green' + bright-yellow = 'bright yellow' + bright-blue = 'bright blue' + bright-magenta = 'bright magenta' + bright-cyan = 'bright cyan' + bright-white = 'bright white' + "}); + // TODO: migrate off config::Config and switch to IndexMap + let colors: HashMap = config.get("colors").unwrap(); let mut output: Vec = vec![]; - let mut formatter = - ColorFormatter::for_config(&mut output, &config_builder.build().unwrap(), false) - .unwrap(); - for color in colors { - formatter.push_label(&color.replace(' ', "-")).unwrap(); + let mut formatter = ColorFormatter::for_config(&mut output, &config, false).unwrap(); + for (label, color) in colors.iter().sorted() { + formatter.push_label(label).unwrap(); write!(formatter, " {color} ").unwrap(); formatter.pop_label().unwrap(); writeln!(formatter).unwrap(); } drop(formatter); - insta::assert_snapshot!(String::from_utf8(output).unwrap(), @r###" + insta::assert_snapshot!(String::from_utf8(output).unwrap(), @r"  black  -  red  -  green  -  yellow   blue  -  magenta  -  cyan  -  white   bright black  -  bright red  -  bright green  -  bright yellow   bright blue  -  bright magenta   bright cyan  +  bright green  +  bright magenta  +  bright red   bright white  - "###); +  bright yellow  +  cyan  +  green  +  magenta  +  red  +  white  +  yellow  + "); } #[test] fn test_color_formatter_hex_colors() { // Test the color code for each color. - let labels_and_colors = [ - ["black", "#000000"], - ["white", "#ffffff"], - ["pastel-blue", "#AFE0D9"], - ]; - let mut config_builder = config::Config::builder(); - for [label, color] in labels_and_colors { - // Use the color name as the label. - config_builder = config_builder - .set_override(format!("colors.{label}"), color) - .unwrap(); - } + let config = config_from_string(indoc! {" + [colors] + black = '#000000' + white = '#ffffff' + pastel-blue = '#AFE0D9' + "}); + // TODO: migrate off config::Config and switch to IndexMap + let colors: HashMap = config.get("colors").unwrap(); let mut output: Vec = vec![]; - let mut formatter = - ColorFormatter::for_config(&mut output, &config_builder.build().unwrap(), false) - .unwrap(); - for [label, _] in labels_and_colors { + let mut formatter = ColorFormatter::for_config(&mut output, &config, false).unwrap(); + for label in colors.keys().sorted() { formatter.push_label(&label.replace(' ', "-")).unwrap(); write!(formatter, " {label} ").unwrap(); formatter.pop_label().unwrap(); writeln!(formatter).unwrap(); } drop(formatter); - insta::assert_snapshot!(String::from_utf8(output).unwrap(), @r###" + insta::assert_snapshot!(String::from_utf8(output).unwrap(), @r"  black  -  white   pastel-blue  - "###); +  white  + "); } #[test] diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index 167df32341..c4560156d4 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -444,7 +444,7 @@ mod tests { fn test_get_diff_editor_from_settings() { let get = |text| { let config = config_from_string(text); - let ui = Ui::with_config(&config.merge()).unwrap(); + let ui = Ui::with_config(&config).unwrap(); let settings = UserSettings::from_config(config); DiffEditor::from_settings( &ui, @@ -661,7 +661,7 @@ mod tests { fn test_get_merge_editor_from_settings() { let get = |text| { let config = config_from_string(text); - let ui = Ui::with_config(&config.merge()).unwrap(); + let ui = Ui::with_config(&config).unwrap(); let settings = UserSettings::from_config(config); MergeEditor::from_settings(&ui, &settings, ConflictMarkerStyle::Diff) .map(|editor| editor.tool) diff --git a/cli/src/text_util.rs b/cli/src/text_util.rs index a6e7e5f44c..a7f88dc659 100644 --- a/cli/src/text_util.rs +++ b/cli/src/text_util.rs @@ -488,18 +488,27 @@ pub fn parse_author(author: &str) -> Result<(String, String), &'static str> { mod tests { use std::io::Write as _; + use indoc::indoc; + use jj_lib::config::ConfigLayer; + use jj_lib::config::ConfigSource; + use jj_lib::config::StackedConfig; + use super::*; use crate::formatter::ColorFormatter; use crate::formatter::PlainTextFormatter; fn format_colored(write: impl FnOnce(&mut dyn Formatter) -> io::Result<()>) -> String { - let config = config::Config::builder() - .set_override("colors.cyan", "cyan") - .unwrap() - .set_override("colors.red", "red") - .unwrap() - .build() - .unwrap(); + let mut config = StackedConfig::empty(); + config.add_layer( + ConfigLayer::parse( + ConfigSource::Default, + indoc! {" + colors.cyan = 'cyan' + colors.red = 'red' + "}, + ) + .unwrap(), + ); let mut output = Vec::new(); let mut formatter = ColorFormatter::for_config(&mut output, &config, false).unwrap(); write(&mut formatter).unwrap(); diff --git a/cli/src/ui.rs b/cli/src/ui.rs index 1bd935d75e..333bce96a1 100644 --- a/cli/src/ui.rs +++ b/cli/src/ui.rs @@ -33,6 +33,7 @@ use std::thread::JoinHandle; use indoc::indoc; use itertools::Itertools as _; use jj_lib::config::ConfigError; +use jj_lib::config::StackedConfig; use minus::MinusError; use minus::Pager as MinusPager; use tracing::instrument; @@ -262,8 +263,8 @@ pub struct Ui { output: UiOutput, } -fn progress_indicator_setting(config: &config::Config) -> bool { - config.get_bool("ui.progress-indicator").unwrap_or(true) +fn progress_indicator_setting(config: &StackedConfig) -> bool { + config.get("ui.progress-indicator").unwrap_or(true) } #[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] @@ -301,16 +302,16 @@ impl fmt::Display for ColorChoice { } } -fn color_setting(config: &config::Config) -> ColorChoice { +fn color_setting(config: &StackedConfig) -> ColorChoice { config - .get_string("ui.color") + .get::("ui.color") .ok() .and_then(|s| s.parse().ok()) .unwrap_or_default() } fn prepare_formatter_factory( - config: &config::Config, + config: &StackedConfig, stdout: &Stdout, ) -> Result { let terminal = stdout.is_terminal(); @@ -331,8 +332,8 @@ fn prepare_formatter_factory( } } -fn be_quiet(config: &config::Config) -> bool { - config.get_bool("ui.quiet").unwrap_or_default() +fn be_quiet(config: &StackedConfig) -> bool { + config.get("ui.quiet").unwrap_or_default() } #[derive(Clone, Copy, Debug, Default, Eq, PartialEq, serde::Deserialize)] @@ -343,20 +344,20 @@ pub enum PaginationChoice { Auto, } -fn pagination_setting(config: &config::Config) -> Result { +fn pagination_setting(config: &StackedConfig) -> Result { config .get::("ui.paginate") .map_err(|err| config_error_with_message("Invalid `ui.paginate`", err)) } -fn pager_setting(config: &config::Config) -> Result { +fn pager_setting(config: &StackedConfig) -> Result { config .get::("ui.pager") .map_err(|err| config_error_with_message("Invalid `ui.pager`", err)) } impl Ui { - pub fn with_config(config: &config::Config) -> Result { + pub fn with_config(config: &StackedConfig) -> Result { let quiet = be_quiet(config); let formatter_factory = prepare_formatter_factory(config, &io::stdout())?; let progress_indicator = progress_indicator_setting(config); @@ -370,7 +371,7 @@ impl Ui { }) } - pub fn reset(&mut self, config: &config::Config) -> Result<(), CommandError> { + pub fn reset(&mut self, config: &StackedConfig) -> Result<(), CommandError> { self.quiet = be_quiet(config); self.paginate = pagination_setting(config)?; self.pager_cmd = pager_setting(config)?; diff --git a/lib/src/config.rs b/lib/src/config.rs index 27b7907cca..79f68094e7 100644 --- a/lib/src/config.rs +++ b/lib/src/config.rs @@ -15,7 +15,6 @@ //! Configuration store helpers. use std::borrow::Borrow; -use std::borrow::Cow; use std::fmt; use std::ops::Range; use std::path::Path; @@ -23,7 +22,6 @@ use std::path::PathBuf; use std::slice; use std::str::FromStr; -use config::Source as _; use itertools::Itertools as _; use serde::Deserialize; @@ -64,48 +62,6 @@ impl ConfigNamePathBuf { pub fn push(&mut self, key: impl Into) { self.0.push(key.into()); } - - /// Looks up value in the given `config`. - /// - /// This is a workaround for the `config.get()` API, which doesn't support - /// literal path expression. If we implement our own config abstraction, - /// this method should be moved there. - pub fn lookup_value(&self, config: &config::Config) -> Result { - // Use config.get() if the TOML keys can be converted to config path - // syntax. This should be cheaper than cloning the whole config map. - let (key_prefix, components) = self.split_safe_prefix(); - let value: ConfigValue = match &key_prefix { - Some(key) => config.get(key)?, - None => config.collect()?.into(), - }; - components - .iter() - .try_fold(value, |value, key| { - let mut table = value.into_table().ok()?; - table.remove(key.get()) - }) - .ok_or_else(|| ConfigError::NotFound(self.to_string())) - } - - /// Splits path to dotted literal expression and remainder. - /// - /// The literal expression part doesn't contain meta characters other than - /// ".", therefore it can be passed in to `config.get()`. - /// https://github.com/mehcode/config-rs/issues/110 - fn split_safe_prefix(&self) -> (Option>, &[toml_edit::Key]) { - // https://github.com/mehcode/config-rs/blob/v0.13.4/src/path/parser.rs#L15 - let is_ident = |key: &str| { - key.chars() - .all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') - }; - let pos = self.0.iter().take_while(|&k| is_ident(k)).count(); - let safe_key = match pos { - 0 => None, - 1 => Some(Cow::Borrowed(self.0[0].get())), - _ => Some(Cow::Owned(self.0[..pos].iter().join("."))), - }; - (safe_key, &self.0[pos..]) - } } impl> FromIterator for ConfigNamePathBuf { @@ -291,10 +247,15 @@ impl ConfigLayer { .try_collect() } + // Add .get_value(name) if needed. look_up_*() are low-level API. + /// Looks up item by the `name` path. Returns `Some(item)` if an item /// found at the path. Returns `Err(item)` if middle node wasn't a table. - fn look_up_item(&self, name: &ConfigNamePathBuf) -> Result, &ConfigValue> { - look_up_item(&self.data.cache, name) + pub fn look_up_item( + &self, + name: impl ToConfigNamePath, + ) -> Result, &ConfigValue> { + look_up_item(&self.data.cache, name.into_name_path().borrow()) } } @@ -397,17 +358,6 @@ impl StackedConfig { &self.layers } - /// Creates new merged config. - pub fn merge(&self) -> config::Config { - self.layers - .iter() - .fold(config::Config::builder(), |builder, layer| { - builder.add_source(layer.data.clone()) - }) - .build() - .expect("loaded configs should be merged without error") - } - /// Looks up value of the specified type `T` from all layers, merges sub /// fields as needed. pub fn get<'de, T: Deserialize<'de>>( @@ -509,43 +459,6 @@ mod tests { use super::*; - #[test] - fn test_split_safe_config_name_path() { - let parse = |s| ConfigNamePathBuf::from_str(s).unwrap(); - let key = |s: &str| toml_edit::Key::new(s); - - // Empty (or root) path isn't recognized by config::Config::get() - assert_eq!( - ConfigNamePathBuf::root().split_safe_prefix(), - (None, [].as_slice()) - ); - - assert_eq!( - parse("Foo-bar_1").split_safe_prefix(), - (Some("Foo-bar_1".into()), [].as_slice()) - ); - assert_eq!( - parse("'foo()'").split_safe_prefix(), - (None, [key("foo()")].as_slice()) - ); - assert_eq!( - parse("foo.'bar()'").split_safe_prefix(), - (Some("foo".into()), [key("bar()")].as_slice()) - ); - assert_eq!( - parse("foo.'bar()'.baz").split_safe_prefix(), - (Some("foo".into()), [key("bar()"), key("baz")].as_slice()) - ); - assert_eq!( - parse("foo.bar").split_safe_prefix(), - (Some("foo.bar".into()), [].as_slice()) - ); - assert_eq!( - parse("foo.bar.'baz()'").split_safe_prefix(), - (Some("foo.bar".into()), [key("baz()")].as_slice()) - ); - } - #[test] fn test_stacked_config_layer_order() { let empty_data = || config::Config::builder().build().unwrap(); @@ -631,43 +544,26 @@ mod tests { a.d = ['a.d #1'] "})); - assert_eq!(config.merge().get::("a.b.c").unwrap(), "a.b.c #0"); assert_eq!(config.get::("a.b.c").unwrap(), "a.b.c #0"); - assert_eq!( - config.merge().get::>("a.d").unwrap(), - vec!["a.d #1".to_owned()] - ); assert_eq!( config.get::>("a.d").unwrap(), vec!["a.d #1".to_owned()] ); // Table "a.b" exists, but key doesn't - assert_matches!( - config.merge().get::("a.b.missing"), - Err(ConfigError::NotFound(name)) if name == "a.b.missing" - ); assert_matches!( config.get::("a.b.missing"), Err(ConfigError::NotFound(name)) if name == "a.b.missing" ); // Node "a.b.c" is not a table - assert_matches!( - config.merge().get::("a.b.c.d"), - Err(ConfigError::NotFound(name)) if name == "a.b.c.d" - ); assert_matches!( config.get::("a.b.c.d"), Err(ConfigError::NotFound(name)) if name == "a.b.c.d" ); // Type error - assert_matches!( - config.merge().get::("a.b"), - Err(ConfigError::Type { key: Some(name), .. }) if name == "a.b" - ); assert_matches!( config.get::("a.b"), Err(ConfigError::Type { key: Some(name), .. }) if name == "a.b" @@ -685,13 +581,8 @@ mod tests { a.b = 'a.b #1' "})); - assert_eq!(config.merge().get::("a.b").unwrap(), "a.b #1"); assert_eq!(config.get::("a.b").unwrap(), "a.b #1"); - assert_matches!( - config.merge().get::("a.b.c"), - Err(ConfigError::NotFound(name)) if name == "a.b.c" - ); assert_matches!( config.get::("a.b.c"), Err(ConfigError::NotFound(name)) if name == "a.b.c" @@ -712,7 +603,6 @@ mod tests { let expected = parse_to_table(indoc! {" c = 'a.b.c #1' "}); - assert_eq!(config.merge().get_table("a.b").unwrap(), expected); assert_eq!(config.get_table("a.b").unwrap(), expected); } @@ -736,7 +626,6 @@ mod tests { b = 'a.b #0' c = 'a.c #1' "}); - assert_eq!(config.merge().get_table("a").unwrap(), expected); assert_eq!(config.get_table("a").unwrap(), expected); } @@ -758,7 +647,6 @@ mod tests { let expected = parse_to_table(indoc! {" a.b = 'a.a.b #2' "}); - assert_eq!(config.merge().get_table("a").unwrap(), expected); assert_eq!(config.get_table("a").unwrap(), expected); } @@ -781,7 +669,6 @@ mod tests { a.b = 'a.a.b #2' b = 'a.b #0' "}); - assert_eq!(config.merge().get_table("a").unwrap(), expected); assert_eq!(config.get_table("a").unwrap(), expected); } @@ -803,7 +690,6 @@ mod tests { b = 'a.a.b #2' "}); // a is not under a.a, but it should still shadow lower layers - assert_eq!(config.merge().get_table("a.a").unwrap(), expected); assert_eq!(config.get_table("a.a").unwrap(), expected); } }