From 542d09c6a99444f40b034dbf3f384268b3ffc3bb Mon Sep 17 00:00:00 2001 From: Scott Taylor Date: Sat, 21 Dec 2024 13:19:09 -0600 Subject: [PATCH] merge_tools: add "$marker_length" variable Git supports passing the conflict marker length to merge drivers using "%L". It would be useful if we also had a way to pass the marker length to merge tools, since it would allow Git merge drivers to be used with `jj resolve` in more cases. Without this variable, any merge tool that parses or generates conflict markers could fail on files which require conflict markers longer than 7 characters. https://git-scm.com/docs/gitattributes#_defining_a_custom_merge_driver --- CHANGELOG.md | 3 + cli/src/merge_tools/external.rs | 17 +- cli/testing/fake-editor.rs | 16 ++ cli/tests/test_resolve_command.rs | 259 ++++++++++++++++++++++++++++++ docs/config.md | 5 + 5 files changed, 293 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d458d702b..5eb2b46f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). conflicts to be materialized and parsed correctly in files which already contain lines that look like conflict markers. +* New `$marker_length` variable to allow merge tools to support longer conflict + markers (equivalent to "%L" for Git merge drivers). + ### Fixed bugs * The `$NO_COLOR` environment variable must now be non-empty to be respected. diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index 756e3d31b..303bfc75b 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -183,11 +183,13 @@ pub fn run_mergetool_external( .conflict_marker_style .unwrap_or(default_conflict_marker_style); + let uses_marker_length = find_all_variables(&editor.merge_args).contains(&"marker_length"); + // If the merge tool doesn't get conflict markers pre-populated in the output - // file, we should default to accepting MIN_CONFLICT_MARKER_LEN since the - // merge tool is unlikely to know about our rules for conflict marker length. - // In the future, we may want to add a "$markerLength" variable. - let conflict_marker_len = if editor.merge_tool_edits_conflict_markers { + // file and doesn't accept "$marker_length", then we should default to accepting + // MIN_CONFLICT_MARKER_LEN since the merge tool can't know about our rules for + // conflict marker length. + let conflict_marker_len = if editor.merge_tool_edits_conflict_markers || uses_marker_length { choose_materialized_conflict_marker_len(&content) } else { MIN_CONFLICT_MARKER_LEN @@ -220,7 +222,7 @@ pub fn run_mergetool_external( // resolving the root path ever makes sense. "".to_owned() }; - let paths: HashMap<&str, _> = files + let mut variables: HashMap<&str, _> = files .iter() .map(|(role, contents)| -> Result<_, ConflictResolveError> { let path = temp_dir.path().join(format!("{role}{suffix}")); @@ -237,9 +239,10 @@ pub fn run_mergetool_external( )) }) .try_collect()?; + variables.insert("marker_length", conflict_marker_len.to_string()); let mut cmd = Command::new(&editor.program); - cmd.args(interpolate_variables(&editor.merge_args, &paths)); + cmd.args(interpolate_variables(&editor.merge_args, &variables)); tracing::info!(?cmd, "Invoking the external merge tool:"); let exit_status = cmd .status() @@ -260,7 +263,7 @@ pub fn run_mergetool_external( } let output_file_contents: Vec = - std::fs::read(paths.get("output").unwrap()).map_err(ExternalToolError::Io)?; + std::fs::read(variables.get("output").unwrap()).map_err(ExternalToolError::Io)?; if output_file_contents.is_empty() || output_file_contents == initial_output_content { return Err(ConflictResolveError::EmptyOrUnchanged); } diff --git a/cli/testing/fake-editor.rs b/cli/testing/fake-editor.rs index 7e89019aa..0fa4cda2d 100644 --- a/cli/testing/fake-editor.rs +++ b/cli/testing/fake-editor.rs @@ -27,6 +27,8 @@ use itertools::Itertools; struct Args { /// Path to the file to edit file: PathBuf, + /// Other arguments to the editor + other_args: Vec, } fn main() { @@ -64,6 +66,20 @@ fn main() { exit(1) } } + ["expect-arg", index] => { + let index = index.parse::().unwrap(); + let Some(actual) = args.other_args.get(index) else { + eprintln!("fake-editor: Missing argument at index {index}.\n"); + eprintln!("EXPECTED: <{payload}>"); + exit(1) + }; + + if actual != payload { + eprintln!("fake-editor: Unexpected argument at index {index}.\n"); + eprintln!("EXPECTED: <{payload}>\nRECEIVED: <{actual}>"); + exit(1) + } + } ["write"] => { fs::write(&args.file, payload).unwrap_or_else(|_| { panic!("Failed to write file {}", args.file.to_str().unwrap()) diff --git a/cli/tests/test_resolve_command.rs b/cli/tests/test_resolve_command.rs index 06e14118c..7bae69d5b 100644 --- a/cli/tests/test_resolve_command.rs +++ b/cli/tests/test_resolve_command.rs @@ -1096,6 +1096,265 @@ fn test_resolve_conflicts_with_executable() { ); } +#[test] +fn test_resolve_long_conflict_markers() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + + // Makes it easier to read the diffs between conflicts + test_env.add_config("ui.conflict-marker-style = 'snapshot'"); + + // Create a conflict which requires long conflict markers to be materialized + create_commit( + &test_env, + &repo_path, + "base", + &[], + &[("file", "======= base\n")], + ); + create_commit( + &test_env, + &repo_path, + "a", + &["base"], + &[("file", "<<<<<<< a\n")], + ); + create_commit( + &test_env, + &repo_path, + "b", + &["base"], + &[("file", ">>>>>>> b\n")], + ); + create_commit(&test_env, &repo_path, "conflict", &["a", "b"], &[]); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["resolve", "--list"]), + @"file 2-sided conflict"); + insta::assert_snapshot!( + std::fs::read_to_string(repo_path.join("file")).unwrap(), + @r##" + <<<<<<<<<<< Conflict 1 of 1 + +++++++++++ Contents of side #1 + <<<<<<< a + ----------- Contents of base + ======= base + +++++++++++ Contents of side #2 + >>>>>>> b + >>>>>>>>>>> Conflict 1 of 1 ends + "## + ); + let editor_script = test_env.set_up_fake_editor(); + // Allow signaling that conflict markers were produced even if not editing + // conflict markers materialized in the output file + test_env.add_config("merge-tools.fake-editor.merge-conflict-exit-codes = [1]"); + + // By default, conflict markers of length 7 or longer are parsed for + // compatibility with Git merge tools + std::fs::write( + &editor_script, + indoc! {b" + write + <<<<<<< + A + ||||||| + BASE + ======= + B + >>>>>>> + \0fail + "}, + ) + .unwrap(); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["resolve"]); + insta::assert_snapshot!(stdout, @r#""#); + insta::assert_snapshot!(stderr, @r#" + Resolving conflicts in: file + Working copy now at: vruxwmqv 2b985546 conflict | (conflict) conflict + Parent commit : zsuskuln 64177fd4 a | a + Parent commit : royxmykx db442c1e b | b + Added 0 files, modified 1 files, removed 0 files + There are unresolved conflicts at these paths: + file 2-sided conflict + New conflicts appeared in these commits: + vruxwmqv 2b985546 conflict | (conflict) conflict + To resolve the conflicts, start by updating to it: + jj new vruxwmqv + Then use `jj resolve`, or edit the conflict markers in the file directly. + Once the conflicts are resolved, you may want to inspect the result with `jj diff`. + Then run `jj squash` to move the resolution into the conflicted commit. + "#); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), + @r##" + diff --git a/file b/file + --- a/file + +++ b/file + @@ -1,8 +1,8 @@ + -<<<<<<<<<<< Conflict 1 of 1 + -+++++++++++ Contents of side #1 + -<<<<<<< a + ------------ Contents of base + -======= base + -+++++++++++ Contents of side #2 + ->>>>>>> b + ->>>>>>>>>>> Conflict 1 of 1 ends + +<<<<<<< Conflict 1 of 1 + ++++++++ Contents of side #1 + +A + +------- Contents of base + +BASE + ++++++++ Contents of side #2 + +B + +>>>>>>> Conflict 1 of 1 ends + "##); + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["resolve", "--list"]), + @"file 2-sided conflict" + ); + + // If the merge tool edits the output file with materialized markers, the + // markers must match the length of the materialized markers to be parsed + test_env.jj_cmd_ok(&repo_path, &["undo"]); + std::fs::write( + &editor_script, + indoc! {b" + dump editor + \0write + <<<<<<<<<<< + <<<<<<< A + ||||||||||| + ======= BASE + =========== + >>>>>>> B + >>>>>>>>>>> + \0fail + "}, + ) + .unwrap(); + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &[ + "resolve", + "--config=merge-tools.fake-editor.merge-tool-edits-conflict-markers=true", + ], + ); + insta::assert_snapshot!(stdout, @r#""#); + insta::assert_snapshot!(stderr, @r#" + Resolving conflicts in: file + Working copy now at: vruxwmqv fac9406d conflict | (conflict) conflict + Parent commit : zsuskuln 64177fd4 a | a + Parent commit : royxmykx db442c1e b | b + Added 0 files, modified 1 files, removed 0 files + There are unresolved conflicts at these paths: + file 2-sided conflict + New conflicts appeared in these commits: + vruxwmqv fac9406d conflict | (conflict) conflict + To resolve the conflicts, start by updating to it: + jj new vruxwmqv + Then use `jj resolve`, or edit the conflict markers in the file directly. + Once the conflicts are resolved, you may want to inspect the result with `jj diff`. + Then run `jj squash` to move the resolution into the conflicted commit. + "#); + insta::assert_snapshot!( + std::fs::read_to_string(test_env.env_root().join("editor")).unwrap(), @r##" + <<<<<<<<<<< Conflict 1 of 1 + +++++++++++ Contents of side #1 + <<<<<<< a + ----------- Contents of base + ======= base + +++++++++++ Contents of side #2 + >>>>>>> b + >>>>>>>>>>> Conflict 1 of 1 ends + "##); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), + @r##" + diff --git a/file b/file + --- a/file + +++ b/file + @@ -1,8 +1,8 @@ + <<<<<<<<<<< Conflict 1 of 1 + +++++++++++ Contents of side #1 + -<<<<<<< a + +<<<<<<< A + ----------- Contents of base + -======= base + +======= BASE + +++++++++++ Contents of side #2 + ->>>>>>> b + +>>>>>>> B + >>>>>>>>>>> Conflict 1 of 1 ends + "##); + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["resolve", "--list"]), + @"file 2-sided conflict" + ); + + // If the merge tool accepts the marker length as an argument, then the conflict + // markers should be at least as long as "$marker_length" + test_env.jj_cmd_ok(&repo_path, &["undo"]); + std::fs::write( + &editor_script, + indoc! {b" + expect-arg 0 + 11\0write + <<<<<<<<<<< + <<<<<<< A + ||||||||||| + ======= BASE + =========== + >>>>>>> B + >>>>>>>>>>> + \0fail + "}, + ) + .unwrap(); + let (stdout, stderr) = test_env.jj_cmd_ok( + &repo_path, + &[ + "resolve", + r#"--config=merge-tools.fake-editor.merge-args=["$output", "$marker_length"]"#, + ], + ); + insta::assert_snapshot!(stdout, @r#""#); + insta::assert_snapshot!(stderr, @r#" + Resolving conflicts in: file + Working copy now at: vruxwmqv 1b29631a conflict | (conflict) conflict + Parent commit : zsuskuln 64177fd4 a | a + Parent commit : royxmykx db442c1e b | b + Added 0 files, modified 1 files, removed 0 files + There are unresolved conflicts at these paths: + file 2-sided conflict + New conflicts appeared in these commits: + vruxwmqv 1b29631a conflict | (conflict) conflict + To resolve the conflicts, start by updating to it: + jj new vruxwmqv + Then use `jj resolve`, or edit the conflict markers in the file directly. + Once the conflicts are resolved, you may want to inspect the result with `jj diff`. + Then run `jj squash` to move the resolution into the conflicted commit. + "#); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), + @r##" + diff --git a/file b/file + --- a/file + +++ b/file + @@ -1,8 +1,8 @@ + <<<<<<<<<<< Conflict 1 of 1 + +++++++++++ Contents of side #1 + -<<<<<<< a + +<<<<<<< A + ----------- Contents of base + -======= base + +======= BASE + +++++++++++ Contents of side #2 + ->>>>>>> b + +>>>>>>> B + >>>>>>>>>>> Conflict 1 of 1 ends + "##); + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["resolve", "--list"]), + @"file 2-sided conflict" + ); +} + #[test] fn test_multiple_conflicts() { let mut test_env = TestEnvironment::default(); diff --git a/docs/config.md b/docs/config.md index a23665fde..7a0de1290 100644 --- a/docs/config.md +++ b/docs/config.md @@ -860,6 +860,11 @@ merge-tool-edits-conflict-markers = true # See below for an explanation - `$base` is replaced with the path to a file containing the contents of the conflicted file in the last common ancestor of the two sides of the conflict. +- `$marker_length` is replaced with the length of the conflict markers which + should be used for the file. This can be useful if the merge tool parses + and/or generates conflict markers. Usually, `jj` uses conflict markers of + length 7, but they can be longer if necessary to make parsing unambiguous. + ### Editing conflict markers with a tool or a text editor By default, the merge tool starts with an empty output file. If the tool puts