From 779b8ba318a4f2480a442839dfa6299c54cd18dd Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Tue, 27 Jun 2023 12:56:44 -0700 Subject: [PATCH] files: replace uses of `MergeHunk` by `Conflict` Since `Conflict`s can represent the resolved state, so `Conflict` can represent the states that we use `MergeHunk` for. `MergeHunk` does force the user to handle the resolved case, which may be useful. I suppose one could use the same argument for making `Conflict` an enum, i.e. if we think that `MergeHunk`'s two variants are beneficial, then we should consider making `Conflict` an enum with those two variants. --- lib/src/conflicts.rs | 161 +++++++++++++++++------------------- lib/src/files.rs | 66 ++++++--------- lib/tests/test_conflicts.rs | 129 +++++++++++++++-------------- 3 files changed, 173 insertions(+), 183 deletions(-) diff --git a/lib/src/conflicts.rs b/lib/src/conflicts.rs index d11f7b9882..ce10916b86 100644 --- a/lib/src/conflicts.rs +++ b/lib/src/conflicts.rs @@ -19,7 +19,7 @@ use itertools::Itertools; use crate::backend::{BackendResult, FileId, ObjectId, TreeValue}; use crate::diff::{find_line_ranges, Diff, DiffHunk}; -use crate::files::{ContentHunk, MergeHunk, MergeResult}; +use crate::files::{ContentHunk, MergeResult}; use crate::merge::trivial_merge; use crate::repo_path::RepoPath; use crate::store::Store; @@ -48,6 +48,10 @@ impl Conflict { Conflict { removes, adds } } + pub fn resolved(value: T) -> Self { + Conflict::new(vec![], vec![value]) + } + /// Create a `Conflict` from a `removes` and `adds`, padding with `None` to /// make sure that there is exactly one more `adds` than `removes`. pub fn from_legacy_form(removes: Vec, adds: Vec) -> Conflict> { @@ -242,23 +246,20 @@ impl Conflict> { return Ok(None); }; for hunk in hunks { - match hunk { - MergeHunk::Resolved(slice) => { - for buf in &mut removed_content { - buf.extend_from_slice(&slice.0); - } - for buf in &mut added_content { - buf.extend_from_slice(&slice.0); - } + if let Some(slice) = hunk.as_resolved() { + for buf in &mut removed_content { + buf.extend_from_slice(&slice.0); } - MergeHunk::Conflict(conflict) => { - let (removes, adds) = conflict.take(); - for (i, buf) in removes.into_iter().enumerate() { - removed_content[i].extend(buf.0); - } - for (i, buf) in adds.into_iter().enumerate() { - added_content[i].extend(buf.0); - } + for buf in &mut added_content { + buf.extend_from_slice(&slice.0); + } + } else { + let (removes, adds) = hunk.take(); + for (i, buf) in removes.into_iter().enumerate() { + removed_content[i].extend(buf.0); + } + for (i, buf) in adds.into_iter().enumerate() { + added_content[i].extend(buf.0); } } } @@ -423,60 +424,56 @@ pub fn materialize_merge_result( } MergeResult::Conflict(hunks) => { for hunk in hunks { - match hunk { - MergeHunk::Resolved(content) => { - output.write_all(&content.0)?; - } - MergeHunk::Conflict(conflict) => { - output.write_all(CONFLICT_START_LINE)?; - let mut add_index = 0; - for left in conflict.removes() { - let right1 = if let Some(right1) = conflict.adds().get(add_index) { - right1 - } else { - // If we have no more positive terms, emit the remaining negative - // terms as snapshots. - output.write_all(CONFLICT_MINUS_LINE)?; - output.write_all(&left.0)?; - continue; - }; - let diff1 = - Diff::for_tokenizer(&[&left.0, &right1.0], &find_line_ranges) + if let Some(content) = hunk.as_resolved() { + output.write_all(&content.0)?; + } else { + output.write_all(CONFLICT_START_LINE)?; + let mut add_index = 0; + for left in hunk.removes() { + let right1 = if let Some(right1) = hunk.adds().get(add_index) { + right1 + } else { + // If we have no more positive terms, emit the remaining negative + // terms as snapshots. + output.write_all(CONFLICT_MINUS_LINE)?; + output.write_all(&left.0)?; + continue; + }; + let diff1 = Diff::for_tokenizer(&[&left.0, &right1.0], &find_line_ranges) + .hunks() + .collect_vec(); + // Check if the diff against the next positive term is better. Since + // we want to preserve the order of the terms, we don't match against + // any later positive terms. + if let Some(right2) = hunk.adds().get(add_index + 1) { + let diff2 = + Diff::for_tokenizer(&[&left.0, &right2.0], &find_line_ranges) .hunks() .collect_vec(); - // Check if the diff against the next positive term is better. Since - // we want to preserve the order of the terms, we don't match against - // any later positive terms. - if let Some(right2) = conflict.adds().get(add_index + 1) { - let diff2 = - Diff::for_tokenizer(&[&left.0, &right2.0], &find_line_ranges) - .hunks() - .collect_vec(); - if diff_size(&diff2) < diff_size(&diff1) { - // If the next positive term is a better match, emit - // the current positive term as a snapshot and the next - // positive term as a diff. - output.write_all(CONFLICT_PLUS_LINE)?; - output.write_all(&right1.0)?; - output.write_all(CONFLICT_DIFF_LINE)?; - write_diff_hunks(&diff2, output)?; - add_index += 2; - continue; - } + if diff_size(&diff2) < diff_size(&diff1) { + // If the next positive term is a better match, emit + // the current positive term as a snapshot and the next + // positive term as a diff. + output.write_all(CONFLICT_PLUS_LINE)?; + output.write_all(&right1.0)?; + output.write_all(CONFLICT_DIFF_LINE)?; + write_diff_hunks(&diff2, output)?; + add_index += 2; + continue; } - - output.write_all(CONFLICT_DIFF_LINE)?; - write_diff_hunks(&diff1, output)?; - add_index += 1; } - // Emit the remaining positive terms as snapshots. - for slice in &conflict.adds()[add_index..] { - output.write_all(CONFLICT_PLUS_LINE)?; - output.write_all(&slice.0)?; - } - output.write_all(CONFLICT_END_LINE)?; + output.write_all(CONFLICT_DIFF_LINE)?; + write_diff_hunks(&diff1, output)?; + add_index += 1; + } + + // Emit the remaining positive terms as snapshots. + for slice in &hunk.adds()[add_index..] { + output.write_all(CONFLICT_PLUS_LINE)?; + output.write_all(&slice.0)?; } + output.write_all(CONFLICT_END_LINE)?; } } } @@ -500,7 +497,11 @@ fn diff_size(hunks: &[DiffHunk]) -> usize { /// will be considered invalid if they don't have the expected arity. // TODO: "parse" is not usually the opposite of "materialize", so maybe we // should rename them to "serialize" and "deserialize"? -pub fn parse_conflict(input: &[u8], num_removes: usize, num_adds: usize) -> Option> { +pub fn parse_conflict( + input: &[u8], + num_removes: usize, + num_adds: usize, +) -> Option>> { if input.is_empty() { return None; } @@ -514,19 +515,13 @@ pub fn parse_conflict(input: &[u8], num_removes: usize, num_adds: usize) -> Opti } else if conflict_start.is_some() && line == CONFLICT_END_LINE { let conflict_body = &input[conflict_start.unwrap() + CONFLICT_START_LINE.len()..pos]; let hunk = parse_conflict_hunk(conflict_body); - match &hunk { - MergeHunk::Conflict(conflict) - if conflict.removes().len() == num_removes - && conflict.adds().len() == num_adds => - { - let resolved_slice = &input[resolved_start..conflict_start.unwrap()]; - if !resolved_slice.is_empty() { - hunks.push(MergeHunk::Resolved(ContentHunk(resolved_slice.to_vec()))); - } - hunks.push(hunk); - resolved_start = pos + line.len(); + if hunk.removes().len() == num_removes && hunk.adds().len() == num_adds { + let resolved_slice = &input[resolved_start..conflict_start.unwrap()]; + if !resolved_slice.is_empty() { + hunks.push(Conflict::resolved(ContentHunk(resolved_slice.to_vec()))); } - _ => {} + hunks.push(hunk); + resolved_start = pos + line.len(); } conflict_start = None; } @@ -537,7 +532,7 @@ pub fn parse_conflict(input: &[u8], num_removes: usize, num_adds: usize) -> Opti None } else { if resolved_start < input.len() { - hunks.push(MergeHunk::Resolved(ContentHunk( + hunks.push(Conflict::resolved(ContentHunk( input[resolved_start..].to_vec(), ))); } @@ -545,7 +540,7 @@ pub fn parse_conflict(input: &[u8], num_removes: usize, num_adds: usize) -> Opti } } -fn parse_conflict_hunk(input: &[u8]) -> MergeHunk { +fn parse_conflict_hunk(input: &[u8]) -> Conflict { enum State { Diff, Minus, @@ -586,7 +581,7 @@ fn parse_conflict_hunk(input: &[u8]) -> MergeHunk { adds.last_mut().unwrap().0.extend_from_slice(rest); } else { // Doesn't look like a conflict - return MergeHunk::Resolved(ContentHunk(vec![])); + return Conflict::resolved(ContentHunk(vec![])); } } State::Minus => { @@ -597,12 +592,12 @@ fn parse_conflict_hunk(input: &[u8]) -> MergeHunk { } State::Unknown => { // Doesn't look like a conflict - return MergeHunk::Resolved(ContentHunk(vec![])); + return Conflict::resolved(ContentHunk(vec![])); } } } - MergeHunk::Conflict(Conflict::new(removes, adds)) + Conflict::new(removes, adds) } #[cfg(test)] diff --git a/lib/src/files.rs b/lib/src/files.rs index 50102a08ca..d053d21644 100644 --- a/lib/src/files.rs +++ b/lib/src/files.rs @@ -152,16 +152,10 @@ impl Debug for ContentHunk { } } -#[derive(PartialEq, Eq, Clone, Debug)] -pub enum MergeHunk { - Resolved(ContentHunk), - Conflict(Conflict), -} - #[derive(PartialEq, Eq, Clone, Debug)] pub enum MergeResult { Resolved(ContentHunk), - Conflict(Vec), + Conflict(Vec>), } /// A region where the base and two sides match. @@ -183,7 +177,7 @@ pub fn merge(removes: &[&[u8]], adds: &[&[u8]]) -> MergeResult { let diff = Diff::for_tokenizer(&diff_inputs, &diff::find_line_ranges); let mut resolved_hunk = ContentHunk(vec![]); - let mut merge_hunks: Vec = vec![]; + let mut merge_hunks: Vec> = vec![]; for diff_hunk in diff.hunks() { match diff_hunk { DiffHunk::Matching(content) => { @@ -194,10 +188,10 @@ pub fn merge(removes: &[&[u8]], adds: &[&[u8]]) -> MergeResult { resolved_hunk.0.extend(*resolved); } else { if !resolved_hunk.0.is_empty() { - merge_hunks.push(MergeHunk::Resolved(resolved_hunk)); + merge_hunks.push(Conflict::resolved(resolved_hunk)); resolved_hunk = ContentHunk(vec![]); } - merge_hunks.push(MergeHunk::Conflict(Conflict::new( + merge_hunks.push(Conflict::new( parts[..num_diffs] .iter() .map(|part| ContentHunk(part.to_vec())) @@ -206,7 +200,7 @@ pub fn merge(removes: &[&[u8]], adds: &[&[u8]]) -> MergeResult { .iter() .map(|part| ContentHunk(part.to_vec())) .collect_vec(), - ))); + )); } } } @@ -216,7 +210,7 @@ pub fn merge(removes: &[&[u8]], adds: &[&[u8]]) -> MergeResult { MergeResult::Resolved(resolved_hunk) } else { if !resolved_hunk.0.is_empty() { - merge_hunks.push(MergeHunk::Resolved(resolved_hunk)); + merge_hunks.push(Conflict::resolved(resolved_hunk)); } MergeResult::Conflict(merge_hunks) } @@ -272,10 +266,10 @@ mod tests { // One side modified, two sides added assert_eq!( merge(&[b"a", b""], &[b"b", b"b", b"b"]), - MergeResult::Conflict(vec![MergeHunk::Conflict(Conflict::new( + MergeResult::Conflict(vec![Conflict::new( vec![hunk(b"a"), hunk(b"")], vec![hunk(b"b"), hunk(b"b"), hunk(b"b")] - ))]) + )]) ); // All sides removed same content assert_eq!( @@ -285,10 +279,10 @@ mod tests { // One side modified, two sides removed assert_eq!( merge(&[b"a\n", b"a\n"], &[b"b\n", b"", b""]), - MergeResult::Conflict(vec![MergeHunk::Conflict(Conflict::new( + MergeResult::Conflict(vec![Conflict::new( vec![hunk(b"a\n"), hunk(b"a\n")], vec![hunk(b"b\n"), hunk(b""), hunk(b"")] - ))]) + )]) ); // Three sides made the same change assert_eq!( @@ -298,26 +292,26 @@ mod tests { // One side removed, one side modified assert_eq!( merge(&[b"a\n"], &[b"", b"b\n"]), - MergeResult::Conflict(vec![MergeHunk::Conflict(Conflict::new( + MergeResult::Conflict(vec![Conflict::new( vec![hunk(b"a\n")], vec![hunk(b""), hunk(b"b\n")] - ))]) + )]) ); // One side modified, one side removed assert_eq!( merge(&[b"a\n"], &[b"b\n", b""]), - MergeResult::Conflict(vec![MergeHunk::Conflict(Conflict::new( + MergeResult::Conflict(vec![Conflict::new( vec![hunk(b"a\n")], vec![hunk(b"b\n"), hunk(b"")] - ))]) + )]) ); // Two sides modified in different ways assert_eq!( merge(&[b"a"], &[b"b", b"c"]), - MergeResult::Conflict(vec![MergeHunk::Conflict(Conflict::new( + MergeResult::Conflict(vec![Conflict::new( vec![hunk(b"a")], vec![hunk(b"b"), hunk(b"c")] - ))]) + )]) ); // Two of three sides don't change, third side changes assert_eq!( @@ -332,10 +326,10 @@ mod tests { // One side unchanged, two other sides make the different change assert_eq!( merge(&[b"a", b"a"], &[b"b", b"a", b"c"]), - MergeResult::Conflict(vec![MergeHunk::Conflict(Conflict::new( + MergeResult::Conflict(vec![Conflict::new( vec![hunk(b"a"), hunk(b"a")], vec![hunk(b"b"), hunk(b"a"), hunk(b"c")] - ))]) + )]) ); // Merge of an unresolved conflict and another branch, where the other branch // undid the change from one of the inputs to the unresolved conflict in the @@ -347,18 +341,18 @@ mod tests { // Merge of an unresolved conflict and another branch. assert_eq!( merge(&[b"a", b"b"], &[b"c", b"d", b"e"]), - MergeResult::Conflict(vec![MergeHunk::Conflict(Conflict::new( + MergeResult::Conflict(vec![Conflict::new( vec![hunk(b"a"), hunk(b"b")], vec![hunk(b"c"), hunk(b"d"), hunk(b"e")] - ))]) + )]) ); // Two sides made the same change, third side made a different change assert_eq!( merge(&[b"a", b"b"], &[b"c", b"c", b"c"]), - MergeResult::Conflict(vec![MergeHunk::Conflict(Conflict::new( + MergeResult::Conflict(vec![Conflict::new( vec![hunk(b"a"), hunk(b"b")], vec![hunk(b"c"), hunk(b"c"), hunk(b"c")] - ))]) + )]) ); } @@ -368,11 +362,8 @@ mod tests { assert_eq!( merge(&[b"a\n"], &[b"a\nb\n", b"a\nc\n"]), MergeResult::Conflict(vec![ - MergeHunk::Resolved(hunk(b"a\n")), - MergeHunk::Conflict(Conflict::new( - vec![hunk(b"")], - vec![hunk(b"b\n"), hunk(b"c\n")] - )) + Conflict::resolved(hunk(b"a\n")), + Conflict::new(vec![hunk(b"")], vec![hunk(b"b\n"), hunk(b"c\n")]) ]) ); // Two sides changed different lines: no conflict @@ -384,12 +375,9 @@ mod tests { assert_eq!( merge(&[b"a\nb\nc\n"], &[b"a\nb1\nc\n", b"a\nb2\nc\n"]), MergeResult::Conflict(vec![ - MergeHunk::Resolved(hunk(b"a\n")), - MergeHunk::Conflict(Conflict::new( - vec![hunk(b"b\n")], - vec![hunk(b"b1\n"), hunk(b"b2\n")] - )), - MergeHunk::Resolved(hunk(b"c\n")) + Conflict::resolved(hunk(b"a\n")), + Conflict::new(vec![hunk(b"b\n")], vec![hunk(b"b1\n"), hunk(b"b2\n")]), + Conflict::resolved(hunk(b"c\n")) ]) ); // One side changes a line and adds a block after. The other side just adds the diff --git a/lib/tests/test_conflicts.rs b/lib/tests/test_conflicts.rs index 8cba9114d6..2b9a1f3f7b 100644 --- a/lib/tests/test_conflicts.rs +++ b/lib/tests/test_conflicts.rs @@ -320,31 +320,30 @@ line 5 right @r###" Some( [ - Conflict( - Conflict { - removes: [ - "line 1\nline 2\n", - ], - adds: [ - "line 1 left\nline 2 left\n", - "line 1 right\nline 2\n", - ], - }, - ), - Resolved( - "line 3\n", - ), - Conflict( - Conflict { - removes: [ - "line 4\nline 5\n", - ], - adds: [ - "line 4\nline 5 left\n", - "line 4 right\nline 5 right\n", - ], - }, - ), + Conflict { + removes: [ + "line 1\nline 2\n", + ], + adds: [ + "line 1 left\nline 2 left\n", + "line 1 right\nline 2\n", + ], + }, + Conflict { + removes: [], + adds: [ + "line 3\n", + ], + }, + Conflict { + removes: [ + "line 4\nline 5\n", + ], + adds: [ + "line 4\nline 5 left\n", + "line 4 right\nline 5 right\n", + ], + }, ], ) "###); @@ -489,23 +488,27 @@ line 5 @r###" Some( [ - Resolved( - "line 1\n", - ), - Conflict( - Conflict { - removes: [ - "line 2\nline 3\nline 4\n", - ], - adds: [ - "line 2\nleft\nline 4\n", - "right\n", - ], - }, - ), - Resolved( - "line 5\n", - ), + Conflict { + removes: [], + adds: [ + "line 1\n", + ], + }, + Conflict { + removes: [ + "line 2\nline 3\nline 4\n", + ], + adds: [ + "line 2\nleft\nline 4\n", + "right\n", + ], + }, + Conflict { + removes: [], + adds: [ + "line 5\n", + ], + }, ], ) "### @@ -539,25 +542,29 @@ line 5 @r###" Some( [ - Resolved( - "line 1\n", - ), - Conflict( - Conflict { - removes: [ - "line 2\nline 3\nline 4\n", - "line 2\nline 3\nline 4\n", - ], - adds: [ - "line 2\nleft\nline 4\n", - "right\n", - "line 2\nforward\nline 3\nline 4\n", - ], - }, - ), - Resolved( - "line 5\n", - ), + Conflict { + removes: [], + adds: [ + "line 1\n", + ], + }, + Conflict { + removes: [ + "line 2\nline 3\nline 4\n", + "line 2\nline 3\nline 4\n", + ], + adds: [ + "line 2\nleft\nline 4\n", + "right\n", + "line 2\nforward\nline 3\nline 4\n", + ], + }, + Conflict { + removes: [], + adds: [ + "line 5\n", + ], + }, ], ) "###