diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 13166011f0..03268e77f8 100644 --- a/cli/src/cli_util.rs +++ b/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::from_settings(ui, &self.settings) + pub fn diff_editor(&self, ui: &Ui) -> Result { + 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 { + 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 { - 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 { - 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 { - 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 { + 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, diff --git a/cli/src/commands/commit.rs b/cli/src/commands/commit.rs index fa33f5bfd5..a6f4906ae0 100644 --- a/cli/src/commands/commit.rs +++ b/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(), diff --git a/cli/src/commands/diffedit.rs b/cli/src/commands/diffedit.rs index 7181ecd1d5..6adf43f728 100644 --- a/cli/src/commands/diffedit.rs +++ b/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 { diff --git a/cli/src/commands/move.rs b/cli/src/commands/move.rs index 02bc0e4602..ea47374c9c 100644 --- a/cli/src/commands/move.rs +++ b/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)?; diff --git a/cli/src/commands/resolve.rs b/cli/src/commands/resolve.rs index 9422faed18..0e5370cc75 100644 --- a/cli/src/commands/resolve.rs +++ b/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) diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index 58e3062b45..ff3d918629 100644 --- a/cli/src/commands/split.rs +++ b/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(()); diff --git a/cli/src/commands/squash.rs b/cli/src/commands/squash.rs index 00fd40baf2..b66e4c7724 100644 --- a/cli/src/commands/squash.rs +++ b/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")); } diff --git a/cli/src/commands/unsquash.rs b/cli/src/commands/unsquash.rs index c423bb92f9..db70767729 100644 --- a/cli/src/commands/unsquash.rs +++ b/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, diff --git a/cli/src/config/misc.toml b/cli/src/config/misc.toml index e247778fdc..c3daf74b99 100644 --- a/cli/src/config/misc.toml +++ b/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 diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index 886fd817e0..91961e0b72 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -85,71 +85,6 @@ pub enum ConflictResolveError { Backend(#[from] jj_lib::backend::BackendError), } -pub fn run_mergetool( - editor: &MergeEditor, - tree: &MergedTree, - repo_path: &RepoPath, -) -> Result { - let conflict = match tree.path_value(repo_path).into_resolved() { - Err(conflict) => conflict, - Ok(Some(_)) => return Err(ConflictResolveError::NotAConflict(repo_path.to_owned())), - Ok(None) => return Err(ConflictResolveError::PathNotFound(repo_path.to_owned())), - }; - let file_merge = conflict.to_file_merge().ok_or_else(|| { - let mut summary_bytes: Vec = vec![]; - conflict - .describe(&mut summary_bytes) - .expect("Writing to an in-memory buffer should never fail"); - ConflictResolveError::NotNormalFiles( - repo_path.to_owned(), - String::from_utf8_lossy(summary_bytes.as_slice()).to_string(), - ) - })?; - // We only support conflicts with 2 sides (3-way conflicts) - if file_merge.num_sides() > 2 { - return Err(ConflictResolveError::ConflictTooComplicated { - path: repo_path.to_owned(), - sides: file_merge.num_sides(), - }); - }; - let content = extract_as_single_hunk(&file_merge, tree.store(), repo_path).block_on(); - - 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) - } - } -} - -pub fn edit_diff( - editor: &DiffEditor, - left_tree: &MergedTree, - right_tree: &MergedTree, - matcher: &dyn Matcher, - instructions: Option<&str>, - base_ignores: Arc, -) -> Result { - // Start a diff editor on the two directories. - match &editor.tool { - MergeTool::Builtin => { - let tree_id = edit_diff_builtin(left_tree, right_tree, matcher).map_err(Box::new)?; - Ok(tree_id) - } - MergeTool::External(editor) => edit_diff_external( - editor, - left_tree, - right_tree, - matcher, - instructions, - base_ignores, - ), - } -} - #[derive(Debug, Error)] pub enum MergeToolConfigError { #[error(transparent)] @@ -226,11 +161,17 @@ pub fn get_external_tool_config( #[derive(Clone, Debug)] pub struct DiffEditor { tool: MergeTool, + base_ignores: Arc, + use_instructions: bool, } impl DiffEditor { /// Loads the default diff editor from the settings. - pub fn from_settings(ui: &Ui, settings: &UserSettings) -> Result { + pub fn from_settings( + ui: &Ui, + settings: &UserSettings, + base_ignores: Arc, + ) -> Result { let args = editor_args_from_settings(ui, settings, "ui.diff-editor")?; let tool = if let CommandNameAndArgs::String(name) = &args { get_tool_config(settings, name)? @@ -238,7 +179,37 @@ impl DiffEditor { None } .unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_edit_args(&args))); - Ok(DiffEditor { tool }) + Ok(DiffEditor { + tool, + base_ignores, + use_instructions: settings.config().get_bool("ui.diff-instructions")?, + }) + } + + /// Starts a diff editor on the two directories. + pub fn edit( + &self, + left_tree: &MergedTree, + right_tree: &MergedTree, + matcher: &dyn Matcher, + instructions: Option<&str>, + ) -> Result { + match &self.tool { + MergeTool::Builtin => { + Ok(edit_diff_builtin(left_tree, right_tree, matcher).map_err(Box::new)?) + } + MergeTool::External(editor) => { + let instructions = self.use_instructions.then_some(instructions).flatten(); + edit_diff_external( + editor, + left_tree, + right_tree, + matcher, + instructions, + self.base_ignores.clone(), + ) + } + } } } @@ -265,6 +236,47 @@ impl MergeEditor { } Ok(MergeEditor { tool }) } + + /// Starts a merge editor for the specified file. + pub fn edit_file( + &self, + tree: &MergedTree, + repo_path: &RepoPath, + ) -> Result { + let conflict = match tree.path_value(repo_path).into_resolved() { + Err(conflict) => conflict, + Ok(Some(_)) => return Err(ConflictResolveError::NotAConflict(repo_path.to_owned())), + Ok(None) => return Err(ConflictResolveError::PathNotFound(repo_path.to_owned())), + }; + let file_merge = conflict.to_file_merge().ok_or_else(|| { + let mut summary_bytes: Vec = vec![]; + conflict + .describe(&mut summary_bytes) + .expect("Writing to an in-memory buffer should never fail"); + ConflictResolveError::NotNormalFiles( + repo_path.to_owned(), + String::from_utf8_lossy(summary_bytes.as_slice()).to_string(), + ) + })?; + // We only support conflicts with 2 sides (3-way conflicts) + if file_merge.num_sides() > 2 { + return Err(ConflictResolveError::ConflictTooComplicated { + path: repo_path.to_owned(), + sides: file_merge.num_sides(), + }); + }; + let content = extract_as_single_hunk(&file_merge, tree.store(), repo_path).block_on(); + + match &self.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, + ), + } + } } #[cfg(test)] @@ -286,7 +298,8 @@ mod tests { let config = config_from_string(text); let ui = Ui::with_config(&config).unwrap(); let settings = UserSettings::from_config(config); - DiffEditor::from_settings(&ui, &settings).map(|editor| editor.tool) + DiffEditor::from_settings(&ui, &settings, GitIgnoreFile::empty()) + .map(|editor| editor.tool) }; // Default diff --git a/lib/src/settings.rs b/lib/src/settings.rs index 6b4653f223..8ab207bcc4 100644 --- a/lib/src/settings.rs +++ b/lib/src/settings.rs @@ -228,10 +228,6 @@ impl UserSettings { .unwrap_or(false) } - pub fn diff_instructions(&self) -> bool { - self.config.get_bool("ui.diff-instructions").unwrap_or(true) - } - pub fn config(&self) -> &config::Config { &self.config }