diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 2d12938922..4b940fdd73 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -86,11 +86,13 @@ use crate::formatter::{FormatRecorder, Formatter, PlainTextFormatter}; use crate::git_util::{ is_colocated_git_workspace, print_failed_git_export, print_git_import_stats, }; -use crate::merge_tools::{ConflictResolveError, DiffEditError, DiffGenerateError}; +use crate::merge_tools::{ + ConflictResolveError, DiffEditError, DiffEditor, DiffGenerateError, MergeEditor, +}; use crate::template_parser::{TemplateAliasesMap, TemplateParseError, TemplateParseErrorKind}; use crate::templater::Template; use crate::ui::{ColorChoice, Ui}; -use crate::{commit_templater, text_util}; +use crate::{commit_templater, merge_tools, text_util}; #[derive(Clone, Debug)] pub enum CommandError { @@ -1106,6 +1108,18 @@ impl WorkspaceCommandHelper { Ok(git_ignores) } + /// Loads diff editor from the settings. + // TODO: override settings by --tool= option (#2575) + pub fn diff_editor(&self, ui: &Ui) -> Result { + DiffEditor::from_settings(ui, &self.settings) + } + + /// Loads 3-way merge editor from the settings. + // TODO: override settings by --tool= option (#2575) + pub fn merge_editor(&self, ui: &Ui) -> Result { + MergeEditor::from_settings(ui, &self.settings) + } + pub fn resolve_single_op(&self, op_str: &str) -> Result { op_walk::resolve_op_with_repo(self.repo(), op_str) } @@ -1733,21 +1747,20 @@ impl WorkspaceCommandTransaction<'_> { self.tx.mut_repo().edit(workspace_id, commit) } + // TODO: inline this pub fn run_mergetool( &self, - ui: &Ui, + editor: &MergeEditor, tree: &MergedTree, repo_path: &RepoPath, ) -> Result { - let settings = &self.helper.settings; - Ok(crate::merge_tools::run_mergetool( - ui, tree, repo_path, settings, - )?) + Ok(merge_tools::run_mergetool(editor, tree, repo_path)?) } + // TODO: maybe capture parameters by diff_editor(), and inline this? pub fn edit_diff( &self, - ui: &Ui, + editor: &DiffEditor, left_tree: &MergedTree, right_tree: &MergedTree, matcher: &dyn Matcher, @@ -1760,28 +1773,27 @@ impl WorkspaceCommandTransaction<'_> { } else { None }; - Ok(crate::merge_tools::edit_diff( - ui, + Ok(merge_tools::edit_diff( + editor, left_tree, right_tree, matcher, instructions, base_ignores, - settings, )?) } + // TODO: maybe inline or extract to free function (no dependency on self) pub fn select_diff( &self, - ui: &Ui, + interactive_editor: Option<&DiffEditor>, left_tree: &MergedTree, right_tree: &MergedTree, matcher: &dyn Matcher, instructions: Option<&str>, - interactive: bool, ) -> Result { - if interactive { - self.edit_diff(ui, left_tree, right_tree, matcher, instructions) + if let Some(editor) = interactive_editor { + self.edit_diff(editor, left_tree, right_tree, matcher, instructions) } else { let new_tree_id = restore_tree(right_tree, left_tree, matcher)?; Ok(new_tree_id) diff --git a/cli/src/commands/commit.rs b/cli/src/commands/commit.rs index 4e7effec98..fa33f5bfd5 100644 --- a/cli/src/commands/commit.rs +++ b/cli/src/commands/commit.rs @@ -49,6 +49,11 @@ pub(crate) fn cmd_commit( .ok_or_else(|| user_error("This command requires a working copy"))?; let commit = workspace_command.repo().store().get_commit(commit_id)?; let matcher = workspace_command.matcher_from_values(&args.paths)?; + let interactive_editor = if args.interactive { + Some(workspace_command.diff_editor(ui)?) + } else { + None + }; let mut tx = workspace_command.start_transaction(); let base_tree = merge_commit_trees(tx.repo(), &commit.parents())?; let instructions = format!( @@ -62,12 +67,11 @@ new working-copy commit. tx.format_commit_summary(&commit) ); let tree_id = tx.select_diff( - ui, + interactive_editor.as_ref(), &base_tree, &commit.tree()?, matcher.as_ref(), Some(&instructions), - args.interactive, )?; let middle_tree = tx.repo().store().get_root_tree(&tree_id)?; if !args.paths.is_empty() && middle_tree.id() == base_tree.id() { diff --git a/cli/src/commands/diffedit.rs b/cli/src/commands/diffedit.rs index cb1bad77b6..7181ecd1d5 100644 --- a/cli/src/commands/diffedit.rs +++ b/cli/src/commands/diffedit.rs @@ -77,6 +77,7 @@ pub(crate) fn cmd_diffedit( }; workspace_command.check_rewritable([&target_commit])?; + let diff_editor = workspace_command.diff_editor(ui)?; let mut tx = workspace_command.start_transaction(); let instructions = format!( "\ @@ -91,7 +92,7 @@ don't make any changes, then the operation will be aborted.", let base_tree = merge_commit_trees(tx.repo(), base_commits.as_slice())?; let tree = target_commit.tree()?; let tree_id = tx.edit_diff( - ui, + &diff_editor, &base_tree, &tree, &EverythingMatcher, diff --git a/cli/src/commands/move.rs b/cli/src/commands/move.rs index 9d3b5528e5..02bc0e4602 100644 --- a/cli/src/commands/move.rs +++ b/cli/src/commands/move.rs @@ -69,6 +69,11 @@ pub(crate) fn cmd_move( } workspace_command.check_rewritable([&source, &destination])?; let matcher = workspace_command.matcher_from_values(&args.paths)?; + let interactive_editor = if args.interactive { + Some(workspace_command.diff_editor(ui)?) + } else { + None + }; let mut tx = workspace_command.start_transaction(); let parent_tree = merge_commit_trees(tx.repo(), &source.parents())?; let source_tree = source.tree()?; @@ -89,14 +94,13 @@ from the source will be moved into the destination. tx.format_commit_summary(&destination) ); let new_parent_tree_id = tx.select_diff( - ui, + interactive_editor.as_ref(), &parent_tree, &source_tree, matcher.as_ref(), Some(&instructions), - args.interactive, )?; - if args.interactive && new_parent_tree_id == parent_tree.id() { + if interactive_editor.is_some() && new_parent_tree_id == parent_tree.id() { return Err(user_error("No changes to move")); } let new_parent_tree = tx.repo().store().get_root_tree(&new_parent_tree_id)?; diff --git a/cli/src/commands/resolve.rs b/cli/src/commands/resolve.rs index 9f4260a515..9422faed18 100644 --- a/cli/src/commands/resolve.rs +++ b/cli/src/commands/resolve.rs @@ -96,13 +96,14 @@ pub(crate) fn cmd_resolve( let (repo_path, _) = conflicts.first().unwrap(); workspace_command.check_rewritable([&commit])?; + let merge_editor = workspace_command.merge_editor(ui)?; writeln!( ui.stderr(), "Resolving conflicts in: {}", workspace_command.format_file_path(repo_path) )?; let mut tx = workspace_command.start_transaction(); - let new_tree_id = tx.run_mergetool(ui, &tree, repo_path)?; + let new_tree_id = tx.run_mergetool(&merge_editor, &tree, repo_path)?; let new_commit = tx .mut_repo() .rewrite_commit(command.settings(), &commit) diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index 7f342fad96..58e3062b45 100644 --- a/cli/src/commands/split.rs +++ b/cli/src/commands/split.rs @@ -58,10 +58,14 @@ pub(crate) fn cmd_split( let commit = workspace_command.resolve_single_rev(&args.revision)?; workspace_command.check_rewritable([&commit])?; let matcher = workspace_command.matcher_from_values(&args.paths)?; + let interactive_editor = if args.interactive || args.paths.is_empty() { + Some(workspace_command.diff_editor(ui)?) + } else { + None + }; let mut tx = workspace_command.start_transaction(); let end_tree = commit.tree()?; let base_tree = merge_commit_trees(tx.repo(), &commit.parents())?; - let interactive = args.interactive || args.paths.is_empty(); let instructions = format!( "\ You are splitting a commit in two: {} @@ -77,14 +81,13 @@ don't make any changes, then the operation will be aborted. // Prompt the user to select the changes they want for the first commit. let selected_tree_id = tx.select_diff( - ui, + interactive_editor.as_ref(), &base_tree, &end_tree, matcher.as_ref(), Some(&instructions), - interactive, )?; - if &selected_tree_id == commit.tree_id() && interactive { + if &selected_tree_id == commit.tree_id() && interactive_editor.is_some() { // The user selected everything from the original commit. writeln!(ui.stderr(), "Nothing changed.")?; return Ok(()); diff --git a/cli/src/commands/squash.rs b/cli/src/commands/squash.rs index 94863a3148..00fd40baf2 100644 --- a/cli/src/commands/squash.rs +++ b/cli/src/commands/squash.rs @@ -66,6 +66,11 @@ pub(crate) fn cmd_squash( let parent = &parents[0]; workspace_command.check_rewritable(&parents[..1])?; let matcher = workspace_command.matcher_from_values(&args.paths)?; + let interactive_editor = if args.interactive { + Some(workspace_command.diff_editor(ui)?) + } else { + None + }; let mut tx = workspace_command.start_transaction(); let instructions = format!( "\ @@ -86,15 +91,14 @@ from the source will be moved into the parent. let parent_tree = parent.tree()?; let tree = commit.tree()?; let new_parent_tree_id = tx.select_diff( - ui, + interactive_editor.as_ref(), &parent_tree, &tree, matcher.as_ref(), Some(&instructions), - args.interactive, )?; if &new_parent_tree_id == parent.tree_id() { - if args.interactive { + if interactive_editor.is_some() { return Err(user_error("No changes selected")); } diff --git a/cli/src/commands/unsquash.rs b/cli/src/commands/unsquash.rs index a1ac8e0d07..c423bb92f9 100644 --- a/cli/src/commands/unsquash.rs +++ b/cli/src/commands/unsquash.rs @@ -61,10 +61,15 @@ pub(crate) fn cmd_unsquash( } let parent = &parents[0]; workspace_command.check_rewritable(&parents[..1])?; + let interactive_editor = if args.interactive { + Some(workspace_command.diff_editor(ui)?) + } else { + None + }; let mut tx = workspace_command.start_transaction(); let parent_base_tree = merge_commit_trees(tx.repo(), &parent.parents())?; let new_parent_tree_id; - if args.interactive { + if let Some(diff_editor) = &interactive_editor { let instructions = format!( "\ You are moving changes from: {} @@ -82,7 +87,7 @@ aborted. ); let parent_tree = parent.tree()?; new_parent_tree_id = tx.edit_diff( - ui, + diff_editor, &parent_base_tree, &parent_tree, &EverythingMatcher, diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index ddf65ab57e..e009b8afc9 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -422,7 +422,7 @@ fn find_all_variables(args: &[String]) -> impl Iterator { } pub fn edit_diff_external( - editor: ExternalMergeTool, + editor: &ExternalMergeTool, left_tree: &MergedTree, right_tree: &MergedTree, matcher: &dyn Matcher, diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index 947621a908..1089acc31c 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -86,10 +86,9 @@ pub enum ConflictResolveError { } pub fn run_mergetool( - ui: &Ui, + editor: &MergeEditor, tree: &MergedTree, repo_path: &RepoPath, - settings: &UserSettings, ) -> Result { let conflict = match tree.path_value(repo_path).into_resolved() { Err(conflict) => conflict, @@ -115,30 +114,27 @@ pub fn run_mergetool( }; let content = extract_as_single_hunk(&file_merge, tree.store(), repo_path).block_on(); - let editor = MergeEditor::from_settings(ui, settings)?.tool; - match editor { + match &editor.tool { MergeTool::Builtin => { let tree_id = edit_merge_builtin(tree, repo_path, content).map_err(Box::new)?; Ok(tree_id) } - MergeTool::External(editor) => external::run_mergetool_external( - &editor, file_merge, content, repo_path, conflict, tree, - ), + MergeTool::External(editor) => { + external::run_mergetool_external(editor, file_merge, content, repo_path, conflict, tree) + } } } pub fn edit_diff( - ui: &Ui, + editor: &DiffEditor, left_tree: &MergedTree, right_tree: &MergedTree, matcher: &dyn Matcher, instructions: Option<&str>, base_ignores: Arc, - settings: &UserSettings, ) -> Result { // Start a diff editor on the two directories. - let editor = DiffEditor::from_settings(ui, settings)?.tool; - match editor { + match &editor.tool { MergeTool::Builtin => { let tree_id = edit_diff_builtin(left_tree, right_tree, matcher).map_err(Box::new)?; Ok(tree_id) diff --git a/cli/tests/test_resolve_command.rs b/cli/tests/test_resolve_command.rs index e90613c8e2..c48b39de69 100644 --- a/cli/tests/test_resolve_command.rs +++ b/cli/tests/test_resolve_command.rs @@ -408,6 +408,7 @@ fn test_too_many_parents() { let error = test_env.jj_cmd_failure(&repo_path, &["resolve"]); insta::assert_snapshot!(error, @r###" + Using default editor ':builtin'; run `jj config set --user ui.merge-editor :builtin` to disable this message. Resolving conflicts in: file Error: Failed to resolve conflicts Caused by: The conflict at "file" has 3 sides. At most 2 sides are supported. @@ -485,6 +486,7 @@ fn test_file_vs_dir() { "###); let error = test_env.jj_cmd_failure(&repo_path, &["resolve"]); insta::assert_snapshot!(error, @r###" + Using default editor ':builtin'; run `jj config set --user ui.merge-editor :builtin` to disable this message. Resolving conflicts in: file Error: Failed to resolve conflicts Caused by: Only conflicts that involve normal files (not symlinks, not executable, etc.) are supported. Conflict summary for "file": @@ -541,6 +543,7 @@ fn test_description_with_dir_and_deletion() { "###); let error = test_env.jj_cmd_failure(&repo_path, &["resolve"]); insta::assert_snapshot!(error, @r###" + Using default editor ':builtin'; run `jj config set --user ui.merge-editor :builtin` to disable this message. Resolving conflicts in: file Error: Failed to resolve conflicts Caused by: Only conflicts that involve normal files (not symlinks, not executable, etc.) are supported. Conflict summary for "file":