trees: try to resolve file conflicts after simplifying conflicts

This fixes a bug I've run into somewhat frequently. What happens is
that if you have a conflict on top of another conflict and you resolve
the conflict in the bottom commit, we just simplify the `Conflict`
object in the second commit, but we don't try to resolve the new
conflict. That shows up as an unexpected "conflict" in `jj log`
output, and when you check out the commit, there are actually no
conflicts, so you can just `jj squash` right away.

This patch fixes that bug. It also teaches the code to work with more
than 3 parts in the merge, so if there's a 5-way conflict, for
example, we still try to resolve it if possible.

lib/src/testutils.rs

@@ -14,7 +14,7 @@
 
 use std::fs;
 use std::fs::OpenOptions;
-use std::io::Write;
+use std::io::{Read, Write};
 use std::sync::Arc;
 
 use itertools::Itertools;
@@ -63,6 +63,13 @@ pub fn init_repo(settings: &UserSettings, use_git: bool) -> (TempDir, Arc<Readon
     (temp_dir, repo)
 }
 
+pub fn read_file(store: &Store, path: &RepoPath, id: &FileId) -> Vec<u8> {
+    let mut reader = store.read_file(path, id).unwrap();
+    let mut content = vec![];
+    reader.read_to_end(&mut content).unwrap();
+    content
+}
+
 pub fn write_file(store: &Store, path: &RepoPath, contents: &str) -> FileId {
     store.write_file(path, &mut contents.as_bytes()).unwrap()
 }

lib/src/tree.rs

@@ -19,6 +19,8 @@ use std::iter::Peekable;
 use std::pin::Pin;
 use std::sync::Arc;
 
+use itertools::Itertools;
+
 use crate::backend::{
     BackendError, Conflict, ConflictId, ConflictPart, TreeEntriesNonRecursiveIter, TreeEntry,
     TreeId, TreeValue,
@@ -546,6 +548,7 @@ fn merge_tree_value(
     //   * try to resolve file conflicts by merging the file contents
     //   * leave other conflicts (e.g. file/dir conflicts, remove/modify conflicts)
     //     unresolved
+
     Ok(match (maybe_base, maybe_side1, maybe_side2) {
         (
             Some(TreeValue::Tree(base)),
@@ -565,92 +568,124 @@ fn merge_tree_value(
             }
         }
         _ => {
-            let maybe_merged = match (maybe_base, maybe_side1, maybe_side2) {
+            // Start by creating a Conflict object. Conflicts can cleanly represent a single
-                (
+            // resolved state, the absence of a state, or a conflicted state.
-                    Some(TreeValue::Normal {
+            let mut conflict = Conflict::default();
-                        id: base_id,
+            if let Some(base) = maybe_base {
-                        executable: base_executable,
+                conflict.removes.push(ConflictPart {
-                    }),
+                    value: base.clone(),
-                    Some(TreeValue::Normal {
+                });
-                        id: side1_id,
+            }
-                        executable: side1_executable,
+            if let Some(side1) = maybe_side1 {
-                    }),
+                conflict.adds.push(ConflictPart {
-                    Some(TreeValue::Normal {
+                    value: side1.clone(),
-                        id: side2_id,
+                });
-                        executable: side2_executable,
+            }
-                    }),
+            if let Some(side2) = maybe_side2 {
-                ) => {
+                conflict.adds.push(ConflictPart {
-                    let executable = if base_executable == side1_executable {
+                    value: side2.clone(),
-                        *side2_executable
+                });
-                    } else if base_executable == side2_executable {
+            }
-                        *side1_executable
+            let conflict = simplify_conflict(store, &conflict)?;
-                    } else {
+            if conflict.adds.is_empty() {
-                        assert_eq!(side1_executable, side2_executable);
+                // If there are no values to add, then the path doesn't exist
-                        *side1_executable
+                return Ok(None);
-                    };
+            }
-
+            if conflict.removes.is_empty() && conflict.adds.len() == 1 {
-                    let filename = dir.join(basename);
+                // A single add means that the current state is that state.
-                    let mut base_content = vec![];
+                return Ok(Some(conflict.adds[0].value.clone()));
-                    store
+            }
-                        .read_file(&filename, base_id)?
+            let filename = dir.join(basename);
-                        .read_to_end(&mut base_content)?;
+            if let Some((merged_content, executable)) =
-                    let mut side1_content = vec![];
+                try_resolve_file_conflict(store, &filename, &conflict)?
-                    store
+            {
-                        .read_file(&filename, side1_id)?
+                let id = store.write_file(&filename, &mut merged_content.as_slice())?;
-                        .read_to_end(&mut side1_content)?;
+                Some(TreeValue::Normal { id, executable })
-                    let mut side2_content = vec![];
+            } else {
-                    store
+                let conflict_id = store.write_conflict(&conflict)?;
-                        .read_file(&filename, side2_id)?
+                Some(TreeValue::Conflict(conflict_id))
-                        .read_to_end(&mut side2_content)?;
-
-                    let merge_result =
-                        files::merge(&[&base_content], &[&side1_content, &side2_content]);
-                    match merge_result {
-                        MergeResult::Resolved(merged_content) => {
-                            let id = store.write_file(&filename, &mut merged_content.as_slice())?;
-                            Some(TreeValue::Normal { id, executable })
-                        }
-                        MergeResult::Conflict(_) => None,
-                    }
-                }
-                _ => None,
-            };
-            match maybe_merged {
-                Some(merged) => Some(merged),
-                None => {
-                    let mut conflict = Conflict::default();
-                    if let Some(base) = maybe_base {
-                        conflict.removes.push(ConflictPart {
-                            value: base.clone(),
-                        });
-                    }
-                    if let Some(side1) = maybe_side1 {
-                        conflict.adds.push(ConflictPart {
-                            value: side1.clone(),
-                        });
-                    }
-                    if let Some(side2) = maybe_side2 {
-                        conflict.adds.push(ConflictPart {
-                            value: side2.clone(),
-                        });
-                    }
-                    let conflict = simplify_conflict(store, &conflict)?;
-                    if conflict.adds.is_empty() {
-                        // If there are no values to add, then the path doesn't exist
-                        None
-                    } else if conflict.removes.is_empty() && conflict.adds.len() == 1 {
-                        // A single add means that the current state is that state.
-                        Some(conflict.adds[0].value.clone())
-                    } else {
-                        let conflict_id = store.write_conflict(&conflict)?;
-                        Some(TreeValue::Conflict(conflict_id))
-                    }
-                }
             }
         }
     })
 }
 
+fn try_resolve_file_conflict(
+    store: &Store,
+    filename: &RepoPath,
+    conflict: &Conflict,
+) -> Result<Option<(Vec<u8>, bool)>, BackendError> {
+    // If there are any non-file parts in the conflict, we can't merge it. We check
+    // early so we don't waste time reading file contents if we can't merge them
+    // anyway. At the same time we determine whether the resulting file should
+    // be executable.
+    let mut exec_delta = 0;
+    let mut regular_delta = 0;
+    let mut removed_file_ids = vec![];
+    let mut added_file_ids = vec![];
+    for part in &conflict.removes {
+        match &part.value {
+            TreeValue::Normal { id, executable } => {
+                if *executable {
+                    exec_delta -= 1;
+                } else {
+                    regular_delta -= 1;
+                }
+                removed_file_ids.push(id.clone());
+            }
+            _ => {
+                return Ok(None);
+            }
+        }
+    }
+    for part in &conflict.adds {
+        match &part.value {
+            TreeValue::Normal { id, executable } => {
+                if *executable {
+                    exec_delta += 1;
+                } else {
+                    regular_delta += 1;
+                }
+                added_file_ids.push(id.clone());
+            }
+            _ => {
+                return Ok(None);
+            }
+        }
+    }
+    let executable = if exec_delta > 0 && regular_delta <= 0 {
+        true
+    } else if regular_delta > 0 && exec_delta <= 0 {
+        false
+    } else {
+        // We're unable to determine whether the result should be executable
+        return Ok(None);
+    };
+    let mut removed_contents = vec![];
+    let mut added_contents = vec![];
+    for file_id in removed_file_ids {
+        let mut content = vec![];
+        store
+            .read_file(filename, &file_id)?
+            .read_to_end(&mut content)?;
+        removed_contents.push(content);
+    }
+    for file_id in added_file_ids {
+        let mut content = vec![];
+        store
+            .read_file(filename, &file_id)?
+            .read_to_end(&mut content)?;
+        added_contents.push(content);
+    }
+    let merge_result = files::merge(
+        &removed_contents.iter().map(Vec::as_slice).collect_vec(),
+        &added_contents.iter().map(Vec::as_slice).collect_vec(),
+    );
+    match merge_result {
+        MergeResult::Resolved(merged_content) => Ok(Some((merged_content, executable))),
+        MergeResult::Conflict(_) => Ok(None),
+    }
+}
+
 fn conflict_part_to_conflict(store: &Store, part: &ConflictPart) -> Result<Conflict, BackendError> {
     match &part.value {
         TreeValue::Conflict(id) => {
@@ -666,10 +701,7 @@ fn conflict_part_to_conflict(store: &Store, part: &ConflictPart) -> Result<Confl
     }
 }
 
-fn simplify_conflict(
+fn simplify_conflict(store: &Store, conflict: &Conflict) -> Result<Conflict, BackendError> {
-    store: &Store,
-    conflict: &Conflict,
-) -> Result<Conflict, BackendError> {
     // Important cases to simplify:
     //
     // D

lib/tests/test_merge_trees.rs

@@ -12,9 +12,15 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#![feature(assert_matches)]
+
+use std::assert_matches::assert_matches;
+
 use itertools::Itertools;
 use jujutsu_lib::backend::{ConflictPart, TreeValue};
+use jujutsu_lib::commit_builder::CommitBuilder;
 use jujutsu_lib::repo_path::{RepoPath, RepoPathComponent};
+use jujutsu_lib::rewrite::rebase_commit;
 use jujutsu_lib::tree::Tree;
 use jujutsu_lib::{testutils, tree};
 use test_case::test_case;
@@ -558,3 +564,81 @@ fn test_simplify_conflict(use_git: bool) {
         _ => panic!("unexpected value"),
     };
 }
+
+#[test_case(false ; "local backend")]
+#[test_case(true ; "git backend")]
+fn test_simplify_conflict_after_resolving_parent(use_git: bool) {
+    let settings = testutils::user_settings();
+    let (_temp_dir, repo) = testutils::init_repo(&settings, use_git);
+
+    // Set up a repo like this:
+    // D
+    // | C
+    // | B
+    // |/
+    // A
+    //
+    // Commit A has a file with 3 lines. B and D make conflicting changes to the
+    // first line. C changes the third line. We then rebase B and C onto D,
+    // which creates a conflict. We resolve the conflict in the first line and
+    // rebase C2 (the rebased C) onto the resolved conflict. C3 should not have
+    // a conflict since it changed an unrelated line.
+    let path = RepoPath::from_internal_string("dir/file");
+    let mut tx = repo.start_transaction("test");
+    let tree_a = testutils::create_tree(&repo, &[(&path, "abc\ndef\nghi\n")]);
+    let commit_a = CommitBuilder::for_new_commit(&settings, repo.store(), tree_a.id().clone())
+        .write_to_repo(tx.mut_repo());
+    let tree_b = testutils::create_tree(&repo, &[(&path, "Abc\ndef\nghi\n")]);
+    let commit_b = CommitBuilder::for_new_commit(&settings, repo.store(), tree_b.id().clone())
+        .set_parents(vec![commit_a.id().clone()])
+        .write_to_repo(tx.mut_repo());
+    let tree_c = testutils::create_tree(&repo, &[(&path, "Abc\ndef\nGhi\n")]);
+    let commit_c = CommitBuilder::for_new_commit(&settings, repo.store(), tree_c.id().clone())
+        .set_parents(vec![commit_b.id().clone()])
+        .write_to_repo(tx.mut_repo());
+    let tree_d = testutils::create_tree(&repo, &[(&path, "abC\ndef\nghi\n")]);
+    let commit_d = CommitBuilder::for_new_commit(&settings, repo.store(), tree_d.id().clone())
+        .set_parents(vec![commit_a.id().clone()])
+        .write_to_repo(tx.mut_repo());
+
+    let commit_b2 = rebase_commit(&settings, tx.mut_repo(), &commit_b, &[commit_d]);
+    let commit_c2 = rebase_commit(&settings, tx.mut_repo(), &commit_c, &[commit_b2.clone()]);
+
+    // Test the setup: Both B and C should have conflicts.
+    assert_matches!(
+        commit_b2.tree().path_value(&path),
+        Some(TreeValue::Conflict(_))
+    );
+    assert_matches!(
+        commit_c2.tree().path_value(&path),
+        Some(TreeValue::Conflict(_))
+    );
+
+    // Create the resolved B and rebase C on top.
+    let tree_b3 = testutils::create_tree(&repo, &[(&path, "AbC\ndef\nghi\n")]);
+    let commit_b3 = CommitBuilder::for_rewrite_from(&settings, repo.store(), &commit_b2)
+        .set_tree(tree_b3.id().clone())
+        .write_to_repo(tx.mut_repo());
+    let commit_c3 = rebase_commit(&settings, tx.mut_repo(), &commit_c2, &[commit_b3]);
+    let repo = tx.commit();
+
+    // The conflict should now be resolved.
+    let resolved_value = commit_c3.tree().path_value(&path);
+    match resolved_value {
+        Some(TreeValue::Normal {
+            id,
+            executable: false,
+        }) => {
+            assert_eq!(
+                testutils::read_file(repo.store(), &path, &id).as_slice(),
+                b"AbC\ndef\nGhi\n"
+            );
+        }
+        other => {
+            panic!("unexpected value: {:#?}", other);
+        }
+    }
+}
+
+// TODO: Add tests for simplification of multi-way conflicts. Both the content
+// and the executable bit need testing.