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

files: make merge() require one more adds than removes

Browse source 
All call paths already check before calling the function that the
condition is true. One caller - `tree::try_resolve_file_conflict()` -
checks it itself. The other caller -
`conflicts::materialize_merge_result()` - doesn't, but its callers
have checked it via `extract_file_conflict_as_single_hunk()`.

The deleted comment about empty strings seems to be obsolete since
e48ace56d1. The caller pads the inputs with empty strings since that
commit.

I think we should ideally change this function's signature to make it
impossible to call it with bad inputs, and I hope to get back to that
soon.
This commit is contained in:
Martin von Zweigbergk 2023-05-14 06:38:55 -07:00 committed by Martin von Zweigbergk
parent 3069718c0f
commit 1ec906b114
1 changed files with 9 additions and 22 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

31
lib/src/files.rs
View file

@ -207,11 +207,8 @@ struct SyncRegion {
right: Range<usize>,
}
// TODO: Should we require `add.len() == removes.len() + 1`? If that condition
// is false, it effectively means that we should pretend that there are empty
// strings in `removes` or `adds` to make it true. Maybe we should have to
// caller make it explicitly that way.
pub fn merge(removes: &[&[u8]], adds: &[&[u8]]) -> MergeResult {
assert_eq!(adds.len(), removes.len() + 1);
let num_removes = removes.len();
// TODO: Using the first remove as base (first in the inputs) is how it's
// usually done for 3-way conflicts. Are there better heuristics when there are
@ -225,9 +222,7 @@ pub fn merge(removes: &[&[u8]], adds: &[&[u8]]) -> MergeResult {
for diff_hunk in diff.hunks() {
match diff_hunk {
DiffHunk::Matching(content) => {
if adds.len() > removes.len() {
resolved_hunk.extend(content);
}
resolved_hunk.extend(content);
}
DiffHunk::Different(parts) => {
let mut removed_parts = parts[..num_removes].to_vec();
@ -338,28 +333,28 @@ mod tests {
);
// All sides added same content
assert_eq!(
merge(&[], &[b"a\n", b"a\n", b"a\n"]),
merge(&[b"", b""], &[b"a\n", b"a\n", b"a\n"]),
MergeResult::Resolved(b"a\n".to_vec())
);
// One side modified, two sides added
assert_eq!(
merge(&[b"a"], &[b"b", b"b", b"b"]),
merge(&[b"a", b""], &[b"b", b"b", b"b"]),
MergeResult::Conflict(vec![MergeHunk::Conflict(ConflictHunk {
removes: vec![b"a".to_vec()],
removes: vec![b"a".to_vec(), b"".to_vec()],
adds: vec![b"b".to_vec(), b"b".to_vec(), b"b".to_vec()]
})])
);
// All sides removed same content
assert_eq!(
merge(&[b"a\n", b"a\n", b"a\n"], &[]),
merge(&[b"a\n", b"a\n", b"a\n"], &[b"", b"", b"", b""]),
MergeResult::Resolved(b"".to_vec())
);
// One side modified, two sides removed
assert_eq!(
merge(&[b"a\n", b"a\n", b"a\n"], &[b""]),
merge(&[b"a\n", b"a\n"], &[b"b\n", b"", b""]),
MergeResult::Conflict(vec![MergeHunk::Conflict(ConflictHunk {
removes: vec![b"a\n".to_vec(), b"a\n".to_vec(), b"a\n".to_vec()],
adds: vec![b"".to_vec()]
removes: vec![b"a\n".to_vec(), b"a\n".to_vec()],
adds: vec![b"b\n".to_vec(), b"".to_vec(), b"".to_vec()]
})])
);
// Three sides made the same change
@ -367,14 +362,6 @@ mod tests {
merge(&[b"a", b"a"], &[b"b", b"b", b"b"]),
MergeResult::Resolved(b"b".to_vec())
);
// One side unchanged, one side added
assert_eq!(
merge(&[b"a\n"], &[b"a\nb\n"]),
MergeResult::Conflict(vec![MergeHunk::Conflict(ConflictHunk {
removes: vec![b"".to_vec()],
adds: vec![b"b\n".to_vec()]
})])
);
// Two sides left one line unchanged, and added conflicting additional lines
assert_eq!(
merge(&[b"a\n"], &[b"a\nb\n", b"a\nc\n"]),