From 8ee919dcbb55dbaf11aedf79058c38ebb1ff2073 Mon Sep 17 00:00:00 2001 From: David Barnett Date: Thu, 12 Jan 2023 00:24:39 -0600 Subject: [PATCH] Don't output implicit defaults from "config list" Accept an --include-defaults arg to enable including those. Listing a nonexistent name is no longer an error, but does output a warning to stderr when no config entries match to explain why there's no other output. --- src/cli_util.rs | 51 +++---- src/commands/mod.rs | 64 ++++++--- src/config.rs | 253 +++++++++++++++++++++++++++++++++-- tests/test_config_command.rs | 21 ++- 4 files changed, 326 insertions(+), 63 deletions(-) diff --git a/src/cli_util.rs b/src/cli_util.rs index 152e7224b..8133b7014 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -55,7 +55,7 @@ use jujutsu_lib::{dag_walk, file_util, git, revset}; use thiserror::Error; use tracing_subscriber::prelude::*; -use crate::config::{FullCommandArgs, LayeredConfigs}; +use crate::config::{AnnotatedValue, FullCommandArgs, LayeredConfigs}; use crate::formatter::{Formatter, PlainTextFormatter}; use crate::merge_tools::{ConflictResolveError, DiffEditError}; use crate::ui::{ColorChoice, Ui}; @@ -280,17 +280,20 @@ pub struct CommandHelper { string_args: Vec, global_args: GlobalArgs, settings: UserSettings, + layered_configs: LayeredConfigs, maybe_workspace_loader: Result, store_factories: StoreFactories, } impl CommandHelper { + #[allow(clippy::too_many_arguments)] pub fn new( app: Command, cwd: PathBuf, string_args: Vec, global_args: GlobalArgs, settings: UserSettings, + layered_configs: LayeredConfigs, maybe_workspace_loader: Result, store_factories: StoreFactories, ) -> Self { @@ -300,6 +303,7 @@ impl CommandHelper { string_args, global_args, settings, + layered_configs, maybe_workspace_loader, store_factories, } @@ -325,6 +329,13 @@ impl CommandHelper { &self.settings } + pub fn resolved_config_values( + &self, + prefix: &[&str], + ) -> Result, crate::config::ConfigError> { + self.layered_configs.resolved_config_values(prefix) + } + fn workspace_helper_internal( &self, ui: &mut Ui, @@ -1472,44 +1483,21 @@ pub fn write_commit_summary( template.format(commit, formatter) } -pub fn write_config_entry( - ui: &mut Ui, - path: &str, - value: config::Value, -) -> Result<(), CommandError> { - match value.kind { - // Handle table values specially to render each child nicely on its own line. - config::ValueKind::Table(table) => { - // TODO: Remove sorting when config crate maintains deterministic ordering. - for (key, table_val) in table.into_iter().sorted_by_key(|(k, _)| k.to_owned()) { - let key_path = match path { - "" => key, - _ => format!("{path}.{key}"), - }; - write_config_entry(ui, key_path.as_str(), table_val)?; - } - } - _ => writeln!(ui, "{path}={}", serialize_config_value(value))?, - }; - Ok(()) -} - // TODO: Use a proper TOML library to serialize instead. -fn serialize_config_value(value: config::Value) -> String { - match value.kind { +pub fn serialize_config_value(value: &config::Value) -> String { + match &value.kind { config::ValueKind::Table(table) => format!( "{{{}}}", // TODO: Remove sorting when config crate maintains deterministic ordering. table - .into_iter() - .sorted_by_key(|(k, _)| k.to_owned()) + .iter() + .sorted_by_key(|(k, _)| *k) .map(|(k, v)| format!("{k}={}", serialize_config_value(v))) .join(", ") ), - config::ValueKind::Array(vals) => format!( - "[{}]", - vals.into_iter().map(serialize_config_value).join(", ") - ), + config::ValueKind::Array(vals) => { + format!("[{}]", vals.iter().map(serialize_config_value).join(", ")) + } config::ValueKind::String(val) => format!("{val:?}"), _ => value.to_string(), } @@ -2017,6 +2005,7 @@ impl CliRunner { string_args, args.global_args, settings, + layered_configs, maybe_workspace_loader, self.store_factories.unwrap_or_default(), ); diff --git a/src/commands/mod.rs b/src/commands/mod.rs index a918e4ce9..8fb865b81 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -24,7 +24,6 @@ use std::{fs, io}; use clap::builder::NonEmptyStringValueParser; use clap::{ArgGroup, ArgMatches, Command, CommandFactory, FromArgMatches, Subcommand}; -use config::Source; use indexmap::{IndexMap, IndexSet}; use itertools::Itertools; use jujutsu_lib::backend::{CommitId, ObjectId, TreeValue}; @@ -47,11 +46,11 @@ use pest::Parser; use crate::cli_util::{ self, check_stale_working_copy, print_checkout_stats, resolve_base_revs, run_ui_editor, - short_commit_hash, user_error, user_error_with_hint, write_config_entry, Args, CommandError, - CommandHelper, DescriptionArg, RevisionArg, WorkspaceCommandHelper, + serialize_config_value, short_commit_hash, user_error, user_error_with_hint, Args, + CommandError, CommandHelper, DescriptionArg, RevisionArg, WorkspaceCommandHelper, DESCRIPTION_PLACEHOLDER_TEMPLATE, }; -use crate::config::config_path; +use crate::config::{config_path, AnnotatedValue, ConfigSource}; use crate::diff_util::{self, DiffFormat, DiffFormatArgs}; use crate::formatter::{Formatter, PlainTextFormatter}; use crate::graphlog::{get_graphlog, Edge}; @@ -172,8 +171,11 @@ enum ConfigSubcommand { /// An optional name of a specific config option to look up. #[arg(value_parser=NonEmptyStringValueParser::new())] name: Option, - // TODO(#531): Support --show-origin using LayeredConfigs. - // TODO(#531): Support ConfigArgs (--user or --repo) and --all. + /// Whether to explicitly include built-in default values in the list. + #[arg(long)] + include_defaults: bool, + // TODO(#1047): Support --show-origin using LayeredConfigs. + // TODO(#1047): Support ConfigArgs (--user or --repo). }, #[command(visible_alias("e"))] Edit { @@ -1029,22 +1031,42 @@ fn cmd_config( ui.request_pager(); let settings = command.settings(); match subcommand { - ConfigSubcommand::List { name } => { - let raw_values = match name { - Some(name) => { - settings - .config() - .get::(name) - .map_err(|e| match e { - config::ConfigError::NotFound { .. } => { - user_error("key not found in config") - } - _ => e.into(), - })? + ConfigSubcommand::List { + name, + include_defaults, + } => { + let name_path = name + .as_ref() + .map_or(vec![], |name| name.split('.').collect_vec()); + let values = command.resolved_config_values(&name_path)?; + let mut wrote_values = false; + for AnnotatedValue { + path, + value, + source, + is_overridden, + } in &values + { + // Remove overridden values. + // TODO(#1047): Allow printing overridden values via `--include-overridden`. + if *is_overridden { + continue; } - None => settings.config().collect()?.into(), - }; - write_config_entry(ui, name.as_deref().unwrap_or(""), raw_values)?; + // Skip built-ins if not included. + if !*include_defaults && *source == ConfigSource::Default { + continue; + } + writeln!(ui, "{}={}", path.join("."), serialize_config_value(value))?; + wrote_values = true; + } + if !wrote_values { + // Note to stderr explaining why output is empty. + if let Some(name) = name { + writeln!(ui.warning(), "No matching config key for {name}")?; + } else { + writeln!(ui.warning(), "No config to list")?; + } + } } ConfigSubcommand::Edit { config_args } => { let edit_path = if config_args.user { diff --git a/src/config.rs b/src/config.rs index 5c5364e42..cf86b295d 100644 --- a/src/config.rs +++ b/src/config.rs @@ -13,10 +13,13 @@ // limitations under the License. use std::borrow::Cow; +use std::collections::HashSet; use std::path::{Path, PathBuf}; use std::process::Command; use std::{env, fmt}; +use config::Source; +use itertools::Itertools; use thiserror::Error; #[derive(Error, Debug)] @@ -27,6 +30,24 @@ pub enum ConfigError { AmbiguousSource(PathBuf, PathBuf), } +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum ConfigSource { + Default, + Env, + // TODO: Track explicit file paths, especially for when user config is a dir. + User, + Repo, + CommandArg, +} + +#[derive(Clone, Debug, PartialEq)] +pub struct AnnotatedValue { + pub path: Vec, + pub value: config::Value, + pub source: ConfigSource, + pub is_overridden: bool, +} + /// Set of configs which can be merged as needed. /// /// Sources from the lowest precedence: @@ -85,23 +106,84 @@ impl LayeredConfigs { /// Creates new merged config. pub fn merge(&self) -> config::Config { - let config_sources = [ - Some(&self.default), - Some(&self.env_base), - self.user.as_ref(), - self.repo.as_ref(), - Some(&self.env_overrides), - self.arg_overrides.as_ref(), - ]; - config_sources + self.sources() .into_iter() - .flatten() + .map(|(_, config)| config) .fold(config::Config::builder(), |builder, source| { builder.add_source(source.clone()) }) .build() .expect("loaded configs should be merged without error") } + + fn sources(&self) -> Vec<(ConfigSource, &config::Config)> { + let config_sources = [ + (ConfigSource::Default, Some(&self.default)), + (ConfigSource::Env, Some(&self.env_base)), + (ConfigSource::User, self.user.as_ref()), + (ConfigSource::Repo, self.repo.as_ref()), + (ConfigSource::Env, Some(&self.env_overrides)), + (ConfigSource::CommandArg, self.arg_overrides.as_ref()), + ]; + config_sources + .into_iter() + .filter_map(|(source, config)| config.map(|c| (source, c))) + .collect_vec() + } + + pub fn resolved_config_values( + &self, + filter_prefix: &[&str], + ) -> Result, ConfigError> { + // Collect annotated values from each config. + let mut config_vals = vec![]; + + let prefix_key = match filter_prefix { + &[] => None, + _ => Some(filter_prefix.join(".")), + }; + for (source, config) in self.sources() { + let top_value = match prefix_key { + Some(ref key) => match config.get(key) { + Err(config::ConfigError::NotFound { .. }) => continue, + val => val?, + }, + None => config.collect()?.into(), + }; + let mut config_stack: Vec<(Vec<&str>, &config::Value)> = + vec![(filter_prefix.to_vec(), &top_value)]; + while let Some((path, value)) = config_stack.pop() { + match &value.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() { + let mut key_path = path.to_vec(); + key_path.push(k); + config_stack.push((key_path, v)); + } + } + _ => { + config_vals.push(AnnotatedValue { + path: path.iter().map(|&s| s.to_owned()).collect_vec(), + value: value.to_owned(), + source: source.to_owned(), + // Note: Value updated below. + is_overridden: false, + }); + } + } + } + } + + // Walk through config values in reverse order and mark each overridden value as + // overridden. + let mut keys_found = HashSet::new(); + for val in config_vals.iter_mut().rev() { + val.is_overridden = !keys_found.insert(&val.path); + } + + Ok(config_vals) + } } pub fn config_path() -> Result, ConfigError> { @@ -340,4 +422,155 @@ mod tests { assert_eq!(args, FullCommandArgs::String("emacs -nw".to_owned())); assert_eq!(args.args(), ["emacs", "-nw"].as_ref()); } + + #[test] + fn test_layered_configs_resolved_config_values_empty() { + let empty_config = config::Config::default(); + let layered_configs = LayeredConfigs { + default: empty_config.to_owned(), + env_base: empty_config.to_owned(), + user: None, + repo: None, + env_overrides: empty_config, + arg_overrides: None, + }; + assert_eq!(layered_configs.resolved_config_values(&[]).unwrap(), []); + } + + #[test] + fn test_layered_configs_resolved_config_values_single_key() { + let empty_config = config::Config::default(); + let env_base_config = config::Config::builder() + .set_override("user.name", "base-user-name") + .unwrap() + .set_override("user.email", "base@user.email") + .unwrap() + .build() + .unwrap(); + let repo_config = config::Config::builder() + .set_override("user.email", "repo@user.email") + .unwrap() + .build() + .unwrap(); + let layered_configs = LayeredConfigs { + default: empty_config.to_owned(), + env_base: env_base_config, + user: None, + repo: Some(repo_config), + env_overrides: empty_config, + arg_overrides: None, + }; + // Note: "email" is alphabetized, before "name" from same layer. + insta::assert_debug_snapshot!( + layered_configs.resolved_config_values(&[]).unwrap(), + @r###" + [ + AnnotatedValue { + path: [ + "user", + "email", + ], + value: Value { + origin: None, + kind: String( + "base@user.email", + ), + }, + source: Env, + is_overridden: true, + }, + AnnotatedValue { + path: [ + "user", + "name", + ], + value: Value { + origin: None, + kind: String( + "base-user-name", + ), + }, + source: Env, + is_overridden: false, + }, + AnnotatedValue { + path: [ + "user", + "email", + ], + value: Value { + origin: None, + kind: String( + "repo@user.email", + ), + }, + source: Repo, + is_overridden: false, + }, + ] + "### + ); + } + + #[test] + fn test_layered_configs_resolved_config_values_filter_path() { + let empty_config = config::Config::default(); + let user_config = config::Config::builder() + .set_override("test-table1.foo", "user-FOO") + .unwrap() + .set_override("test-table2.bar", "user-BAR") + .unwrap() + .build() + .unwrap(); + let repo_config = config::Config::builder() + .set_override("test-table1.bar", "repo-BAR") + .unwrap() + .build() + .unwrap(); + let layered_configs = LayeredConfigs { + default: empty_config.to_owned(), + env_base: empty_config.to_owned(), + user: Some(user_config), + repo: Some(repo_config), + env_overrides: empty_config, + arg_overrides: None, + }; + insta::assert_debug_snapshot!( + layered_configs + .resolved_config_values(&["test-table1"]) + .unwrap(), + @r###" + [ + AnnotatedValue { + path: [ + "test-table1", + "foo", + ], + value: Value { + origin: None, + kind: String( + "user-FOO", + ), + }, + source: User, + is_overridden: false, + }, + AnnotatedValue { + path: [ + "test-table1", + "bar", + ], + value: Value { + origin: None, + kind: String( + "repo-BAR", + ), + }, + source: Repo, + is_overridden: false, + }, + ] + "### + ); + } } diff --git a/tests/test_config_command.rs b/tests/test_config_command.rs index c79f816f0..1ee7a4583 100644 --- a/tests/test_config_command.rs +++ b/tests/test_config_command.rs @@ -38,6 +38,22 @@ fn test_config_list_single() { "###); } +#[test] +fn test_config_list_nonexistent() { + let test_env = TestEnvironment::default(); + let assert = test_env + .jj_cmd( + test_env.env_root(), + &["config", "list", "nonexistent-test-key"], + ) + .assert() + .success() + .stdout(""); + insta::assert_snapshot!(common::get_stderr_string(&assert), @r###" + No matching config key for nonexistent-test-key + "###); +} + #[test] fn test_config_list_table() { let test_env = TestEnvironment::default(); @@ -121,7 +137,10 @@ fn test_config_layer_override_default() { let config_key = "merge-tools.vimdiff.program"; // Default - let stdout = test_env.jj_cmd_success(&repo_path, &["config", "list", config_key]); + let stdout = test_env.jj_cmd_success( + &repo_path, + &["config", "list", config_key, "--include-defaults"], + ); insta::assert_snapshot!(stdout, @r###" merge-tools.vimdiff.program="vim" "###);