cli: make "jj config edit" always open file, not directory

I don't think the new behavior is strictly better, but it's more consistent
with the other "jj config" commands so we can remove the special case for
"jj config edit".
This commit is contained in:
Yuya Nishihara 2024-12-14 15:13:46 +09:00
parent ac52e43435
commit e50673d5ac
5 changed files with 34 additions and 71 deletions

View file

@ -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.

View file

@ -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())
}

View file

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

View file

@ -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<std::fs::File> {
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<Option<&Path>, 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(())
}
}

View file

@ -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]