resolve: try to resolve all conflicted files in fileset

CHANGELOG.md

@@ -34,6 +34,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 * The deprecated `[alias]` config section is no longer respected. Move command
   aliases to the `[aliases]` section.
 
+* `jj resolve` will now attempt to resolve all conflicted files instead of
+  resolving the first conflicted file. To resolve a single file, pass a file
+  path to `jj resolve`.
+
 ### Deprecations
 
 * `--config-toml=TOML` is deprecated in favor of `--config=NAME=VALUE` and

cli/src/command_error.rs

@@ -420,7 +420,11 @@ impl From<DiffRenderError> for CommandError {
 
 impl From<ConflictResolveError> for CommandError {
     fn from(err: ConflictResolveError) -> Self {
-        user_error_with_message("Failed to resolve conflicts", err)
+        match err {
+            ConflictResolveError::Backend(err) => err.into(),
+            ConflictResolveError::Io(err) => err.into(),
+            _ => user_error_with_message("Failed to resolve conflicts", err),
+        }
     }
 }
 
@@ -795,7 +799,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(());
     };

cli/src/commands/resolve.rs

@@ -28,10 +28,13 @@ 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. External merge tools will be
+/// invoked for each conflicted file one-by-one until all conflicts are
+/// resolved. To stop resolving conflicts, exit the merge tool without making
+/// any changes.
 ///
 /// 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
@@ -52,18 +55,16 @@ pub(crate) struct ResolveArgs {
         add = ArgValueCandidates::new(complete::mutable_revisions),
     )]
     revision: RevisionArg,
-    /// Instead of resolving one conflict, list all the conflicts
+    /// Instead of resolving conflicts, list all the conflicts
     // TODO: Also have a `--summary` option. `--list` currently acts like
     // `diff --summary`, but should be more verbose.
     #[arg(long, short)]
     list: bool,
     /// Specify 3-way merge tool to be used
     #[arg(long, conflicts_with = "list", value_name = "NAME")]
     tool: Option<String>,
-    /// Restrict to these paths when searching for a conflict to resolve. We
-    /// will attempt to resolve the first conflict we can find. You can use
-    /// the `--list` argument to find paths to use here.
-    // TODO: Find the conflict we can resolve even if it's not the first one.
+    /// Only resolve conflicts in these paths. You can use the `--list` argument
+    /// to find paths to use here.
     #[arg(
         value_name = "FILESETS",
         value_hint = clap::ValueHint::AnyPath,
@@ -103,16 +104,19 @@ pub(crate) fn cmd_resolve(
         );
     };
 
-    let (repo_path, _) = conflicts.first().unwrap();
+    let repo_paths = conflicts
+        .iter()
+        .map(|(path, _)| path.as_ref())
+        .collect_vec();
     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 new_tree_id = merge_editor.edit_file(
+        ui,
+        tx.base_workspace_helper().path_converter(),
+        &tree,
+        &repo_paths,
+    )?;
     let new_commit = tx
         .repo_mut()
         .rewrite_commit(command.settings(), &commit)

cli/src/merge_tools/builtin.rs

@@ -2,7 +2,6 @@ use std::borrow::Cow;
 use std::path::Path;
 use std::sync::Arc;
 
-use bstr::BString;
 use futures::StreamExt;
 use futures::TryFutureExt;
 use futures::TryStreamExt;
@@ -32,6 +31,8 @@ use jj_lib::store::Store;
 use pollster::FutureExt;
 use thiserror::Error;
 
+use super::MergeToolFile;
+
 #[derive(Debug, Error)]
 pub enum BuiltinToolError {
     #[error("Failed to record changes")]
@@ -637,33 +638,50 @@ fn make_merge_sections(
     Ok(sections)
 }
 
+fn make_merge_file(
+    merge_tool_file: &MergeToolFile,
+) -> Result<scm_record::File<'static>, BuiltinToolError> {
+    let merge_result = files::merge(&merge_tool_file.content);
+    let sections = make_merge_sections(merge_result)?;
+    Ok(scm_record::File {
+        old_path: None,
+        // Path for displaying purposes, not for file access.
+        path: Cow::Owned(
+            merge_tool_file
+                .repo_path
+                .to_fs_path_unchecked(Path::new("")),
+        ),
+        file_mode: None,
+        sections,
+    })
+}
+
 pub fn edit_merge_builtin(
     tree: &MergedTree,
-    path: &RepoPath,
-    content: Merge<BString>,
+    merge_tool_files: &[MergeToolFile],
 ) -> Result<MergedTreeId, BuiltinToolError> {
-    let merge_result = files::merge(&content);
-    let sections = make_merge_sections(merge_result)?;
     let mut input = scm_record::helpers::CrosstermInput;
     let recorder = scm_record::Recorder::new(
         scm_record::RecordState {
             is_read_only: false,
-            files: vec![scm_record::File {
-                old_path: None,
-                // Path for displaying purposes, not for file access.
-                path: Cow::Owned(path.to_fs_path_unchecked(Path::new(""))),
-                file_mode: None,
-                sections,
-            }],
+            files: merge_tool_files.iter().map(make_merge_file).try_collect()?,
             commits: Default::default(),
         },
         &mut input,
     );
     let state = recorder.run()?;
 
-    let file = state.files.into_iter().exactly_one().unwrap();
-    apply_diff_builtin(tree.store(), tree, tree, vec![path.to_owned()], &[file])
-        .map_err(BuiltinToolError::BackendError)
+    apply_diff_builtin(
+        tree.store(),
+        tree,
+        tree,
+        merge_tool_files
+            .iter()
+            .map(|file| file.repo_path.clone())
+            .collect_vec(),
+        &state.files,
+    )
+    .map_err(BuiltinToolError::BackendError)
 }
 
 #[cfg(test)]

cli/src/merge_tools/external.rs

@@ -8,7 +8,6 @@ use std::sync::Arc;
 
 use bstr::BString;
 use itertools::Itertools;
-use jj_lib::backend::FileId;
 use jj_lib::backend::MergedTreeId;
 use jj_lib::backend::TreeValue;
 use jj_lib::conflicts;
@@ -17,10 +16,10 @@ use jj_lib::conflicts::ConflictMarkerStyle;
 use jj_lib::gitignore::GitIgnoreFile;
 use jj_lib::matchers::Matcher;
 use jj_lib::merge::Merge;
-use jj_lib::merge::MergedTreeValue;
 use jj_lib::merged_tree::MergedTree;
 use jj_lib::merged_tree::MergedTreeBuilder;
-use jj_lib::repo_path::RepoPath;
+use jj_lib::repo_path::RepoPathUiConverter;
+use jj_lib::store::Store;
 use jj_lib::working_copy::CheckoutOptions;
 use pollster::FutureExt;
 use thiserror::Error;
@@ -33,6 +32,8 @@ use super::diff_working_copies::DiffSide;
 use super::ConflictResolveError;
 use super::DiffEditError;
 use super::DiffGenerateError;
+use super::MergeToolFile;
+use crate::command_error::print_error_sources;
 use crate::config::find_all_variables;
 use crate::config::interpolate_variables;
 use crate::config::CommandNameAndArgs;
@@ -168,21 +169,26 @@ pub enum ExternalToolError {
     Io(#[source] std::io::Error),
 }
 
-pub fn run_mergetool_external(
+fn run_mergetool_external_single_file(
     editor: &ExternalMergeTool,
-    file_merge: Merge<Option<FileId>>,
-    content: Merge<BString>,
-    repo_path: &RepoPath,
-    conflict: MergedTreeValue,
-    tree: &MergedTree,
+    store: &Store,
+    merge_tool_file: &MergeToolFile,
     default_conflict_marker_style: ConflictMarkerStyle,
-) -> Result<MergedTreeId, ConflictResolveError> {
+    tree_builder: &mut MergedTreeBuilder,
+) -> Result<(), ConflictResolveError> {
+    let MergeToolFile {
+        repo_path,
+        conflict,
+        file_merge,
+        content,
+    } = merge_tool_file;
+
     let conflict_marker_style = editor
         .conflict_marker_style
         .unwrap_or(default_conflict_marker_style);
 
     let initial_output_content = if editor.merge_tool_edits_conflict_markers {
-        materialize_merge_result_to_bytes(&content, conflict_marker_style)
+        materialize_merge_result_to_bytes(content, conflict_marker_style)
     } else {
         BString::default()
     };
@@ -252,16 +258,15 @@ pub fn run_mergetool_external(
 
     let new_file_ids = if editor.merge_tool_edits_conflict_markers || exit_status_implies_conflict {
         conflicts::update_from_content(
-            &file_merge,
-            tree.store(),
+            file_merge,
+            store,
             repo_path,
             output_file_contents.as_slice(),
             conflict_marker_style,
         )
         .block_on()?
     } else {
-        let new_file_id = tree
-            .store()
+        let new_file_id = store
             .write_file(repo_path, &mut output_file_contents.as_slice())
             .block_on()?;
         Merge::normal(new_file_id)
@@ -284,8 +289,49 @@ pub fn run_mergetool_external(
         }),
         Err(new_file_ids) => conflict.with_new_file_ids(&new_file_ids),
     };
-    let mut tree_builder = MergedTreeBuilder::new(tree.id());
     tree_builder.set_or_remove(repo_path.to_owned(), new_tree_value);
+    Ok(())
+}
+
+pub fn run_mergetool_external(
+    ui: &Ui,
+    path_converter: &RepoPathUiConverter,
+    editor: &ExternalMergeTool,
+    tree: &MergedTree,
+    merge_tool_files: &[MergeToolFile],
+    default_conflict_marker_style: ConflictMarkerStyle,
+) -> Result<MergedTreeId, ConflictResolveError> {
+    let mut tree_builder = MergedTreeBuilder::new(tree.id());
+    for (i, merge_tool_file) in merge_tool_files.iter().enumerate() {
+        writeln!(
+            ui.status(),
+            "Resolving conflicts in: {}",
+            path_converter.format_file_path(&merge_tool_file.repo_path)
+        )?;
+        match run_mergetool_external_single_file(
+            editor,
+            tree.store(),
+            merge_tool_file,
+            default_conflict_marker_style,
+            &mut tree_builder,
+        ) {
+            Ok(()) => {}
+            Err(err) if i == 0 => {
+                // Since no conflicts were successfully resolved, return the error
+                return Err(err);
+            }
+            Err(err) => {
+                // Since some conflicts were already successfully resolved, just print a warning
+                // and stop resolving conflicts
+                writeln!(
+                    ui.warning_default(),
+                    "Stopping due to error after resolving {i} conflicts"
+                )?;
+                print_error_sources(ui, Some(&err))?;
+                break;
+            }
+        }
+    }
     let new_tree = tree_builder.write_tree(tree.store())?;
     Ok(new_tree)
 }