Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
cli: make "jj config edit" always open file, not directory
Browse files Browse the repository at this point in the history
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".
yuja committed Dec 15, 2024
1 parent 0d1c488 commit b138da9
Showing 5 changed files with 34 additions and 71 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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.

7 changes: 5 additions & 2 deletions cli/src/commands/config/edit.rs
Original file line number Diff line number Diff line change
@@ -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())
}
19 changes: 0 additions & 19 deletions cli/src/commands/config/mod.rs
Original file line number Diff line number Diff line change
@@ -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
46 changes: 1 addition & 45 deletions cli/src/config.rs
Original file line number Diff line number Diff line change
@@ -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(())
}
}
30 changes: 25 additions & 5 deletions cli/tests/test_config_command.rs
Original file line number Diff line number Diff line change
@@ -784,32 +784,52 @@ 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();
test_env.jj_cmd_ok(&repo_path, &["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]
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]

0 comments on commit b138da9

Please sign in to comment.