cli: resolve: do not use .with_new_file_ids() to construct resolved file value

This reverts a01d0bf "cli: resolve: leave executable bit unchanged when
using external tool", and updates handling of resolved content.

Fixes #6250

Author: yuja

CHANGELOG.md

@@ -40,6 +40,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ### Fixed bugs
 
+* Fixed crash on change-delete conflict resolution.
+  [#6250](https://github.com/jj-vcs/jj/issues/6250)
+
 ### Packaging changes
 
 * Jujutsu now uses

cli/src/merge_tools/external.rs

@@ -9,6 +9,7 @@ use std::sync::Arc;
 use bstr::BString;
 use itertools::Itertools as _;
 use jj_lib::backend::MergedTreeId;
+use jj_lib::backend::TreeValue;
 use jj_lib::conflicts;
 use jj_lib::conflicts::choose_materialized_conflict_marker_len;
 use jj_lib::conflicts::materialize_merge_result_to_bytes_with_marker_len;
@@ -308,13 +309,15 @@ fn run_mergetool_external_single_file(
             ExternalToolError::InvalidConflictMarkers { exit_status },
         ));
     }
-    // Update the file ids only, leaving the executable flags unchanged
-    let new_file_ids = if let Some(resolved) = new_file_ids.as_resolved() {
-        Merge::from_vec(vec![resolved.clone(); conflict.as_slice().len()])
-    } else {
-        new_file_ids
+
+    let new_tree_value = match new_file_ids.into_resolved() {
+        Ok(file_id) => {
+            let executable = file.executable.expect("should have been resolved");
+            Merge::resolved(file_id.map(|id| TreeValue::File { id, executable }))
+        }
+        // Update the file ids only, leaving the executable flags unchanged
+        Err(file_ids) => conflict.with_new_file_ids(&file_ids),
     };
-    let new_tree_value = conflict.with_new_file_ids(&new_file_ids);
     tree_builder.set_or_remove(repo_path.to_owned(), new_tree_value);
     Ok(())
 }

cli/src/merge_tools/mod.rs

@@ -21,6 +21,7 @@ use std::sync::Arc;
 use itertools::Itertools as _;
 use jj_lib::backend::BackendError;
 use jj_lib::backend::MergedTreeId;
+use jj_lib::backend::TreeValue;
 use jj_lib::config::ConfigGetError;
 use jj_lib::config::ConfigGetResultExt as _;
 use jj_lib::config::ConfigNamePathBuf;
@@ -460,17 +461,11 @@ fn pick_conflict_side(
     for merge_tool_file in merge_tool_files {
         // We use file IDs here to match the logic for the other external merge tools.
         // This ensures that the behavior is consistent.
-        let file_id = merge_tool_file.file.ids.get_add(add_index).unwrap();
-        // Update the file IDs only, leaving the executable flags unchanged
-        let new_tree_value = merge_tool_file
-            .conflict
-            .with_new_file_ids(&Merge::from_vec(vec![
-                file_id.clone();
-                merge_tool_file
-                    .conflict
-                    .as_slice()
-                    .len()
-            ]));
+        let file = &merge_tool_file.file;
+        let file_id = file.ids.get_add(add_index).unwrap();
+        let executable = file.executable.expect("should have been resolved");
+        let new_tree_value =
+            Merge::resolved(file_id.clone().map(|id| TreeValue::File { id, executable }));
         tree_builder.set_or_remove(merge_tool_file.repo_path.clone(), new_tree_value);
     }
     tree_builder.write_tree(tree.store())

cli/tests/test_resolve_command.rs

@@ -1048,6 +1048,86 @@ fn test_resolve_conflicts_with_executable() {
     file1    2-sided conflict including an executable
     [EOF]
     ");
+
+    // Pick "our" contents, but merges executable bits
+    work_dir.run_jj(["undo"]).success();
+    let output = work_dir.run_jj(["resolve", "--tool=:ours"]);
+    insta::assert_snapshot!(output, @r"
+    ------- stderr -------
+    Working copy  (@) now at: znkkpsqq 5b59d7f0 conflict | conflict
+    Parent commit (@-)      : mzvwutvl 08932848 a | a
+    Parent commit (@-)      : yqosqzyt b69b3de6 b | b
+    Added 0 files, modified 2 files, removed 0 files
+    [EOF]
+    ");
+    insta::assert_snapshot!(work_dir.run_jj(["diff", "--git"]), @r"
+    diff --git a/file1 b/file1
+    index 0000000000..da0f8ed91a 100755
+    --- a/file1
+    +++ b/file1
+    @@ -1,7 +1,1 @@
+    -<<<<<<< Conflict 1 of 1
+    -%%%%%%% Changes from base to side #1
+    --base1
+    -+a1
+    -+++++++ Contents of side #2
+    -b1
+    ->>>>>>> Conflict 1 of 1 ends
+    +a1
+    diff --git a/file2 b/file2
+    index 0000000000..c1827f07e1 100755
+    --- a/file2
+    +++ b/file2
+    @@ -1,7 +1,1 @@
+    -<<<<<<< Conflict 1 of 1
+    -%%%%%%% Changes from base to side #1
+    --base2
+    -+a2
+    -+++++++ Contents of side #2
+    -b2
+    ->>>>>>> Conflict 1 of 1 ends
+    +a2
+    [EOF]
+    ");
+
+    // Pick "their" contents, but merges executable bits
+    work_dir.run_jj(["undo"]).success();
+    let output = work_dir.run_jj(["resolve", "--tool=:theirs"]);
+    insta::assert_snapshot!(output, @r"
+    ------- stderr -------
+    Working copy  (@) now at: znkkpsqq 087b8637 conflict | conflict
+    Parent commit (@-)      : mzvwutvl 08932848 a | a
+    Parent commit (@-)      : yqosqzyt b69b3de6 b | b
+    Added 0 files, modified 2 files, removed 0 files
+    [EOF]
+    ");
+    insta::assert_snapshot!(work_dir.run_jj(["diff", "--git"]), @r"
+    diff --git a/file1 b/file1
+    index 0000000000..c9c6af7f78 100755
+    --- a/file1
+    +++ b/file1
+    @@ -1,7 +1,1 @@
+    -<<<<<<< Conflict 1 of 1
+    -%%%%%%% Changes from base to side #1
+    --base1
+    -+a1
+    -+++++++ Contents of side #2
+     b1
+    ->>>>>>> Conflict 1 of 1 ends
+    diff --git a/file2 b/file2
+    index 0000000000..e6bfff5c1d 100755
+    --- a/file2
+    +++ b/file2
+    @@ -1,7 +1,1 @@
+    -<<<<<<< Conflict 1 of 1
+    -%%%%%%% Changes from base to side #1
+    --base2
+    -+a2
+    -+++++++ Contents of side #2
+     b2
+    ->>>>>>> Conflict 1 of 1 ends
+    [EOF]
+    ");
 }
 
 #[test]
@@ -1213,7 +1293,19 @@ fn test_resolve_change_delete_executable() {
     insta::assert_snapshot!(output, @"");
     std::fs::write(&editor_script, "write\nresolved\n").unwrap();
     let output = work_dir.run_jj(["resolve", "file2"]);
-    assert_eq!(output.status.code(), Some(101)); // FIXME
+    insta::assert_snapshot!(output, @r"
+    ------- stderr -------
+    Resolving conflicts in: file2
+    Working copy  (@) now at: kmkuslsw cab8be9b conflict | (conflict) conflict
+    Parent commit (@-)      : mzvwutvl 4a44e1a9 a | a
+    Parent commit (@-)      : vruxwmqv 19e7d2ff b | b
+    Added 0 files, modified 1 files, removed 0 files
+    Warning: There are unresolved conflicts at these paths:
+    file3    2-sided conflict including an executable
+    file4    2-sided conflict including 1 deletion
+    file5    2-sided conflict including 1 deletion and an executable
+    [EOF]
+    ");
 
     // Exec bit conflict can be resolved by chmod
     let output = work_dir.run_jj(["resolve", "file3"]);
@@ -1233,17 +1325,34 @@ fn test_resolve_change_delete_executable() {
 
     // Take modified content, the executable bit should be kept as "-"
     let output = work_dir.run_jj(["resolve", "file4", "--tool=:ours"]);
-    assert_eq!(output.status.code(), Some(101)); // FIXME
+    insta::assert_snapshot!(output, @r"
+    ------- stderr -------
+    Working copy  (@) now at: kmkuslsw 14b5c3d2 conflict | (conflict) conflict
+    Parent commit (@-)      : mzvwutvl 4a44e1a9 a | a
+    Parent commit (@-)      : vruxwmqv 19e7d2ff b | b
+    Added 0 files, modified 1 files, removed 0 files
+    Warning: There are unresolved conflicts at these paths:
+    file5    2-sided conflict including 1 deletion and an executable
+    [EOF]
+    ");
 
     // Take modified content, the executable bit should be kept as "x"
     let output = work_dir.run_jj(["resolve", "file5", "--tool=:theirs"]);
-    assert_eq!(output.status.code(), Some(101)); // FIXME
+    insta::assert_snapshot!(output, @r"
+    ------- stderr -------
+    Working copy  (@) now at: kmkuslsw 5de68577 conflict | conflict
+    Parent commit (@-)      : mzvwutvl 4a44e1a9 a | a
+    Parent commit (@-)      : vruxwmqv 19e7d2ff b | b
+    Added 0 files, modified 1 files, removed 0 files
+    Existing conflicts were resolved or abandoned from 1 commits.
+    [EOF]
+    ");
 
     insta::assert_snapshot!(file_list("all()"), @r"
-    file2 c -
+    file2 - -
     file3 - x
-    file4 c -
-    file5 c x
+    file4 - -
+    file5 - x
     [EOF]
     ");
 }