From 6f50c97db81d45f5ae92c9873c8a6d5773f807b6 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Fri, 15 Mar 2024 17:26:14 -0700 Subject: [PATCH] merge tools: split DiffEditWorkingCopies from DiffWorkingCopies As suggested by @yuja in https://github.com/martinvonz/jj/pull/3296#discussion_r1525829712 --- cli/src/merge_tools/diff_working_copies.rs | 160 +++++++++++---------- cli/src/merge_tools/external.rs | 9 +- 2 files changed, 88 insertions(+), 81 deletions(-) diff --git a/cli/src/merge_tools/diff_working_copies.rs b/cli/src/merge_tools/diff_working_copies.rs index ca0c1a3143..c41451b0ab 100644 --- a/cli/src/merge_tools/diff_working_copies.rs +++ b/cli/src/merge_tools/diff_working_copies.rs @@ -36,7 +36,6 @@ pub(crate) struct DiffWorkingCopies { left_tree_state: TreeState, right_tree_state: TreeState, output_tree_state: Option, - instructions_path_to_cleanup: Option, } impl DiffWorkingCopies { @@ -177,50 +176,55 @@ pub(crate) fn check_out_trees( left_tree_state, right_tree_state, output_tree_state, - instructions_path_to_cleanup: None, }) } -/// Checks out the trees, populates JJ_INSTRUCTIONS, and makes appropriate sides -/// readonly. -pub fn check_out_trees_for_diffedit( - store: &Arc, - left_tree: &MergedTree, - right_tree: &MergedTree, - matcher: &dyn Matcher, - output_is: Option, - instructions: Option<&str>, -) -> Result { - let mut diff_wc = check_out_trees(store, left_tree, right_tree, matcher, output_is)?; - let got_output_field = output_is.is_some(); +pub(crate) struct DiffEditWorkingCopies { + pub working_copies: DiffWorkingCopies, + instructions_path_to_cleanup: Option, +} - set_readonly_recursively(diff_wc.left_working_copy_path()) - .map_err(ExternalToolError::SetUpDir)?; - if got_output_field { - set_readonly_recursively(diff_wc.right_working_copy_path()) +impl DiffEditWorkingCopies { + /// Checks out the trees, populates JJ_INSTRUCTIONS, and makes appropriate + /// sides readonly. + pub fn check_out_trees_for_diffedit( + store: &Arc, + left_tree: &MergedTree, + right_tree: &MergedTree, + matcher: &dyn Matcher, + output_is: Option, + instructions: Option<&str>, + ) -> Result { + let diff_wc = check_out_trees(store, left_tree, right_tree, matcher, output_is)?; + let got_output_field = output_is.is_some(); + + set_readonly_recursively(diff_wc.left_working_copy_path()) .map_err(ExternalToolError::SetUpDir)?; - } - let output_wc_path = diff_wc - .output_working_copy_path() - .unwrap_or_else(|| diff_wc.right_working_copy_path()); - let instructions_path = output_wc_path.join("JJ-INSTRUCTIONS"); - // In the unlikely event that the file already exists, then the user will simply - // not get any instructions. - let add_instructions = if !instructions_path.exists() { - instructions - } else { - None - }; - if let Some(instructions) = add_instructions { - let mut output_instructions_file = - File::create(&instructions_path).map_err(ExternalToolError::SetUpDir)?; - if diff_wc.right_working_copy_path() != output_wc_path { - let mut right_instructions_file = - File::create(diff_wc.right_working_copy_path().join("JJ-INSTRUCTIONS")) - .map_err(ExternalToolError::SetUpDir)?; - right_instructions_file - .write_all( - b"\ + if got_output_field { + set_readonly_recursively(diff_wc.right_working_copy_path()) + .map_err(ExternalToolError::SetUpDir)?; + } + let output_wc_path = diff_wc + .output_working_copy_path() + .unwrap_or_else(|| diff_wc.right_working_copy_path()); + let instructions_path = output_wc_path.join("JJ-INSTRUCTIONS"); + // In the unlikely event that the file already exists, then the user will simply + // not get any instructions. + let add_instructions = if !instructions_path.exists() { + instructions + } else { + None + }; + if let Some(instructions) = add_instructions { + let mut output_instructions_file = + File::create(&instructions_path).map_err(ExternalToolError::SetUpDir)?; + if diff_wc.right_working_copy_path() != output_wc_path { + let mut right_instructions_file = + File::create(diff_wc.right_working_copy_path().join("JJ-INSTRUCTIONS")) + .map_err(ExternalToolError::SetUpDir)?; + right_instructions_file + .write_all( + b"\ The content of this pane should NOT be edited. Any edits will be lost. @@ -229,16 +233,16 @@ the following instructions may have been written with a 2-pane diff editing in mind and be a little inaccurate. ", - ) - .map_err(ExternalToolError::SetUpDir)?; - right_instructions_file - .write_all(instructions.as_bytes()) - .map_err(ExternalToolError::SetUpDir)?; - // Note that some diff tools might not show this message and delete the contents - // of the output dir instead. Meld does show this message. - output_instructions_file - .write_all( - b"\ + ) + .map_err(ExternalToolError::SetUpDir)?; + right_instructions_file + .write_all(instructions.as_bytes()) + .map_err(ExternalToolError::SetUpDir)?; + // Note that some diff tools might not show this message and delete the contents + // of the output dir instead. Meld does show this message. + output_instructions_file + .write_all( + b"\ Please make your edits in this pane. You are using the experimental 3-pane diff editor config. Some of @@ -246,35 +250,39 @@ the following instructions may have been written with a 2-pane diff editing in mind and be a little inaccurate. ", - ) + ) + .map_err(ExternalToolError::SetUpDir)?; + } + output_instructions_file + .write_all(instructions.as_bytes()) .map_err(ExternalToolError::SetUpDir)?; - } - output_instructions_file - .write_all(instructions.as_bytes()) - .map_err(ExternalToolError::SetUpDir)?; - diff_wc.instructions_path_to_cleanup = Some(instructions_path); + }; + + Ok(Self { + working_copies: diff_wc, + instructions_path_to_cleanup: add_instructions.map(|_| instructions_path), + }) } - Ok(diff_wc) -} + pub fn snapshot_diffedit_results( + self, + base_ignores: Arc, + ) -> Result { + if let Some(path) = self.instructions_path_to_cleanup { + std::fs::remove_file(path).ok(); + } -pub fn snapshot_diffedit_results( - diff_wc: DiffWorkingCopies, - base_ignores: Arc, -) -> Result { - if let Some(path) = diff_wc.instructions_path_to_cleanup { - std::fs::remove_file(path).ok(); + let diff_wc = self.working_copies; + // Snapshot changes in the temporary output directory. + let mut output_tree_state = diff_wc + .output_tree_state + .unwrap_or(diff_wc.right_tree_state); + output_tree_state.snapshot(SnapshotOptions { + base_ignores, + fsmonitor_kind: FsmonitorKind::None, + progress: None, + max_new_file_size: u64::MAX, + })?; + Ok(output_tree_state.current_tree_id().clone()) } - - // Snapshot changes in the temporary output directory. - let mut output_tree_state = diff_wc - .output_tree_state - .unwrap_or(diff_wc.right_tree_state); - output_tree_state.snapshot(SnapshotOptions { - base_ignores, - fsmonitor_kind: FsmonitorKind::None, - progress: None, - max_new_file_size: u64::MAX, - })?; - Ok(output_tree_state.current_tree_id().clone()) } diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index a5a6122472..1bd29a56aa 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -16,8 +16,7 @@ use regex::{Captures, Regex}; use thiserror::Error; use super::diff_working_copies::{ - check_out_trees, check_out_trees_for_diffedit, new_utf8_temp_dir, set_readonly_recursively, - snapshot_diffedit_results, DiffSide, + check_out_trees, new_utf8_temp_dir, set_readonly_recursively, DiffEditWorkingCopies, DiffSide, }; use super::{ConflictResolveError, DiffEditError, DiffGenerateError}; use crate::config::CommandNameAndArgs; @@ -261,7 +260,7 @@ pub fn edit_diff_external( ) -> Result { let got_output_field = find_all_variables(&editor.edit_args).contains(&"output"); let store = left_tree.store(); - let diff_wc = check_out_trees_for_diffedit( + let diff_wc = DiffEditWorkingCopies::check_out_trees_for_diffedit( store, left_tree, right_tree, @@ -270,7 +269,7 @@ pub fn edit_diff_external( instructions, )?; - let patterns = diff_wc.to_command_variables(); + let patterns = diff_wc.working_copies.to_command_variables(); let mut cmd = Command::new(&editor.program); cmd.args(interpolate_variables(&editor.edit_args, &patterns)); tracing::info!(?cmd, "Invoking the external diff editor:"); @@ -286,7 +285,7 @@ pub fn edit_diff_external( })); } - snapshot_diffedit_results(diff_wc, base_ignores) + diff_wc.snapshot_diffedit_results(base_ignores) } /// Generates textual diff by the specified `tool`, and writes into `writer`.