From 409356fa5bb0042ee1cc0ac09289ea88808aec4e Mon Sep 17 00:00:00 2001 From: Waleed Khan Date: Tue, 29 Aug 2023 23:13:36 +0200 Subject: [PATCH] merge_tools: enable `:builtin` as default diff/merge editor --- CHANGELOG.md | 3 ++ cli/src/diff_util.rs | 20 ++++++--- cli/src/merge_tools/mod.rs | 85 +++++++++++--------------------------- docs/config.md | 22 ++-------- 4 files changed, 44 insertions(+), 86 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d2b41c3e9..a9168107d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `jj log -T 'description ++ "\0"' --no-graph` to output descriptions only, but be able to tell where the boundaries are +* jj now bundles a TUI tool to use as the default diff and merge editors. (The + previous default was `meld`.) + ### Fixed bugs ## [0.9.0] - 2023-09-06 diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index 4dacd53441..f3b8a05fd2 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -35,7 +35,7 @@ use unicode_width::UnicodeWidthStr as _; use crate::cli_util::{CommandError, WorkspaceCommandHelper}; use crate::formatter::Formatter; -use crate::merge_tools::{self, ExternalMergeTool}; +use crate::merge_tools::{self, ExternalMergeTool, MergeTool}; use crate::text_util; use crate::ui::Ui; @@ -124,8 +124,13 @@ fn diff_formats_from_args( .collect_vec(); if let Some(name) = &args.tool { let tool = merge_tools::get_tool_config(settings, name)? - .unwrap_or_else(|| ExternalMergeTool::with_program(name)); - formats.push(DiffFormat::Tool(Box::new(tool))); + .unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_program(name))); + match tool { + MergeTool::Builtin => {} + MergeTool::External(tool) => { + formats.push(DiffFormat::Tool(Box::new(tool))); + } + } } Ok(formats) } @@ -135,8 +140,13 @@ fn default_diff_format(settings: &UserSettings) -> Result {} + MergeTool::External(tool) => { + return Ok(DiffFormat::Tool(Box::new(tool))); + } + } } let name = if let Some(name) = config.get_string("ui.diff.format").optional()? { name diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index 96b9dc8ea7..cc6af159c1 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -33,6 +33,8 @@ pub use self::external::{generate_diff, ExternalMergeTool}; use crate::config::CommandNameAndArgs; use crate::ui::Ui; +const BUILTIN_EDITOR_NAME: &str = ":builtin"; + #[derive(Debug, Error)] pub enum DiffEditError { #[error(transparent)] @@ -166,7 +168,7 @@ fn editor_args_from_settings( if let Some(args) = settings.config().get(key).optional()? { Ok(args) } else { - let default_editor = "meld"; + let default_editor = BUILTIN_EDITOR_NAME; writeln!( ui.hint(), "Using default editor '{default_editor}'; you can change this by setting {key}" @@ -180,7 +182,11 @@ fn editor_args_from_settings( pub fn get_tool_config( settings: &UserSettings, name: &str, -) -> Result, ConfigError> { +) -> Result, ConfigError> { + if name == BUILTIN_EDITOR_NAME { + return Ok(Some(MergeTool::Builtin)); + } + const TABLE_KEY: &str = "merge-tools"; let tools_table = settings.config().get_table(TABLE_KEY)?; if let Some(v) = tools_table.get(name) { @@ -193,7 +199,7 @@ pub fn get_tool_config( if result.program.is_empty() { result.program.clone_from(&name.to_string()); }; - Ok(Some(result)) + Ok(Some(MergeTool::External(result))) } else { Ok(None) } @@ -204,7 +210,7 @@ pub fn get_tool_config( pub fn get_tool_config_from_args( settings: &UserSettings, args: &CommandNameAndArgs, -) -> Result, ConfigError> { +) -> Result, ConfigError> { match args { CommandNameAndArgs::String(name) => get_tool_config(settings, name), CommandNameAndArgs::Vec(_) | CommandNameAndArgs::Structured { .. } => Ok(None), @@ -217,8 +223,8 @@ fn get_diff_editor_from_settings( ) -> Result { let args = editor_args_from_settings(ui, settings, "ui.diff-editor")?; let editor = get_tool_config_from_args(settings, &args)? - .unwrap_or_else(|| ExternalMergeTool::with_edit_args(&args)); - Ok(MergeTool::External(editor)) + .unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_edit_args(&args))); + Ok(editor) } fn get_merge_tool_from_settings( @@ -226,14 +232,15 @@ fn get_merge_tool_from_settings( settings: &UserSettings, ) -> Result { let args = editor_args_from_settings(ui, settings, "ui.merge-editor")?; - let editor = get_tool_config_from_args(settings, &args)? - .unwrap_or_else(|| ExternalMergeTool::with_merge_args(&args)); - if editor.merge_args.is_empty() { - Err(ExternalToolError::MergeArgsNotConfigured { - tool_name: args.to_string(), - }) - } else { - Ok(MergeTool::External(editor)) + let mergetool = get_tool_config_from_args(settings, &args)? + .unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_merge_args(&args))); + match mergetool { + MergeTool::External(mergetool) if mergetool.merge_args.is_empty() => { + Err(ExternalToolError::MergeArgsNotConfigured { + tool_name: args.to_string(), + }) + } + mergetool => Ok(mergetool), } } @@ -260,30 +267,7 @@ mod tests { }; // Default - insta::assert_debug_snapshot!(get("").unwrap(), @r###" - External( - ExternalMergeTool { - program: "meld", - diff_args: [ - "$left", - "$right", - ], - edit_args: [ - "$left", - "$right", - ], - merge_args: [ - "$left", - "$base", - "$right", - "-o", - "$output", - "--auto-merge", - ], - merge_tool_edits_conflict_markers: false, - }, - ) - "###); + insta::assert_debug_snapshot!(get("").unwrap(), @"Builtin"); // Just program name, edit_args are filled by default insta::assert_debug_snapshot!(get(r#"ui.diff-editor = "my-diff""#).unwrap(), @r###" @@ -432,30 +416,7 @@ mod tests { }; // Default - insta::assert_debug_snapshot!(get("").unwrap(), @r###" - External( - ExternalMergeTool { - program: "meld", - diff_args: [ - "$left", - "$right", - ], - edit_args: [ - "$left", - "$right", - ], - merge_args: [ - "$left", - "$base", - "$right", - "-o", - "$output", - "--auto-merge", - ], - merge_tool_edits_conflict_markers: false, - }, - ) - "###); + insta::assert_debug_snapshot!(get("").unwrap(), @"Builtin"); // Just program name insta::assert_debug_snapshot!(get(r#"ui.merge-editor = "my-merge""#).unwrap_err(), @r###" diff --git a/docs/config.md b/docs/config.md index 3127cd5ea9..18e10ac4b9 100644 --- a/docs/config.md +++ b/docs/config.md @@ -346,8 +346,9 @@ Obviously, you would only set one line, don't copy them all in! ## Editing diffs -The `ui.diff-editor` setting affects the tool used for editing diffs (e.g. -`jj split`, `jj amend -i`). The default is `meld`. +The `ui.diff-editor` setting affects the tool used for editing diffs (e.g. `jj +split`, `jj amend -i`). The default is the special value `:builtin`, which +launches a TUI tool to edit the diff in your terminal. `jj` makes the following substitutions: @@ -388,23 +389,6 @@ the directory where the diff editor will be expected to put the result of the user's edits. Initially, the contents of `$output` will be the same as the contents of `$right`. -### Setting up `scm-diff-editor` - -`scm-diff-editor` is a terminal-based diff editor that is part of -the [git-branchless](https://github.com/arxanas/git-branchless) suite of tools. -It's a good alternative to Meld, especially if you don't have a graphical -environment (e.g. when using SSH). To install it: - -```shell -cargo install --git https://github.com/arxanas/git-branchless scm-record --features scm-diff-editor -``` - -Then config it as follows: - -```toml -ui.diff-editor = ["scm-diff-editor", "--dir-diff", "$left", "$right"] -``` - ### `JJ-INSTRUCTIONS` When editing a diff, jj will include a synthetic file called `JJ-INSTRUCTIONS`