From 01a39543fb20831880708dcf6c7a110b62624948 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 12 Dec 2024 15:53:31 +0900 Subject: [PATCH 1/5] config: forward write_config_value_to_file() to ConfigLayer::set_value() write_config_value_to_file() will be inlined to callers. --- cli/src/config.rs | 66 ++++++++++---------------------- cli/tests/test_config_command.rs | 14 ++++--- 2 files changed, 28 insertions(+), 52 deletions(-) diff --git a/cli/src/config.rs b/cli/src/config.rs index cacadcdcc1..af6b8f5e00 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -426,23 +426,20 @@ pub fn parse_config_args(toml_strs: &[ConfigArg]) -> Result, Co .try_collect() } -fn read_config(path: &Path) -> Result, CommandError> { - let config_toml = std::fs::read_to_string(path).or_else(|err| { - match err.kind() { +fn load_config_file_or_empty( + source: ConfigSource, + path: &Path, +) -> Result { + match ConfigLayer::load_from_file(source, path.into()) { + Ok(layer) => Ok(layer), + Err(ConfigLoadError::Read(err)) if err.error.kind() == std::io::ErrorKind::NotFound => { // If config doesn't exist yet, read as empty and we'll write one. - std::io::ErrorKind::NotFound => Ok("".to_string()), - _ => Err(user_error_with_message( - format!("Failed to read file {path}", path = path.display()), - err, - )), + let mut layer = ConfigLayer::empty(source); + layer.path = Some(path.into()); + Ok(layer) } - })?; - config_toml.parse().map_err(|err| { - user_error_with_message( - format!("Failed to parse file {path}", path = path.display()), - err, - ) - }) + Err(err) => Err(err), + } } fn write_config(path: &Path, doc: &toml_edit::DocumentMut) -> Result<(), CommandError> { @@ -459,43 +456,20 @@ pub fn write_config_value_to_file( value: toml_edit::Value, path: &Path, ) -> Result<(), CommandError> { - let mut doc = read_config(path)?.into_mut(); - - // Apply config value - let mut target_table = doc.as_table_mut(); - let mut key_parts_iter = key.components(); - let last_key_part = key_parts_iter.next_back().expect("key must not be empty"); - for key_part in key_parts_iter { - target_table = target_table - .entry(key_part) - .or_insert_with(|| toml_edit::Item::Table(toml_edit::Table::new())) - .as_table_mut() - .ok_or_else(|| { - user_error(format!( - "Failed to set {key}: would overwrite non-table value with parent table" - )) - })?; - } - // Error out if overwriting non-scalar value for key (table or array) with - // scalar. - match target_table.get(last_key_part) { - None | Some(toml_edit::Item::None | toml_edit::Item::Value(_)) => {} - Some(toml_edit::Item::Table(_) | toml_edit::Item::ArrayOfTables(_)) => { - return Err(user_error(format!( - "Failed to set {key}: would overwrite entire table" - ))); - } - } - target_table[last_key_part] = toml_edit::Item::Value(value); - - write_config(path, &doc) + // TODO: Load config layer by caller. Here we use a dummy source for now. + let mut layer = load_config_file_or_empty(ConfigSource::User, path)?; + layer + .set_value(key, value) + .map_err(|err| user_error_with_message(format!("Failed to set {key}"), err))?; + write_config(path, &layer.data) } pub fn remove_config_value_from_file( key: &ConfigNamePathBuf, path: &Path, ) -> Result<(), CommandError> { - let mut doc = read_config(path)?.into_mut(); + // TODO: Load config layer by caller. Here we use a dummy source for now. + let mut doc = load_config_file_or_empty(ConfigSource::User, path)?.data; // Find target table let mut key_iter = key.components(); diff --git a/cli/tests/test_config_command.rs b/cli/tests/test_config_command.rs index 2c1b9f3c7d..191a2507b5 100644 --- a/cli/tests/test_config_command.rs +++ b/cli/tests/test_config_command.rs @@ -578,9 +578,10 @@ fn test_config_set_type_mismatch() { &repo_path, &["config", "set", "--user", "test-table", "not-a-table"], ); - insta::assert_snapshot!(stderr, @r###" - Error: Failed to set test-table: would overwrite entire table - "###); + insta::assert_snapshot!(stderr, @r" + Error: Failed to set test-table + Caused by: Would overwrite entire table test-table + "); // But it's fine to overwrite arrays and inline tables test_env.jj_cmd_success( @@ -617,9 +618,10 @@ fn test_config_set_nontable_parent() { &repo_path, &["config", "set", "--user", "test-nontable.foo", "test-val"], ); - insta::assert_snapshot!(stderr, @r###" - Error: Failed to set test-nontable.foo: would overwrite non-table value with parent table - "###); + insta::assert_snapshot!(stderr, @r" + Error: Failed to set test-nontable.foo + Caused by: Would overwrite non-table value with parent table test-nontable + "); } #[test] From 456405b000837d503c13c56b469ed7616ae82622 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 12 Dec 2024 16:10:38 +0900 Subject: [PATCH 2/5] config: add layer.delete_value(name) method Since .get("path.to.non-table.children") returns NotFound, I made .delete_value() not fail in that case. The path doesn't exist, so .delete_value() should be noop. remove_config_value_from_file() will be inlined to callers. --- cli/src/config.rs | 36 +++---------- cli/tests/test_config_command.rs | 7 ++- lib/src/config.rs | 86 ++++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 31 deletions(-) diff --git a/cli/src/config.rs b/cli/src/config.rs index af6b8f5e00..aba8b2793f 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -469,36 +469,14 @@ pub fn remove_config_value_from_file( path: &Path, ) -> Result<(), CommandError> { // TODO: Load config layer by caller. Here we use a dummy source for now. - let mut doc = load_config_file_or_empty(ConfigSource::User, path)?.data; - - // Find target table - let mut key_iter = key.components(); - let last_key = key_iter.next_back().expect("key must not be empty"); - let target_table = key_iter.try_fold(doc.as_table_mut(), |table, key| { - table - .get_mut(key) - .ok_or_else(|| user_error(format!(r#""{key}" doesn't exist"#))) - .and_then(|table| { - table - .as_table_mut() - .ok_or_else(|| user_error(format!(r#""{key}" is not a table"#))) - }) - })?; - - // Remove config value - match target_table.entry(last_key) { - toml_edit::Entry::Occupied(entry) => { - if entry.get().is_table() { - return Err(user_error(format!("Won't remove table {key}"))); - } - entry.remove(); - } - toml_edit::Entry::Vacant(_) => { - return Err(user_error(format!(r#""{key}" doesn't exist"#))); - } + let mut layer = load_config_file_or_empty(ConfigSource::User, path)?; + let old_value = layer + .delete_value(key) + .map_err(|err| user_error_with_message(format!("Failed to unset {key}"), err))?; + if old_value.is_none() { + return Err(user_error(format!(r#""{key}" doesn't exist"#))); } - - write_config(path, &doc) + write_config(path, &layer.data) } /// Command name and arguments specified by config. diff --git a/cli/tests/test_config_command.rs b/cli/tests/test_config_command.rs index 191a2507b5..d974463c97 100644 --- a/cli/tests/test_config_command.rs +++ b/cli/tests/test_config_command.rs @@ -655,7 +655,7 @@ fn test_config_unset_inline_table_key() { &["config", "unset", "--user", "inline-table.foo"], ); - insta::assert_snapshot!(stderr, @r#"Error: "inline-table" is not a table"#); + insta::assert_snapshot!(stderr, @r#"Error: "inline-table.foo" doesn't exist"#); } #[test] @@ -685,7 +685,10 @@ fn test_config_unset_table_like() { test_env.env_root(), &["config", "unset", "--user", "non-inline-table"], ); - insta::assert_snapshot!(stderr, @"Error: Won't remove table non-inline-table"); + insta::assert_snapshot!(stderr, @r" + Error: Failed to unset non-inline-table + Caused by: Would delete entire table non-inline-table + "); let user_config_toml = std::fs::read_to_string(&user_config_path).unwrap(); insta::assert_snapshot!(user_config_toml, @r" diff --git a/lib/src/config.rs b/lib/src/config.rs index 7557feba61..c1beb4fdc9 100644 --- a/lib/src/config.rs +++ b/lib/src/config.rs @@ -95,6 +95,12 @@ pub enum ConfigUpdateError { /// Dotted config name path. name: String, }, + /// Table exists at the path, which shouldn't be deleted. + #[error("Would delete entire table {name}")] + WouldDeleteTable { + /// Dotted config name path. + name: String, + }, } /// Extension methods for `Result`. @@ -392,6 +398,39 @@ impl ConfigLayer { } } } + + /// Deletes value specified by the `name` path. Returns old value if any. + /// + /// Returns `Ok(None)` if middle node wasn't a table or a value wasn't + /// found. Returns `Err` if attempted to delete a table. + pub fn delete_value( + &mut self, + name: impl ToConfigNamePath, + ) -> Result, ConfigUpdateError> { + let would_delete_table = |name| ConfigUpdateError::WouldDeleteTable { name }; + let name = name.into_name_path(); + let name = name.borrow(); + let mut keys = name.components(); + let leaf_key = keys + .next_back() + .ok_or_else(|| would_delete_table(name.to_string()))?; + let root_table = self.data.as_table_mut(); + let Some(parent_table) = + keys.try_fold(root_table, |table, key| table.get_mut(key)?.as_table_mut()) + else { + return Ok(None); + }; + match parent_table.entry(leaf_key) { + toml_edit::Entry::Occupied(entry) => { + if !entry.get().is_value() { + return Err(would_delete_table(name.to_string())); + } + let old_item = entry.remove(); + Ok(Some(old_item.into_value().unwrap())) + } + toml_edit::Entry::Vacant(_) => Ok(None), + } + } } /// Looks up item from the `root_item`. Returns `Some(item)` if an item found at @@ -759,6 +798,53 @@ mod tests { "#); } + #[test] + fn test_config_layer_delete_value() { + let mut layer = ConfigLayer::empty(ConfigSource::User); + // Cannot delete the root table + assert_matches!( + layer.delete_value(ConfigNamePathBuf::root()), + Err(ConfigUpdateError::WouldDeleteTable { name }) if name.is_empty() + ); + + // Insert some values + layer.set_value("foo", 1).unwrap(); + layer.set_value("bar.baz.blah", "2").unwrap(); + layer + .set_value("bar.qux", ConfigValue::from_iter([("inline", "table")])) + .unwrap(); + insta::assert_snapshot!(layer.data, @r#" + foo = 1 + + [bar] + qux = { inline = "table" } + + [bar.baz] + blah = "2" + "#); + + // Can delete value + let old_value = layer.delete_value("foo").unwrap(); + assert_eq!(old_value.and_then(|v| v.as_integer()), Some(1)); + // Can delete inline table + let old_value = layer.delete_value("bar.qux").unwrap(); + assert!(old_value.is_some_and(|v| v.is_inline_table())); + // Cannot delete table + assert_matches!( + layer.delete_value("bar"), + Err(ConfigUpdateError::WouldDeleteTable { name }) if name == "bar" + ); + // Deleting a non-table child isn't an error because the value doesn't + // exist + assert_matches!(layer.delete_value("bar.baz.blah.blah"), Ok(None)); + insta::assert_snapshot!(layer.data, @r#" + [bar] + + [bar.baz] + blah = "2" + "#); + } + #[test] fn test_stacked_config_layer_order() { let empty_data = || DocumentMut::new(); From caf4e538e1b54d60b3baea2dd991d370b9e48687 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 12 Dec 2024 23:07:46 +0900 Subject: [PATCH 3/5] config: add convenient ConfigLayer wrapper that provides .save() method I'm going to remove write/remove_config_value_to/from_file() functions, but I don't want to copy layer.path.expect(..) to all callers. --- cli/src/command_error.rs | 7 ++++ cli/src/config.rs | 41 +++++------------------ lib/src/config.rs | 71 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 32 deletions(-) diff --git a/cli/src/command_error.rs b/cli/src/command_error.rs index 48e0178912..89c4ec58bf 100644 --- a/cli/src/command_error.rs +++ b/cli/src/command_error.rs @@ -22,6 +22,7 @@ use std::sync::Arc; use itertools::Itertools as _; use jj_lib::backend::BackendError; +use jj_lib::config::ConfigFileSaveError; use jj_lib::config::ConfigGetError; use jj_lib::config::ConfigLoadError; use jj_lib::dsl_util::Diagnostics; @@ -246,6 +247,12 @@ impl From for CommandError { } } +impl From for CommandError { + fn from(err: ConfigFileSaveError) -> Self { + user_error(err) + } +} + impl From for CommandError { fn from(err: ConfigGetError) -> Self { let hint = match &err { diff --git a/cli/src/config.rs b/cli/src/config.rs index aba8b2793f..df32a992d0 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -22,6 +22,7 @@ use std::path::PathBuf; use std::process::Command; use itertools::Itertools; +use jj_lib::config::ConfigFile; use jj_lib::config::ConfigLayer; use jj_lib::config::ConfigLoadError; use jj_lib::config::ConfigNamePathBuf; @@ -426,42 +427,17 @@ pub fn parse_config_args(toml_strs: &[ConfigArg]) -> Result, Co .try_collect() } -fn load_config_file_or_empty( - source: ConfigSource, - path: &Path, -) -> Result { - match ConfigLayer::load_from_file(source, path.into()) { - Ok(layer) => Ok(layer), - Err(ConfigLoadError::Read(err)) if err.error.kind() == std::io::ErrorKind::NotFound => { - // If config doesn't exist yet, read as empty and we'll write one. - let mut layer = ConfigLayer::empty(source); - layer.path = Some(path.into()); - Ok(layer) - } - Err(err) => Err(err), - } -} - -fn write_config(path: &Path, doc: &toml_edit::DocumentMut) -> Result<(), CommandError> { - std::fs::write(path, doc.to_string()).map_err(|err| { - user_error_with_message( - format!("Failed to write file {path}", path = path.display()), - err, - ) - }) -} - pub fn write_config_value_to_file( key: &ConfigNamePathBuf, value: toml_edit::Value, path: &Path, ) -> Result<(), CommandError> { // TODO: Load config layer by caller. Here we use a dummy source for now. - let mut layer = load_config_file_or_empty(ConfigSource::User, path)?; - layer - .set_value(key, value) + let mut file = ConfigFile::load_or_empty(ConfigSource::User, path)?; + file.set_value(key, value) .map_err(|err| user_error_with_message(format!("Failed to set {key}"), err))?; - write_config(path, &layer.data) + file.save()?; + Ok(()) } pub fn remove_config_value_from_file( @@ -469,14 +445,15 @@ pub fn remove_config_value_from_file( path: &Path, ) -> Result<(), CommandError> { // TODO: Load config layer by caller. Here we use a dummy source for now. - let mut layer = load_config_file_or_empty(ConfigSource::User, path)?; - let old_value = layer + let mut file = ConfigFile::load_or_empty(ConfigSource::User, path)?; + let old_value = file .delete_value(key) .map_err(|err| user_error_with_message(format!("Failed to unset {key}"), err))?; if old_value.is_none() { return Err(user_error(format!(r#""{key}" doesn't exist"#))); } - write_config(path, &layer.data) + file.save()?; + Ok(()) } /// Command name and arguments specified by config. diff --git a/lib/src/config.rs b/lib/src/config.rs index c1beb4fdc9..0f89aff68d 100644 --- a/lib/src/config.rs +++ b/lib/src/config.rs @@ -18,6 +18,7 @@ use std::borrow::Borrow; use std::convert::Infallible; use std::fmt; use std::fs; +use std::io; use std::ops::Range; use std::path::Path; use std::path::PathBuf; @@ -58,6 +59,11 @@ pub enum ConfigLoadError { }, } +/// Error that can occur when saving config variables to file. +#[derive(Debug, Error)] +#[error("Failed to write configuration file")] +pub struct ConfigFileSaveError(#[source] pub PathError); + /// Error that can occur when looking up config variable. #[derive(Debug, Error)] pub enum ConfigGetError { @@ -467,6 +473,71 @@ fn ensure_parent_table<'a, 'b>( Ok((parent_table, leaf_key)) } +/// Wrapper for file-based [`ConfigLayer`], providing convenient methods for +/// modification. +#[derive(Debug)] +pub struct ConfigFile { + layer: ConfigLayer, +} + +impl ConfigFile { + /// Loads TOML file from the specified `path` if exists. Returns an empty + /// object if the file doesn't exist. + pub fn load_or_empty( + source: ConfigSource, + path: impl Into, + ) -> Result { + let layer = match ConfigLayer::load_from_file(source, path.into()) { + Ok(layer) => layer, + Err(ConfigLoadError::Read(PathError { path, error })) + if error.kind() == io::ErrorKind::NotFound => + { + ConfigLayer { + source, + path: Some(path), + data: DocumentMut::new(), + } + } + Err(err) => return Err(err), + }; + Ok(ConfigFile { layer }) + } + + /// Writes serialized data to the source file. + pub fn save(&self) -> Result<(), ConfigFileSaveError> { + fs::write(self.path(), self.layer.data.to_string()) + .context(self.path()) + .map_err(ConfigFileSaveError) + } + + /// Source file path. + pub fn path(&self) -> &Path { + self.layer.path.as_ref().expect("path must be known") + } + + /// Returns the underlying config layer. + pub fn layer(&self) -> &ConfigLayer { + &self.layer + } + + /// See [`ConfigLayer::set_value()`]. + pub fn set_value( + &mut self, + name: impl ToConfigNamePath, + new_value: impl Into, + ) -> Result, ConfigUpdateError> { + self.layer.set_value(name, new_value) + } + + /// See [`ConfigLayer::delete_value()`]. + pub fn delete_value( + &mut self, + name: impl ToConfigNamePath, + ) -> Result, ConfigUpdateError> { + self.layer.delete_value(name) + } +} + /// Stack of configuration layers which can be merged as needed. /// /// A [`StackedConfig`] is something like a read-only `overlayfs`. Tables and From 49d70e6c181fb18ab6a2f0684e2685c6c7abc520 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 13 Dec 2024 17:10:45 +0900 Subject: [PATCH 4/5] cli: git: extract helper function that sets up trunk() alias --- cli/src/commands/git/clone.rs | 18 ++++++------------ cli/src/commands/git/init.rs | 23 +++++++---------------- cli/src/commands/git/mod.rs | 24 ++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 28 deletions(-) diff --git a/cli/src/commands/git/clone.rs b/cli/src/commands/git/clone.rs index 57f0261089..9c478a85a9 100644 --- a/cli/src/commands/git/clone.rs +++ b/cli/src/commands/git/clone.rs @@ -19,7 +19,6 @@ use std::num::NonZeroU32; use std::path::Path; use std::path::PathBuf; -use jj_lib::config::ConfigNamePathBuf; use jj_lib::git; use jj_lib::git::GitFetchError; use jj_lib::git::GitFetchStats; @@ -27,6 +26,7 @@ use jj_lib::repo::Repo; use jj_lib::str_util::StringPattern; use jj_lib::workspace::Workspace; +use super::write_repository_level_trunk_alias; use crate::cli_util::CommandHelper; use crate::cli_util::WorkspaceCommandHelper; use crate::command_error::cli_error; @@ -34,7 +34,6 @@ use crate::command_error::user_error; use crate::command_error::user_error_with_message; use crate::command_error::CommandError; use crate::commands::git::maybe_add_gitignore; -use crate::config::write_config_value_to_file; use crate::git_util::get_git_repo; use crate::git_util::map_git_error; use crate::git_util::print_git_import_stats; @@ -165,16 +164,11 @@ pub fn cmd_git_clone( let (mut workspace_command, stats) = clone_result?; if let Some(default_branch) = &stats.default_branch { - // Set repository level `trunk()` alias to the default remote branch. - let config_path = workspace_command.repo_path().join("config.toml"); - write_config_value_to_file( - &ConfigNamePathBuf::from_iter(["revset-aliases", "trunk()"]), - format!("{default_branch}@{remote_name}").into(), - &config_path, - )?; - writeln!( - ui.status(), - "Setting the revset alias \"trunk()\" to \"{default_branch}@{remote_name}\"" + write_repository_level_trunk_alias( + ui, + workspace_command.repo_path(), + remote_name, + default_branch, )?; let default_branch_remote_ref = workspace_command diff --git a/cli/src/commands/git/init.rs b/cli/src/commands/git/init.rs index 7677fbb783..b2b02ee49d 100644 --- a/cli/src/commands/git/init.rs +++ b/cli/src/commands/git/init.rs @@ -17,7 +17,6 @@ use std::path::Path; use std::path::PathBuf; use std::sync::Arc; -use jj_lib::config::ConfigNamePathBuf; use jj_lib::file_util; use jj_lib::git; use jj_lib::git::parse_git_ref; @@ -26,6 +25,7 @@ use jj_lib::repo::ReadonlyRepo; use jj_lib::repo::Repo; use jj_lib::workspace::Workspace; +use super::write_repository_level_trunk_alias; use crate::cli_util::print_trackable_remote_bookmarks; use crate::cli_util::start_repo_transaction; use crate::cli_util::CommandHelper; @@ -35,7 +35,6 @@ use crate::command_error::user_error_with_hint; use crate::command_error::user_error_with_message; use crate::command_error::CommandError; use crate::commands::git::maybe_add_gitignore; -use crate::config::write_config_value_to_file; use crate::git_util::get_git_repo; use crate::git_util::is_colocated_git_workspace; use crate::git_util::print_failed_git_export; @@ -240,20 +239,12 @@ pub fn maybe_set_repository_level_trunk_alias( let git_repo = get_git_repo(workspace_command.repo().store())?; if let Ok(reference) = git_repo.find_reference("refs/remotes/origin/HEAD") { if let Some(reference_name) = reference.symbolic_target() { - if let Some(RefName::RemoteBranch { - branch: default_branch, - .. - }) = parse_git_ref(reference_name) - { - let config_path = workspace_command.repo_path().join("config.toml"); - write_config_value_to_file( - &ConfigNamePathBuf::from_iter(["revset-aliases", "trunk()"]), - format!("{default_branch}@origin").into(), - &config_path, - )?; - writeln!( - ui.status(), - "Setting the revset alias \"trunk()\" to \"{default_branch}@origin\"", + if let Some(RefName::RemoteBranch { branch, .. }) = parse_git_ref(reference_name) { + write_repository_level_trunk_alias( + ui, + workspace_command.repo_path(), + "origin", + &branch, )?; } }; diff --git a/cli/src/commands/git/mod.rs b/cli/src/commands/git/mod.rs index 1c7fee9818..e874cd5676 100644 --- a/cli/src/commands/git/mod.rs +++ b/cli/src/commands/git/mod.rs @@ -21,7 +21,10 @@ pub mod push; pub mod remote; pub mod submodule; +use std::path::Path; + use clap::Subcommand; +use jj_lib::config::ConfigNamePathBuf; use self::clone::cmd_git_clone; use self::clone::GitCloneArgs; @@ -43,6 +46,7 @@ use crate::cli_util::CommandHelper; use crate::cli_util::WorkspaceCommandHelper; use crate::command_error::user_error_with_message; use crate::command_error::CommandError; +use crate::config::write_config_value_to_file; use crate::ui::Ui; /// Commands for working with Git remotes and the underlying Git repo @@ -102,3 +106,23 @@ fn get_single_remote(git_repo: &git2::Repository) -> Result, Comm _ => None, }) } + +/// Sets repository level `trunk()` alias to the specified remote branch. +fn write_repository_level_trunk_alias( + ui: &Ui, + repo_path: &Path, + remote: &str, + branch: &str, +) -> Result<(), CommandError> { + let config_path = repo_path.join("config.toml"); + write_config_value_to_file( + &ConfigNamePathBuf::from_iter(["revset-aliases", "trunk()"]), + format!("{branch}@{remote}").into(), + &config_path, + )?; + writeln!( + ui.status(), + r#"Setting the revset alias "trunk()" to "{branch}@{remote}""#, + )?; + Ok(()) +} From 53dd7381a527782ccdd2733d04b140bd660d761f Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 13 Dec 2024 17:18:04 +0900 Subject: [PATCH 5/5] cli: inline write/remove_config_value_to/from_file() They are short, and it wouldn't make much sense to do load, mutate one entry, and save in one function. In later patches, "jj config set"/"unset" will be changed to reuse the loaded ConfigLayer if the layer can be unambiguously selected. --- cli/src/commands/config/mod.rs | 13 ++++++++++++ cli/src/commands/config/set.rs | 16 ++++++--------- cli/src/commands/config/unset.rs | 18 ++++++++--------- cli/src/commands/git/mod.rs | 14 ++++++------- cli/src/config.rs | 34 -------------------------------- 5 files changed, 34 insertions(+), 61 deletions(-) diff --git a/cli/src/commands/config/mod.rs b/cli/src/commands/config/mod.rs index 6f85f532b5..be1476ccdd 100644 --- a/cli/src/commands/config/mod.rs +++ b/cli/src/commands/config/mod.rs @@ -21,6 +21,7 @@ mod unset; use std::path::Path; +use jj_lib::config::ConfigFile; use jj_lib::config::ConfigSource; use tracing::instrument; @@ -83,6 +84,18 @@ impl ConfigLevelArgs { panic!("No config_level provided") } } + + fn edit_config_file(&self, config_env: &ConfigEnv) -> Result { + let path = self.new_config_file_path(config_env)?; + if path.is_dir() { + return Err(user_error(format!( + "Can't set config in path {path} (dirs not supported)", + path = path.display() + ))); + } + let source = self.get_source_kind().unwrap(); + Ok(ConfigFile::load_or_empty(source, path)?) + } } /// Manage config options diff --git a/cli/src/commands/config/set.rs b/cli/src/commands/config/set.rs index 2ddf032f0a..3759c30b46 100644 --- a/cli/src/commands/config/set.rs +++ b/cli/src/commands/config/set.rs @@ -23,11 +23,10 @@ use tracing::instrument; use super::ConfigLevelArgs; use crate::cli_util::CommandHelper; use crate::cli_util::WorkspaceCommandHelper; -use crate::command_error::user_error; +use crate::command_error::user_error_with_message; use crate::command_error::CommandError; use crate::complete; use crate::config::parse_toml_value_or_bare_string; -use crate::config::write_config_value_to_file; use crate::ui::Ui; /// Update config file to set the given option to a given value. @@ -53,13 +52,7 @@ pub fn cmd_config_set( command: &CommandHelper, args: &ConfigSetArgs, ) -> Result<(), CommandError> { - let config_path = args.level.new_config_file_path(command.config_env())?; - if config_path.is_dir() { - return Err(user_error(format!( - "Can't set config in path {path} (dirs not supported)", - path = config_path.display() - ))); - } + let mut file = args.level.edit_config_file(command.config_env())?; // TODO(#531): Infer types based on schema (w/ --type arg to override). let value = parse_toml_value_or_bare_string(&args.value); @@ -72,7 +65,10 @@ pub fn cmd_config_set( check_wc_author(ui, command, &value, AuthorChange::Email)?; }; - write_config_value_to_file(&args.name, value, config_path) + file.set_value(&args.name, value) + .map_err(|err| user_error_with_message(format!("Failed to set {}", args.name), err))?; + file.save()?; + Ok(()) } /// Returns the commit of the working copy if it exists. diff --git a/cli/src/commands/config/unset.rs b/cli/src/commands/config/unset.rs index 6f96028df1..290fdec144 100644 --- a/cli/src/commands/config/unset.rs +++ b/cli/src/commands/config/unset.rs @@ -19,9 +19,9 @@ use tracing::instrument; use super::ConfigLevelArgs; use crate::cli_util::CommandHelper; use crate::command_error::user_error; +use crate::command_error::user_error_with_message; use crate::command_error::CommandError; use crate::complete; -use crate::config::remove_config_value_from_file; use crate::ui::Ui; /// Update config file to unset the given option. @@ -39,13 +39,13 @@ pub fn cmd_config_unset( command: &CommandHelper, args: &ConfigUnsetArgs, ) -> Result<(), CommandError> { - let config_path = args.level.new_config_file_path(command.config_env())?; - if config_path.is_dir() { - return Err(user_error(format!( - "Can't set config in path {path} (dirs not supported)", - path = config_path.display() - ))); + let mut file = args.level.edit_config_file(command.config_env())?; + let old_value = file + .delete_value(&args.name) + .map_err(|err| user_error_with_message(format!("Failed to unset {}", args.name), err))?; + if old_value.is_none() { + return Err(user_error(format!(r#""{}" doesn't exist"#, args.name))); } - - remove_config_value_from_file(&args.name, config_path) + file.save()?; + Ok(()) } diff --git a/cli/src/commands/git/mod.rs b/cli/src/commands/git/mod.rs index e874cd5676..a434d0c6c2 100644 --- a/cli/src/commands/git/mod.rs +++ b/cli/src/commands/git/mod.rs @@ -24,7 +24,8 @@ pub mod submodule; use std::path::Path; use clap::Subcommand; -use jj_lib::config::ConfigNamePathBuf; +use jj_lib::config::ConfigFile; +use jj_lib::config::ConfigSource; use self::clone::cmd_git_clone; use self::clone::GitCloneArgs; @@ -46,7 +47,6 @@ use crate::cli_util::CommandHelper; use crate::cli_util::WorkspaceCommandHelper; use crate::command_error::user_error_with_message; use crate::command_error::CommandError; -use crate::config::write_config_value_to_file; use crate::ui::Ui; /// Commands for working with Git remotes and the underlying Git repo @@ -114,12 +114,10 @@ fn write_repository_level_trunk_alias( remote: &str, branch: &str, ) -> Result<(), CommandError> { - let config_path = repo_path.join("config.toml"); - write_config_value_to_file( - &ConfigNamePathBuf::from_iter(["revset-aliases", "trunk()"]), - format!("{branch}@{remote}").into(), - &config_path, - )?; + let mut file = ConfigFile::load_or_empty(ConfigSource::Repo, repo_path.join("config.toml"))?; + file.set_value(["revset-aliases", "trunk()"], format!("{branch}@{remote}")) + .expect("initial repo config shouldn't have invalid values"); + file.save()?; writeln!( ui.status(), r#"Setting the revset alias "trunk()" to "{branch}@{remote}""#, diff --git a/cli/src/config.rs b/cli/src/config.rs index df32a992d0..b6d43640e1 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -22,7 +22,6 @@ use std::path::PathBuf; use std::process::Command; use itertools::Itertools; -use jj_lib::config::ConfigFile; use jj_lib::config::ConfigLayer; use jj_lib::config::ConfigLoadError; use jj_lib::config::ConfigNamePathBuf; @@ -34,10 +33,6 @@ use regex::Regex; use thiserror::Error; use tracing::instrument; -use crate::command_error::user_error; -use crate::command_error::user_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"); @@ -427,35 +422,6 @@ pub fn parse_config_args(toml_strs: &[ConfigArg]) -> Result, Co .try_collect() } -pub fn write_config_value_to_file( - key: &ConfigNamePathBuf, - value: toml_edit::Value, - path: &Path, -) -> Result<(), CommandError> { - // TODO: Load config layer by caller. Here we use a dummy source for now. - let mut file = ConfigFile::load_or_empty(ConfigSource::User, path)?; - file.set_value(key, value) - .map_err(|err| user_error_with_message(format!("Failed to set {key}"), err))?; - file.save()?; - Ok(()) -} - -pub fn remove_config_value_from_file( - key: &ConfigNamePathBuf, - path: &Path, -) -> Result<(), CommandError> { - // TODO: Load config layer by caller. Here we use a dummy source for now. - let mut file = ConfigFile::load_or_empty(ConfigSource::User, path)?; - let old_value = file - .delete_value(key) - .map_err(|err| user_error_with_message(format!("Failed to unset {key}"), err))?; - if old_value.is_none() { - return Err(user_error(format!(r#""{key}" doesn't exist"#))); - } - file.save()?; - Ok(()) -} - /// Command name and arguments specified by config. #[derive(Clone, Debug, Eq, PartialEq, serde::Deserialize)] #[serde(untagged)]