From c9891d36e6ad26fa565a4f611ff50230815002dc Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 29 Feb 2024 17:52:18 +0900 Subject: [PATCH 1/6] merge_tools: don't enable fsmonitor (and file size limit) in temporary snapshot Because the snapshot directory is removed at the end of the function, it doesn't make sense to enable watchman in it. The max_new_file_size parameter might be somewhat useful, but it's unlikely that the temporary directory contains gigantic node_modules tree for example. OTOH, the base_ignores matters since it may contain common ignore patterns like *~. This eliminates most of the UserSettings dependencies from this function. --- cli/src/merge_tools/external.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index af588a1da6..f9c59e2b0d 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -10,6 +10,7 @@ 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; @@ -516,14 +517,15 @@ diff editing in mind and be a little inaccurate. 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()) } From 53f49c08f83bcc8c4c6728c543d67debd4dd0822 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 29 Feb 2024 18:11:11 +0900 Subject: [PATCH 2/6] merge_tools: take diff editor instruction as Option<&str> This clarifies that the instructions text can be omitted. All callers appear to pass non-empty instructions, though. --- cli/src/cli_util.rs | 4 ++-- cli/src/commands/commit.rs | 2 +- cli/src/commands/diffedit.rs | 8 +++++++- cli/src/commands/move.rs | 2 +- cli/src/commands/split.rs | 2 +- cli/src/commands/squash.rs | 2 +- cli/src/commands/unsquash.rs | 2 +- cli/src/merge_tools/external.rs | 15 +++++++++------ cli/src/merge_tools/mod.rs | 2 +- 9 files changed, 24 insertions(+), 15 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 1acc9295ec..5b4067791d 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -1751,7 +1751,7 @@ impl WorkspaceCommandTransaction<'_> { left_tree: &MergedTree, right_tree: &MergedTree, matcher: &dyn Matcher, - instructions: &str, + instructions: Option<&str>, ) -> Result { let base_ignores = self.helper.base_ignores()?; let settings = &self.helper.settings; @@ -1772,7 +1772,7 @@ impl WorkspaceCommandTransaction<'_> { left_tree: &MergedTree, right_tree: &MergedTree, matcher: &dyn Matcher, - instructions: &str, + instructions: Option<&str>, interactive: bool, ) -> Result { if interactive { diff --git a/cli/src/commands/commit.rs b/cli/src/commands/commit.rs index 458e8593b3..4e7effec98 100644 --- a/cli/src/commands/commit.rs +++ b/cli/src/commands/commit.rs @@ -66,7 +66,7 @@ new working-copy commit. &base_tree, &commit.tree()?, matcher.as_ref(), - &instructions, + Some(&instructions), args.interactive, )?; let middle_tree = tx.repo().store().get_root_tree(&tree_id)?; diff --git a/cli/src/commands/diffedit.rs b/cli/src/commands/diffedit.rs index d077237fd9..cb1bad77b6 100644 --- a/cli/src/commands/diffedit.rs +++ b/cli/src/commands/diffedit.rs @@ -90,7 +90,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( + ui, + &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 2a7ceb1bc3..9d3b5528e5 100644 --- a/cli/src/commands/move.rs +++ b/cli/src/commands/move.rs @@ -93,7 +93,7 @@ from the source will be moved into the destination. &parent_tree, &source_tree, matcher.as_ref(), - &instructions, + Some(&instructions), args.interactive, )?; if args.interactive && new_parent_tree_id == parent_tree.id() { diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index 7b1563a761..7f342fad96 100644 --- a/cli/src/commands/split.rs +++ b/cli/src/commands/split.rs @@ -81,7 +81,7 @@ don't make any changes, then the operation will be aborted. &base_tree, &end_tree, matcher.as_ref(), - &instructions, + Some(&instructions), interactive, )?; if &selected_tree_id == commit.tree_id() && interactive { diff --git a/cli/src/commands/squash.rs b/cli/src/commands/squash.rs index d249048a80..94863a3148 100644 --- a/cli/src/commands/squash.rs +++ b/cli/src/commands/squash.rs @@ -90,7 +90,7 @@ from the source will be moved into the parent. &parent_tree, &tree, matcher.as_ref(), - &instructions, + Some(&instructions), args.interactive, )?; if &new_parent_tree_id == parent.tree_id() { diff --git a/cli/src/commands/unsquash.rs b/cli/src/commands/unsquash.rs index 4ad63261d7..a1ac8e0d07 100644 --- a/cli/src/commands/unsquash.rs +++ b/cli/src/commands/unsquash.rs @@ -86,7 +86,7 @@ aborted. &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")); diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index f9c59e2b0d..15c58d5797 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -427,7 +427,7 @@ pub fn edit_diff_external( left_tree: &MergedTree, right_tree: &MergedTree, matcher: &dyn Matcher, - instructions: &str, + instructions: Option<&str>, base_ignores: Arc, settings: &UserSettings, ) -> Result { @@ -452,10 +452,13 @@ 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 settings.diff_instructions() && !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 { @@ -513,7 +516,7 @@ 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(); } diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index a6c3310e80..f686b9859c 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -132,7 +132,7 @@ pub fn edit_diff( left_tree: &MergedTree, right_tree: &MergedTree, matcher: &dyn Matcher, - instructions: &str, + instructions: Option<&str>, base_ignores: Arc, settings: &UserSettings, ) -> Result { From bdf5eed1142ac0f71f085ea746209fa2ac8de010 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 29 Feb 2024 18:25:15 +0900 Subject: [PATCH 3/6] merge_tools: process "ui.diff-instructions" option by caller This gets rid of the last UserSettings dependency from edit_diff_external(). I'm going to remove it from edit_diff() too, and let callers pass a preconfigured MergeTool struct instead. These changes will make it easier to add --tool= argument #2575. --- cli/src/cli_util.rs | 5 +++++ cli/src/merge_tools/external.rs | 5 +---- cli/src/merge_tools/mod.rs | 1 - cli/tests/test_diffedit_command.rs | 30 +++++++++++++++++++++++------- 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 5b4067791d..2d12938922 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -1755,6 +1755,11 @@ impl WorkspaceCommandTransaction<'_> { ) -> Result { let base_ignores = self.helper.base_ignores()?; let settings = &self.helper.settings; + let instructions = if settings.diff_instructions() { + instructions + } else { + None + }; Ok(crate::merge_tools::edit_diff( ui, left_tree, diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index 15c58d5797..ddf65ab57e 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -17,7 +17,6 @@ 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; @@ -429,7 +428,6 @@ pub fn edit_diff_external( matcher: &dyn Matcher, instructions: Option<&str>, base_ignores: Arc, - settings: &UserSettings, ) -> Result { let got_output_field = find_all_variables(&editor.edit_args).contains(&"output"); let store = left_tree.store(); @@ -452,8 +450,7 @@ 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 = if settings.diff_instructions() && !instructions_path_to_cleanup.exists() - { + let add_instructions = if !instructions_path_to_cleanup.exists() { instructions } else { None diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index f686b9859c..98c4278200 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -150,7 +150,6 @@ pub fn edit_diff( matcher, instructions, base_ignores, - settings, ), } } diff --git a/cli/tests/test_diffedit_command.rs b/cli/tests/test_diffedit_command.rs index 383f7a82f9..69619b49f7 100644 --- a/cli/tests/test_diffedit_command.rs +++ b/cli/tests/test_diffedit_command.rs @@ -47,6 +47,22 @@ fn test_diffedit() { M file2 "###); + // Try again with ui.diff-instructions=false + std::fs::write(&edit_script, "files-before file1 file2\0files-after file2").unwrap(); + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &["diffedit", "--config-toml=ui.diff-instructions=false"], + ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Nothing changed. + "###); + let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]); + insta::assert_snapshot!(stdout, @r###" + D file1 + M file2 + "###); + // Nothing happens if the diff-editor exits with an error std::fs::write(&edit_script, "rm file2\0fail").unwrap(); insta::assert_snapshot!(&test_env.jj_cmd_failure(&repo_path, &["diffedit"]), @r###" @@ -64,8 +80,8 @@ fn test_diffedit() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["diffedit"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - Created kkmpptxz 1930da4a (no description set) - Working copy now at: kkmpptxz 1930da4a (no description set) + Created kkmpptxz ae84b067 (no description set) + Working copy now at: kkmpptxz ae84b067 (no description set) Parent commit : rlvkpnrz 613028a4 (no description set) Added 0 files, modified 1 files, removed 0 files "###); @@ -80,10 +96,10 @@ fn test_diffedit() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["diffedit", "-r", "@-"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - Created rlvkpnrz c03ae967 (no description set) + Created rlvkpnrz 51a4f74c (no description set) Rebased 1 descendant commits - Working copy now at: kkmpptxz 2a4dc204 (no description set) - Parent commit : rlvkpnrz c03ae967 (no description set) + Working copy now at: kkmpptxz 574af856 (no description set) + Parent commit : rlvkpnrz 51a4f74c (no description set) Added 0 files, modified 1 files, removed 0 files "###); let contents = String::from_utf8(std::fs::read(repo_path.join("file3")).unwrap()).unwrap(); @@ -101,8 +117,8 @@ fn test_diffedit() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["diffedit", "--from", "@--"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - Created kkmpptxz 15f2c966 (no description set) - Working copy now at: kkmpptxz 15f2c966 (no description set) + Created kkmpptxz cd638328 (no description set) + Working copy now at: kkmpptxz cd638328 (no description set) Parent commit : rlvkpnrz 613028a4 (no description set) Added 0 files, modified 0 files, removed 1 files "###); From a6ad59d96fa171ebe5489629e6404d91d3868863 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 29 Feb 2024 18:33:44 +0900 Subject: [PATCH 4/6] merge_tools: introduce Diff/MergeEditor newtypes I'm going to make them be loaded by caller, and these newtypes will provide extra compile-time safety (plus nicer API to be added later.) The error types will be cleaned up later patches. --- cli/src/merge_tools/mod.rs | 91 ++++++++++++++++++++++---------------- 1 file changed, 54 insertions(+), 37 deletions(-) diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index 98c4278200..947621a908 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -115,7 +115,7 @@ pub fn run_mergetool( }; let content = extract_as_single_hunk(&file_merge, tree.store(), repo_path).block_on(); - let editor = get_merge_tool_from_settings(ui, settings)?; + let editor = MergeEditor::from_settings(ui, settings)?.tool; match editor { MergeTool::Builtin => { let tree_id = edit_merge_builtin(tree, repo_path, content).map_err(Box::new)?; @@ -137,7 +137,7 @@ pub fn edit_diff( settings: &UserSettings, ) -> Result { // Start a diff editor on the two directories. - let editor = get_diff_editor_from_settings(ui, settings)?; + let editor = DiffEditor::from_settings(ui, settings)?.tool; match editor { MergeTool::Builtin => { let tree_id = edit_diff_builtin(left_tree, right_tree, matcher).map_err(Box::new)?; @@ -215,38 +215,51 @@ pub fn get_external_tool_config( } } -fn get_diff_editor_from_settings( - ui: &Ui, - settings: &UserSettings, -) -> Result { - let args = editor_args_from_settings(ui, settings, "ui.diff-editor")?; - let editor = if let CommandNameAndArgs::String(name) = &args { - get_tool_config(settings, name)? - } else { - None - } - .unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_edit_args(&args))); - Ok(editor) +/// Configured diff editor. +#[derive(Clone, Debug)] +pub struct DiffEditor { + tool: MergeTool, } -fn get_merge_tool_from_settings( - ui: &Ui, - settings: &UserSettings, -) -> Result { - let args = editor_args_from_settings(ui, settings, "ui.merge-editor")?; - let mergetool = if let CommandNameAndArgs::String(name) = &args { - get_tool_config(settings, name)? - } else { - None +impl DiffEditor { + /// Loads the default diff editor from the settings. + // TODO: extract tool configuration error from DiffEditError + pub fn from_settings(ui: &Ui, settings: &UserSettings) -> 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).map_err(ExternalToolError::Config)? + } else { + None + } + .unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_edit_args(&args))); + Ok(DiffEditor { tool }) } - .unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_merge_args(&args))); - match mergetool { - MergeTool::External(mergetool) if mergetool.merge_args.is_empty() => { - Err(ExternalToolError::MergeArgsNotConfigured { +} + +/// Configured 3-way merge editor. +#[derive(Clone, Debug)] +pub struct MergeEditor { + tool: MergeTool, +} + +impl MergeEditor { + /// Loads the default 3-way merge editor from the settings. + // TODO: extract tool configuration error from ConflictResolveError + pub fn from_settings(ui: &Ui, settings: &UserSettings) -> Result { + let args = editor_args_from_settings(ui, settings, "ui.merge-editor")?; + let tool = if let CommandNameAndArgs::String(name) = &args { + get_tool_config(settings, name).map_err(ExternalToolError::Config)? + } else { + None + } + .unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_merge_args(&args))); + if matches!(&tool, MergeTool::External(mergetool) if mergetool.merge_args.is_empty()) { + return Err(ExternalToolError::MergeArgsNotConfigured { tool_name: args.to_string(), - }) + } + .into()); } - mergetool => Ok(mergetool), + Ok(MergeEditor { tool }) } } @@ -269,7 +282,7 @@ mod tests { let config = config_from_string(text); let ui = Ui::with_config(&config).unwrap(); let settings = UserSettings::from_config(config); - get_diff_editor_from_settings(&ui, &settings) + DiffEditor::from_settings(&ui, &settings).map(|editor| editor.tool) }; // Default @@ -418,7 +431,7 @@ mod tests { let config = config_from_string(text); let ui = Ui::with_config(&config).unwrap(); let settings = UserSettings::from_config(config); - get_merge_tool_from_settings(&ui, &settings) + MergeEditor::from_settings(&ui, &settings).map(|editor| editor.tool) }; // Default @@ -426,9 +439,11 @@ mod tests { // Just program name insta::assert_debug_snapshot!(get(r#"ui.merge-editor = "my-merge""#).unwrap_err(), @r###" - MergeArgsNotConfigured { - tool_name: "my-merge", - } + ExternalTool( + MergeArgsNotConfigured { + tool_name: "my-merge", + }, + ) "###); // String args @@ -516,9 +531,11 @@ mod tests { // List args should never be a merge-tools key insta::assert_debug_snapshot!( get(r#"ui.merge-editor = ["meld"]"#).unwrap_err(), @r###" - MergeArgsNotConfigured { - tool_name: "meld", - } + ExternalTool( + MergeArgsNotConfigured { + tool_name: "meld", + }, + ) "###); // Invalid type From 0a9c2d3308571d8d924699441270cf6d2b898e65 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 29 Feb 2024 18:51:04 +0900 Subject: [PATCH 5/6] merge_tools: load diff/merge editor settings by caller This moves the config loading closer to CLI args where --tool= 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 | 42 ++++++++++++++++++++----------- cli/src/commands/commit.rs | 8 ++++-- cli/src/commands/diffedit.rs | 3 ++- cli/src/commands/move.rs | 10 +++++--- cli/src/commands/resolve.rs | 3 ++- cli/src/commands/split.rs | 11 +++++--- cli/src/commands/squash.rs | 10 +++++--- cli/src/commands/unsquash.rs | 9 +++++-- cli/src/merge_tools/external.rs | 2 +- cli/src/merge_tools/mod.rs | 18 ++++++------- cli/tests/test_resolve_command.rs | 3 +++ 11 files changed, 76 insertions(+), 43 deletions(-) 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": From a50acea7e0251f5b9878271338a99d36faf3f396 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 29 Feb 2024 20:05:40 +0900 Subject: [PATCH 6/6] merge_tools: extract configuration error to separate type write!(ui.hint(), ..) error is suppressed because it seemed weird if the configuration error had io::Error variant. The write error isn't important anyway. --- cli/src/cli_util.rs | 11 ++++++-- cli/src/merge_tools/external.rs | 8 ------ cli/src/merge_tools/mod.rs | 46 ++++++++++++++++++--------------- 3 files changed, 34 insertions(+), 31 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 4b940fdd73..13166011f0 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -88,6 +88,7 @@ use crate::git_util::{ }; use crate::merge_tools::{ ConflictResolveError, DiffEditError, DiffEditor, DiffGenerateError, MergeEditor, + MergeToolConfigError, }; use crate::template_parser::{TemplateAliasesMap, TemplateParseError, TemplateParseErrorKind}; use crate::templater::Template; @@ -353,6 +354,12 @@ impl From for CommandError { } } +impl From for CommandError { + fn from(err: MergeToolConfigError) -> Self { + user_error_with_message("Failed to load tool configuration", err) + } +} + impl From for CommandError { fn from(err: git2::Error) -> Self { user_error_with_message("Git operation failed", err) @@ -1110,13 +1117,13 @@ impl WorkspaceCommandHelper { /// Loads diff editor from the settings. // TODO: override settings by --tool= option (#2575) - pub fn diff_editor(&self, ui: &Ui) -> Result { + 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 { + pub fn merge_editor(&self, ui: &Ui) -> Result { MergeEditor::from_settings(ui, &self.settings) } diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index e009b8afc9..d3f9ce2850 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -5,7 +5,6 @@ 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}; @@ -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)" diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index 1089acc31c..886fd817e0 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -150,6 +150,17 @@ pub fn edit_diff( } } +#[derive(Debug, Error)] +pub enum MergeToolConfigError { + #[error(transparent)] + 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 }, +} + #[derive(Clone, Debug, Eq, PartialEq)] pub enum MergeTool { Builtin, @@ -161,7 +172,7 @@ fn editor_args_from_settings( ui: &Ui, settings: &UserSettings, key: &str, -) -> Result { +) -> Result { // TODO: Make this configuration have a table of possible editors and detect the // best one here. if let Some(args) = settings.config().get(key).optional()? { @@ -173,7 +184,7 @@ fn editor_args_from_settings( "Using default editor '{default_editor}'; run `jj config set --user {key} :builtin` \ to disable this message." ) - .map_err(ExternalToolError::Io)?; + .ok(); Ok(default_editor.into()) } } @@ -219,11 +230,10 @@ pub struct DiffEditor { impl DiffEditor { /// Loads the default diff editor from the settings. - // TODO: extract tool configuration error from DiffEditError - pub fn from_settings(ui: &Ui, settings: &UserSettings) -> Result { + pub fn from_settings(ui: &Ui, settings: &UserSettings) -> 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).map_err(ExternalToolError::Config)? + get_tool_config(settings, name)? } else { None } @@ -240,20 +250,18 @@ pub struct MergeEditor { impl MergeEditor { /// Loads the default 3-way merge editor from the settings. - // TODO: extract tool configuration error from ConflictResolveError - pub fn from_settings(ui: &Ui, settings: &UserSettings) -> Result { + pub fn from_settings(ui: &Ui, settings: &UserSettings) -> Result { let args = editor_args_from_settings(ui, settings, "ui.merge-editor")?; let tool = if let CommandNameAndArgs::String(name) = &args { - get_tool_config(settings, name).map_err(ExternalToolError::Config)? + get_tool_config(settings, name)? } else { None } .unwrap_or_else(|| MergeTool::External(ExternalMergeTool::with_merge_args(&args))); if matches!(&tool, MergeTool::External(mergetool) if mergetool.merge_args.is_empty()) { - return Err(ExternalToolError::MergeArgsNotConfigured { + return Err(MergeToolConfigError::MergeArgsNotConfigured { tool_name: args.to_string(), - } - .into()); + }); } Ok(MergeEditor { tool }) } @@ -435,11 +443,9 @@ mod tests { // Just program name insta::assert_debug_snapshot!(get(r#"ui.merge-editor = "my-merge""#).unwrap_err(), @r###" - ExternalTool( - MergeArgsNotConfigured { - tool_name: "my-merge", - }, - ) + MergeArgsNotConfigured { + tool_name: "my-merge", + } "###); // String args @@ -527,11 +533,9 @@ mod tests { // List args should never be a merge-tools key insta::assert_debug_snapshot!( get(r#"ui.merge-editor = ["meld"]"#).unwrap_err(), @r###" - ExternalTool( - MergeArgsNotConfigured { - tool_name: "meld", - }, - ) + MergeArgsNotConfigured { + tool_name: "meld", + } "###); // Invalid type