From 274c161d293ce5011c94ba1f5502b065692a9601 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Tue, 15 Aug 2023 17:54:03 -0700 Subject: [PATCH 1/4] cli_utils: reduce duplication in `select_diff` --- cli/src/cli_util.rs | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index d7d703224c..099b9f5628 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -1485,16 +1485,7 @@ impl WorkspaceCommandTransaction<'_> { matcher: &dyn Matcher, ) -> Result { if interactive { - let base_ignores = self.helper.base_ignores(); - let settings = &self.helper.settings; - Ok(crate::merge_tools::edit_diff( - ui, - left_tree, - right_tree, - instructions, - base_ignores, - settings, - )?) + self.edit_diff(ui, left_tree, right_tree, instructions) } else if matcher.visit(&RepoPath::root()) == Visit::AllRecursively { // Optimization for a common case Ok(right_tree.id().clone()) From 81a148275ce2fd87b33b1d3d6455bb3cb916b7e4 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Wed, 16 Aug 2023 17:19:24 -0700 Subject: [PATCH 2/4] fake_diff_editor: Allow specifying extra arguments to be ignored --- cli/testing/fake-diff-editor.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cli/testing/fake-diff-editor.rs b/cli/testing/fake-diff-editor.rs index e4a98d2f8f..c76e17f56a 100644 --- a/cli/testing/fake-diff-editor.rs +++ b/cli/testing/fake-diff-editor.rs @@ -28,6 +28,10 @@ struct Args { /// Path to the "after" directory after: PathBuf, + + /// Ignored argument + #[arg(long)] + _ignore: Vec, } fn files_recursively(dir: &Path) -> HashSet { From e9e4a6f9b310f7f42d10f9ac20457628bbf71a1b Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Mon, 7 Aug 2023 16:40:10 -0700 Subject: [PATCH 3/4] merge_tools: function to extract all variables from tool arguments To be used in the next commit --- cli/src/merge_tools/external.rs | 52 ++++++++++++++++++++++++++------- 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index 68bbfa9d56..50791e01aa 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -344,27 +344,39 @@ pub fn run_mergetool_external( Ok(tree_builder.write_tree()) } +// Not interested in $UPPER_CASE_VARIABLES +static VARIABLE_REGEX: once_cell::sync::Lazy = + once_cell::sync::Lazy::new(|| Regex::new(r"\$([a-z0-9_]+)\b").unwrap()); + fn interpolate_variables>( args: &[String], variables: &HashMap<&str, V>, ) -> Vec { - // Not interested in $UPPER_CASE_VARIABLES - let re = Regex::new(r"\$([a-z0-9_]+)\b").unwrap(); args.iter() .map(|arg| { - re.replace_all(arg, |caps: &Captures| { - let name = &caps[1]; - if let Some(subst) = variables.get(name) { - subst.as_ref().to_owned() - } else { - caps[0].to_owned() - } - }) - .into_owned() + VARIABLE_REGEX + .replace_all(arg, |caps: &Captures| { + let name = &caps[1]; + if let Some(subst) = variables.get(name) { + subst.as_ref().to_owned() + } else { + caps[0].to_owned() + } + }) + .into_owned() }) .collect() } +/// 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.find_iter(arg)) + .map(|single_match| single_match.as_str()[1..].to_owned()) + .collect() +} + pub fn edit_diff_external( editor: ExternalMergeTool, left_tree: &Tree, @@ -515,4 +527,22 @@ mod tests { ["$left $right"], ); } + + #[test] + fn test_find_all_variables() { + assert_eq!( + find_all_variables( + &[ + "$left", + "$right", + "--two=$1 and $2", + "--can-be-part-of-string=$output", + "$NOT_CAPITALS", + "--can-repeat=$right" + ] + .map(ToOwned::to_owned), + ), + ["left", "right", "1", "2", "output", "right"], + ); + } } From a9dcf678e48e3d129a6032669ad0437f17710464 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Mon, 7 Aug 2023 16:40:10 -0700 Subject: [PATCH 4/4] 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 77c6834dc9..77a12e72a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -71,6 +71,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 50791e01aa..26e431a117 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, }) } @@ -369,7 +409,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.find_iter(arg)) @@ -385,20 +424,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)?; } @@ -418,17 +507,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`. @@ -441,7 +532,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 aa8426bab3..c5f9554dd4 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; @@ -173,6 +173,122 @@ fn test_diffedit_new_file() { "###); } +#[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