From 99cb0ba7c5cef0d8b53dbfe64a168764e50a4dc2 Mon Sep 17 00:00:00 2001 From: David Barnett Date: Sat, 14 Jan 2023 22:23:44 -0600 Subject: [PATCH] Implement "config set" subcommand Uses toml_edit to support simple config edits like: jj config set --repo user.email "somebody@example.com" --- CHANGELOG.md | 3 + Cargo.lock | 41 ++++++++++++ Cargo.toml | 1 + src/cli_util.rs | 94 ++++++++++++++++++++++++++- src/commands/mod.rs | 78 ++++++++++++++++------- tests/common/mod.rs | 19 ++++-- tests/test_config_command.rs | 119 ++++++++++++++++++++++++++++++++++- 7 files changed, 326 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bf80b4f640..4f59ba4281 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * `jj git fetch` now supports a `--branch` argument to fetch some of the branches only. +* `jj config set` command allows simple config edits like + `jj config set --repo user.email "somebody@example.com"` + ### Fixed bugs * Modify/delete conflicts now include context lines diff --git a/Cargo.lock b/Cargo.lock index 1ac201bac9..a5bd84f484 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -829,6 +829,7 @@ dependencies = [ "textwrap 0.16.0", "thiserror", "timeago", + "toml_edit", "tracing", "tracing-subscriber", ] @@ -1023,6 +1024,15 @@ dependencies = [ "minimal-lexical", ] +[[package]] +name = "nom8" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ae01545c9c7fc4486ab7debaf2aad7003ac19431791868fb2e8066df97fad2f8" +dependencies = [ + "memchr", +] + [[package]] name = "normalize-line-endings" version = "0.3.0" @@ -1594,6 +1604,15 @@ dependencies = [ "serde", ] +[[package]] +name = "serde_spanned" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0efd8caf556a6cebd3b285caf480045fcc1ac04f6bd786b09a6f11af30c4fcf4" +dependencies = [ + "serde", +] + [[package]] name = "sha2" version = "0.10.6" @@ -1866,6 +1885,28 @@ dependencies = [ "serde", ] +[[package]] +name = "toml_datetime" +version = "0.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3ab8ed2edee10b50132aed5f331333428b011c99402b5a534154ed15746f9622" +dependencies = [ + "serde", +] + +[[package]] +name = "toml_edit" +version = "0.19.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "90a238ee2e6ede22fb95350acc78e21dc40da00bb66c0334bde83de4ed89424e" +dependencies = [ + "indexmap", + "nom8", + "serde", + "serde_spanned", + "toml_datetime", +] + [[package]] name = "tracing" version = "0.1.37" diff --git a/Cargo.toml b/Cargo.toml index 221ea60af2..48f942ab2e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -62,6 +62,7 @@ thiserror = "1.0.38" tracing = "0.1.37" tracing-subscriber = { version = "0.3.16", default-features = false, features = ["std", "ansi", "env-filter", "fmt"] } indexmap = "1.9.2" +toml_edit = { version = "0.19.1", features = ["serde"] } [target.'cfg(unix)'.dependencies] libc = { version = "0.2.139" } diff --git a/src/cli_util.rs b/src/cli_util.rs index 3ee6c50b1b..3eb5aea7e1 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -21,6 +21,7 @@ use std::ops::Deref; use std::path::{Path, PathBuf}; use std::process::ExitCode; use std::rc::Rc; +use std::str::FromStr; use std::sync::Arc; use clap::builder::{NonEmptyStringValueParser, TypedValueParser, ValueParserFactory}; @@ -55,10 +56,13 @@ use jujutsu_lib::working_copy::{ use jujutsu_lib::workspace::{Workspace, WorkspaceInitError, WorkspaceLoadError, WorkspaceLoader}; use jujutsu_lib::{dag_walk, file_util, git, revset}; use thiserror::Error; +use toml_edit; use tracing_subscriber::prelude::*; use crate::commit_templater; -use crate::config::{AnnotatedValue, CommandNameAndArgs, LayeredConfigs}; +use crate::config::{ + config_path, AnnotatedValue, CommandNameAndArgs, ConfigSource, LayeredConfigs, +}; use crate::formatter::{Formatter, PlainTextFormatter}; use crate::merge_tools::{ConflictResolveError, DiffEditError}; use crate::template_parser::{TemplateAliasesMap, TemplateParseError}; @@ -1633,6 +1637,94 @@ pub fn serialize_config_value(value: &config::Value) -> String { } } +pub fn write_config_value_to_file( + key: &str, + value_str: &str, + path: &Path, +) -> Result<(), CommandError> { + // Read config + let config_toml = std::fs::read_to_string(path).or_else(|err| { + match err.kind() { + // If config doesn't exist yet, read as empty and we'll write one. + std::io::ErrorKind::NotFound => Ok("".to_string()), + _ => Err(user_error(format!( + "Failed to read file {path}: {err:?}", + path = path.display() + ))), + } + })?; + let mut doc = toml_edit::Document::from_str(&config_toml).map_err(|err| { + user_error(format!( + "Failed to parse file {path}: {err:?}", + path = path.display() + )) + })?; + + // Apply config value + // Iterpret value as string unless it's another simple scalar type. + // TODO(#531): Infer types based on schema (w/ --type arg to override). + let item = match toml_edit::Value::from_str(value_str) { + Ok(value @ toml_edit::Value::Boolean(..)) + | Ok(value @ toml_edit::Value::Integer(..)) + | Ok(value @ toml_edit::Value::Float(..)) + | Ok(value @ toml_edit::Value::String(..)) => toml_edit::value(value), + _ => toml_edit::value(value_str), + }; + let mut target_table = doc.as_table_mut(); + let mut key_parts_iter = key.split('.'); + // Note: split guarantees at least one item. + let last_key_part = key_parts_iter.next_back().unwrap(); + for key_part in key_parts_iter { + target_table = target_table + .entry(key_part) + .or_insert_with(|| toml_edit::Item::Table(toml_edit::Table::new())) + .as_table_mut() + .ok_or_else(|| { + user_error(format!( + "Failed to set {key}: would overwrite non-table value with parent table" + )) + })?; + } + // Error out if overwriting non-scalar value for key (table or array). + match target_table.get(last_key_part) { + None | Some(toml_edit::Item::None) => {} + Some(toml_edit::Item::Value(val)) if !val.is_array() && !val.is_inline_table() => {} + _ => { + return Err(user_error(format!( + "Failed to set {key}: would overwrite entire non-scalar value with scalar" + ))) + } + } + target_table[last_key_part] = item; + + // Write config back + std::fs::write(path, doc.to_string()).map_err(|err| { + user_error(format!( + "Failed to write file {path}: {err:?}", + path = path.display() + )) + }) +} + +pub fn get_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"))? + } + ConfigSource::Repo => workspace_loader.repo_path().join("config.toml"), + _ => { + return Err(user_error(format!( + "Can't get path for config source {config_source:?}" + ))); + } + }; + Ok(edit_path) +} + pub fn run_ui_editor(settings: &UserSettings, edit_path: &PathBuf) -> Result<(), CommandError> { let editor: CommandNameAndArgs = settings .config() diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 9661a2bb3c..e837aad262 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -49,12 +49,13 @@ use jujutsu_lib::{conflicts, file_util, revset}; use maplit::{hashmap, hashset}; use crate::cli_util::{ - self, check_stale_working_copy, print_checkout_stats, resolve_multiple_nonempty_revsets, - resolve_mutliple_nonempty_revsets_flag_guarded, run_ui_editor, serialize_config_value, - short_commit_hash, user_error, user_error_with_hint, Args, CommandError, CommandHelper, - DescriptionArg, RevisionArg, WorkspaceCommandHelper, + self, check_stale_working_copy, get_config_file_path, print_checkout_stats, + resolve_multiple_nonempty_revsets, resolve_mutliple_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, RevisionArg, + WorkspaceCommandHelper, }; -use crate::config::{config_path, AnnotatedValue, ConfigSource}; +use crate::config::{AnnotatedValue, ConfigSource}; use crate::diff_util::{self, DiffFormat, DiffFormatArgs}; use crate::formatter::{Formatter, PlainTextFormatter}; use crate::graphlog::{get_graphlog, Edge}; @@ -153,6 +154,19 @@ struct ConfigArgs { repo: bool, } +impl ConfigArgs { + fn get_source_kind(&self) -> ConfigSource { + if self.user { + ConfigSource::User + } else if self.repo { + ConfigSource::Repo + } else { + // Shouldn't be reachable unless clap ArgGroup is broken. + panic!("No config_level provided"); + } + } +} + /// Manage config options /// /// Operates on jj configuration, which comes from the config file and @@ -163,14 +177,12 @@ struct ConfigArgs { /// /// 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 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 { #[command(visible_alias("l"))] List(ConfigListArgs), + #[command(visible_alias("s"))] + Set(ConfigSetArgs), #[command(visible_alias("e"))] Edit(ConfigEditArgs), } @@ -188,6 +200,18 @@ struct ConfigListArgs { // TODO(#1047): Support ConfigArgs (--user or --repo). } +/// Update config file to set the given option to a given value. +#[derive(clap::Args, Clone, Debug)] +struct ConfigSetArgs { + #[arg(required = true)] + name: String, + #[arg(required = true)] + value: String, + #[clap(flatten)] + config_args: ConfigArgs, +} + +/// Start an editor on a jj config file. #[derive(clap::Args, Clone, Debug)] struct ConfigEditArgs { #[clap(flatten)] @@ -1064,6 +1088,7 @@ fn cmd_config( ) -> Result<(), CommandError> { match subcommand { ConfigSubcommand::List(sub_args) => cmd_config_list(ui, command, sub_args), + ConfigSubcommand::Set(sub_args) => cmd_config_set(ui, command, sub_args), ConfigSubcommand::Edit(sub_args) => cmd_config_edit(ui, command, sub_args), } } @@ -1110,23 +1135,34 @@ fn cmd_config_list( Ok(()) } +fn cmd_config_set( + _ui: &mut Ui, + command: &CommandHelper, + args: &ConfigSetArgs, +) -> Result<(), CommandError> { + let config_path = get_config_file_path( + &args.config_args.get_source_kind(), + command.workspace_loader()?, + )?; + if config_path.is_dir() { + return Err(user_error(format!( + "Can't set config in path {path} (dirs not supported)", + path = config_path.display() + ))); + } + write_config_value_to_file(&args.name, &args.value, &config_path) +} + fn cmd_config_edit( _ui: &mut Ui, command: &CommandHelper, args: &ConfigEditArgs, ) -> Result<(), CommandError> { - let edit_path = if args.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 args.config_args.repo { - let workspace_loader = command.workspace_loader()?; - workspace_loader.repo_path().join("config.toml") - } else { - // Shouldn't be reachable unless clap ArgGroup is broken. - panic!("No config_level provided"); - }; - run_ui_editor(command.settings(), &edit_path)?; - Ok(()) + let config_path = get_config_file_path( + &args.config_args.get_source_kind(), + command.workspace_loader()?, + )?; + run_ui_editor(command.settings(), &config_path) } fn cmd_checkout( diff --git a/tests/common/mod.rs b/tests/common/mod.rs index ff129bc54a..715c7f0350 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -24,7 +24,7 @@ pub struct TestEnvironment { _temp_dir: TempDir, env_root: PathBuf, home_dir: PathBuf, - config_dir: PathBuf, + config_path: PathBuf, env_vars: HashMap, config_file_number: RefCell, command_number: RefCell, @@ -45,7 +45,7 @@ impl Default for TestEnvironment { _temp_dir: tmp_dir, env_root, home_dir, - config_dir, + config_path: config_dir, env_vars, config_file_number: RefCell::new(0), command_number: RefCell::new(0), @@ -64,7 +64,7 @@ impl TestEnvironment { } cmd.env("RUST_BACKTRACE", "1"); cmd.env("HOME", self.home_dir.to_str().unwrap()); - cmd.env("JJ_CONFIG", self.config_dir.to_str().unwrap()); + cmd.env("JJ_CONFIG", self.config_path.to_str().unwrap()); cmd.env("JJ_USER", "Test User"); cmd.env("JJ_EMAIL", "test.user@example.com"); cmd.env("JJ_OP_HOSTNAME", "host.example.com"); @@ -119,18 +119,25 @@ impl TestEnvironment { &self.home_dir } - pub fn config_dir(&self) -> &PathBuf { - &self.config_dir + pub fn config_path(&self) -> &PathBuf { + &self.config_path + } + + pub fn set_config_path(&mut self, config_path: PathBuf) { + self.config_path = config_path; } pub fn add_config(&self, content: &str) { + if self.config_path.is_file() { + panic!("add_config not supported when config_path is a file"); + } // Concatenating two valid TOML files does not (generally) result in a valid // TOML file, so we use create a new file every time instead. let mut config_file_number = self.config_file_number.borrow_mut(); *config_file_number += 1; let config_file_number = *config_file_number; std::fs::write( - self.config_dir + self.config_path .join(format!("config{config_file_number:04}.toml")), content, ) diff --git a/tests/test_config_command.rs b/tests/test_config_command.rs index 53db52ab92..9499a52544 100644 --- a/tests/test_config_command.rs +++ b/tests/test_config_command.rs @@ -265,6 +265,123 @@ fn test_config_layer_workspace() { "###); } +#[test] +fn test_config_set_missing_opts() { + let test_env = TestEnvironment::default(); + let stderr = test_env.jj_cmd_cli_error(test_env.env_root(), &["config", "set"]); + insta::assert_snapshot!(stderr, @r###" + error: The following required arguments were not provided: + <--user|--repo> + + + + Usage: jj config set <--user|--repo> + + For more information try '--help' + "###); +} + +#[test] +fn test_config_set_for_user() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]); + // Point to a config file since `config set` can't handle directories. + let user_config_path = test_env.config_path().join("config.toml"); + test_env.set_config_path(user_config_path.to_owned()); + let repo_path = test_env.env_root().join("repo"); + + test_env.jj_cmd_success( + &repo_path, + &["config", "set", "--user", "test-key", "test-val"], + ); + test_env.jj_cmd_success( + &repo_path, + &["config", "set", "--user", "test-table.foo", "true"], + ); + + // Ensure test-key successfully written to user config. + let user_config_toml = std::fs::read_to_string(&user_config_path) + .unwrap_or_else(|_| panic!("Failed to read file {}", user_config_path.display())); + insta::assert_snapshot!(user_config_toml, @r###" + test-key = "test-val" + + [test-table] + foo = true + "###); +} + +#[test] +fn test_config_set_for_repo() { + let 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"); + test_env.jj_cmd_success( + &repo_path, + &["config", "set", "--repo", "test-key", "test-val"], + ); + test_env.jj_cmd_success( + &repo_path, + &["config", "set", "--repo", "test-table.foo", "true"], + ); + // Ensure test-key successfully written to user config. + let expected_repo_config_path = repo_path.join(".jj/repo/config.toml"); + let repo_config_toml = + std::fs::read_to_string(&expected_repo_config_path).unwrap_or_else(|_| { + panic!( + "Failed to read file {}", + expected_repo_config_path.display() + ) + }); + insta::assert_snapshot!(repo_config_toml, @r###" + test-key = "test-val" + + [test-table] + foo = true + "###); +} + +#[test] +fn test_config_set_type_mismatch() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]); + let user_config_path = test_env.config_path().join("config.toml"); + test_env.set_config_path(user_config_path); + let repo_path = test_env.env_root().join("repo"); + + test_env.jj_cmd_success( + &repo_path, + &["config", "set", "--user", "test-table.foo", "test-val"], + ); + let stderr = test_env.jj_cmd_failure( + &repo_path, + &["config", "set", "--user", "test-table", "not-a-table"], + ); + insta::assert_snapshot!(stderr, @r###" + Error: Failed to set test-table: would overwrite entire non-scalar value with scalar + "###); +} + +#[test] +fn test_config_set_nontable_parent() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]); + let user_config_path = test_env.config_path().join("config.toml"); + test_env.set_config_path(user_config_path); + let repo_path = test_env.env_root().join("repo"); + + test_env.jj_cmd_success( + &repo_path, + &["config", "set", "--user", "test-nontable", "test-val"], + ); + let stderr = test_env.jj_cmd_failure( + &repo_path, + &["config", "set", "--user", "test-nontable.foo", "test-val"], + ); + insta::assert_snapshot!(stderr, @r###" + Error: Failed to set test-nontable.foo: would overwrite non-table value with parent table + "###); +} + #[test] fn test_config_edit_missing_opt() { let test_env = TestEnvironment::default(); @@ -288,7 +405,7 @@ fn test_config_edit_user() { std::fs::write( edit_script, - format!("expectpath\n{}", test_env.config_dir().to_str().unwrap()), + format!("expectpath\n{}", test_env.config_path().to_str().unwrap()), ) .unwrap(); test_env.jj_cmd_success(&repo_path, &["config", "edit", "--user"]);