From 11fc840e2d85a0aa56f6701e623469582ff09c55 Mon Sep 17 00:00:00 2001 From: Scott Taylor Date: Tue, 26 Nov 2024 17:01:48 -0600 Subject: [PATCH] resolve: add --all option If many files are conflicted, it would be nice to be able to resolve all conflicts at once without having to run `jj resolve` multiple times. This is especially nice for merge tools which try to automatically resolve conflicts without user input, but it can also be used for regular merge editors like VS Code. The new `--all` option invokes the tool for each file, stopping immediately if an error occurs. This means the user can cancel the merge partway through without finishing resolving every file. In this case, the error is reported as a warning instead, and the already-resolved files are saved. --- CHANGELOG.md | 2 + cli/src/command_error.rs | 2 +- cli/src/commands/resolve.rs | 59 ++++++++-- cli/tests/cli-reference@.md.snap | 9 +- cli/tests/test_resolve_command.rs | 187 ++++++++++++++++++++++++++++++ 5 files changed, 245 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74521bbd2e..5cae990fb1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * A new Email template type is added. `Signature.email()` now returns an Email template type instead of a String. +* `jj resolve` now supports `--all` to resolve all conflicts instead of just one. + ### Fixed bugs * The `$NO_COLOR` environment variable must now be non-empty to be respected. diff --git a/cli/src/command_error.rs b/cli/src/command_error.rs index 89c4ec58bf..9928fc41b2 100644 --- a/cli/src/command_error.rs +++ b/cli/src/command_error.rs @@ -785,7 +785,7 @@ fn print_error( Ok(()) } -fn print_error_sources(ui: &Ui, source: Option<&dyn error::Error>) -> io::Result<()> { +pub fn print_error_sources(ui: &Ui, source: Option<&dyn error::Error>) -> io::Result<()> { let Some(err) = source else { return Ok(()); }; diff --git a/cli/src/commands/resolve.rs b/cli/src/commands/resolve.rs index bd29479ab1..4d5c384ae7 100644 --- a/cli/src/commands/resolve.rs +++ b/cli/src/commands/resolve.rs @@ -18,20 +18,24 @@ use clap_complete::ArgValueCandidates; use clap_complete::ArgValueCompleter; use itertools::Itertools; use jj_lib::object_id::ObjectId; +use jj_lib::repo::Repo; use tracing::instrument; use crate::cli_util::print_conflicted_paths; use crate::cli_util::CommandHelper; use crate::cli_util::RevisionArg; use crate::command_error::cli_error; +use crate::command_error::print_error_sources; use crate::command_error::CommandError; use crate::complete; use crate::ui::Ui; -/// Resolve a conflicted file with an external merge tool +/// Resolve conflicted files with an external merge tool /// /// Only conflicts that can be resolved with a 3-way merge are supported. See -/// docs for merge tool configuration instructions. +/// docs for merge tool configuration instructions. By default, a single +/// conflicted file will be resolved at a time. Use `--all` to run the merge +/// tool on each conflicted file. /// /// Note that conflicts can also be resolved without using this command. You may /// edit the conflict markers in the conflicted file directly with a text @@ -56,6 +60,13 @@ pub(crate) struct ResolveArgs { // `diff --summary`, but should be more verbose. #[arg(long, short)] list: bool, + /// Instead of resolving one conflict, resolve all conflicts one at a time + /// + /// If conflict resolution is cancelled for a file after successfully + /// resolving other files, the already-resolved files will still be + /// committed. + #[arg(long, short, conflicts_with = "list")] + all: bool, /// Specify 3-way merge tool to be used #[arg(long, conflicts_with = "list", value_name = "NAME")] tool: Option, @@ -101,20 +112,48 @@ pub(crate) fn cmd_resolve( ); }; - let (repo_path, _) = conflicts.first().unwrap(); workspace_command.check_rewritable([commit.id()])?; let merge_editor = workspace_command.merge_editor(ui, args.tool.as_deref())?; - writeln!( - ui.status(), - "Resolving conflicts in: {}", - workspace_command.format_file_path(repo_path) - )?; let mut tx = workspace_command.start_transaction(); - let new_tree_id = merge_editor.edit_file(&tree, repo_path)?; + + let conflicts_to_resolve = if args.all { + &conflicts + } else { + &conflicts[..1] + }; + + let mut new_tree = tree; + for (i, (repo_path, _)) in conflicts_to_resolve.iter().enumerate() { + writeln!( + ui.status(), + "Resolving conflicts in: {}", + tx.base_workspace_helper().format_file_path(repo_path) + )?; + match merge_editor.edit_file(&new_tree, repo_path) { + Ok(tree_id) => { + new_tree = tx.repo().store().get_root_tree(&tree_id)?; + } + Err(err) if i == 0 => { + // Since no conflicts were successfully resolved, return the error + return Err(err.into()); + } + Err(err) => { + // Since some conflicts were already successfully resolved, just print a warning + // and stop here + writeln!( + ui.warning_default(), + "Stopping due to error after resolving {i} conflicts" + )?; + print_error_sources(ui, Some(&err))?; + break; + } + } + } + let new_commit = tx .repo_mut() .rewrite_commit(command.settings(), &commit) - .set_tree_id(new_tree_id) + .set_tree_id(new_tree.id()) .write()?; tx.finish( ui, diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index 80df301245..74c06ce301 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -145,7 +145,7 @@ To get started, see the tutorial at https://martinvonz.github.io/jj/latest/tutor * `parallelize` — Parallelize revisions by making them siblings * `prev` — Change the working copy revision relative to the parent revision * `rebase` — Move revisions to different parent(s) -* `resolve` — Resolve a conflicted file with an external merge tool +* `resolve` — Resolve conflicted files with an external merge tool * `restore` — Restore paths from another revision * `root` — Show the current workspace root directory * `show` — Show commit description and changes in a revision @@ -1880,9 +1880,9 @@ commit. This is true in general; it is not specific to this command. ## `jj resolve` -Resolve a conflicted file with an external merge tool +Resolve conflicted files with an external merge tool -Only conflicts that can be resolved with a 3-way merge are supported. See docs for merge tool configuration instructions. +Only conflicts that can be resolved with a 3-way merge are supported. See docs for merge tool configuration instructions. By default, a single conflicted file will be resolved at a time. Use `--all` to run the merge tool on each conflicted file. Note that conflicts can also be resolved without using this command. You may edit the conflict markers in the conflicted file directly with a text editor. @@ -1898,6 +1898,9 @@ Note that conflicts can also be resolved without using this command. You may edi Default value: `@` * `-l`, `--list` — Instead of resolving one conflict, list all the conflicts +* `-a`, `--all` — Instead of resolving one conflict, resolve all conflicts one at a time + + If conflict resolution is cancelled for a file after successfully resolving other files, the already-resolved files will still be committed. * `--tool ` — Specify 3-way merge tool to be used diff --git a/cli/tests/test_resolve_command.rs b/cli/tests/test_resolve_command.rs index f708031716..b6fbdc72fe 100644 --- a/cli/tests/test_resolve_command.rs +++ b/cli/tests/test_resolve_command.rs @@ -546,6 +546,193 @@ fn test_resolution() { // correctly. } +#[test] +fn test_resolution_with_all() { + 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"); + + // Create two conflicted files, and one non-conflicted file + create_commit( + &test_env, + &repo_path, + "base", + &[], + &[ + ("file1", "base1\n"), + ("file2", "base2\n"), + ("file3", "base3\n"), + ], + ); + create_commit( + &test_env, + &repo_path, + "a", + &["base"], + &[("file1", "a1\n"), ("file2", "a2\n")], + ); + create_commit( + &test_env, + &repo_path, + "b", + &["base"], + &[("file1", "b1\n"), ("file2", "b2\n")], + ); + create_commit(&test_env, &repo_path, "conflict", &["a", "b"], &[]); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["resolve", "--list"]), + @r#" + file1 2-sided conflict + file2 2-sided conflict + "#); + insta::assert_snapshot!( + std::fs::read_to_string(repo_path.join("file1")).unwrap(), + @r##" + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 + -base1 + +a1 + +++++++ Contents of side #2 + b1 + >>>>>>> Conflict 1 of 1 ends + "## + ); + insta::assert_snapshot!( + std::fs::read_to_string(repo_path.join("file2")).unwrap(), + @r##" + <<<<<<< Conflict 1 of 1 + %%%%%%% Changes from base to side #1 + -base2 + +a2 + +++++++ Contents of side #2 + b2 + >>>>>>> Conflict 1 of 1 ends + "## + ); + let editor_script = test_env.set_up_fake_editor(); + + // Test resolving both conflicts + std::fs::write( + &editor_script, + [ + "write\nresolution1\n", + "next invocation\n", + "write\nresolution2\n", + ] + .join("\0"), + ) + .unwrap(); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["resolve", "--all"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r#" + Resolving conflicts in: file1 + Resolving conflicts in: file2 + Working copy now at: vruxwmqv 8fffa1fb conflict | conflict + Parent commit : zsuskuln 9db7fdfb a | a + Parent commit : royxmykx d67e26e4 b | b + Added 0 files, modified 2 files, removed 0 files + "#); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), + @r##" + diff --git a/file1 b/file1 + index 0000000000..95cc18629d 100644 + --- a/file1 + +++ b/file1 + @@ -1,7 +1,1 @@ + -<<<<<<< Conflict 1 of 1 + -%%%%%%% Changes from base to side #1 + --base1 + -+a1 + -+++++++ Contents of side #2 + -b1 + ->>>>>>> Conflict 1 of 1 ends + +resolution1 + diff --git a/file2 b/file2 + index 0000000000..775f078581 100644 + --- a/file2 + +++ b/file2 + @@ -1,7 +1,1 @@ + -<<<<<<< Conflict 1 of 1 + -%%%%%%% Changes from base to side #1 + --base2 + -+a2 + -+++++++ Contents of side #2 + -b2 + ->>>>>>> Conflict 1 of 1 ends + +resolution2 + "##); + insta::assert_snapshot!(test_env.jj_cmd_cli_error(&repo_path, &["resolve", "--list"]), + @r###" + Error: No conflicts found at this revision + "###); + + // Test resolving one conflict, then exiting without resolving the second one + test_env.jj_cmd_ok(&repo_path, &["undo"]); + std::fs::write( + &editor_script, + ["write\nresolution1\n", "next invocation\n", "fail"].join("\0"), + ) + .unwrap(); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["resolve", "--all"]); + insta::assert_snapshot!(stdout, @r#""#); + insta::assert_snapshot!(stderr.replace("exit code", "exit status"), @r#" + Resolving conflicts in: file1 + Resolving conflicts in: file2 + Warning: Stopping due to error after resolving 1 conflicts + Caused by: Tool exited with exit status: 1 (run with --debug to see the exact invocation) + Working copy now at: vruxwmqv 22b6cad7 conflict | (conflict) conflict + Parent commit : zsuskuln 9db7fdfb a | a + Parent commit : royxmykx d67e26e4 b | b + Added 0 files, modified 1 files, removed 0 files + There are unresolved conflicts at these paths: + file2 2-sided conflict + New conflicts appeared in these commits: + vruxwmqv 22b6cad7 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/file1 b/file1 + index 0000000000..95cc18629d 100644 + --- a/file1 + +++ b/file1 + @@ -1,7 +1,1 @@ + -<<<<<<< Conflict 1 of 1 + -%%%%%%% Changes from base to side #1 + --base1 + -+a1 + -+++++++ Contents of side #2 + -b1 + ->>>>>>> Conflict 1 of 1 ends + +resolution1 + "##); + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["resolve", "--list"]), + @"file2 2-sided conflict" + ); + + // Test immediately failing to resolve any conflict + test_env.jj_cmd_ok(&repo_path, &["undo"]); + std::fs::write(&editor_script, "fail").unwrap(); + let stderr = test_env.jj_cmd_failure(&repo_path, &["resolve", "--all"]); + insta::assert_snapshot!(stderr.replace("exit code", "exit status"), @r#" + Resolving conflicts in: file1 + Error: Failed to resolve conflicts + Caused by: Tool exited with exit status: 1 (run with --debug to see the exact invocation) + "#); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "--git"]), @""); + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["resolve", "--list"]), + @r#" + file1 2-sided conflict + file2 2-sided conflict + "# + ); +} + fn check_resolve_produces_input_file( test_env: &mut TestEnvironment, repo_path: &Path,