diff --git a/CHANGELOG.md b/CHANGELOG.md index 252f5aad9..adef563cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). documentation](docs/config.md#dotted-style-headings-and-inline-tables) for details. +* `jj config edit --user` now opens a file even if `$JJ_CONFIG` points to a + directory. If there are multiple config files, the command will fail. + * The deprecated `[alias]` config section is no longer respected. Move command aliases to the `[aliases]` section. diff --git a/cli/src/commands/config/edit.rs b/cli/src/commands/config/edit.rs index de3269c4c..f59b6fe37 100644 --- a/cli/src/commands/config/edit.rs +++ b/cli/src/commands/config/edit.rs @@ -36,6 +36,9 @@ pub fn cmd_config_edit( command: &CommandHelper, args: &ConfigEditArgs, ) -> Result<(), CommandError> { - let config_path = args.level.new_config_file_path(command.config_env())?; - run_ui_editor(command.settings(), config_path) + let file = args.level.edit_config_file(command)?; + if !file.path().exists() { + file.save()?; + } + run_ui_editor(command.settings(), file.path()) } diff --git a/cli/src/commands/config/mod.rs b/cli/src/commands/config/mod.rs index c840a08f6..2a49bf80b 100644 --- a/cli/src/commands/config/mod.rs +++ b/cli/src/commands/config/mod.rs @@ -67,25 +67,6 @@ impl ConfigLevelArgs { } } - fn new_config_file_path<'a>( - &self, - config_env: &'a ConfigEnv, - ) -> Result<&'a Path, CommandError> { - if self.user { - // TODO(#531): Special-case for editors that can't handle viewing - // directories? - config_env - .new_user_config_path()? - .ok_or_else(|| user_error("No user config path found to edit")) - } else if self.repo { - config_env - .repo_config_path() - .ok_or_else(|| user_error("No repo config path found to edit")) - } else { - panic!("No config_level provided") - } - } - fn config_path<'a>(&self, config_env: &'a ConfigEnv) -> Result<&'a Path, CommandError> { if self.user { config_env diff --git a/cli/src/config.rs b/cli/src/config.rs index 521735329..3ce9c2b8e 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -52,8 +52,6 @@ pub fn parse_toml_value_or_bare_string(value_str: &str) -> toml_edit::Value { pub enum ConfigEnvError { #[error("Both {0} and {1} exist. Please consolidate your configs in one of them.")] AmbiguousSource(PathBuf, PathBuf), - #[error(transparent)] - CreateFile(std::io::Error), } /// Configuration variable with its source information. @@ -164,18 +162,6 @@ fn create_dir_all(path: &Path) -> std::io::Result<()> { dir.create(path) } -fn create_config_file(path: &Path) -> std::io::Result { - if let Some(parent) = path.parent() { - create_dir_all(parent)?; - } - // TODO: Use File::create_new once stabilized. - std::fs::OpenOptions::new() - .read(true) - .write(true) - .create_new(true) - .open(path) -} - // The struct exists so that we can mock certain global values in unit tests. #[derive(Clone, Default, Debug)] struct UnresolvedConfigEnv { @@ -246,27 +232,6 @@ impl ConfigEnv { } } - /// Returns a path to the user-specific config file. - /// - /// If no config file is found, tries to guess a reasonable new location for - /// it. If a path to a new config file is returned, the parent directory may - /// be created as a result of this call. - pub fn new_user_config_path(&self) -> Result, ConfigEnvError> { - match &self.user_config_path { - ConfigPath::Existing(path) => Ok(Some(path)), - ConfigPath::New(path) => { - // TODO: Maybe we shouldn't create new file here. Not all - // callers need an empty file. "jj config set" doesn't have - // to create an empty file to be overwritten. Since it's unclear - // who and when to update ConfigPath::New(_) to ::Existing(_), - // it's probably better to not cache the path existence. - create_config_file(path).map_err(ConfigEnvError::CreateFile)?; - Ok(Some(path)) - } - ConfigPath::Unavailable => Ok(None), - } - } - /// Returns user configuration files for modification. Instantiates one if /// `config` has no user configuration layers. /// @@ -1116,19 +1081,10 @@ mod tests { let env = self .resolve(tmp.path()) .map_err(|e| anyhow!("new_config_path: {e}"))?; - let got = env - .new_user_config_path() - .map_err(|e| anyhow!("new_config_path: {e}"))?; + let got = env.user_config_path(); if got != want.as_deref() { return Err(anyhow!("new_config_path: got {got:?}, want {want:?}")); } - if let Some(path) = got { - if !Path::new(&path).is_file() { - return Err(anyhow!( - "new_config_path returned {path:?} which is not a file" - )); - } - } Ok(()) } } diff --git a/cli/tests/test_config_command.rs b/cli/tests/test_config_command.rs index fdaf00adb..5e41bdad8 100644 --- a/cli/tests/test_config_command.rs +++ b/cli/tests/test_config_command.rs @@ -784,6 +784,8 @@ fn test_config_edit_user() { let mut test_env = TestEnvironment::default(); test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); let repo_path = test_env.env_root().join("repo"); + // Remove one of the config file to disambiguate + std::fs::remove_file(test_env.last_config_file_path()).unwrap(); let edit_script = test_env.set_up_fake_editor(); std::fs::write(edit_script, "dump-path path").unwrap(); @@ -791,7 +793,25 @@ fn test_config_edit_user() { let edited_path = PathBuf::from(std::fs::read_to_string(test_env.env_root().join("path")).unwrap()); - assert_eq!(edited_path, dunce::simplified(test_env.config_path())); + assert_eq!( + edited_path, + dunce::simplified(&test_env.last_config_file_path()) + ); +} + +#[test] +fn test_config_edit_user_new_file() { + let mut test_env = TestEnvironment::default(); + let user_config_path = test_env.config_path().join("config").join("file.toml"); + test_env.set_up_fake_editor(); // set $EDITOR, but added configuration is ignored + test_env.set_config_path(user_config_path.clone()); + assert!(!user_config_path.exists()); + + test_env.jj_cmd_ok(test_env.env_root(), &["config", "edit", "--user"]); + assert!( + user_config_path.exists(), + "new file and directory should be created" + ); } #[test] @@ -799,17 +819,17 @@ fn test_config_edit_repo() { let mut test_env = TestEnvironment::default(); test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); let repo_path = test_env.env_root().join("repo"); + let repo_config_path = repo_path.join(PathBuf::from_iter([".jj", "repo", "config.toml"])); let edit_script = test_env.set_up_fake_editor(); + assert!(!repo_config_path.exists()); std::fs::write(edit_script, "dump-path path").unwrap(); test_env.jj_cmd_ok(&repo_path, &["config", "edit", "--repo"]); let edited_path = PathBuf::from(std::fs::read_to_string(test_env.env_root().join("path")).unwrap()); - assert_eq!( - edited_path, - dunce::simplified(&repo_path.join(".jj/repo/config.toml")) - ); + assert_eq!(edited_path, dunce::simplified(&repo_config_path)); + assert!(repo_config_path.exists(), "new file should be created"); } #[test]