Add a config edit command to open jj config in editor

Part of #531 to define the overall `config` command.

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
+* `jj config list` command prints values from config and `config edit` opens
-  coming soon).
+  the config in an editor.
 
 * `jj debug config-schema` command prints out JSON schema for the jj TOML config
   file format.

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)

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,
+    resolve_base_revs, run_ui_editor, short_change_hash, short_commit_description,
-    user_error_with_hint, write_commit_summary, write_config_entry, Args, CommandError,
+    short_commit_hash, user_error, user_error_with_hint, write_commit_summary, write_config_entry,
-    CommandHelper, DescriptionArg, RevisionArg, WorkspaceCommandHelper,
+    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<String>,
 }
 
-/// 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
+/// Note: Currently only supports getting config options and editing config
-/// setting options and editing config files is also planned (see
+/// 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<String>,
-        // 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
+                Some(name) => {
-                    .settings()
+                    settings
-                    .config()
+                        .config()
-                    .get::<config::Value>(name)
+                        .get::<config::Value>(name)
-                    .map_err(|e| match e {
+                        .map_err(|e| match e {
-                        config::ConfigError::NotFound { .. } => {
+                            config::ConfigError::NotFound { .. } => {
-                            user_error("key not found in config")
+                                user_error("key not found in config")
-                        }
+                            }
-                        _ => e.into(),
+                            _ => e.into(),
-                    })?,
+                        })?
-                None => command.settings().config().collect()?.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
+    run_ui_editor(settings, &description_file_path)?;
-        .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"
-        )));
-    }
 
     let description = fs::read_to_string(&description_file_path).map_err(|e| {
         user_error(format!(

src/config.rs

@@ -104,7 +104,7 @@ impl LayeredConfigs {
     }
 }
 
-fn config_path() -> Result<Option<PathBuf>, ConfigError> {
+pub fn config_path() -> Result<Option<PathBuf>, ConfigError> {
     if let Ok(config_path) = env::var("JJ_CONFIG") {
         // TODO: We should probably support colon-separated (std::env::split_paths)
         // paths here

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}");

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.

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