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

@@ -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<String>,
     global_args: GlobalArgs,
     settings: UserSettings,
+    layered_configs: LayeredConfigs,
     maybe_workspace_loader: Result<WorkspaceLoader, CommandError>,
     store_factories: StoreFactories,
 }
 
 impl CommandHelper {
+    #[allow(clippy::too_many_arguments)]
     pub fn new(
         app: Command,
         cwd: PathBuf,
         string_args: Vec<String>,
         global_args: GlobalArgs,
         settings: UserSettings,
+        layered_configs: LayeredConfigs,
         maybe_workspace_loader: Result<WorkspaceLoader, CommandError>,
         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<Vec<AnnotatedValue>, 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 {
+pub fn serialize_config_value(value: &config::Value) -> String {
-    match value.kind {
+    match &value.kind {
         config::ValueKind::Table(table) => format!(
             "{{{}}}",
             // TODO: Remove sorting when config crate maintains deterministic ordering.
             table
-                .into_iter()
+                .iter()
-                .sorted_by_key(|(k, _)| k.to_owned())
+                .sorted_by_key(|(k, _)| *k)
                 .map(|(k, v)| format!("{k}={}", serialize_config_value(v)))
                 .join(", ")
         ),
-        config::ValueKind::Array(vals) => format!(
+        config::ValueKind::Array(vals) => {
-            "[{}]",
+            format!("[{}]", vals.iter().map(serialize_config_value).join(", "))
-            vals.into_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(),
         );

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,
+    serialize_config_value, short_commit_hash, user_error, user_error_with_hint, Args,
-    CommandHelper, DescriptionArg, RevisionArg, WorkspaceCommandHelper,
+    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<String>,
-        // TODO(#531): Support --show-origin using LayeredConfigs.
+        /// Whether to explicitly include built-in default values in the list.
-        // TODO(#531): Support ConfigArgs (--user or --repo) and --all.
+        #[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 } => {
+        ConfigSubcommand::List {
-            let raw_values = match name {
+            name,
-                Some(name) => {
+            include_defaults,
-                    settings
+        } => {
-                        .config()
+            let name_path = name
-                        .get::<config::Value>(name)
+                .as_ref()
-                        .map_err(|e| match e {
+                .map_or(vec![], |name| name.split('.').collect_vec());
-                            config::ConfigError::NotFound { .. } => {
+            let values = command.resolved_config_values(&name_path)?;
-                                user_error("key not found in config")
+            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;
+                }
+                // 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")?;
                 }
-                            _ => e.into(),
-                        })?
             }
-                None => settings.config().collect()?.into(),
-            };
-            write_config_entry(ui, name.as_deref().unwrap_or(""), raw_values)?;
         }
         ConfigSubcommand::Edit { config_args } => {
             let edit_path = if config_args.user {

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<String>,
+    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 = [
+        self.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
             .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<Vec<AnnotatedValue>, 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<Option<PathBuf>, 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,
+            },
+        ]
+        "###
+        );
+    }
 }

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"
     "###);