diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f9e75625..d5b3ee780 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### New features +* `jj config edit --user` and `jj config set --user` will now pick a default + config location if no existing file is found, potentially creating parent directories. + * `jj log` output is now topologically grouped. [#242](https://github.com/martinvonz/jj/issues/242) diff --git a/Cargo.lock b/Cargo.lock index dc41d50f7..cf9b13fe5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -109,9 +109,9 @@ dependencies = [ [[package]] name = "anyhow" -version = "1.0.68" +version = "1.0.72" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2cb2f989d18dd141ab8ae82f64d1a8cdd37e0840f73a406896cf5e99502fab61" +checksum = "3b13c32d80ecc7ab747b80c3784bce54ee8a7a0cc4fbda9bf4cda2cf6fe90854" [[package]] name = "assert_cmd" @@ -997,6 +997,7 @@ checksum = "6c8af84674fe1f223a982c933a0ee1086ac4d4052aa0fb8060c12c6ad838e754" name = "jj-cli" version = "0.8.0" dependencies = [ + "anyhow", "assert_cmd", "assert_matches", "cargo_metadata", diff --git a/Cargo.toml b/Cargo.toml index d9392406a..a92c6a1d1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -69,6 +69,7 @@ tracing-subscriber = { version = "0.3.17", default-features = false, features = libc = { version = "0.2.147" } [dev-dependencies] +anyhow = "1.0.72" assert_cmd = "2.0.8" assert_matches = "1.5.0" insta = { version = "1.31.0", features = ["filters"] } diff --git a/src/cli_util.rs b/src/cli_util.rs index 8cc5a7d75..afaaf6609 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -68,7 +68,7 @@ use tracing_chrome::ChromeLayerBuilder; use tracing_subscriber::prelude::*; use crate::config::{ - config_path, AnnotatedValue, CommandNameAndArgs, ConfigSource, LayeredConfigs, + new_config_path, AnnotatedValue, CommandNameAndArgs, ConfigSource, LayeredConfigs, }; use crate::formatter::{FormatRecorder, Formatter, PlainTextFormatter}; use crate::merge_tools::{ConflictResolveError, DiffEditError}; @@ -2015,14 +2015,14 @@ pub fn write_config_value_to_file( }) } -pub fn get_config_file_path( +pub fn get_new_config_file_path( config_source: &ConfigSource, workspace_loader: &WorkspaceLoader, ) -> Result { let edit_path = match config_source { // TODO(#531): Special-case for editors that can't handle viewing directories? ConfigSource::User => { - config_path()?.ok_or_else(|| user_error("No repo config path found to edit"))? + new_config_path()?.ok_or_else(|| user_error("No repo config path found to edit"))? } ConfigSource::Repo => workspace_loader.repo_path().join("config.toml"), _ => { diff --git a/src/commands/mod.rs b/src/commands/mod.rs index df17fda73..61de64640 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -55,7 +55,7 @@ use jj_lib::{file_util, revset}; use maplit::{hashmap, hashset}; use crate::cli_util::{ - check_stale_working_copy, get_config_file_path, print_checkout_stats, + check_stale_working_copy, get_new_config_file_path, print_checkout_stats, resolve_multiple_nonempty_revsets, resolve_multiple_nonempty_revsets_flag_guarded, run_ui_editor, serialize_config_value, short_commit_hash, user_error, user_error_with_hint, write_config_value_to_file, Args, CommandError, CommandHelper, DescriptionArg, @@ -1234,7 +1234,7 @@ fn cmd_config_set( command: &CommandHelper, args: &ConfigSetArgs, ) -> Result<(), CommandError> { - let config_path = get_config_file_path( + let config_path = get_new_config_file_path( &args.config_args.get_source_kind(), command.workspace_loader()?, )?; @@ -1252,7 +1252,7 @@ fn cmd_config_edit( command: &CommandHelper, args: &ConfigEditArgs, ) -> Result<(), CommandError> { - let config_path = get_config_file_path( + let config_path = get_new_config_file_path( &args.config_args.get_source_kind(), command.workspace_loader()?, )?; diff --git a/src/config.rs b/src/config.rs index 247832585..3b9b342e9 100644 --- a/src/config.rs +++ b/src/config.rs @@ -29,6 +29,8 @@ pub enum ConfigError { ConfigReadError(#[from] config::ConfigError), #[error("Both {0} and {1} exist. Please consolidate your configs in one of them.")] AmbiguousSource(PathBuf, PathBuf), + #[error(transparent)] + ConfigCreateError(#[from] std::io::Error), } #[derive(Clone, Debug, PartialEq, Eq)] @@ -83,7 +85,7 @@ impl LayeredConfigs { } pub fn read_user_config(&mut self) -> Result<(), ConfigError> { - self.user = config_path()? + self.user = existing_config_path()? .map(|path| read_config_path(&path)) .transpose()?; Ok(()) @@ -190,30 +192,126 @@ impl LayeredConfigs { } } -pub fn config_path() -> Result, ConfigError> { - if let Ok(config_path) = env::var("JJ_CONFIG") { - // TODO: We should probably support colon-separated (std::env::split_paths) - // paths here - Ok(Some(PathBuf::from(config_path))) - } else { - // TODO: Should we drop the final `/config.toml` and read all files in the - // directory? - let platform_specific_config_path = dirs::config_dir() - .map(|config_dir| config_dir.join("jj").join("config.toml")) - .filter(|path| path.exists()); - let home_config_path = dirs::home_dir() - .map(|home_dir| home_dir.join(".jjconfig.toml")) - .filter(|path| path.exists()); - match (&platform_specific_config_path, &home_config_path) { - (Some(xdg_config_path), Some(home_config_path)) => Err(ConfigError::AmbiguousSource( - xdg_config_path.clone(), - home_config_path.clone(), - )), - _ => Ok(platform_specific_config_path.or(home_config_path)), +enum ConfigPath { + /// Existing config file path. + Existing(PathBuf), + /// Could not find any config file, but a new file can be created at the + /// specified location. + New(PathBuf), + /// Could not find any config file. + Unavailable, +} + +impl ConfigPath { + fn new(path: Option) -> Self { + match path { + Some(path) if path.exists() => ConfigPath::Existing(path), + Some(path) => ConfigPath::New(path), + None => ConfigPath::Unavailable, } } } +/// Like std::fs::create_dir_all but creates new directories to be accessible to +/// the user only on Unix (chmod 700). +fn create_dir_all(path: &Path) -> std::io::Result<()> { + let mut dir = std::fs::DirBuilder::new(); + dir.recursive(true); + #[cfg(unix)] + { + use std::os::unix::fs::DirBuilderExt; + dir.mode(0o700); + } + 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 ConfigEnv { + config_dir: Option, + home_dir: Option, + jj_config: Option, +} + +impl ConfigEnv { + fn new() -> Self { + ConfigEnv { + config_dir: dirs::config_dir(), + home_dir: dirs::home_dir(), + jj_config: env::var("JJ_CONFIG").ok(), + } + } + + fn config_path(self) -> Result { + if let Some(path) = self.jj_config { + // TODO: We should probably support colon-separated (std::env::split_paths) + return Ok(ConfigPath::new(Some(PathBuf::from(path)))); + } + // TODO: Should we drop the final `/config.toml` and read all files in the + // directory? + let platform_config_path = ConfigPath::new(self.config_dir.map(|mut config_dir| { + config_dir.push("jj"); + config_dir.push("config.toml"); + config_dir + })); + let home_config_path = ConfigPath::new(self.home_dir.map(|mut home_dir| { + home_dir.push(".jjconfig.toml"); + home_dir + })); + use ConfigPath::*; + match (platform_config_path, home_config_path) { + (Existing(platform_config_path), Existing(home_config_path)) => Err( + ConfigError::AmbiguousSource(platform_config_path, home_config_path), + ), + (Existing(path), _) | (_, Existing(path)) => Ok(Existing(path)), + (New(path), _) | (_, New(path)) => Ok(New(path)), + (Unavailable, Unavailable) => Ok(Unavailable), + } + } + + fn existing_config_path(self) -> Result, ConfigError> { + match self.config_path()? { + ConfigPath::Existing(path) => Ok(Some(path)), + _ => Ok(None), + } + } + + fn new_config_path(self) -> Result, ConfigError> { + match self.config_path()? { + ConfigPath::Existing(path) => Ok(Some(path)), + ConfigPath::New(path) => { + create_config_file(&path)?; + Ok(Some(path)) + } + ConfigPath::Unavailable => Ok(None), + } + } +} + +pub fn existing_config_path() -> Result, ConfigError> { + ConfigEnv::new().existing_config_path() +} + +/// 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_config_path() -> Result, ConfigError> { + ConfigEnv::new().new_config_path() +} + /// Environment variables that should be overridden by config values fn env_base() -> config::Config { let mut builder = config::Config::builder(); @@ -632,4 +730,228 @@ mod tests { "### ); } + + #[test] + fn test_config_path_home_dir_existing() -> anyhow::Result<()> { + TestCase { + files: vec!["home/.jjconfig.toml"], + cfg: ConfigEnv { + home_dir: Some("home".into()), + ..Default::default() + }, + want: Want::ExistingAndNew("home/.jjconfig.toml"), + } + .run() + } + + #[test] + fn test_config_path_home_dir_new() -> anyhow::Result<()> { + TestCase { + files: vec![], + cfg: ConfigEnv { + home_dir: Some("home".into()), + ..Default::default() + }, + want: Want::New("home/.jjconfig.toml"), + } + .run() + } + + #[test] + fn test_config_path_config_dir_existing() -> anyhow::Result<()> { + TestCase { + files: vec!["config/jj/config.toml"], + cfg: ConfigEnv { + config_dir: Some("config".into()), + ..Default::default() + }, + want: Want::ExistingAndNew("config/jj/config.toml"), + } + .run() + } + + #[test] + fn test_config_path_config_dir_new() -> anyhow::Result<()> { + TestCase { + files: vec![], + cfg: ConfigEnv { + config_dir: Some("config".into()), + ..Default::default() + }, + want: Want::New("config/jj/config.toml"), + } + .run() + } + + #[test] + fn test_config_path_new_prefer_config_dir() -> anyhow::Result<()> { + TestCase { + files: vec![], + cfg: ConfigEnv { + config_dir: Some("config".into()), + home_dir: Some("home".into()), + ..Default::default() + }, + want: Want::New("config/jj/config.toml"), + } + .run() + } + + #[test] + fn test_config_path_jj_config_existing() -> anyhow::Result<()> { + TestCase { + files: vec!["custom.toml"], + cfg: ConfigEnv { + jj_config: Some("custom.toml".into()), + ..Default::default() + }, + want: Want::ExistingAndNew("custom.toml"), + } + .run() + } + + #[test] + fn test_config_path_jj_config_new() -> anyhow::Result<()> { + TestCase { + files: vec![], + cfg: ConfigEnv { + jj_config: Some("custom.toml".into()), + ..Default::default() + }, + want: Want::New("custom.toml"), + } + .run() + } + + #[test] + fn test_config_path_config_pick_config_dir() -> anyhow::Result<()> { + TestCase { + files: vec!["config/jj/config.toml"], + cfg: ConfigEnv { + home_dir: Some("home".into()), + config_dir: Some("config".into()), + ..Default::default() + }, + want: Want::ExistingAndNew("config/jj/config.toml"), + } + .run() + } + + #[test] + fn test_config_path_config_pick_home_dir() -> anyhow::Result<()> { + TestCase { + files: vec!["home/.jjconfig.toml"], + cfg: ConfigEnv { + home_dir: Some("home".into()), + config_dir: Some("config".into()), + ..Default::default() + }, + want: Want::ExistingAndNew("home/.jjconfig.toml"), + } + .run() + } + + #[test] + fn test_config_path_none() -> anyhow::Result<()> { + TestCase { + files: vec![], + cfg: Default::default(), + want: Want::None, + } + .run() + } + + #[test] + fn test_config_path_ambiguous() -> anyhow::Result<()> { + let tmp = setup_config_fs(&vec!["home/.jjconfig.toml", "config/jj/config.toml"])?; + let cfg = ConfigEnv { + home_dir: Some(tmp.path().join("home")), + config_dir: Some(tmp.path().join("config")), + ..Default::default() + }; + use assert_matches::assert_matches; + assert_matches!( + cfg.clone().existing_config_path(), + Err(ConfigError::AmbiguousSource(_, _)) + ); + assert_matches!( + cfg.clone().new_config_path(), + Err(ConfigError::AmbiguousSource(_, _)) + ); + Ok(()) + } + + fn setup_config_fs(files: &Vec<&'static str>) -> anyhow::Result { + let tmp = testutils::new_temp_dir(); + for file in files { + let path = tmp.path().join(file); + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent)?; + } + std::fs::File::create(path)?; + } + Ok(tmp) + } + + enum Want { + None, + New(&'static str), + ExistingAndNew(&'static str), + } + + struct TestCase { + files: Vec<&'static str>, + cfg: ConfigEnv, + want: Want, + } + + impl TestCase { + fn config(&self, root: &Path) -> ConfigEnv { + ConfigEnv { + config_dir: self.cfg.config_dir.as_ref().map(|p| root.join(p)), + home_dir: self.cfg.home_dir.as_ref().map(|p| root.join(p)), + jj_config: self + .cfg + .jj_config + .as_ref() + .map(|p| root.join(p).to_str().unwrap().to_string()), + } + } + + fn run(self) -> anyhow::Result<()> { + use anyhow::anyhow; + let tmp = setup_config_fs(&self.files)?; + + let check = |name, f: fn(ConfigEnv) -> Result<_, _>, want: Option<_>| { + let got = f(self.config(tmp.path())).map_err(|e| anyhow!("{name}: {e}"))?; + let want = want.map(|p| tmp.path().join(p)); + if got != want { + Err(anyhow!("{name}: got {got:?}, want {want:?}")) + } else { + Ok(got) + } + }; + + let (want_existing, want_new) = match self.want { + Want::None => (None, None), + Want::New(want) => (None, Some(want)), + Want::ExistingAndNew(want) => (Some(want.clone()), Some(want)), + }; + + check( + "existing_config_path", + ConfigEnv::existing_config_path, + want_existing, + )?; + let got = check("new_config_path", ConfigEnv::new_config_path, want_new)?; + 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(()) + } + } }