ok/jj
1
0
Fork 0
forked from mirrors/jj
Code Pull requests Activity

trees: try to resolve file conflicts after simplifying conflicts

Browse source 
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.
This commit is contained in:
Martin von Zweigbergk 2021-10-20 22:09:09 -07:00
parent f1e2124a64
commit c0ae4b16e8
3 changed files with 209 additions and 86 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

9
lib/src/testutils.rs
View file

@ -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()
}

202
lib/src/tree.rs
View file

@ -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<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(
store: &Store,
conflict: &Conflict,
) -> Result<Conflict, BackendError> {
fn simplify_conflict(store: &Store, conflict: &Conflict) -> Result<Conflict, BackendError> {
// Important cases to simplify:
//
// D

84
lib/tests/test_merge_trees.rs
View file

@ -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.