From ac52e43435c35c0781fb66935728432e81c84482 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 12 Dec 2024 23:07:46 +0900 Subject: [PATCH] cli: let "config set"/"unset" continue if file to edit can be disambiguated If the user config path was an empty directory, these commands would fail with EISDIR instead of "can't set config in path (dirs not supported)". I think this can be changed later to create new "$dir/config.toml" file. Maybe we can also change the default path to ~/.config/jj directory, and load all *.toml files from there? --- cli/src/commands/config/mod.rs | 38 +++++++++++++----- cli/src/commands/config/set.rs | 2 +- cli/src/commands/config/unset.rs | 2 +- cli/src/config.rs | 66 ++++++++++++++++++++++++++++++++ cli/tests/common/mod.rs | 6 +++ cli/tests/test_config_command.rs | 66 ++++++++++++++++++++------------ lib/src/config.rs | 16 ++++++++ 7 files changed, 161 insertions(+), 35 deletions(-) diff --git a/cli/src/commands/config/mod.rs b/cli/src/commands/config/mod.rs index f6fdf6e1cf..c840a08f68 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 itertools::Itertools as _; use jj_lib::config::ConfigFile; use jj_lib::config::ConfigSource; use tracing::instrument; @@ -99,16 +100,35 @@ impl ConfigLevelArgs { } } - 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() - ))); + fn edit_config_file(&self, command: &CommandHelper) -> Result { + let config_env = command.config_env(); + let config = command.settings().config(); + let pick_one = |mut files: Vec, not_found_error: &str| { + if files.len() > 1 { + // TODO: prompt or pick the last? + return Err(user_error(format!( + "Cannot determine config file to edit:\n{}", + files + .iter() + .map(|file| format!(" {}", file.path().display())) + .join("\n") + ))); + } + files.pop().ok_or_else(|| user_error(not_found_error)) + }; + if self.user { + pick_one( + config_env.user_config_files(config)?, + "No user config path found to edit", + ) + } else if self.repo { + pick_one( + config_env.repo_config_files(config)?, + "No repo config path found to edit", + ) + } else { + panic!("No config_level provided") } - let source = self.get_source_kind().unwrap(); - Ok(ConfigFile::load_or_empty(source, path)?) } } diff --git a/cli/src/commands/config/set.rs b/cli/src/commands/config/set.rs index 3759c30b46..349e5e039b 100644 --- a/cli/src/commands/config/set.rs +++ b/cli/src/commands/config/set.rs @@ -52,7 +52,7 @@ pub fn cmd_config_set( command: &CommandHelper, args: &ConfigSetArgs, ) -> Result<(), CommandError> { - let mut file = args.level.edit_config_file(command.config_env())?; + let mut file = args.level.edit_config_file(command)?; // TODO(#531): Infer types based on schema (w/ --type arg to override). let value = parse_toml_value_or_bare_string(&args.value); diff --git a/cli/src/commands/config/unset.rs b/cli/src/commands/config/unset.rs index 290fdec144..deb68ac629 100644 --- a/cli/src/commands/config/unset.rs +++ b/cli/src/commands/config/unset.rs @@ -39,7 +39,7 @@ pub fn cmd_config_unset( command: &CommandHelper, args: &ConfigUnsetArgs, ) -> Result<(), CommandError> { - let mut file = args.level.edit_config_file(command.config_env())?; + let mut file = args.level.edit_config_file(command)?; let old_value = file .delete_value(&args.name) .map_err(|err| user_error_with_message(format!("Failed to unset {}", args.name), err))?; diff --git a/cli/src/config.rs b/cli/src/config.rs index a6fb2ecd51..521735329e 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; @@ -266,6 +267,34 @@ impl ConfigEnv { } } + /// Returns user configuration files for modification. Instantiates one if + /// `config` has no user configuration layers. + /// + /// The parent directory for the new file may be created by this function. + /// If the user configuration path is unknown, this function returns an + /// empty `Vec`. + pub fn user_config_files( + &self, + config: &StackedConfig, + ) -> Result, ConfigLoadError> { + config_files_for(config, ConfigSource::User, || self.new_user_config_file()) + } + + fn new_user_config_file(&self) -> Result, ConfigLoadError> { + self.user_config_path() + .map(|path| { + // No need to propagate io::Error here. If the directory + // couldn't be created, file.save() would fail later. + if let Some(dir) = path.parent() { + create_dir_all(dir).ok(); + } + // The path doesn't usually exist, but we shouldn't overwrite it + // with an empty config if it did exist. + ConfigFile::load_or_empty(ConfigSource::User, path) + }) + .transpose() + } + /// Loads user-specific config files into the given `config`. The old /// user-config layers will be replaced if any. #[instrument] @@ -300,6 +329,27 @@ impl ConfigEnv { } } + /// Returns repo configuration files for modification. Instantiates one if + /// `config` has no repo configuration layers. + /// + /// If the repo path is unknown, this function returns an empty `Vec`. Since + /// the repo config path cannot be a directory, the returned `Vec` should + /// have at most one config file. + pub fn repo_config_files( + &self, + config: &StackedConfig, + ) -> Result, ConfigLoadError> { + config_files_for(config, ConfigSource::Repo, || self.new_repo_config_file()) + } + + fn new_repo_config_file(&self) -> Result, ConfigLoadError> { + self.repo_config_path() + // The path doesn't usually exist, but we shouldn't overwrite it + // with an empty config if it did exist. + .map(|path| ConfigFile::load_or_empty(ConfigSource::Repo, path)) + .transpose() + } + /// Loads repo-specific config file into the given `config`. The old /// repo-config layer will be replaced if any. #[instrument] @@ -312,6 +362,22 @@ impl ConfigEnv { } } +fn config_files_for( + config: &StackedConfig, + source: ConfigSource, + new_file: impl FnOnce() -> Result, ConfigLoadError>, +) -> Result, ConfigLoadError> { + let mut files = config + .layers_for(source) + .iter() + .filter_map(|layer| ConfigFile::from_layer(layer.clone()).ok()) + .collect_vec(); + if files.is_empty() { + files.extend(new_file()?); + } + Ok(files) +} + /// Initializes stacked config with the given `default_layers` and infallible /// sources. /// diff --git a/cli/tests/common/mod.rs b/cli/tests/common/mod.rs index c70beca6ee..7b69f45ade 100644 --- a/cli/tests/common/mod.rs +++ b/cli/tests/common/mod.rs @@ -248,6 +248,12 @@ impl TestEnvironment { &self.config_path } + pub fn last_config_file_path(&self) -> PathBuf { + let config_file_number = self.config_file_number.borrow(); + self.config_path + .join(format!("config{config_file_number:04}.toml")) + } + pub fn set_config_path(&mut self, config_path: PathBuf) { self.config_path = config_path; } diff --git a/cli/tests/test_config_command.rs b/cli/tests/test_config_command.rs index 3ce6c9e8b7..fdaf00adb2 100644 --- a/cli/tests/test_config_command.rs +++ b/cli/tests/test_config_command.rs @@ -201,6 +201,7 @@ bar fn test_config_list_layer() { let mut test_env = TestEnvironment::default(); test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + // Test with fresh new config file let user_config_path = test_env.config_path().join("config.toml"); test_env.set_config_path(user_config_path.clone()); let repo_path = test_env.env_root().join("repo"); @@ -473,7 +474,7 @@ fn test_config_set_bad_opts() { fn test_config_set_for_user() { let mut test_env = TestEnvironment::default(); test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); - // Point to a config file since `config set` can't handle directories. + // Test with fresh new config file let user_config_path = test_env.config_path().join("config.toml"); test_env.set_config_path(user_config_path.clone()); let repo_path = test_env.env_root().join("repo"); @@ -503,6 +504,36 @@ fn test_config_set_for_user() { "###); } +#[test] +fn test_config_set_for_user_directory() { + let test_env = TestEnvironment::default(); + + test_env.jj_cmd_ok( + test_env.env_root(), + &["config", "set", "--user", "test-key", "test-val"], + ); + insta::assert_snapshot!( + std::fs::read_to_string(test_env.last_config_file_path()).unwrap(), + @r#" + test-key = "test-val" + + [template-aliases] + 'format_time_range(time_range)' = 'time_range.start() ++ " - " ++ time_range.end()' + "#); + + // Add one more config file to the directory + test_env.add_config(""); + let stderr = test_env.jj_cmd_failure( + test_env.env_root(), + &["config", "set", "--user", "test-key", "test-val"], + ); + insta::assert_snapshot!(stderr, @r" + Error: Cannot determine config file to edit: + $TEST_ENV/config/config0001.toml + $TEST_ENV/config/config0002.toml + "); +} + #[test] fn test_config_set_for_repo() { let test_env = TestEnvironment::default(); @@ -537,6 +568,7 @@ fn test_config_set_for_repo() { fn test_config_set_toml_types() { let mut test_env = TestEnvironment::default(); test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + // Test with fresh new config file let user_config_path = test_env.config_path().join("config.toml"); test_env.set_config_path(user_config_path.clone()); let repo_path = test_env.env_root().join("repo"); @@ -563,10 +595,8 @@ fn test_config_set_toml_types() { #[test] fn test_config_set_type_mismatch() { - let mut test_env = TestEnvironment::default(); + let test_env = TestEnvironment::default(); test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); - let user_config_path = test_env.config_path().join("config.toml"); - test_env.set_config_path(user_config_path); let repo_path = test_env.env_root().join("repo"); test_env.jj_cmd_ok( @@ -603,10 +633,8 @@ fn test_config_set_type_mismatch() { #[test] fn test_config_set_nontable_parent() { - let mut test_env = TestEnvironment::default(); + let test_env = TestEnvironment::default(); test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); - let user_config_path = test_env.config_path().join("config.toml"); - test_env.set_config_path(user_config_path); let repo_path = test_env.env_root().join("repo"); test_env.jj_cmd_ok( @@ -625,11 +653,8 @@ fn test_config_set_nontable_parent() { #[test] fn test_config_unset_non_existent_key() { - let mut test_env = TestEnvironment::default(); + let test_env = TestEnvironment::default(); test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); - // Point to a config file since `config set` can't handle directories. - let user_config_path = test_env.config_path().join("config.toml"); - test_env.set_config_path(user_config_path); let repo_path = test_env.env_root().join("repo"); let stderr = test_env.jj_cmd_failure(&repo_path, &["config", "unset", "--user", "nonexistent"]); @@ -638,11 +663,8 @@ fn test_config_unset_non_existent_key() { #[test] fn test_config_unset_inline_table_key() { - let mut test_env = TestEnvironment::default(); + let test_env = TestEnvironment::default(); test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); - // Point to a config file since `config set` can't handle directories. - let user_config_path = test_env.config_path().join("config.toml"); - test_env.set_config_path(user_config_path.clone()); let repo_path = test_env.env_root().join("repo"); test_env.jj_cmd_ok( @@ -660,7 +682,7 @@ fn test_config_unset_inline_table_key() { #[test] fn test_config_unset_table_like() { let mut test_env = TestEnvironment::default(); - // Point to a config file since `config unset` can't handle directories. + // Test with fresh new config file let user_config_path = test_env.config_path().join("config.toml"); test_env.set_config_path(user_config_path.clone()); @@ -700,7 +722,7 @@ fn test_config_unset_table_like() { fn test_config_unset_for_user() { let mut test_env = TestEnvironment::default(); test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); - // Point to a config file since `config set` can't handle directories. + // Test with fresh new config file let user_config_path = test_env.config_path().join("config.toml"); test_env.set_config_path(user_config_path.clone()); let repo_path = test_env.env_root().join("repo"); @@ -991,10 +1013,8 @@ fn test_config_path_syntax() { #[test] fn test_config_show_paths() { - let mut test_env = TestEnvironment::default(); + let test_env = TestEnvironment::default(); test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); - let user_config_path = test_env.config_path().join("config.toml"); - test_env.set_config_path(user_config_path.clone()); let repo_path = test_env.env_root().join("repo"); test_env.jj_cmd_ok( @@ -1006,7 +1026,7 @@ fn test_config_show_paths() { Config error: Invalid type or value for ui.paginate Caused by: unknown variant `:builtin`, expected `never` or `auto` - Hint: Check the config file: $TEST_ENV/config/config.toml + Hint: Check the config file: $TEST_ENV/config/config0001.toml For help, see https://martinvonz.github.io/jj/latest/config/. "); } @@ -1039,9 +1059,7 @@ fn test_config_author_change_warning() { #[test] fn test_config_author_change_warning_root_env() { - let mut test_env = TestEnvironment::default(); - let user_config_path = test_env.config_path().join("config.toml"); - test_env.set_config_path(user_config_path); + let test_env = TestEnvironment::default(); test_env.jj_cmd_success( test_env.env_root(), &["config", "set", "--user", "user.email", "'Foo'"], diff --git a/lib/src/config.rs b/lib/src/config.rs index 0f89aff68d..19b4660f9b 100644 --- a/lib/src/config.rs +++ b/lib/src/config.rs @@ -503,6 +503,17 @@ impl ConfigFile { Ok(ConfigFile { layer }) } + /// Wraps file-based [`ConfigLayer`] for modification. Returns `Err(layer)` + /// if the source `path` is unknown. + #[allow(clippy::result_large_err)] // Ok-variant is as large as Err anyway + pub fn from_layer(layer: ConfigLayer) -> Result { + if layer.path.is_some() { + Ok(ConfigFile { layer }) + } else { + Err(layer) + } + } + /// Writes serialized data to the source file. pub fn save(&self) -> Result<(), ConfigFileSaveError> { fs::write(self.path(), self.layer.data.to_string()) @@ -656,6 +667,11 @@ impl StackedConfig { &self.layers } + /// Layers of the specified `source`. + pub fn layers_for(&self, source: ConfigSource) -> &[ConfigLayer] { + &self.layers[self.layer_range(source)] + } + /// Looks up value of the specified type `T` from all layers, merges sub /// fields as needed. pub fn get<'de, T: Deserialize<'de>>(