merge_tools: load diff/merge editor settings by caller

This moves the config loading closer to CLI args where --tool=<name> option
will be processed. The factory function are proxied through the command helper
so that the base_ignores can be attached there later.

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, DiffEditError> {
+        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, ConflictResolveError> {
+        MergeEditor::from_settings(ui, &self.settings)
+    }
+
     pub fn resolve_single_op(&self, op_str: &str) -> Result<Operation, OpsetEvaluationError> {
         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<MergedTreeId, CommandError> {
-        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<MergedTreeId, CommandError> {
-        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)

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() {

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,

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)?;

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)

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(());

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

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,

cli/src/merge_tools/external.rs

@@ -422,7 +422,7 @@ fn find_all_variables(args: &[String]) -> impl Iterator<Item = &str> {
 }
 
 pub fn edit_diff_external(
-    editor: ExternalMergeTool,
+    editor: &ExternalMergeTool,
     left_tree: &MergedTree,
     right_tree: &MergedTree,
     matcher: &dyn Matcher,

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<MergedTreeId, ConflictResolveError> {
     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<GitIgnoreFile>,
-    settings: &UserSettings,
 ) -> Result<MergedTreeId, DiffEditError> {
     // 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)

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":