From d6c1b0bc9c7b05e6d28604bed389db4ddb81ae88 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Sun, 30 Oct 2022 19:41:19 -0700 Subject: [PATCH] `jj resolve`: option to parse conflict markers + tests If this new option is not specified, we start with empty output file and trust the merge tool did a complete merge no matter what the file contains. Includes tests. --- src/diff_edit.rs | 63 +++++++++++----- tests/test_resolve_command.rs | 138 +++++++++++++++++++++++++++++++++- 2 files changed, 182 insertions(+), 19 deletions(-) diff --git a/src/diff_edit.rs b/src/diff_edit.rs index 721e26f9d..96f7bd609 100644 --- a/src/diff_edit.rs +++ b/src/diff_edit.rs @@ -24,6 +24,7 @@ use itertools::Itertools; use jujutsu_lib::backend::{TreeId, TreeValue}; use jujutsu_lib::conflicts::{ describe_conflict, extract_file_conflict_as_single_hunk, materialize_merge_result, + update_conflict_from_content, }; use jujutsu_lib::gitignore::GitIgnoreFile; use jujutsu_lib::matchers::EverythingMatcher; @@ -147,6 +148,8 @@ fn set_readonly_recursively(path: &Path) -> Result<(), std::io::Error> { } } +// TODO: Rearrange the functions. This should be on the bottom, options should +// be on the top. pub fn run_mergetool( ui: &mut Ui, tree: &Tree, @@ -182,16 +185,20 @@ pub fn run_mergetool( }); }; - let mut materialized_conflict: Vec = vec![]; - materialize_merge_result(&content, &mut materialized_conflict) - .expect("Writing to an in-memory buffer should never fail"); - let materialized_conflict = materialized_conflict; - + let editor = get_merge_tool_from_settings(ui)?; + let initial_output_content: Vec = if editor.merge_tool_edits_conflict_markers { + let mut materialized_conflict = vec![]; + materialize_merge_result(&content, &mut materialized_conflict) + .expect("Writing to an in-memory buffer should never fail"); + materialized_conflict + } else { + vec![] + }; let files: HashMap<&str, _> = maplit::hashmap! { "base" => content.removes.pop().unwrap_or_default(), "right" => content.adds.pop().unwrap_or_default(), "left" => content.adds.pop().unwrap_or_default(), - "output" => materialized_conflict.clone(), + "output" => initial_output_content.clone(), }; let temp_dir = tempfile::Builder::new() @@ -219,7 +226,6 @@ pub fn run_mergetool( .collect(); let paths = paths?; - let editor = get_merge_tool_from_settings(ui)?; let args = interpolate_mergetool_filename_patterns(&editor.merge_args, &paths); let args_str = args .iter() @@ -242,21 +248,32 @@ pub fn run_mergetool( } let output_file_contents: Vec = std::fs::read(paths.get("output").unwrap())?; - if output_file_contents.is_empty() || output_file_contents == materialized_conflict { + if output_file_contents.is_empty() || output_file_contents == initial_output_content { return Err(ConflictResolveError::EmptyOrUnchanged); } - // TODO: parse any remaining conflicts (done in followup commit) - let new_file_id = tree - .store() - .write_file(repo_path, &mut File::open(paths.get("output").unwrap())?)?; - let mut tree_builder = tree.store().tree_builder(tree.id().clone()); - tree_builder.set( - repo_path.clone(), + + let mut new_tree_value: Option = None; + if editor.merge_tool_edits_conflict_markers { + if let Some(new_conflict_id) = update_conflict_from_content( + tree.store(), + repo_path, + &conflict_id, + output_file_contents.as_slice(), + )? { + new_tree_value = Some(TreeValue::Conflict(new_conflict_id)); + } + } + let new_tree_value = new_tree_value.unwrap_or({ + let new_file_id = tree + .store() + .write_file(repo_path, &mut File::open(paths.get("output").unwrap())?)?; TreeValue::File { id: new_file_id, executable: false, - }, - ); + } + }); + let mut tree_builder = tree.store().tree_builder(tree.id().clone()); + tree_builder.set(repo_path.clone(), new_tree_value); Ok(tree_builder.write_tree()) } @@ -380,6 +397,17 @@ struct MergeTool { /// strings to be substituted. #[serde(default)] pub merge_args: Vec, + /// If false (default), the `$output` file starts out empty and is accepted + /// as a full conflict resolution as-is by `jj` after the merge tool is + /// done with it. If true, the `$output` file starts out with the + /// contents of the conflict, with JJ's conflict markers. After the + /// merge tool is done, any remaining conflict markers in the + /// file parsed and taken to mean that the conflict was only partially + /// resolved. + // TODO: Instead of a boolean, this could denote the flavor of conflict markers to put in + // the file (`jj` or `diff3` for example). + #[serde(default)] + pub merge_tool_edits_conflict_markers: bool, } impl MergeTool { @@ -388,6 +416,7 @@ impl MergeTool { program: program.to_owned(), edit_args: vec![], merge_args: vec![], + merge_tool_edits_conflict_markers: false, } } } diff --git a/tests/test_resolve_command.rs b/tests/test_resolve_command.rs index c0351b27a..bbe95e6a1 100644 --- a/tests/test_resolve_command.rs +++ b/tests/test_resolve_command.rs @@ -75,8 +75,35 @@ fn test_resolution() { "###); let editor_script = test_env.set_up_fake_editor(); + // Check that output file starts out empty and resolve the conflict + std::fs::write(&editor_script, "expect\n\0write\nresolution\n").unwrap(); + test_env.jj_cmd_success(&repo_path, &["resolve", "file"]); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "-r", "conflict"]), + @r###" + Resolved conflict in file: + 1 1: <<<<<<>>>>>> + "###); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["status"]), + @r###" + Parent commit: 77c5ed9eda54 a + Working copy : 665f83829a6a conflict + Working copy changes: + M file + "###); + + // Check that the output file starts with conflict markers if + // `merge-tool-edits-conflict-markers=true` + test_env.jj_cmd_success(&repo_path, &["undo"]); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "-r", "conflict"]), + @""); std::fs::write( - editor_script, + &editor_script, "expect <<<<<<< %%%%%%% @@ -90,7 +117,15 @@ resolution ", ) .unwrap(); - test_env.jj_cmd_success(&repo_path, &["resolve", "file"]); + test_env.jj_cmd_success( + &repo_path, + &[ + "resolve", + "--config-toml", + "merge-tools.fake-editor.merge-tool-edits-conflict-markers=true", + "file", + ], + ); insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "-r", "conflict"]), @r###" Resolved conflict in file: @@ -102,6 +137,105 @@ resolution 6 : b 7 : >>>>>>> "###); + + // Check that if merge tool leaves conflict markers in output file and + // `merge-tool-edits-conflict-markers=true`, these markers are properly parsed. + test_env.jj_cmd_success(&repo_path, &["undo"]); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "-r", "conflict"]), + @""); + std::fs::write( + &editor_script, + "expect +<<<<<<< +%%%%%%% +-base ++a ++++++++ +b +>>>>>>> +\0write +<<<<<<< +%%%%%%% +-some ++fake ++++++++ +conflict +>>>>>>> +", + ) + .unwrap(); + test_env.jj_cmd_success( + &repo_path, + &[ + "resolve", + "--config-toml", + "merge-tools.fake-editor.merge-tool-edits-conflict-markers=true", + "file", + ], + ); + // Note the "Modified" below + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "-r", "conflict"]), + @r###" + Modified conflict in file: + 1 1: <<<<<<< + 2 2: %%%%%%% + 3 3: -basesome + 4 4: +afake + 5 5: +++++++ + 6 6: bconflict + 7 7: >>>>>>> + "###); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["status"]), + @r###" + Parent commit: 77c5ed9eda54 a + Working copy : cbd3d65d2612 conflict + Working copy changes: + M file + There are unresolved conflicts at these paths: + file + "###); + + // Check that if merge tool leaves conflict markers in output file but + // `merge-tool-edits-conflict-markers=false` or is not specified, + // `jj` considers the conflict resolved. + test_env.jj_cmd_success(&repo_path, &["undo"]); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "-r", "conflict"]), + @""); + std::fs::write( + &editor_script, + "expect +\0write +<<<<<<< +%%%%%%% +-some ++fake ++++++++ +conflict +>>>>>>> +", + ) + .unwrap(); + test_env.jj_cmd_success(&repo_path, &["resolve", "file"]); + // Note the "Resolved" below + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "-r", "conflict"]), + @r###" + Resolved conflict in file: + 1 1: <<<<<<< + 2 2: %%%%%%% + 3 3: -basesome + 4 4: +afake + 5 5: +++++++ + 6 6: bconflict + 7 7: >>>>>>> + "###); + // No "there are unresolved conflicts..." message below + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["status"]), + @r###" + Parent commit: 77c5ed9eda54 a + Working copy : d5b735448648 conflict + Working copy changes: + M file + "###); } fn check_resolve_produces_input_file(