From 199e74cbef3912daf7a008dce5795da8e69d312e Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 12 Dec 2024 22:52:49 +0900 Subject: [PATCH] cli: fix "jj config path" not create new file at default path This patch adds simpler user/repo_config_path() accessors. existing_*_path() are kinda implementation details (for testing), so they are now private methods. new_user_config_path() will be removed later. --- CHANGELOG.md | 2 ++ cli/src/commands/config/mod.rs | 16 ++++++++++++++- cli/src/commands/config/path.rs | 2 +- cli/src/config.rs | 33 +++++++++++++++++++------------ cli/tests/test_config_command.rs | 34 +++++++++++++++++++++----------- 5 files changed, 60 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 37bfed7bba..252f5aad91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * Fixed performance of progress bar rendering when fetching from Git remote. [#5057](https://github.com/martinvonz/jj/issues/5057) +* `jj config path --user` no longer creates new file at the default config path. + ## [0.24.0] - 2024-12-04 ### Release highlights diff --git a/cli/src/commands/config/mod.rs b/cli/src/commands/config/mod.rs index be1476ccdd..f6fdf6e1cf 100644 --- a/cli/src/commands/config/mod.rs +++ b/cli/src/commands/config/mod.rs @@ -78,13 +78,27 @@ impl ConfigLevelArgs { .ok_or_else(|| user_error("No user config path found to edit")) } else if self.repo { config_env - .new_repo_config_path() + .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 + .user_config_path() + .ok_or_else(|| user_error("No user config path found")) + } else if self.repo { + config_env + .repo_config_path() + .ok_or_else(|| user_error("No repo config path found")) + } else { + 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() { diff --git a/cli/src/commands/config/path.rs b/cli/src/commands/config/path.rs index b157495bed..9bdb8445a1 100644 --- a/cli/src/commands/config/path.rs +++ b/cli/src/commands/config/path.rs @@ -39,7 +39,7 @@ pub fn cmd_config_path( command: &CommandHelper, args: &ConfigPathArgs, ) -> Result<(), CommandError> { - let config_path = args.level.new_config_file_path(command.config_env())?; + let config_path = args.level.config_path(command.config_env())?; writeln!( ui.stdout(), "{}", diff --git a/cli/src/config.rs b/cli/src/config.rs index b6d43640e1..a6fb2ecd51 100644 --- a/cli/src/config.rs +++ b/cli/src/config.rs @@ -141,6 +141,13 @@ impl ConfigPath { None => ConfigPath::Unavailable, } } + + fn as_path(&self) -> Option<&Path> { + match self { + ConfigPath::Existing(path) | ConfigPath::New(path) => Some(path), + ConfigPath::Unavailable => None, + } + } } /// Like std::fs::create_dir_all but creates new directories to be accessible to @@ -225,8 +232,13 @@ impl ConfigEnv { }) } + /// Returns a path to the user-specific config file or directory. + pub fn user_config_path(&self) -> Option<&Path> { + self.user_config_path.as_path() + } + /// Returns a path to the existing user-specific config file or directory. - pub fn existing_user_config_path(&self) -> Option<&Path> { + fn existing_user_config_path(&self) -> Option<&Path> { match &self.user_config_path { ConfigPath::Existing(path) => Some(path), _ => None, @@ -243,8 +255,7 @@ impl ConfigEnv { 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. For example, "jj config path" - // should be a readonly operation. "jj config set" doesn't have + // 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. @@ -276,20 +287,16 @@ impl ConfigEnv { self.repo_config_path = ConfigPath::new(Some(path.join("config.toml"))); } - /// Returns a path to the existing repo-specific config file. - pub fn existing_repo_config_path(&self) -> Option<&Path> { - match &self.repo_config_path { - ConfigPath::Existing(path) => Some(path), - _ => None, - } + /// Returns a path to the repo-specific config file. + pub fn repo_config_path(&self) -> Option<&Path> { + self.repo_config_path.as_path() } - /// Returns a path to the repo-specific config file. - pub fn new_repo_config_path(&self) -> Option<&Path> { + /// Returns a path to the existing repo-specific config file. + fn existing_repo_config_path(&self) -> Option<&Path> { match &self.repo_config_path { ConfigPath::Existing(path) => Some(path), - ConfigPath::New(path) => Some(path), - ConfigPath::Unavailable => None, + _ => None, } } diff --git a/cli/tests/test_config_command.rs b/cli/tests/test_config_command.rs index d974463c97..3ce6c9e8b7 100644 --- a/cli/tests/test_config_command.rs +++ b/cli/tests/test_config_command.rs @@ -15,7 +15,6 @@ use std::path::PathBuf; use indoc::indoc; -use insta::assert_snapshot; use itertools::Itertools; use regex::Regex; @@ -793,22 +792,33 @@ fn test_config_edit_repo() { #[test] fn test_config_path() { - let test_env = TestEnvironment::default(); + 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"); - assert_snapshot!( - test_env.jj_cmd_success(&repo_path, &["config", "path", "--user"]), - @r###" - $TEST_ENV/config - "### + let user_config_path = test_env.env_root().join("config.toml"); + let repo_config_path = repo_path.join(PathBuf::from_iter([".jj", "repo", "config.toml"])); + test_env.set_config_path(user_config_path.clone()); + + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["config", "path", "--user"]), + @"$TEST_ENV/config.toml"); + assert!( + !user_config_path.exists(), + "jj config path shouldn't create new file" ); - assert_snapshot!( - test_env.jj_cmd_success(&repo_path, &["config", "path", "--repo"]), - @r###" - $TEST_ENV/repo/.jj/repo/config.toml - "### + + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["config", "path", "--repo"]), + @"$TEST_ENV/repo/.jj/repo/config.toml"); + assert!( + !repo_config_path.exists(), + "jj config path shouldn't create new file" ); + + insta::assert_snapshot!( + test_env.jj_cmd_failure(test_env.env_root(), &["config", "path", "--repo"]), + @"Error: No repo config path found"); } #[test]