From f5426d2eebc05f5453e26a80654083a800dfd435 Mon Sep 17 00:00:00 2001 From: Waleed Khan Date: Tue, 29 Aug 2023 23:13:35 +0200 Subject: [PATCH] merge_tools: create builtin merge editor --- cli/src/cli_util.rs | 2 +- cli/src/merge_tools/builtin.rs | 224 ++++++++++++++++++++++++++++++ cli/src/merge_tools/mod.rs | 8 +- cli/tests/test_resolve_command.rs | 10 +- 4 files changed, 236 insertions(+), 8 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index c3f32920df..1e9e89d9e3 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -248,7 +248,7 @@ impl From for CommandError { impl From for CommandError { fn from(err: ConflictResolveError) -> Self { - user_error(format!("Failed to use external tool to resolve: {err}")) + user_error(format!("Failed to resolve conflicts: {err}")) } } diff --git a/cli/src/merge_tools/builtin.rs b/cli/src/merge_tools/builtin.rs index 8fb3c515ae..8a30ad20af 100644 --- a/cli/src/merge_tools/builtin.rs +++ b/cli/src/merge_tools/builtin.rs @@ -5,6 +5,7 @@ use std::sync::Arc; use itertools::Itertools; use jj_lib::backend::{BackendError, FileId, MergedTreeId, ObjectId, TreeValue}; use jj_lib::diff::{find_line_ranges, Diff, DiffHunk}; +use jj_lib::files::{self, ContentHunk, MergeResult}; use jj_lib::matchers::EverythingMatcher; use jj_lib::merge::Merge; use jj_lib::merged_tree::{MergedTree, MergedTreeBuilder}; @@ -424,8 +425,133 @@ pub fn edit_diff_builtin( Ok(tree_id) } +fn make_merge_sections( + merge_result: MergeResult, + file_merge: Merge>, +) -> Result>, BuiltinToolError> { + let mut sections = Vec::new(); + match merge_result { + MergeResult::Resolved(ContentHunk(buf)) => { + let right_id = file_merge.adds().get(1).and_then(|side| side.clone()); + let contents = buf_to_file_contents(right_id.map(|id| id.hex()), buf); + let section = match contents { + FileContents::Absent => None, + FileContents::Text { + contents, + hash: _, + num_bytes: _, + } => Some(scm_record::Section::Unchanged { + lines: contents + .split_inclusive('\n') + .map(|line| Cow::Owned(line.to_owned())) + .collect(), + }), + FileContents::Binary { hash, num_bytes } => Some(scm_record::Section::Binary { + is_checked: false, + old_description: None, + new_description: Some(Cow::Owned(describe_binary(hash.as_deref(), num_bytes))), + }), + }; + if let Some(section) = section { + sections.push(section); + } + } + MergeResult::Conflict(hunks) => { + for hunk in hunks { + let (removes, adds) = hunk.take(); + sections.push(match adds.as_slice() { + [ContentHunk(contents)] => { + let contents = std::str::from_utf8(contents).map_err(|err| { + BuiltinToolError::DecodeUtf8 { + source: err, + item: "unchanged hunk", + } + })?; + scm_record::Section::Unchanged { + lines: contents + .split_inclusive('\n') + .map(|line| Cow::Owned(line.to_owned())) + .collect(), + } + } + _ => { + let lines: Vec = adds + .into_iter() + .interleave(removes) + .zip( + [ + scm_record::ChangeType::Added, + scm_record::ChangeType::Removed, + ] + .into_iter() + .cycle(), + ) + .map(|(contents, change_type)| -> Result<_, BuiltinToolError> { + let ContentHunk(contents) = contents; + let contents = std::str::from_utf8(&contents).map_err(|err| { + BuiltinToolError::DecodeUtf8 { + source: err, + item: "conflicting hunk", + } + })?; + let changed_lines = + make_section_changed_lines(contents, change_type); + Ok(changed_lines) + }) + .flatten_ok() + .try_collect()?; + scm_record::Section::Changed { lines } + } + }); + } + } + } + Ok(sections) +} + +pub fn edit_merge_builtin( + tree: &MergedTree, + path: &RepoPath, + file_merge: Merge>, + content: Merge, +) -> Result { + let (removes, adds) = content.take(); + let removed_slices = removes + .iter() + .map(|ContentHunk(v)| v.as_slice()) + .collect_vec(); + let added_slices = adds.iter().map(|ContentHunk(v)| v.as_slice()).collect_vec(); + let merge_result = files::merge(&removed_slices, &added_slices); + + let sections = make_merge_sections(merge_result, file_merge)?; + let recorder = scm_record::Recorder::new( + scm_record::RecordState { + is_read_only: false, + files: vec![scm_record::File { + old_path: None, + path: Cow::Owned(path.to_fs_path(Path::new(""))), + file_mode: None, + sections, + }], + }, + scm_record::EventSource::Crossterm, + ); + let state = recorder.run()?; + + let file = state.files.into_iter().exactly_one().unwrap(); + apply_diff_builtin( + tree.store().clone(), + tree, + tree, + vec![path.clone()], + &[file], + ) + .map_err(BuiltinToolError::BackendError) +} + #[cfg(test)] mod tests { + use jj_lib::conflicts::extract_as_single_hunk; use jj_lib::repo::Repo; use testutils::TestRepo; @@ -572,4 +698,102 @@ mod tests { "all-changes tree was different", ); } + + #[test] + fn test_make_merge_sections() { + let test_repo = TestRepo::init(false); + let store = test_repo.repo.store(); + + let path = RepoPath::from_internal_string("file"); + let base_tree = testutils::create_tree( + &test_repo.repo, + &[(&path, "base 1\nbase 2\nbase 3\nbase 4\nbase 5\n")], + ); + let left_tree = testutils::create_tree( + &test_repo.repo, + &[(&path, "left 1\nbase 2\nbase 3\nbase 4\nleft 5\n")], + ); + let right_tree = testutils::create_tree( + &test_repo.repo, + &[(&path, "right 1\nbase 2\nbase 3\nbase 4\nright 5\n")], + ); + + fn to_file_id(tree_value: Merge>) -> Option { + match tree_value.into_resolved() { + Ok(Some(TreeValue::File { id, executable: _ })) => Some(id.clone()), + other => { + panic!("merge should have been a FileId: {other:?}") + } + } + } + let merge = Merge::new( + vec![to_file_id(base_tree.path_value(&path))], + vec![ + to_file_id(left_tree.path_value(&path)), + to_file_id(right_tree.path_value(&path)), + ], + ); + let content = extract_as_single_hunk(&merge, store, &path); + let removes = content + .removes() + .iter() + .map(|ContentHunk(buf)| buf.as_slice()) + .collect_vec(); + let adds = content + .adds() + .iter() + .map(|ContentHunk(buf)| buf.as_slice()) + .collect_vec(); + let merge_result = files::merge(&removes, &adds); + let sections = make_merge_sections(merge_result, merge).unwrap(); + insta::assert_debug_snapshot!(sections, @r###" + [ + Changed { + lines: [ + SectionChangedLine { + is_checked: false, + change_type: Added, + line: "left 1\n", + }, + SectionChangedLine { + is_checked: false, + change_type: Removed, + line: "base 1\n", + }, + SectionChangedLine { + is_checked: false, + change_type: Added, + line: "right 1\n", + }, + ], + }, + Unchanged { + lines: [ + "base 2\n", + "base 3\n", + "base 4\n", + ], + }, + Changed { + lines: [ + SectionChangedLine { + is_checked: false, + change_type: Added, + line: "left 5\n", + }, + SectionChangedLine { + is_checked: false, + change_type: Removed, + line: "base 5\n", + }, + SectionChangedLine { + is_checked: false, + change_type: Added, + line: "right 5\n", + }, + ], + }, + ] + "###); + } } diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index 2a6544621c..3214aad5bb 100644 --- a/cli/src/merge_tools/mod.rs +++ b/cli/src/merge_tools/mod.rs @@ -27,7 +27,7 @@ use jj_lib::settings::{ConfigResultExt as _, UserSettings}; use jj_lib::working_copy::SnapshotError; use thiserror::Error; -use self::builtin::{edit_diff_builtin, BuiltinToolError}; +use self::builtin::{edit_diff_builtin, edit_merge_builtin, BuiltinToolError}; use self::external::{edit_diff_external, DiffCheckoutError, ExternalToolError}; pub use self::external::{generate_diff, ExternalMergeTool}; use crate::config::CommandNameAndArgs; @@ -113,7 +113,11 @@ pub fn run_mergetool( let editor = get_merge_tool_from_settings(ui, settings)?; match editor { - MergeTool::Builtin => unimplemented!("run_mergetool with builtin mergetool"), + MergeTool::Builtin => { + let tree_id = + edit_merge_builtin(tree, repo_path, file_merge, content).map_err(Box::new)?; + Ok(tree_id) + } MergeTool::External(editor) => external::run_mergetool_external( &editor, file_merge, content, repo_path, conflict, tree, ), diff --git a/cli/tests/test_resolve_command.rs b/cli/tests/test_resolve_command.rs index ab6722680a..c02db17022 100644 --- a/cli/tests/test_resolve_command.rs +++ b/cli/tests/test_resolve_command.rs @@ -288,8 +288,8 @@ fn check_resolve_produces_input_file( // in the future. See also https://github.com/mitsuhiko/insta/issues/313. assert_eq!( &test_env.jj_cmd_failure(repo_path, &["resolve", "--config-toml", &merge_arg_config]), - "Error: Failed to use external tool to resolve: The output file is either unchanged or \ - empty after the editor quit (run with --verbose to see the exact invocation).\n" + "Error: Failed to resolve conflicts: The output file is either unchanged or empty after \ + the editor quit (run with --verbose to see the exact invocation).\n" ); } @@ -397,7 +397,7 @@ fn test_too_many_parents() { let error = test_env.jj_cmd_failure(&repo_path, &["resolve"]); insta::assert_snapshot!(error, @r###" - Error: Failed to use external tool to resolve: The conflict at "file" has 3 sides. At most 2 sides are supported. + Error: Failed to resolve conflicts: The conflict at "file" has 3 sides. At most 2 sides are supported. "###); } @@ -472,7 +472,7 @@ fn test_file_vs_dir() { "###); let error = test_env.jj_cmd_failure(&repo_path, &["resolve"]); insta::assert_snapshot!(error, @r###" - Error: Failed to use external tool to resolve: Only conflicts that involve normal files (not symlinks, not executable, etc.) are supported. Conflict summary for "file": + Error: Failed to resolve conflicts: Only conflicts that involve normal files (not symlinks, not executable, etc.) are supported. Conflict summary for "file": Conflict: Removing file with id df967b96a579e45a18b8251732d16804b2e56a55 Adding file with id 78981922613b2afb6025042ff6bd878ac1994e85 @@ -526,7 +526,7 @@ fn test_description_with_dir_and_deletion() { "###); let error = test_env.jj_cmd_failure(&repo_path, &["resolve"]); insta::assert_snapshot!(error, @r###" - Error: Failed to use external tool to resolve: Only conflicts that involve normal files (not symlinks, not executable, etc.) are supported. Conflict summary for "file": + Error: Failed to resolve conflicts: Only conflicts that involve normal files (not symlinks, not executable, etc.) are supported. Conflict summary for "file": Conflict: Removing file with id df967b96a579e45a18b8251732d16804b2e56a55 Removing file with id df967b96a579e45a18b8251732d16804b2e56a55