From e6b3b9c09be2fb89c65941013f8b511ae31fd372 Mon Sep 17 00:00:00 2001 From: David Barnett Date: Wed, 4 Jan 2023 22:55:20 -0600 Subject: [PATCH] Add a `config edit` command to open jj config in editor Part of #531 to define the overall `config` command. --- CHANGELOG.md | 4 +- src/cli_util.rs | 21 +++++++- src/commands.rs | 95 +++++++++++++++++++++++------------- src/config.rs | 2 +- testing/fake-editor.rs | 12 ++++- tests/common/mod.rs | 4 ++ tests/test_config_command.rs | 47 ++++++++++++++++++ 7 files changed, 145 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a5b386a8..fb2007424 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,8 +27,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * `jj git push` now accepts multiple `--branch`/`--change` arguments -* `jj config list` command prints values from config (with other subcommands - coming soon). +* `jj config list` command prints values from config and `config edit` opens + the config in an editor. * `jj debug config-schema` command prints out JSON schema for the jj TOML config file format. diff --git a/src/cli_util.rs b/src/cli_util.rs index 2ada03862..c532b2507 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -52,7 +52,7 @@ use jujutsu_lib::{dag_walk, file_util, git, revset}; use thiserror::Error; use tracing_subscriber::prelude::*; -use crate::config::LayeredConfigs; +use crate::config::{FullCommandArgs, LayeredConfigs}; use crate::formatter::Formatter; use crate::merge_tools::{ConflictResolveError, DiffEditError}; use crate::templater::TemplateFormatter; @@ -1376,6 +1376,25 @@ fn serialize_config_value(value: config::Value) -> String { } } +pub fn run_ui_editor(settings: &UserSettings, edit_path: &PathBuf) -> Result<(), CommandError> { + let editor: FullCommandArgs = settings + .config() + .get("ui.editor") + .unwrap_or_else(|_| "pico".into()); + let exit_status = editor + .to_command() + .arg(edit_path) + .status() + .map_err(|_| user_error(format!("Failed to run editor '{editor}'")))?; + if !exit_status.success() { + return Err(user_error(format!( + "Editor '{editor}' exited with an error" + ))); + } + + Ok(()) +} + pub fn short_commit_description(commit: &Commit) -> String { let first_line = commit.description().split('\n').next().unwrap(); format!("{} ({})", short_commit_hash(commit.id()), first_line) diff --git a/src/commands.rs b/src/commands.rs index 4917c7b70..a1c55635c 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -44,18 +44,18 @@ use jujutsu_lib::settings::UserSettings; use jujutsu_lib::store::Store; use jujutsu_lib::tree::{merge_trees, Tree}; use jujutsu_lib::view::View; -use jujutsu_lib::workspace::Workspace; +use jujutsu_lib::workspace::{Workspace, WorkspaceLoader}; use jujutsu_lib::{conflicts, file_util, git, revset}; use maplit::{hashmap, hashset}; use pest::Parser; use crate::cli_util::{ self, check_stale_working_copy, print_checkout_stats, print_failed_git_export, - resolve_base_revs, short_change_hash, short_commit_description, short_commit_hash, user_error, - user_error_with_hint, write_commit_summary, write_config_entry, Args, CommandError, - CommandHelper, DescriptionArg, RevisionArg, WorkspaceCommandHelper, + resolve_base_revs, run_ui_editor, short_change_hash, short_commit_description, + short_commit_hash, user_error, user_error_with_hint, write_commit_summary, write_config_entry, + Args, CommandError, CommandHelper, DescriptionArg, RevisionArg, WorkspaceCommandHelper, }; -use crate::config::FullCommandArgs; +use crate::config::config_path; use crate::diff_util::{self, DiffFormat, DiffFormatArgs}; use crate::formatter::{Formatter, PlainTextFormatter}; use crate::graphlog::{AsciiGraphDrawer, Edge}; @@ -145,18 +145,31 @@ struct InitArgs { git_repo: Option, } -/// Get config options +#[derive(clap::Args, Clone, Debug)] +#[command(group = clap::ArgGroup::new("config_level").multiple(false).required(true))] +struct ConfigArgs { + /// Target the user-level config + #[arg(long, group = "config_level")] + user: bool, + + /// Target the repo-level config + #[arg(long, group = "config_level")] + repo: bool, +} + +/// Manage config options /// /// Operates on jj configuration, which comes from the config file and /// environment variables. Uses the config file at ~/.jjconfig.toml or /// $XDG_CONFIG_HOME/jj/config.toml, unless overridden with the JJ_CONFIG -/// environment variable. +/// environment variable, combined with repo config at ~/.jj/repo/config.toml +/// if present. /// /// For supported config options and more details about jj config, see /// https://github.com/martinvonz/jj/blob/main/docs/config.md. /// -/// Note: Currently only supports getting config options, but support for -/// setting options and editing config files is also planned (see +/// Note: Currently only supports getting config options and editing config +/// files, but support for setting options is also planned (see /// https://github.com/martinvonz/jj/issues/531). #[derive(clap::Subcommand, Clone, Debug)] enum ConfigSubcommand { @@ -166,7 +179,13 @@ enum ConfigSubcommand { /// An optional name of a specific config option to look up. #[arg(value_parser=NonEmptyStringValueParser::new())] name: Option, - // TODO: Support --show-origin once mehcode/config-rs#319 is done. + // TODO(#531): Support --show-origin using LayeredConfigs. + // TODO(#531): Support ConfigArgs (--user or --repo) and --all. + }, + #[command(visible_alias("e"))] + Edit { + #[clap(flatten)] + config_args: ConfigArgs, }, } @@ -1215,23 +1234,42 @@ fn cmd_config( subcommand: &ConfigSubcommand, ) -> Result<(), CommandError> { ui.request_pager(); + let settings = command.settings(); match subcommand { ConfigSubcommand::List { name } => { let raw_values = match name { - Some(name) => command - .settings() - .config() - .get::(name) - .map_err(|e| match e { - config::ConfigError::NotFound { .. } => { - user_error("key not found in config") - } - _ => e.into(), - })?, - None => command.settings().config().collect()?.into(), + Some(name) => { + settings + .config() + .get::(name) + .map_err(|e| match e { + config::ConfigError::NotFound { .. } => { + user_error("key not found in config") + } + _ => e.into(), + })? + } + None => settings.config().collect()?.into(), }; write_config_entry(ui, name.as_deref().unwrap_or(""), raw_values)?; } + ConfigSubcommand::Edit { config_args } => { + let edit_path = if config_args.user { + // TODO(#531): Special-case for editors that can't handle viewing directories? + config_path()?.ok_or_else(|| user_error("No repo config path found to edit"))? + } else if config_args.repo { + let workspace_command = command.workspace_helper(ui)?; + let workspace_path = workspace_command.workspace_root(); + WorkspaceLoader::init(workspace_path) + .unwrap() + .repo_path() + .join("config.toml") + } else { + // Shouldn't be reachable unless clap ArgGroup is broken. + panic!("No config_level provided"); + }; + run_ui_editor(settings, &edit_path)?; + } } Ok(()) @@ -1898,20 +1936,7 @@ fn edit_description( )) })?; - let editor: FullCommandArgs = settings - .config() - .get("ui.editor") - .unwrap_or_else(|_| "pico".into()); - let exit_status = editor - .to_command() - .arg(&description_file_path) - .status() - .map_err(|_| user_error(format!("Failed to run editor '{editor}'")))?; - if !exit_status.success() { - return Err(user_error(format!( - "Editor '{editor}' exited with an error" - ))); - } + run_ui_editor(settings, &description_file_path)?; let description = fs::read_to_string(&description_file_path).map_err(|e| { user_error(format!( diff --git a/src/config.rs b/src/config.rs index ef05607de..5bb7418ec 100644 --- a/src/config.rs +++ b/src/config.rs @@ -104,7 +104,7 @@ impl LayeredConfigs { } } -fn config_path() -> Result, ConfigError> { +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 diff --git a/testing/fake-editor.rs b/testing/fake-editor.rs index 30e644ea5..caa3f473d 100644 --- a/testing/fake-editor.rs +++ b/testing/fake-editor.rs @@ -58,8 +58,18 @@ fn main() { exit(1) } } + ["expectpath"] => { + let actual = args.file.to_str().unwrap(); + if actual != payload { + eprintln!("fake-editor: Unexpected path.\n"); + eprintln!("EXPECTED: <{payload}>\nRECEIVED: <{actual}>"); + exit(1) + } + } ["write"] => { - fs::write(&args.file, payload).unwrap(); + fs::write(&args.file, payload).unwrap_or_else(|_| { + panic!("Failed to write file {}", args.file.to_str().unwrap()) + }); } _ => { eprintln!("fake-editor: unexpected command: {command}"); diff --git a/tests/common/mod.rs b/tests/common/mod.rs index aab190fd9..d2838884b 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -111,6 +111,10 @@ impl TestEnvironment { &self.home_dir } + pub fn config_dir(&self) -> &PathBuf { + &self.config_dir + } + pub fn add_config(&self, content: &[u8]) { // Concatenating two valid TOML files does not (generally) result in a valid // TOML file, so we use create a new file every time instead. diff --git a/tests/test_config_command.rs b/tests/test_config_command.rs index 517457b4f..59dc6794d 100644 --- a/tests/test_config_command.rs +++ b/tests/test_config_command.rs @@ -251,6 +251,53 @@ fn test_config_layer_workspace() { "###); } +#[test] +fn test_config_edit_missing_opt() { + let test_env = TestEnvironment::default(); + let stderr = test_env.jj_cmd_cli_error(test_env.env_root(), &["config", "edit"]); + insta::assert_snapshot!(stderr, @r###" + error: The following required arguments were not provided: + <--user|--repo> + + Usage: jj config edit <--user|--repo> + + For more information try '--help' + "###); +} + +#[test] +fn test_config_edit_user() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + let edit_script = test_env.set_up_fake_editor(); + + std::fs::write( + edit_script, + format!("expectpath\n{}", test_env.config_dir().to_str().unwrap()), + ) + .unwrap(); + test_env.jj_cmd_success(&repo_path, &["config", "edit", "--user"]); +} + +#[test] +fn test_config_edit_repo() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + let edit_script = test_env.set_up_fake_editor(); + + std::fs::write( + edit_script, + format!( + "expectpath\n{}", + repo_path.join(".jj/repo/config.toml").to_str().unwrap() + ), + ) + .unwrap(); + test_env.jj_cmd_success(&repo_path, &["config", "edit", "--repo"]); +} + fn find_stdout_lines(keyname_pattern: &str, stdout: &str) -> String { let key_line_re = Regex::new(&format!(r"(?m)^{keyname_pattern}=.*$")).unwrap(); key_line_re