Make optional the path argument to resolve, allow partial paths

Also allows several paths to be specified. By default, `jj resolve`
will find the first conflict that matches provided paths (if any)
and try to resolve it.

src/cli_util.rs

@@ -766,13 +766,9 @@ impl WorkspaceCommandHelper {
         &self,
         ui: &mut Ui,
         tree: &Tree,
-        path: &str,
+        repo_path: &RepoPath,
     ) -> Result<TreeId, CommandError> {
-        Ok(crate::diff_edit::run_mergetool(
+        Ok(crate::diff_edit::run_mergetool(ui, tree, repo_path)?)
-            ui,
-            tree,
-            &self.parse_file_path(path)?,
-        )?)
     }
 
     pub fn edit_diff(

src/commands.rs

@@ -508,9 +508,6 @@ struct UnsquashArgs {
 //   - `jj resolve --list` to list conflicts
 //   - `jj resolve --editor` to resolve a conflict in the default text editor. Should work for
 //     conflicts with 3+ adds. Useful to resolve conflicts in a commit other than the current one.
-//   - Make the `path` argument optional and/or repeatable. If a specific file is not specified,
-//     either pick an arbitrary file with a conflict (e.g. first one `jj resolve --list` shows),
-//     offer a UI, and/or loop over all the conflicted files.
 //   - A way to help split commits with conflicts that are too complicated (more than two sides)
 //     into commits with simpler conflicts. In case of a tree with many merges, we could for example
 //     point to existing commits with simpler conflicts where resolving those conflicts would help
@@ -519,10 +516,12 @@ struct UnsquashArgs {
 struct ResolveArgs {
     #[arg(long, short, default_value = "@")]
     revision: String,
-    /// The path to a file with conflicts. You can use `jj status` to find such
+    /// Restrict to these paths when searching for a conflict to resolve. We
-    /// files
+    /// will attempt to resolve the first conflict we can find. You can use  `jj
+    /// status` to find such files
+    // TODO: Find the conflict we can resolve even if it's not the first one.
     #[arg(value_hint = clap::ValueHint::AnyPath)]
-    path: String,
+    paths: Vec<String>,
 }
 
 /// Restore paths from another revision
@@ -2870,13 +2869,27 @@ fn cmd_resolve(
     args: &ResolveArgs,
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
+    let matcher = workspace_command.matcher_from_values(&args.paths)?;
     let commit = workspace_command.resolve_single_rev(&args.revision)?;
+    let tree = commit.tree();
+    let conflicts = tree.conflicts_matching(matcher.as_ref());
+    if conflicts.is_empty() {
+        return Err(CommandError::CliError(
+            "No conflicts found ".to_string()
+                + (if args.paths.is_empty() {
+                    "at this revision"
+                } else {
+                    "at the given path(s)"
+                }),
+        ));
+    }
+    let (repo_path, _) = conflicts.get(0).unwrap();
     workspace_command.check_rewriteable(&commit)?;
     let mut tx = workspace_command.start_transaction(&format!(
         "Resolve conflicts in commit {}",
         commit.id().hex()
     ));
-    let new_tree_id = workspace_command.run_mergetool(ui, &commit.tree(), &args.path)?;
+    let new_tree_id = workspace_command.run_mergetool(ui, &commit.tree(), repo_path)?;
     CommitBuilder::for_rewrite_from(ui.settings(), &commit)
         .set_tree(new_tree_id)
         .write_to_repo(tx.mut_repo());

tests/test_resolve_command.rs

@@ -77,8 +77,8 @@ 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"]);
+    test_env.jj_cmd_success(&repo_path, &["resolve"]);
-    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "-r", "conflict"]), 
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), 
     @r###"
     Resolved conflict in file:
        1    1: <<<<<<<resolution
@@ -101,7 +101,7 @@ fn test_resolution() {
     // 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"]), 
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), 
     @"");
     std::fs::write(
         &editor_script,
@@ -124,10 +124,9 @@ resolution
             "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"]), 
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), 
     @r###"
     Resolved conflict in file:
        1    1: <<<<<<<resolution
@@ -142,7 +141,7 @@ resolution
     // 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"]), 
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), 
     @"");
     std::fs::write(
         &editor_script,
@@ -171,11 +170,10 @@ conflict
             "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"]), 
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), 
     @r###"
     Modified conflict in file:
        1    1: <<<<<<<
@@ -201,7 +199,7 @@ conflict
     // `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"]), 
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), 
     @"");
     std::fs::write(
         &editor_script,
@@ -217,9 +215,9 @@ conflict
 ",
     )
     .unwrap();
-    test_env.jj_cmd_success(&repo_path, &["resolve", "file"]);
+    test_env.jj_cmd_success(&repo_path, &["resolve"]);
     // Note the "Resolved" below
-    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", "-r", "conflict"]), 
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), 
     @r###"
     Resolved conflict in file:
        1    1: <<<<<<<
@@ -239,6 +237,9 @@ conflict
     Working copy changes:
     M file
     "###);
+
+    // TODO: Check that running `jj new` and then `jj resolve -r conflict` works
+    // correctly.
 }
 
 fn check_resolve_produces_input_file(
@@ -251,10 +252,8 @@ fn check_resolve_produces_input_file(
     std::fs::write(editor_script, format!("expect\n{expected_content}")).unwrap();
 
     let merge_arg_config = format!(r#"merge-tools.fake-editor.merge-args = ["${role}"]"#);
-    let error = test_env.jj_cmd_failure(
+    let error =
-        repo_path,
+        test_env.jj_cmd_failure(repo_path, &["resolve", "--config-toml", &merge_arg_config]);
-        &["resolve", "--config-toml", &merge_arg_config, "file"],
-    );
     // This error means that fake-editor exited successfully but did not modify the
     // output file.
     insta::assert_snapshot!(error, @r###"
@@ -347,7 +346,7 @@ fn test_too_many_parents() {
     create_commit(&test_env, &repo_path, "c", &["base"], &[("file", "c\n")]);
     create_commit(&test_env, &repo_path, "conflict", &["a", "b", "c"], &[]);
 
-    let error = test_env.jj_cmd_failure(&repo_path, &["resolve", "file"]);
+    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 2 removes and 3 adds.
     At most 1 remove and 2 adds are supported.
@@ -407,7 +406,7 @@ fn test_file_vs_dir() {
     std::fs::write(repo_path.join("file").join("placeholder"), "").unwrap();
     create_commit(&test_env, &repo_path, "conflict", &["a", "b"], &[]);
 
-    let error = test_env.jj_cmd_failure(&repo_path, &["resolve", "file"]);
+    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":
     Conflict:
@@ -417,3 +416,164 @@ fn test_file_vs_dir() {
 
     "###);
 }
+
+#[test]
+fn test_multiple_conflicts() {
+    let mut test_env = TestEnvironment::default();
+    test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]);
+    let repo_path = test_env.env_root().join("repo");
+
+    create_commit(
+        &test_env,
+        &repo_path,
+        "base",
+        &[],
+        &[("file1", "first base\n"), ("file2", "second base\n")],
+    );
+    create_commit(
+        &test_env,
+        &repo_path,
+        "a",
+        &["base"],
+        &[("file1", "first a\n"), ("file2", "second a\n")],
+    );
+    create_commit(
+        &test_env,
+        &repo_path,
+        "b",
+        &["base"],
+        &[("file1", "first b\n"), ("file2", "second b\n")],
+    );
+    create_commit(&test_env, &repo_path, "conflict", &["a", "b"], &[]);
+    // Test the setup
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+        @   conflict
+        |\  
+        o | b
+        | o a
+        |/  
+        o base
+        o 
+        "###);
+    insta::assert_snapshot!(
+    std::fs::read_to_string(repo_path.join("file1")).unwrap()
+        , @r###"
+    <<<<<<<
+    %%%%%%%
+    -first base
+    +first a
+    +++++++
+    first b
+    >>>>>>>
+    "###);
+    insta::assert_snapshot!(
+    std::fs::read_to_string(repo_path.join("file2")).unwrap()
+        , @r###"
+    <<<<<<<
+    %%%%%%%
+    -second base
+    +second a
+    +++++++
+    second b
+    >>>>>>>
+    "###);
+
+    let editor_script = test_env.set_up_fake_editor();
+
+    // Check that we can manually pick which of the conflicts to resolve first
+    std::fs::write(&editor_script, "expect\n\0write\nresolution file2\n").unwrap();
+    test_env.jj_cmd_success(&repo_path, &["resolve", "file2"]);
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), 
+    @r###"
+    Resolved conflict in file2:
+       1     : <<<<<<<
+       2     : %%%%%%%
+       3    1: -secondresolution basefile2
+       4     : +second a
+       5     : +++++++
+       6     : second b
+       7     : >>>>>>>
+    "###);
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["status"]), 
+    @r###"
+    Parent commit: 9a25592b7aee a
+    Working copy : 00dac0bec4b1 conflict
+    Working copy changes:
+    M file1
+    M file2
+    There are unresolved conflicts at these paths:
+    file1
+    "###);
+
+    // For the rest of the test, we call `jj resolve` several times in a row to
+    // resolve each conflict in the order it chooses.
+    test_env.jj_cmd_success(&repo_path, &["undo"]);
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), 
+    @"");
+    std::fs::write(
+        &editor_script,
+        "expect\n\0write\nfirst resolution for auto-chosen file\n",
+    )
+    .unwrap();
+    test_env.jj_cmd_success(&repo_path, &["resolve"]);
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), 
+    @r###"
+    Resolved conflict in file1:
+       1     : <<<<<<<
+       2     : %%%%%%%
+       3    1: -first base
+       4    1: +firstresolution a
+       5     : +++++++
+       6    1: firstfor bauto-chosen file
+       7     : >>>>>>>
+    "###);
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["status"]), 
+    @r###"
+    Parent commit: 9a25592b7aee a
+    Working copy : f06196060882 conflict
+    Working copy changes:
+    M file1
+    M file2
+    There are unresolved conflicts at these paths:
+    file2
+    "###);
+    std::fs::write(
+        &editor_script,
+        "expect\n\0write\nsecond resolution for auto-chosen file\n",
+    )
+    .unwrap();
+
+    test_env.jj_cmd_success(&repo_path, &["resolve"]);
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff"]), 
+    @r###"
+    Resolved conflict in file1:
+       1     : <<<<<<<
+       2     : %%%%%%%
+       3    1: -first base
+       4    1: +firstresolution a
+       5     : +++++++
+       6    1: firstfor bauto-chosen file
+       7     : >>>>>>>
+    Resolved conflict in file2:
+       1     : <<<<<<<
+       2     : %%%%%%%
+       3    1: -second base
+       4    1: +secondresolution a
+       5     : +++++++
+       6    1: secondfor bauto-chosen file
+       7     : >>>>>>>
+    "###);
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["status"]), 
+    @r###"
+    Parent commit: 9a25592b7aee a
+    Working copy : 0fafecce390d conflict
+    Working copy changes:
+    M file1
+    M file2
+    "###);
+
+    insta::assert_snapshot!(test_env.jj_cmd_cli_error(&repo_path, &["resolve"]), 
+    @r###"
+    Error: No conflicts found at this revision
+    "###);
+}