cli: fix interactive diff selection to not retain unmatched files

This patch doesn't fix DiffEditor::edit() API, which is fundamentally broken if matcher argument is specified. I'm not sure if the builtin behavior is correct or not. Suppose we add "jj diffedit FILESETS..", it would probably make sense to leave unmatched paths unmodified because it is the command to edit the destination (or right) tree. This is the edit_diff_external() behavior. Fixes #5252
jj-vcs · Jan 4, 2025 · 0d022f1 · 0d022f1
1 parent c99c97c
commit 0d022f1
Show file tree

Hide file tree

Showing 5 changed files with 160 additions and 2 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -36,6 +36,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ### Fixed bugs
 
+* Fixed diff selection by external tools with `jj split`/`commit -i FILESETS`.
+  [#5252](https://github.com/jj-vcs/jj/issues/5252)
+
 ## [0.25.0] - 2025-01-01
 
 ### Release highlights

diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs
@@ -2862,17 +2862,24 @@ impl DiffSelector {
 
     /// Restores diffs from the `right_tree` to the `left_tree` by using an
     /// interactive editor if enabled.
+    ///
+    /// Only files matching the `matcher` will be copied to the new tree.
     pub fn select(
         &self,
         left_tree: &MergedTree,
         right_tree: &MergedTree,
         matcher: &dyn Matcher,
         format_instructions: impl FnOnce() -> String,
     ) -> Result<MergedTreeId, CommandError> {
+        let selected_tree_id = restore_tree(right_tree, left_tree, matcher)?;
         match self {
-            DiffSelector::NonInteractive => Ok(restore_tree(right_tree, left_tree, matcher)?),
+            DiffSelector::NonInteractive => Ok(selected_tree_id),
             DiffSelector::Interactive(editor) => {
-                Ok(editor.edit(left_tree, right_tree, matcher, format_instructions)?)
+                // edit_diff_external() is designed to edit the right tree,
+                // whereas we want to update the left tree. Unmatched paths
+                // shouldn't be based off the right tree.
+                let right_tree = right_tree.store().get_root_tree(&selected_tree_id)?;
+                Ok(editor.edit(left_tree, &right_tree, matcher, format_instructions)?)
             }
         }
     }

diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs
@@ -227,6 +227,13 @@ impl DiffEditor {
     }
 
     /// Starts a diff editor on the two directories.
+    // FIXME: edit_diff_builtin() applies diff on left_tree to create new tree,
+    // whereas edit_diff_external() updates the right_tree. This means that the
+    // matcher argument is interpreted quite differently. For the builtin tool,
+    // it specifies the maximum set of files to be copied from the right tree.
+    // For the external tool, it specifies the files to be modified in the right
+    // tree. If we adopt the interpretation of the builtin tool,
+    // DiffSelector::select() should also be updated.
     pub fn edit(
         &self,
         left_tree: &MergedTree,

diff --git a/cli/tests/test_commit_command.rs b/cli/tests/test_commit_command.rs
@@ -150,6 +150,76 @@ fn test_commit_interactive() {
 
     JJ: Lines starting with "JJ:" (like this one) will be removed.
     "###);
+
+    let stdout = test_env.jj_cmd_success(&workspace_path, &["log", "--summary"]);
+    insta::assert_snapshot!(stdout, @r"
+    @  mzvwutvl [email protected] 2001-02-03 08:05:11 21b846a6
+    │  (no description set)
+    │  A file2
+    ○  qpvuntsm [email protected] 2001-02-03 08:05:11 7d156390
+    │  add files
+    │  A file1
+    ◆  zzzzzzzz root() 00000000
+    ");
+}
+
+#[test]
+fn test_commit_interactive_with_paths() {
+    let mut test_env = TestEnvironment::default();
+    test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
+    let workspace_path = test_env.env_root().join("repo");
+
+    std::fs::write(workspace_path.join("file2"), "").unwrap();
+    std::fs::write(workspace_path.join("file3"), "").unwrap();
+    test_env.jj_cmd_ok(&workspace_path, &["new", "-medit"]);
+    std::fs::write(workspace_path.join("file1"), "foo\n").unwrap();
+    std::fs::write(workspace_path.join("file2"), "bar\n").unwrap();
+    std::fs::write(workspace_path.join("file3"), "baz\n").unwrap();
+
+    let edit_script = test_env.set_up_fake_editor();
+    std::fs::write(edit_script, ["dump editor"].join("\0")).unwrap();
+    let diff_editor = test_env.set_up_fake_diff_editor();
+    let diff_script = [
+        "files-before file2",
+        "files-after JJ-INSTRUCTIONS file1 file2",
+        "reset file2",
+    ]
+    .join("\0");
+    std::fs::write(diff_editor, diff_script).unwrap();
+
+    // Select file1 and file2 by args, then select file1 interactively
+    let (_stdout, stderr) =
+        test_env.jj_cmd_ok(&workspace_path, &["commit", "-i", "file1", "file2"]);
+    insta::assert_snapshot!(stderr, @r"
+    Working copy now at: kkmpptxz f3e6062e (no description set)
+    Parent commit      : rlvkpnrz 9453cb28 edit
+    ");
+
+    insta::assert_snapshot!(
+        std::fs::read_to_string(test_env.env_root().join("editor")).unwrap(), @r#"
+    edit
+
+    JJ: This commit contains the following changes:
+    JJ:     A file1
+
+    JJ: Lines starting with "JJ:" (like this one) will be removed.
+    "#);
+
+    let stdout = test_env.jj_cmd_success(&workspace_path, &["log", "--summary"]);
+    insta::assert_snapshot!(stdout, @r"
+    @  kkmpptxz [email protected] 2001-02-03 08:05:09 f3e6062e
+    │  (no description set)
+    │  M file2
+    │  M file3
+    ○  rlvkpnrz [email protected] 2001-02-03 08:05:09 9453cb28
+    │  edit
+    │  A file1
+    ○  qpvuntsm [email protected] 2001-02-03 08:05:08 497ed465
+    │  (no description set)
+    │  A file2
+    │  A file3
+    ◆  zzzzzzzz root() 00000000
+    ");
 }
 
 #[test]

diff --git a/cli/tests/test_split_command.rs b/cli/tests/test_split_command.rs
@@ -603,4 +603,75 @@ fn test_split_interactive() {
     Working copy now at: rlvkpnrz 9ed12e4c (no description set)
     Parent commit      : qpvuntsm 0e15949e (no description set)
     "###);
+
+    let stdout = test_env.jj_cmd_success(&workspace_path, &["log", "--summary"]);
+    insta::assert_snapshot!(stdout, @r"
+    @  rlvkpnrz [email protected] 2001-02-03 08:05:08 9ed12e4c
+    │  (no description set)
+    │  A file2
+    ○  qpvuntsm [email protected] 2001-02-03 08:05:08 0e15949e
+    │  (no description set)
+    │  A file1
+    ◆  zzzzzzzz root() 00000000
+    ");
+}
+
+#[test]
+fn test_split_interactive_with_paths() {
+    let mut test_env = TestEnvironment::default();
+    test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
+    let workspace_path = test_env.env_root().join("repo");
+
+    std::fs::write(workspace_path.join("file2"), "").unwrap();
+    std::fs::write(workspace_path.join("file3"), "").unwrap();
+    test_env.jj_cmd_ok(&workspace_path, &["new"]);
+    std::fs::write(workspace_path.join("file1"), "foo\n").unwrap();
+    std::fs::write(workspace_path.join("file2"), "bar\n").unwrap();
+    std::fs::write(workspace_path.join("file3"), "baz\n").unwrap();
+
+    let edit_script = test_env.set_up_fake_editor();
+    std::fs::write(edit_script, ["dump editor"].join("\0")).unwrap();
+    let diff_editor = test_env.set_up_fake_diff_editor();
+    let diff_script = [
+        "files-before file2",
+        "files-after JJ-INSTRUCTIONS file1 file2",
+        "reset file2",
+    ]
+    .join("\0");
+    std::fs::write(diff_editor, diff_script).unwrap();
+
+    // Select file1 and file2 by args, then select file1 interactively
+    let (_stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split", "-i", "file1", "file2"]);
+    insta::assert_snapshot!(stderr, @r"
+    First part: rlvkpnrz e3d766b8 (no description set)
+    Second part: kkmpptxz 4cf22d3b (no description set)
+    Working copy now at: kkmpptxz 4cf22d3b (no description set)
+    Parent commit      : rlvkpnrz e3d766b8 (no description set)
+    ");
+
+    insta::assert_snapshot!(
+        std::fs::read_to_string(test_env.env_root().join("editor")).unwrap(), @r###"
+    JJ: Enter a description for the first commit.
+
+    JJ: This commit contains the following changes:
+    JJ:     A file1
+
+    JJ: Lines starting with "JJ:" (like this one) will be removed.
+    "###);
+
+    let stdout = test_env.jj_cmd_success(&workspace_path, &["log", "--summary"]);
+    insta::assert_snapshot!(stdout, @r"
+    @  kkmpptxz [email protected] 2001-02-03 08:05:09 4cf22d3b
+    │  (no description set)
+    │  M file2
+    │  M file3
+    ○  rlvkpnrz [email protected] 2001-02-03 08:05:09 e3d766b8
+    │  (no description set)
+    │  A file1
+    ○  qpvuntsm [email protected] 2001-02-03 08:05:08 497ed465
+    │  (no description set)
+    │  A file2
+    │  A file3
+    ◆  zzzzzzzz root() 00000000
+    ");
 }