diff --git a/lib/src/testutils.rs b/lib/src/testutils.rs index 27d36d159..6c1773751 100644 --- a/lib/src/testutils.rs +++ b/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 Vec { + 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() } diff --git a/lib/src/tree.rs b/lib/src/tree.rs index 865d96276..e3fa5edbe 100644 --- a/lib/src/tree.rs +++ b/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) { - ( - Some(TreeValue::Normal { - id: base_id, - executable: base_executable, - }), - Some(TreeValue::Normal { - id: side1_id, - executable: side1_executable, - }), - Some(TreeValue::Normal { - id: side2_id, - executable: side2_executable, - }), - ) => { - let executable = if base_executable == side1_executable { - *side2_executable - } else if base_executable == side2_executable { - *side1_executable - } else { - assert_eq!(side1_executable, side2_executable); - *side1_executable - }; - - let filename = dir.join(basename); - let mut base_content = vec![]; - store - .read_file(&filename, base_id)? - .read_to_end(&mut base_content)?; - let mut side1_content = vec![]; - store - .read_file(&filename, side1_id)? - .read_to_end(&mut side1_content)?; - let mut side2_content = vec![]; - store - .read_file(&filename, side2_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)) - } - } + // Start by creating a Conflict object. Conflicts can cleanly represent a single + // resolved state, the absence of a state, or a conflicted state. + 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 + return Ok(None); + } + if conflict.removes.is_empty() && conflict.adds.len() == 1 { + // A single add means that the current state is that state. + return Ok(Some(conflict.adds[0].value.clone())); + } + let filename = dir.join(basename); + if let Some((merged_content, executable)) = + try_resolve_file_conflict(store, &filename, &conflict)? + { + let id = store.write_file(&filename, &mut merged_content.as_slice())?; + Some(TreeValue::Normal { id, executable }) + } 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, 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 { match &part.value { TreeValue::Conflict(id) => { @@ -666,10 +701,7 @@ fn conflict_part_to_conflict(store: &Store, part: &ConflictPart) -> Result Result { +fn simplify_conflict(store: &Store, conflict: &Conflict) -> Result { // Important cases to simplify: // // D diff --git a/lib/tests/test_merge_trees.rs b/lib/tests/test_merge_trees.rs index 8f3c78c77..393c0773e 100644 --- a/lib/tests/test_merge_trees.rs +++ b/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.