From 94c4c07d23f440180244ae45ceae4d600c75215c Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Mon, 7 Aug 2023 16:40:10 -0700 Subject: [PATCH] merge_tools: Allow 3-pane diff editing As discussed in https://github.com/martinvonz/jj/discussions/1905#discussioncomment-6589673 --- CHANGELOG.md | 4 + cli/src/config/merge_tools.toml | 4 + cli/src/merge_tools/external.rs | 123 +++++++++++++++++++++++++---- cli/tests/test_diffedit_command.rs | 118 ++++++++++++++++++++++++++- docs/config.md | 14 ++++ 5 files changed, 246 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 722a2abbcc..32728eac12 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 external program. For configuration, see [the documentation](docs/config.md). [#1886](https://github.com/martinvonz/jj/issues/1886) +* A new experimental diff editor `meld-3` is introduced that sets up Meld to + allow you to see both sides of the original diff while editing. This can be + used with `jj split`, `jj move -i`, etc. + * `jj log`/`obslog`/`op log` now supports `--limit N` option to show the first `N` entries. diff --git a/cli/src/config/merge_tools.toml b/cli/src/config/merge_tools.toml index 3bd1e1e08b..20213e6d7e 100644 --- a/cli/src/config/merge_tools.toml +++ b/cli/src/config/merge_tools.toml @@ -7,6 +7,10 @@ merge-args = ["$base", "$left", "$right", "-o", "$output", "--auto"] edit-args = ["$left", "$right"] merge-args = ["$left", "$base", "$right", "-o", "$output", "--auto-merge"] +[merge-tools.meld-3] +program="meld" +edit-args = ["$left", "$output", "$right", "-o", "$output"] + [merge-tools.vimdiff] program = "vim" # `-d` enables diff mode. `-f` makes vim run in foreground even if it starts a GUI. diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index a25870e406..ba7709398c 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -144,6 +144,7 @@ struct DiffWorkingCopies { _temp_dir: TempDir, left_tree_state: TreeState, right_tree_state: TreeState, + output_tree_state: Option, } impl DiffWorkingCopies { @@ -155,13 +156,28 @@ impl DiffWorkingCopies { self.right_tree_state.working_copy_path() } + fn output_working_copy_path(&self) -> Option<&Path> { + self.output_tree_state + .as_ref() + .map(|state| state.working_copy_path()) + } + fn to_command_variables(&self) -> HashMap<&'static str, &str> { let left_wc_dir = self.left_working_copy_path(); let right_wc_dir = self.right_working_copy_path(); - maplit::hashmap! { + let mut result = maplit::hashmap! { "left" => left_wc_dir.to_str().expect("temp_dir should be valid utf-8"), "right" => right_wc_dir.to_str().expect("temp_dir should be valid utf-8"), + }; + if let Some(output_wc_dir) = self.output_working_copy_path() { + result.insert( + "output", + output_wc_dir + .to_str() + .expect("temp_dir should be valid utf-8"), + ); } + result } } @@ -205,12 +221,19 @@ fn set_readonly_recursively(path: &Path) -> Result<(), std::io::Error> { } } +#[derive(Debug, Clone, Copy)] +enum DiffSide { + // Left, + Right, +} + /// Check out the two trees in temporary directories. Only include changed files /// in the sparse checkout patterns. fn check_out_trees( store: &Arc, left_tree: &Tree, right_tree: &Tree, + output_is: Option, matcher: &dyn Matcher, ) -> Result { let changed_files = left_tree @@ -235,12 +258,29 @@ fn check_out_trees( right_wc_dir, right_state_dir, right_tree, - changed_files, + changed_files.clone(), )?; + let output_tree_state = output_is + .map(|output_side| { + let output_wc_dir = temp_dir.path().join("output"); + let output_state_dir = temp_dir.path().join("output_state"); + check_out( + store.clone(), + output_wc_dir, + output_state_dir, + match output_side { + // DiffSide::Left => left_tree, + DiffSide::Right => right_tree, + }, + changed_files, + ) + }) + .transpose()?; Ok(DiffWorkingCopies { _temp_dir: temp_dir, left_tree_state, right_tree_state, + output_tree_state, }) } @@ -371,7 +411,6 @@ fn interpolate_variables>( } /// Return all variable names found in the args, without the dollar sign -#[allow(unused)] fn find_all_variables(args: &[String]) -> Vec { args.iter() .flat_map(|arg| VARIABLE_REGEX.as_ref().unwrap().find_iter(arg)) @@ -387,20 +426,70 @@ pub fn edit_diff_external( base_ignores: Arc, settings: &UserSettings, ) -> Result { + let got_output_field = find_all_variables(&editor.edit_args).contains(&"output".to_owned()); let store = left_tree.store(); - let diff_wc = check_out_trees(store, left_tree, right_tree, &EverythingMatcher)?; + let diff_wc = check_out_trees( + store, + left_tree, + right_tree, + got_output_field.then_some(DiffSide::Right), + &EverythingMatcher, + )?; set_readonly_recursively(diff_wc.left_working_copy_path()) .map_err(ExternalToolError::SetUpDir)?; - let instructions_path = diff_wc.right_working_copy_path().join("JJ-INSTRUCTIONS"); + 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_to_cleanup = 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 = - settings.diff_instructions() && !instructions.is_empty() && !instructions_path.exists(); + let add_instructions = settings.diff_instructions() + && !instructions.is_empty() + && !instructions_path_to_cleanup.exists(); if add_instructions { - // 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::SetUpDir)?; - file.write_all(instructions.as_bytes()) + let mut output_instructions_file = + File::create(&instructions_path_to_cleanup).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. + +You are using the experimental 3-pane diff editor config. Some of +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"\ +Please make your edits in this pane. + +You are using the experimental 3-pane diff editor config. Some of +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)?; } @@ -420,17 +509,19 @@ pub fn edit_diff_external( })); } if add_instructions { - std::fs::remove_file(instructions_path).ok(); + std::fs::remove_file(instructions_path_to_cleanup).ok(); } - let mut right_tree_state = diff_wc.right_tree_state; - right_tree_state.snapshot(SnapshotOptions { + 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: settings.fsmonitor_kind()?, progress: None, max_new_file_size: settings.max_new_file_size()?, })?; - Ok(right_tree_state.current_tree_id().clone()) + Ok(output_tree_state.current_tree_id().clone()) } /// Generates textual diff by the specified `tool`, and writes into `writer`. @@ -443,7 +534,7 @@ pub fn generate_diff( tool: &ExternalMergeTool, ) -> Result<(), DiffGenerateError> { let store = left_tree.store(); - let diff_wc = check_out_trees(store, left_tree, right_tree, matcher)?; + let diff_wc = check_out_trees(store, left_tree, right_tree, None, matcher)?; set_readonly_recursively(diff_wc.left_working_copy_path()) .map_err(ExternalToolError::SetUpDir)?; set_readonly_recursively(diff_wc.right_working_copy_path()) diff --git a/cli/tests/test_diffedit_command.rs b/cli/tests/test_diffedit_command.rs index 93d3685db2..ac0a80ced0 100644 --- a/cli/tests/test_diffedit_command.rs +++ b/cli/tests/test_diffedit_command.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use crate::common::TestEnvironment; +use crate::common::{escaped_fake_diff_editor_path, TestEnvironment}; pub mod common; @@ -110,6 +110,122 @@ fn test_diffedit() { "###); } +#[test] +fn test_diffedit_3pane() { + 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"); + + std::fs::write(repo_path.join("file1"), "a\n").unwrap(); + test_env.jj_cmd_success(&repo_path, &["new"]); + std::fs::write(repo_path.join("file2"), "a\n").unwrap(); + std::fs::write(repo_path.join("file3"), "a\n").unwrap(); + test_env.jj_cmd_success(&repo_path, &["new"]); + std::fs::remove_file(repo_path.join("file1")).unwrap(); + std::fs::write(repo_path.join("file2"), "b\n").unwrap(); + + // 2 configs for a 3-pane setup. In the first, "$right" is passed to what the + // fake diff editor considers the "after" state. + let config_with_right_as_after = format!( + r#"ui.diff-editor=["{}", "$left", "$right", "--ignore=$output"]"#, + escaped_fake_diff_editor_path() + ); + let config_with_output_as_after = format!( + r#"ui.diff-editor=["{}", "$left", "$output", "--ignore=$right"]"#, + escaped_fake_diff_editor_path() + ); + let edit_script = test_env.env_root().join("diff_edit_script"); + std::fs::write(&edit_script, "").unwrap(); + test_env.add_env_var("DIFF_EDIT_SCRIPT", edit_script.to_str().unwrap()); + + // Nothing happens if we make no changes + std::fs::write( + &edit_script, + "files-before file1 file2\0files-after JJ-INSTRUCTIONS file2", + ) + .unwrap(); + let stdout = test_env.jj_cmd_success( + &repo_path, + &["diffedit", "--config-toml", &config_with_output_as_after], + ); + insta::assert_snapshot!(stdout, @r###" + Nothing changed. + "###); + let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]); + insta::assert_snapshot!(stdout, @r###" + R file1 + M file2 + "###); + // Nothing happens if we make no changes, `config_with_right_as_after` version + let stdout = test_env.jj_cmd_success( + &repo_path, + &["diffedit", "--config-toml", &config_with_right_as_after], + ); + insta::assert_snapshot!(stdout, @r###" + Nothing changed. + "###); + let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]); + insta::assert_snapshot!(stdout, @r###" + R file1 + M file2 + "###); + + // Can edit changes to individual files + std::fs::write(&edit_script, "reset file2").unwrap(); + let stdout = test_env.jj_cmd_success( + &repo_path, + &["diffedit", "--config-toml", &config_with_output_as_after], + ); + insta::assert_snapshot!(stdout, @r###" + Created kkmpptxz 1930da4a (no description set) + Working copy now at: kkmpptxz 1930da4a (no description set) + Parent commit : rlvkpnrz 613028a4 (no description set) + Added 0 files, modified 1 files, removed 0 files + "###); + let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]); + insta::assert_snapshot!(stdout, @r###" + R file1 + "###); + + // Can write something new to `file1` + test_env.jj_cmd_success(&repo_path, &["undo"]); + std::fs::write(&edit_script, "write file1\nnew content").unwrap(); + let stdout = test_env.jj_cmd_success( + &repo_path, + &["diffedit", "--config-toml", &config_with_output_as_after], + ); + insta::assert_snapshot!(stdout, @r###" + Created kkmpptxz ff2907b6 (no description set) + Working copy now at: kkmpptxz ff2907b6 (no description set) + Parent commit : rlvkpnrz 613028a4 (no description set) + Added 1 files, modified 0 files, removed 0 files + "###); + let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]); + insta::assert_snapshot!(stdout, @r###" + M file1 + M file2 + "###); + + // But nothing happens if we modify the right side + test_env.jj_cmd_success(&repo_path, &["undo"]); + std::fs::write(&edit_script, "write file1\nnew content").unwrap(); + let stdout = test_env.jj_cmd_success( + &repo_path, + &["diffedit", "--config-toml", &config_with_right_as_after], + ); + insta::assert_snapshot!(stdout, @r###" + Nothing changed. + "###); + let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]); + insta::assert_snapshot!(stdout, @r###" + R file1 + M file2 + "###); + + // TODO: test with edit_script of "reset file2". This fails on right side + // since the file is readonly. +} + #[test] fn test_diffedit_merge() { let mut test_env = TestEnvironment::default(); diff --git a/docs/config.md b/docs/config.md index 46027b8d3f..d95118cb33 100644 --- a/docs/config.md +++ b/docs/config.md @@ -374,6 +374,20 @@ merge-tools.kdiff3.edit-args = [ "--merge", "--cs", "CreateBakFiles=0", "$left", "$right"] ``` +### Experimental 3-pane diff editing + +The special `"meld-3"` diff editor sets up Meld to show 3 panes: the sides of +the diff on the left and right, and an editing pane in the middle. This allow +you to see both sides of the original diff while editing. If you use +`ui.diff-editor = "meld-3"`, note that you can still get the 2-pane Meld view +using `jj diff --tool meld`. + +To configure other diff editors, you can include `$output` together with `$left` +and `$right` in `merge-tools.TOOL.edit-args`. `jj` will replace `$output` with +the directory where the diff editor will be expected to put the result of the +user's edits. Initially, the contents of `$output` will be the same as the +contents of `$right`. + ### Setting up `scm-diff-editor` `scm-diff-editor` is a terminal-based diff editor that is part of