merge_tools: extract tool configuration from invocation path

cli/src/cli_util.rs

@@ -86,11 +86,14 @@ 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,
+    MergeToolConfigError,
+};
 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 {
@@ -351,6 +354,12 @@ impl From<ConflictResolveError> for CommandError {
     }
 }
 
+impl From<MergeToolConfigError> for CommandError {
+    fn from(err: MergeToolConfigError) -> Self {
+        user_error_with_message("Failed to load tool configuration", err)
+    }
+}
+
 impl From<git2::Error> for CommandError {
     fn from(err: git2::Error) -> Self {
         user_error_with_message("Git operation failed", err)
@@ -1106,6 +1115,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, MergeToolConfigError> {
+        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, MergeToolConfigError> {
+        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,50 +1754,53 @@ 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,
-        instructions: &str,
+        instructions: Option<&str>,
     ) -> Result<MergedTreeId, CommandError> {
         let base_ignores = self.helper.base_ignores()?;
         let settings = &self.helper.settings;
-        Ok(crate::merge_tools::edit_diff(
-            ui,
+        let instructions = if settings.diff_instructions() {
+            instructions
+        } else {
+            None
+        };
+        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: &str,
-        interactive: bool,
+        instructions: Option<&str>,
     ) -> 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(),
-        &instructions,
-        args.interactive,
+        Some(&instructions),
     )?;
     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!(
         "\
@@ -90,7 +91,13 @@ 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, &base_tree, &tree, &EverythingMatcher, &instructions)?;
+    let tree_id = tx.edit_diff(
+        &diff_editor,
+        &base_tree,
+        &tree,
+        &EverythingMatcher,
+        Some(&instructions),
+    )?;
     if tree_id == *target_commit.tree_id() {
         writeln!(ui.stderr(), "Nothing changed.")?;
     } else {

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(),
-        &instructions,
-        args.interactive,
+        Some(&instructions),
     )?;
-    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(),
-        &instructions,
-        interactive,
+        Some(&instructions),
     )?;
-    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(),
-        &instructions,
-        args.interactive,
+        Some(&instructions),
     )?;
     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,11 +87,11 @@ aborted.
         );
         let parent_tree = parent.tree()?;
         new_parent_tree_id = tx.edit_diff(
-            ui,
+            diff_editor,
             &parent_base_tree,
             &parent_tree,
             &EverythingMatcher,
-            &instructions,
+            Some(&instructions),
         )?;
         if new_parent_tree_id == parent_base_tree.id() {
             return Err(user_error("No changes selected"));

cli/src/merge_tools/external.rs

@@ -5,18 +5,17 @@ use std::path::{Path, PathBuf};
 use std::process::{Command, ExitStatus, Stdio};
 use std::sync::Arc;
 
-use config::ConfigError;
 use futures::StreamExt;
 use itertools::Itertools;
 use jj_lib::backend::{FileId, MergedTreeId, TreeValue};
 use jj_lib::conflicts::{self, materialize_merge_result};
+use jj_lib::fsmonitor::FsmonitorKind;
 use jj_lib::gitignore::GitIgnoreFile;
 use jj_lib::local_working_copy::{TreeState, TreeStateError};
 use jj_lib::matchers::Matcher;
 use jj_lib::merge::{Merge, MergedTreeValue};
 use jj_lib::merged_tree::{MergedTree, MergedTreeBuilder};
 use jj_lib::repo_path::{RepoPath, RepoPathBuf};
-use jj_lib::settings::UserSettings;
 use jj_lib::store::Store;
 use jj_lib::working_copy::{CheckoutError, SnapshotOptions};
 use pollster::FutureExt;
@@ -107,13 +106,6 @@ impl ExternalMergeTool {
 
 #[derive(Debug, Error)]
 pub enum ExternalToolError {
-    #[error("Invalid config")]
-    Config(#[from] ConfigError),
-    #[error(
-        "To use `{tool_name}` as a merge tool, the config `merge-tools.{tool_name}.merge-args` \
-         must be defined (see docs for details)"
-    )]
-    MergeArgsNotConfigured { tool_name: String },
     #[error("Error setting up temporary directory")]
     SetUpDir(#[source] std::io::Error),
     // TODO: Remove the "(run with --debug to see the exact invocation)"
@@ -422,13 +414,12 @@ 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,
-    instructions: &str,
+    instructions: Option<&str>,
     base_ignores: Arc<GitIgnoreFile>,
-    settings: &UserSettings,
 ) -> Result<MergedTreeId, DiffEditError> {
     let got_output_field = find_all_variables(&editor.edit_args).contains(&"output");
     let store = left_tree.store();
@@ -451,10 +442,12 @@ pub fn edit_diff_external(
     let instructions_path_to_cleanup = output_wc_path.join("JJ-INSTRUCTIONS");
     // In the unlikely event that the file already exists, then the user will simply
     // not get any instructions.
-    let add_instructions = settings.diff_instructions()
-        && !instructions.is_empty()
-        && !instructions_path_to_cleanup.exists();
-    if add_instructions {
+    let add_instructions = if !instructions_path_to_cleanup.exists() {
+        instructions
+    } else {
+        None
+    };
+    if let Some(instructions) = add_instructions {
         let mut output_instructions_file =
             File::create(&instructions_path_to_cleanup).map_err(ExternalToolError::SetUpDir)?;
         if diff_wc.right_working_copy_path() != output_wc_path {
@@ -512,18 +505,19 @@ diff editing in mind and be a little inaccurate.
             exit_status,
         }));
     }
-    if add_instructions {
+    if add_instructions.is_some() {
         std::fs::remove_file(instructions_path_to_cleanup).ok();
     }
 
+    // Snapshot changes in the temporary output directory.
     let mut output_tree_state = diff_wc
         .output_tree_state
         .unwrap_or(diff_wc.right_tree_state);
     output_tree_state.snapshot(SnapshotOptions {
         base_ignores,
-        fsmonitor_kind: settings.fsmonitor_kind()?,
+        fsmonitor_kind: FsmonitorKind::None,
         progress: None,
-        max_new_file_size: settings.max_new_file_size()?,
+        max_new_file_size: u64::MAX,
     })?;
     Ok(output_tree_state.current_tree_id().clone())
 }