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>>(