merge_tools: Allow 3-pane diff editing

As discussed in #1905 (reply in thread)

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.
 

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.

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<TreeState>,
 }
 
 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<Store>,
     left_tree: &Tree,
     right_tree: &Tree,
+    output_is: Option<DiffSide>,
     matcher: &dyn Matcher,
 ) -> Result<DiffWorkingCopies, DiffCheckoutError> {
     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<V: AsRef<str>>(
 }
 
 /// Return all variable names found in the args, without the dollar sign
-#[allow(unused)]
 fn find_all_variables(args: &[String]) -> Vec<String> {
     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<GitIgnoreFile>,
     settings: &UserSettings,
 ) -> Result<TreeId, DiffEditError> {
+    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())

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();

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