merge_tools: clean up Diff/MergeEditor interface

cli/src/cli_util.rs

@@ -93,7 +93,7 @@ use crate::merge_tools::{
 use crate::template_parser::{TemplateAliasesMap, TemplateParseError, TemplateParseErrorKind};
 use crate::templater::Template;
 use crate::ui::{ColorChoice, Ui};
-use crate::{commit_templater, merge_tools, text_util};
+use crate::{commit_templater, text_util};
 
 #[derive(Clone, Debug)]
 pub enum CommandError {
@@ -1117,8 +1117,19 @@ impl WorkspaceCommandHelper {
 
     /// 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)
+    pub fn diff_editor(&self, ui: &Ui) -> Result<DiffEditor, CommandError> {
+        let base_ignores = self.base_ignores()?;
+        Ok(DiffEditor::from_settings(ui, &self.settings, base_ignores)?)
+    }
+
+    /// Conditionally loads diff editor from the settings.
+    // TODO: override settings by --tool= option (#2575)
+    pub fn diff_selector(&self, ui: &Ui, interactive: bool) -> Result<DiffSelector, CommandError> {
+        if interactive {
+            Ok(DiffSelector::Interactive(self.diff_editor(ui)?))
+        } else {
+            Ok(DiffSelector::NonInteractive)
+        }
     }
 
     /// Loads 3-way merge editor from the settings.
@@ -1754,59 +1765,6 @@ impl WorkspaceCommandTransaction<'_> {
         self.tx.mut_repo().edit(workspace_id, commit)
     }
 
-    // TODO: inline this
-    pub fn run_mergetool(
-        &self,
-        editor: &MergeEditor,
-        tree: &MergedTree,
-        repo_path: &RepoPath,
-    ) -> Result<MergedTreeId, CommandError> {
-        Ok(merge_tools::run_mergetool(editor, tree, repo_path)?)
-    }
-
-    // TODO: maybe capture parameters by diff_editor(), and inline this?
-    pub fn edit_diff(
-        &self,
-        editor: &DiffEditor,
-        left_tree: &MergedTree,
-        right_tree: &MergedTree,
-        matcher: &dyn Matcher,
-        instructions: Option<&str>,
-    ) -> Result<MergedTreeId, CommandError> {
-        let base_ignores = self.helper.base_ignores()?;
-        let settings = &self.helper.settings;
-        let instructions = if settings.diff_instructions() {
-            instructions
-        } else {
-            None
-        };
-        Ok(merge_tools::edit_diff(
-            editor,
-            left_tree,
-            right_tree,
-            matcher,
-            instructions,
-            base_ignores,
-        )?)
-    }
-
-    // TODO: maybe inline or extract to free function (no dependency on self)
-    pub fn select_diff(
-        &self,
-        interactive_editor: Option<&DiffEditor>,
-        left_tree: &MergedTree,
-        right_tree: &MergedTree,
-        matcher: &dyn Matcher,
-        instructions: Option<&str>,
-    ) -> Result<MergedTreeId, CommandError> {
-        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)
-        }
-    }
-
     pub fn format_commit_summary(&self, commit: &Commit) -> String {
         let mut output = Vec::new();
         self.write_commit_summary(&mut PlainTextFormatter::new(&mut output), commit)
@@ -2434,6 +2392,36 @@ pub fn short_operation_hash(operation_id: &OperationId) -> String {
     operation_id.hex()[0..12].to_string()
 }
 
+/// Wrapper around a `DiffEditor` to conditionally start interactive session.
+#[derive(Clone, Debug)]
+pub enum DiffSelector {
+    NonInteractive,
+    Interactive(DiffEditor),
+}
+
+impl DiffSelector {
+    pub fn is_interactive(&self) -> bool {
+        matches!(self, DiffSelector::Interactive(_))
+    }
+
+    /// Restores diffs from the `right_tree` to the `left_tree` by using an
+    /// interactive editor if enabled.
+    pub fn select(
+        &self,
+        left_tree: &MergedTree,
+        right_tree: &MergedTree,
+        matcher: &dyn Matcher,
+        instructions: Option<&str>,
+    ) -> Result<MergedTreeId, CommandError> {
+        match self {
+            DiffSelector::NonInteractive => Ok(restore_tree(right_tree, left_tree, matcher)?),
+            DiffSelector::Interactive(editor) => {
+                Ok(editor.edit(left_tree, right_tree, matcher, instructions)?)
+            }
+        }
+    }
+}
+
 #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
 pub struct RemoteBranchName {
     pub branch: String,

cli/src/commands/commit.rs

@@ -49,11 +49,7 @@ 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 diff_selector = workspace_command.diff_selector(ui, args.interactive)?;
     let mut tx = workspace_command.start_transaction();
     let base_tree = merge_commit_trees(tx.repo(), &commit.parents())?;
     let instructions = format!(
@@ -66,8 +62,7 @@ new working-copy commit.
 ",
         tx.format_commit_summary(&commit)
     );
-    let tree_id = tx.select_diff(
-        interactive_editor.as_ref(),
+    let tree_id = diff_selector.select(
         &base_tree,
         &commit.tree()?,
         matcher.as_ref(),

cli/src/commands/diffedit.rs

@@ -91,13 +91,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(
-        &diff_editor,
-        &base_tree,
-        &tree,
-        &EverythingMatcher,
-        Some(&instructions),
-    )?;
+    let tree_id = diff_editor.edit(&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,11 +69,7 @@ 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 diff_selector = workspace_command.diff_selector(ui, args.interactive)?;
     let mut tx = workspace_command.start_transaction();
     let parent_tree = merge_commit_trees(tx.repo(), &source.parents())?;
     let source_tree = source.tree()?;
@@ -93,14 +89,13 @@ from the source will be moved into the destination.
         tx.format_commit_summary(&source),
         tx.format_commit_summary(&destination)
     );
-    let new_parent_tree_id = tx.select_diff(
-        interactive_editor.as_ref(),
+    let new_parent_tree_id = diff_selector.select(
         &parent_tree,
         &source_tree,
         matcher.as_ref(),
         Some(&instructions),
     )?;
-    if interactive_editor.is_some() && new_parent_tree_id == parent_tree.id() {
+    if diff_selector.is_interactive() && 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

@@ -103,7 +103,7 @@ pub(crate) fn cmd_resolve(
         workspace_command.format_file_path(repo_path)
     )?;
     let mut tx = workspace_command.start_transaction();
-    let new_tree_id = tx.run_mergetool(&merge_editor, &tree, repo_path)?;
+    let new_tree_id = merge_editor.edit_file(&tree, repo_path)?;
     let new_commit = tx
         .mut_repo()
         .rewrite_commit(command.settings(), &commit)

cli/src/commands/split.rs

@@ -58,11 +58,8 @@ 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 diff_selector =
+        workspace_command.diff_selector(ui, args.interactive || args.paths.is_empty())?;
     let mut tx = workspace_command.start_transaction();
     let end_tree = commit.tree()?;
     let base_tree = merge_commit_trees(tx.repo(), &commit.parents())?;
@@ -80,14 +77,9 @@ 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(
-        interactive_editor.as_ref(),
-        &base_tree,
-        &end_tree,
-        matcher.as_ref(),
-        Some(&instructions),
-    )?;
-    if &selected_tree_id == commit.tree_id() && interactive_editor.is_some() {
+    let selected_tree_id =
+        diff_selector.select(&base_tree, &end_tree, matcher.as_ref(), Some(&instructions))?;
+    if &selected_tree_id == commit.tree_id() && diff_selector.is_interactive() {
         // The user selected everything from the original commit.
         writeln!(ui.stderr(), "Nothing changed.")?;
         return Ok(());

cli/src/commands/squash.rs

@@ -66,11 +66,7 @@ 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 diff_selector = workspace_command.diff_selector(ui, args.interactive)?;
     let mut tx = workspace_command.start_transaction();
     let instructions = format!(
         "\
@@ -90,15 +86,10 @@ 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(
-        interactive_editor.as_ref(),
-        &parent_tree,
-        &tree,
-        matcher.as_ref(),
-        Some(&instructions),
-    )?;
+    let new_parent_tree_id =
+        diff_selector.select(&parent_tree, &tree, matcher.as_ref(), Some(&instructions))?;
     if &new_parent_tree_id == parent.tree_id() {
-        if interactive_editor.is_some() {
+        if diff_selector.is_interactive() {
             return Err(user_error("No changes selected"));
         }
 

cli/src/commands/unsquash.rs

@@ -86,8 +86,7 @@ aborted.
             tx.format_commit_summary(&commit)
         );
         let parent_tree = parent.tree()?;
-        new_parent_tree_id = tx.edit_diff(
-            diff_editor,
+        new_parent_tree_id = diff_editor.edit(
             &parent_base_tree,
             &parent_tree,
             &EverythingMatcher,

cli/src/config/misc.toml

@@ -5,6 +5,7 @@
 tree-level-conflicts = true
 
 [ui]
+diff-instructions = true
 paginate = "auto"
 pager = { command = ["less", "-FRX"], env = { LESSCHARSET = "utf-8" } }
 log-word-wrap = false