8000 cli: make "jj config edit" always open file, not directory · torquestomp/jj@e50673d · GitHub
[go: up one dir, main page]
Skip to content

Commit e50673d

Browse files
yujayuja
committed
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".
1 parent ac52e43 commit e50673d

File tree

5 files changed

+34
-71
lines changed

5 files changed

+34
-71
lines changed

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
2424
documentation](docs/config.md#dotted-style-headings-and-inline-tables) for
2525
details.
2626

27+
* `jj config edit --user` now opens a file even if `$JJ_CONFIG` points to a
28+
directory. If there are multiple config files, the command will fail.
29+
2730
* The deprecated `[alias]` config section is no longer respected. Move command
2831
aliases to the `[aliases]` section.
2932

‎cli/src/commands/config/edit.rs‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,9 @@ pub fn cmd_config_edit(
3636
command: &CommandHelper,
3737
args: &ConfigEditArgs,
3838
) -> Result<(), CommandError> {
39-
let config_path = args.level.new_config_file_path(command.config_env())?;
40-
run_ui_editor(command.settings(), config_path)
39+
let file = args.level.edit_config_file(command)?;
40+
if !file.path().exists() {
41+
file.save()?;
42+
}
43+
run_ui_editor(command.settings(), file.path())
4144
}

‎cli/src/commands/config/mod.rs‎

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -67,25 +67,6 @@ impl ConfigLevelArgs {
6767
}
6868
}
6969

70-
fn new_config_file_path<'a>(
71-
&self,
72-
config_env: &'a ConfigEnv,
73-
) -> Result<&'a Path, CommandError> {
74-
if self.user {
75-
// TODO(#531): Special-case for editors that can't handle viewing
76-
// directories?
77-
config_env
78-
.new_user_config_path()?
79-
.ok_or_else(|| user_error("No user config path found to edit"))
80-
} else if self.repo {
81-
config_env
82-
.repo_config_path()
83-
.ok_or_else(|| user_error("No repo config path found to edit"))
84-
} else {
85-
panic!("No config_level provided")
86-
}
87-
}
88-
8970
fn config_path<'a>(&self, config_env: &'a ConfigEnv) -> Result<&'a Path, CommandError> {
9071
if self.user {
9172
config_env

‎cli/src/config.rs‎

Lines changed: 1 addition & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,6 @@ pub fn parse_toml_value_or_bare_string(value_str: &str) -> toml_edit::Value {
5252
pub enum ConfigEnvError {
5353
#[error("Both {0} and {1} exist. Please consolidate your configs in one of them.")]
5454
AmbiguousSource(PathBuf, PathBuf),
55-
#[error(transparent)]
56-
CreateFile(std::io::Error),
5755
}
5856

5957
/// Configuration variable with its source information.
@@ -164,18 +162,6 @@ fn create_dir_all(path: &Path) -> std::io::Result<()> {
164162
dir.create(path)
165163
}
166164

167-
fn create_config_file(path: &Path) -> std::io::Result<std::fs::File> {
168-
if let Some(parent) = path.parent() {
169-
create_dir_all(parent)?;
170-
}
171-
// TODO: Use File::create_new once stabilized.
172-
std::fs::OpenOptions::new()
173-
.read(true)
174-
.write(true)
175-
.create_new(true)
176-
.open(path)
177-
}
178-
179165
// The struct exists so that we can mock certain global values in unit tests.
180166
#[derive(Clone, Default, Debug)]
181167
struct UnresolvedConfigEnv {
@@ -246,27 +232,6 @@ impl ConfigEnv {
246232
}
247233
}
248234

249-
/// Returns a path to the user-specific config file.
250-
///
251-
/// If no config file is found, tries to guess a reasonable new location for
252-
/// it. If a path to a new config file is returned, the parent directory may
253-
/// be created as a result of this call.
254-
pub fn new_user_config_path(&self) -> Result<Option<&Path>, ConfigEnvError> {
255-
match &self.user_config_path {
256-
ConfigPath::Existing(path) => Ok(Some(path)),
257-
ConfigPath::New(path) => {
258-
// TODO: Maybe we shouldn't create new file here. Not all
259-
// callers need an empty file. "jj config set" doesn't have
260-
// to create an empty file to be overwritten. Since it's unclear
261-
// who and when to update ConfigPath::New(_) to ::Existing(_),
262-
// it's probably better to not cache the path existence.
263-
create_config_file(path).map_err(ConfigEnvError::CreateFile)?;
264-
Ok(Some(path))
265-
}
266-
ConfigPath::Unavailable => Ok(None),
267-
}
268-
}
269-
270235
/// Returns user configuration files for modification. Instantiates one if
271236
/// `config` has no user configuration layers.
272237
///
@@ -1116,19 +1081,10 @@ mod tests {
11161081
let env = self
11171082
.resolve(tmp.path())
11181083
.map_err(|e| anyhow!("new_config_path: {e}"))?;
1119-
let got = env
1120-
.new_user_config_path()
1121-
.map_err(|e| anyhow!("new_config_path: {e}"))?;
1084+
let got = env.user_config_path();
11221085
if got != want.as_deref() {
11231086
return Err(anyhow!("new_config_path: got {got:?}, want {want:?}"));
11241087
}
1125-
if let Some(path) = got {
1126-
if !Path::new(&path).is_file() {
1127-
return Err(anyhow!(
1128-
"new_config_path returned {path:?} which is not a file"
1129-
));
1130-
}
1131-
}
11321088
Ok(())
11331089
}
11341090
}

‎cli/tests/test_config_command.rs‎

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -784,32 +784,52 @@ fn test_config_edit_user() {
784784
let mut test_env = TestEnvironment::default();
785785
test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
786786
let repo_path = test_env.env_root().join("repo");
787+
// Remove one of the config file to disambiguate
788+
std::fs::remove_file(test_env.last_config_file_path()).unwrap();
787789
let edit_script = test_env.set_up_fake_editor();
788790

789791
std::fs::write(edit_script, "dump-path path").unwrap();
790792
test_env.jj_cmd_ok(&repo_path, &["config", "edit", "--user"]);
791793

792794
let edited_path =
793795
PathBuf::from(std::fs::read_to_string(test_env.env_root().join("path")).unwrap());
794-
assert_eq!(edited_path, dunce::simplified(test_env.config_path()));
796+
assert_eq!(
797+
edited_path,
798+
dunce::simplified(&test_env.last_config_file_path())
799+
);
800+
}
801+
802+
#[test]
803+
fn test_config_edit_user_new_file() {
804+
let mut test_env = TestEnvironment::default();
805+
let user_config_path = test_env.config_path().join("config").join("file.toml");
806+
test_env.set_up_fake_editor(); // set $EDITOR, but added configuration is ignored
807+
test_env.set_config_path(user_config_path.clone());
808+
assert!(!user_config_path.exists());
809+
810+
test_env.jj_cmd_ok(test_env.env_root(), &["config", "edit", "--user"]);
811+
assert!(
812+
user_config_path.exists(),
813+
"new file and directory should be created"
814+
);
795815
}
796816

797817
#[test]
798818
fn test_config_edit_repo() {
799819
let mut test_env = TestEnvironment::default();
800820
test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
801821
let repo_path = test_env.env_root().join("repo");
822+
let repo_config_path = repo_path.join(PathBuf::from_iter([".jj", "repo", "config.toml"]));
802823
let edit_script = test_env.set_up_fake_editor();
824+
assert!(!repo_config_path.exists());
803825

804826
std::fs::write(edit_script, "dump-path path").unwrap();
805827
test_env.jj_cmd_ok(&repo_path, &["config", "edit", "--repo"]);
806828

807829
let edited_path =
808830
PathBuf::from(std::fs::read_to_string(test_env.env_root().join("path")).unwrap());
809-
assert_eq!(
810-
edited_path,
811-
dunce::simplified(&repo_path.join(".jj/repo/config.toml"))
812-
);
831+
assert_eq!(edited_path, dunce::simplified(&repo_config_path));
832+
assert!(repo_config_path.exists(), "new file should be created");
813833
}
814834

815835
#[test]

0 commit comments

Comments
 (0)
0