New jj resolve command to resolve conflicts

This command uses an external merge tool to resolve conflicts
simple enough that they can be resolved with a 3-way merge.

This commit provides a very basic version of `jj resolve` that
is hardcoded to use vimdiff.

This also slightly changes the errors of the Diff Editor, so that
both the diff editor and `jj resolve can share an error type.

src/cli_util.rs

@@ -52,7 +52,7 @@ use jujutsu_lib::workspace::{Workspace, WorkspaceInitError, WorkspaceLoadError};
 use jujutsu_lib::{dag_walk, file_util, git, revset};
 
 use crate::config::read_config;
-use crate::diff_edit::DiffEditError;
+use crate::diff_edit::{ConflictResolveError, DiffEditError};
 use crate::formatter::Formatter;
 use crate::templater::TemplateFormatter;
 use crate::ui::{ColorChoice, Ui};
@@ -154,6 +154,12 @@ impl From<DiffEditError> for CommandError {
     }
 }
 
+impl From<ConflictResolveError> for CommandError {
+    fn from(err: ConflictResolveError) -> Self {
+        user_error(format!("Failed to use external tool to resolve: {err}"))
+    }
+}
+
 impl From<git2::Error> for CommandError {
     fn from(err: git2::Error) -> Self {
         user_error(format!("Git operation failed: {err}"))
@@ -756,14 +762,33 @@ impl WorkspaceCommandHelper {
         Ok(())
     }
 
+    pub fn run_mergetool(
+        &self,
+        ui: &mut Ui,
+        tree: &Tree,
+        path: &str,
+    ) -> Result<TreeId, CommandError> {
+        Ok(crate::diff_edit::run_mergetool(
+            ui,
+            tree,
+            &self.parse_file_path(path)?,
+        )?)
+    }
+
     pub fn edit_diff(
         &self,
         ui: &mut Ui,
         left_tree: &Tree,
         right_tree: &Tree,
         instructions: &str,
-    ) -> Result<TreeId, DiffEditError> {
+    ) -> Result<TreeId, CommandError> {
-        crate::diff_edit::edit_diff(ui, left_tree, right_tree, instructions, self.base_ignores())
+        Ok(crate::diff_edit::edit_diff(
+            ui,
+            left_tree,
+            right_tree,
+            instructions,
+            self.base_ignores(),
+        )?)
     }
 
     pub fn select_diff(

src/commands.rs

@@ -91,6 +91,7 @@ enum Commands {
     Unsquash(UnsquashArgs),
     Restore(RestoreArgs),
     Touchup(TouchupArgs),
+    Resolve(ResolveArgs),
     Split(SplitArgs),
     /// Merge work from multiple branches
     ///
@@ -495,6 +496,35 @@ struct UnsquashArgs {
     interactive: bool,
 }
 
+/// Resolve a conflicted file 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.
+///
+/// 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.
+//  TODOs:
+//   - `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
+//     simplify the present one.
+#[derive(clap::Args, Clone, Debug)]
+struct ResolveArgs {
+    #[arg(long, short, default_value = "@")]
+    revision: String,
+    /// The path to a file with conflicts. You can use `jj status` to find such
+    /// files
+    #[arg(value_hint = clap::ValueHint::AnyPath)]
+    path: String,
+}
+
 /// Restore paths from another revision
 ///
 /// That means that the paths get the same content in the destination (`--to`)
@@ -2828,6 +2858,25 @@ aborted.
     Ok(())
 }
 
+fn cmd_resolve(
+    ui: &mut Ui,
+    command: &CommandHelper,
+    args: &ResolveArgs,
+) -> Result<(), CommandError> {
+    let mut workspace_command = command.workspace_helper(ui)?;
+    let commit = workspace_command.resolve_single_rev(&args.revision)?;
+    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)?;
+    CommitBuilder::for_rewrite_from(ui.settings(), &commit)
+        .set_tree(new_tree_id)
+        .write_to_repo(tx.mut_repo());
+    workspace_command.finish_transaction(ui, tx)
+}
+
 fn cmd_restore(
     ui: &mut Ui,
     command: &CommandHelper,
@@ -4651,6 +4700,7 @@ pub fn run_command(
         Commands::Merge(sub_args) => cmd_merge(ui, command_helper, sub_args),
         Commands::Rebase(sub_args) => cmd_rebase(ui, command_helper, sub_args),
         Commands::Backout(sub_args) => cmd_backout(ui, command_helper, sub_args),
+        Commands::Resolve(sub_args) => cmd_resolve(ui, command_helper, sub_args),
         Commands::Branch(sub_args) => cmd_branch(ui, command_helper, sub_args),
         Commands::Undo(sub_args) => cmd_op_undo(ui, command_helper, sub_args),
         Commands::Operation(sub_args) => cmd_operation(ui, command_helper, sub_args),

src/diff_edit.rs

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use std::collections::HashMap;
 use std::fs::File;
 use std::io::Write;
 use std::path::{Path, PathBuf};
@@ -20,7 +21,10 @@ use std::sync::Arc;
 
 use config::ConfigError;
 use itertools::Itertools;
-use jujutsu_lib::backend::TreeId;
+use jujutsu_lib::backend::{TreeId, TreeValue};
+use jujutsu_lib::conflicts::{
+    describe_conflict, extract_file_conflict_as_single_hunk, materialize_merge_result,
+};
 use jujutsu_lib::gitignore::GitIgnoreFile;
 use jujutsu_lib::matchers::EverythingMatcher;
 use jujutsu_lib::repo_path::RepoPath;
@@ -33,36 +37,69 @@ use thiserror::Error;
 use crate::ui::Ui;
 
 #[derive(Debug, Error)]
-pub enum DiffEditError {
+pub enum ExternalToolError {
     #[error("Invalid config: {0}")]
     ConfigError(#[from] ConfigError),
-    #[error("The diff tool exited with a non-zero code")]
-    DifftoolAborted,
-    #[error("Failed to write directories to diff: {0:?}")]
-    CheckoutError(CheckoutError),
     #[error("Error setting up temporary directory: {0:?}")]
     SetUpDirError(#[source] std::io::Error),
-    #[error("Error executing editor '{editor_binary}': {source}")]
+    #[error("Error executing '{tool_binary}': {source}")]
-    ExecuteEditorError {
+    FailedToExecute {
-        editor_binary: String,
+        tool_binary: String,
         #[source]
         source: std::io::Error,
     },
+    #[error("Tool exited with a non-zero code.")]
+    ToolAborted,
     #[error("I/O error: {0:?}")]
-    IoError(#[source] std::io::Error),
+    IoError(#[from] std::io::Error),
+}
+
+#[derive(Debug, Error)]
+pub enum DiffEditError {
+    #[error("{0}")]
+    ExternalToolError(#[from] ExternalToolError),
+    #[error("Failed to write directories to diff: {0:?}")]
+    CheckoutError(#[from] CheckoutError),
     #[error("Failed to snapshot changes: {0:?}")]
-    SnapshotError(SnapshotError),
+    SnapshotError(#[from] SnapshotError),
 }
 
-impl From<CheckoutError> for DiffEditError {
+#[derive(Debug, Error)]
-    fn from(err: CheckoutError) -> Self {
+pub enum ConflictResolveError {
-        DiffEditError::CheckoutError(err)
+    #[error("{0}")]
-    }
+    ExternalToolError(#[from] ExternalToolError),
+    #[error("Couldn't find the path {0:?} in this revision")]
+    PathNotFoundError(RepoPath),
+    #[error("Couldn't find any conflicts at {0:?} in this revision")]
+    NotAConflictError(RepoPath),
+    #[error(
+        "Only conflicts that involve normal files (not symlinks, not executable, etc.) are \
+         supported. Conflict summary:\n {1}"
+    )]
+    NotNormalFilesError(RepoPath, String),
+    #[error(
+        "The conflict at {path:?} has {removes} removes and {adds} adds.\nAt most 1 remove and 2 \
+         adds are supported."
+    )]
+    ConflictTooComplicatedError {
+        path: RepoPath,
+        removes: usize,
+        adds: usize,
+    },
+    #[error("The output file is either unchanged or empty after the editor quit.")]
+    EmptyOrUnchanged,
+    #[error("Backend error: {0:?}")]
+    BackendError(#[from] jujutsu_lib::backend::BackendError),
 }
 
-impl From<SnapshotError> for DiffEditError {
+impl From<std::io::Error> for DiffEditError {
-    fn from(err: SnapshotError) -> Self {
+    fn from(err: std::io::Error) -> Self {
-        DiffEditError::SnapshotError(err)
+        DiffEditError::ExternalToolError(ExternalToolError::from(err))
+    }
+}
+impl From<std::io::Error> for ConflictResolveError {
+    fn from(err: std::io::Error) -> Self {
+        ConflictResolveError::ExternalToolError(ExternalToolError::from(err))
     }
 }
 
@@ -73,8 +110,8 @@ fn check_out(
     tree: &Tree,
     sparse_patterns: Vec<RepoPath>,
 ) -> Result<TreeState, DiffEditError> {
-    std::fs::create_dir(&wc_dir).map_err(DiffEditError::SetUpDirError)?;
+    std::fs::create_dir(&wc_dir).map_err(ExternalToolError::SetUpDirError)?;
-    std::fs::create_dir(&state_dir).map_err(DiffEditError::SetUpDirError)?;
+    std::fs::create_dir(&state_dir).map_err(ExternalToolError::SetUpDirError)?;
     let mut tree_state = TreeState::init(store, wc_dir, state_dir);
     tree_state.set_sparse_patterns(sparse_patterns)?;
     tree_state.check_out(tree)?;
@@ -96,6 +133,113 @@ fn set_readonly_recursively(path: &Path) -> Result<(), std::io::Error> {
     }
 }
 
+pub fn run_mergetool(
+    _ui: &mut Ui,
+    tree: &Tree,
+    repo_path: &RepoPath,
+) -> Result<TreeId, ConflictResolveError> {
+    let conflict_id = match tree.path_value(repo_path) {
+        Some(TreeValue::Conflict(id)) => id,
+        Some(_) => return Err(ConflictResolveError::NotAConflictError(repo_path.clone())),
+        None => return Err(ConflictResolveError::PathNotFoundError(repo_path.clone())),
+    };
+    let conflict = tree.store().read_conflict(repo_path, &conflict_id)?;
+    let mut content = match extract_file_conflict_as_single_hunk(tree.store(), repo_path, &conflict)
+    {
+        Some(c) => c,
+        _ => {
+            let mut summary_bytes: Vec<u8> = vec![];
+            describe_conflict(&conflict, &mut summary_bytes)
+                .expect("Writing to an in-memory buffer should never fail");
+            return Err(ConflictResolveError::NotNormalFilesError(
+                repo_path.clone(),
+                String::from_utf8_lossy(summary_bytes.as_slice()).to_string(),
+            ));
+        }
+    };
+    // The usual case is 1 `removes` and 2 `adds`. 0 `removes` means the file did
+    // not exist in the conflict base. Only 1 `adds` may exist for an
+    // edit-delete conflict.
+    if content.removes.len() > 1 || content.adds.len() > 2 {
+        return Err(ConflictResolveError::ConflictTooComplicatedError {
+            path: repo_path.clone(),
+            removes: content.removes.len(),
+            adds: content.adds.len(),
+        });
+    };
+
+    let mut materialized_conflict: Vec<u8> = vec![];
+    materialize_merge_result(&content, &mut materialized_conflict)
+        .expect("Writing to an in-memory buffer should never fail");
+    let materialized_conflict = materialized_conflict;
+
+    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(),
+    };
+
+    let temp_dir = tempfile::Builder::new()
+        .prefix("jj-resolve-")
+        .tempdir()
+        .map_err(ExternalToolError::SetUpDirError)?;
+    let suffix = repo_path
+        .components()
+        .last()
+        .map(|filename| format!("_{}", filename.as_str()))
+        // The default case below should never actually trigger, but we support it just in case
+        // resolving the root path ever makes sense.
+        .unwrap_or_default();
+    let paths: Result<HashMap<&str, _>, ConflictResolveError> = files
+        .iter()
+        .map(|(role, contents)| {
+            let path = temp_dir.path().join(format!("{role}{suffix}"));
+            std::fs::write(&path, contents).map_err(ExternalToolError::SetUpDirError)?;
+            if *role != "output" {
+                // TODO: Should actually ignore the error here, or have a warning.
+                set_readonly_recursively(&path).map_err(ExternalToolError::SetUpDirError)?;
+            }
+            Ok((*role, path))
+        })
+        .collect();
+    let paths = paths?;
+
+    let progname = "vimdiff";
+    let exit_status = Command::new(progname)
+        .args(["-f", "-d"])
+        .arg(paths.get("output").unwrap())
+        .arg("-M")
+        .args(["left", "base", "right"].map(|n| paths.get(n).unwrap()))
+        .args(["-c", "wincmd J", "-c", "setl modifiable write"])
+        .status()
+        .map_err(|e| ExternalToolError::FailedToExecute {
+            tool_binary: progname.to_string(),
+            source: e,
+        })?;
+    if !exit_status.success() {
+        return Err(ConflictResolveError::from(ExternalToolError::ToolAborted));
+    }
+
+    let output_file_contents: Vec<u8> = std::fs::read(paths.get("output").unwrap())?;
+    if output_file_contents.is_empty() || output_file_contents == materialized_conflict {
+        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(),
+        TreeValue::File {
+            id: new_file_id,
+            executable: false,
+        },
+    );
+    Ok(tree_builder.write_tree())
+}
+
 pub fn edit_diff(
     ui: &mut Ui,
     left_tree: &Tree,
@@ -114,7 +258,7 @@ pub fn edit_diff(
     let temp_dir = tempfile::Builder::new()
         .prefix("jj-diff-edit-")
         .tempdir()
-        .map_err(DiffEditError::SetUpDirError)?;
+        .map_err(ExternalToolError::SetUpDirError)?;
     let left_wc_dir = temp_dir.path().join("left");
     let left_state_dir = temp_dir.path().join("left_state");
     let right_wc_dir = temp_dir.path().join("right");
@@ -126,7 +270,7 @@ pub fn edit_diff(
         left_tree,
         changed_files.clone(),
     )?;
-    set_readonly_recursively(&left_wc_dir).map_err(DiffEditError::SetUpDirError)?;
+    set_readonly_recursively(&left_wc_dir).map_err(ExternalToolError::SetUpDirError)?;
     let mut right_tree_state = check_out(
         store.clone(),
         right_wc_dir.clone(),
@@ -139,9 +283,12 @@ pub fn edit_diff(
     // not get any instructions.
     let add_instructions = !instructions.is_empty() && !instructions_path.exists();
     if add_instructions {
-        let mut file = File::create(&instructions_path).map_err(DiffEditError::SetUpDirError)?;
+        // TODO: This can be replaced with std::fs::write. Is this used in other places
+        // as well?
+        let mut file =
+            File::create(&instructions_path).map_err(ExternalToolError::SetUpDirError)?;
         file.write_all(instructions.as_bytes())
-            .map_err(DiffEditError::SetUpDirError)?;
+            .map_err(ExternalToolError::SetUpDirError)?;
     }
 
     // TODO: Make this configuration have a table of possible editors and detect the
@@ -154,23 +301,23 @@ pub fn edit_diff(
                 "Using default editor '{}'; you can change this by setting ui.diff-editor\n",
                 default_editor
             ))
-            .map_err(DiffEditError::IoError)?;
+            .map_err(ExternalToolError::IoError)?;
             default_editor
         }
     };
-    let editor = get_tool(ui.settings(), &editor_name)?;
+    let editor = get_tool(ui.settings(), &editor_name).map_err(ExternalToolError::ConfigError)?;
     // Start a diff editor on the two directories.
     let exit_status = Command::new(&editor.program)
         .args(&editor.edit_args)
         .arg(&left_wc_dir)
         .arg(&right_wc_dir)
         .status()
-        .map_err(|e| DiffEditError::ExecuteEditorError {
+        .map_err(|e| ExternalToolError::FailedToExecute {
-            editor_binary: editor.program,
+            tool_binary: editor.program.clone(),
             source: e,
         })?;
     if !exit_status.success() {
-        return Err(DiffEditError::DifftoolAborted);
+        return Err(DiffEditError::from(ExternalToolError::ToolAborted));
     }
     if add_instructions {
         std::fs::remove_file(instructions_path).ok();

tests/test_restore_command.rs

@@ -133,7 +133,7 @@ fn test_restore_interactive() {
     std::fs::write(&edit_script, "rm file2\0fail").unwrap();
     let stderr = test_env.jj_cmd_failure(&repo_path, &["restore", "-i"]);
     insta::assert_snapshot!(stderr, @r###"
-    Error: Failed to edit diff: The diff tool exited with a non-zero code
+    Error: Failed to edit diff: Tool exited with a non-zero code.
     "###);
     let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]);
     insta::assert_snapshot!(stdout, @r###"

tests/test_touchup_command.rs

@@ -51,7 +51,7 @@ fn test_touchup() {
     std::fs::write(&edit_script, "rm file2\0fail").unwrap();
     let stderr = test_env.jj_cmd_failure(&repo_path, &["touchup"]);
     insta::assert_snapshot!(stderr, @r###"
-    Error: Failed to edit diff: The diff tool exited with a non-zero code
+    Error: Failed to edit diff: Tool exited with a non-zero code.
     "###);
     let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]);
     insta::assert_snapshot!(stdout, @r###"