From 19fd8a917afd1be04847553fee1c57526adf5450 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 10 Jun 2023 14:47:32 -0700 Subject: [PATCH] conflicts: remove `ConflictId` from `update_conflict_from_content()` For tree-level conflicts (#1624), I plan to remove `ConflictId` completely. This commit removes `ConflictId` from `update_conflict_from_content()` by instead making it take a `Conflict>` and return a possibly different such value. I made the call site in `working_copy` avoid writing the conflict to the store if it's unchanged, but I didn't make the same optimization in `merge_tools` becuase it's much more likely to have changed there. --- lib/src/conflicts.rs | 24 +++++++++++++----------- lib/src/working_copy.rs | 12 +++++++++--- lib/tests/test_conflicts.rs | 35 +++++++++++++---------------------- src/merge_tools.rs | 5 +++-- 4 files changed, 38 insertions(+), 38 deletions(-) diff --git a/lib/src/conflicts.rs b/lib/src/conflicts.rs index 9b13ecabd..770d95711 100644 --- a/lib/src/conflicts.rs +++ b/lib/src/conflicts.rs @@ -17,7 +17,7 @@ use std::io::Write; use itertools::Itertools; -use crate::backend::{BackendResult, ConflictId, FileId, ObjectId, TreeValue}; +use crate::backend::{BackendResult, FileId, ObjectId, TreeValue}; use crate::diff::{find_line_ranges, Diff, DiffHunk}; use crate::files::{ConflictHunk, MergeHunk, MergeResult}; use crate::repo_path::RepoPath; @@ -451,10 +451,9 @@ fn parse_conflict_hunk(input: &[u8]) -> MergeHunk { pub fn update_conflict_from_content( store: &Store, path: &RepoPath, - conflict_id: &ConflictId, + conflict: &Conflict>, content: &[u8], -) -> BackendResult> { - let mut conflict = store.read_conflict(path, conflict_id)?; +) -> BackendResult>>> { // TODO: Check that the conflict only involves files and convert it to a // `Conflict>` so we can remove the wildcard pattern in the loops // further down. @@ -465,9 +464,9 @@ pub fn update_conflict_from_content( // conflicts (for example) are not converted to regular files in the working // copy. let mut old_content = Vec::with_capacity(content.len()); - materialize_conflict(store, path, &conflict, &mut old_content).unwrap(); + materialize_conflict(store, path, conflict, &mut old_content).unwrap(); if content == old_content { - return Ok(Some(conflict_id.clone())); + return Ok(Some(conflict.clone())); } let mut removed_content = vec![vec![]; conflict.removes().len()]; @@ -504,6 +503,7 @@ pub fn update_conflict_from_content( // Now write the new files contents we found by parsing the file // with conflict markers. Update the Conflict object with the new // FileIds. + let mut new_removes = vec![]; for (i, buf) in removed_content.iter().enumerate() { match &conflict.removes()[i] { Some(TreeValue::File { id: _, executable }) => { @@ -512,11 +512,12 @@ pub fn update_conflict_from_content( id: file_id, executable: *executable, }; - conflict.set_remove(i, Some(new_value)); + new_removes.push(Some(new_value)); } None if buf.is_empty() => { // The missing side of a conflict is still represented by - // the empty string we materialized it as => nothing to do + // the empty string we materialized it as + new_removes.push(None); } _ => { // The user edited a non-file side. This should never happen. We consider the @@ -525,6 +526,7 @@ pub fn update_conflict_from_content( } } } + let mut new_adds = vec![]; for (i, buf) in added_content.iter().enumerate() { match &conflict.adds()[i] { Some(TreeValue::File { id: _, executable }) => { @@ -533,11 +535,12 @@ pub fn update_conflict_from_content( id: file_id, executable: *executable, }; - conflict.set_add(i, Some(new_value)); + new_adds.push(Some(new_value)); } None if buf.is_empty() => { // The missing side of a conflict is still represented by // the empty string we materialized it as => nothing to do + new_adds.push(None); } _ => { // The user edited a non-file side. This should never happen. We consider the @@ -546,8 +549,7 @@ pub fn update_conflict_from_content( } } } - let new_conflict_id = store.write_conflict(path, &conflict)?; - Ok(Some(new_conflict_id)) + Ok(Some(Conflict::new(new_removes, new_adds))) } #[cfg(test)] diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index 1b36ba405..de9a55481 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -658,17 +658,23 @@ impl TreeState { let mut file = File::open(&disk_path).unwrap(); let mut content = vec![]; file.read_to_end(&mut content).unwrap(); - if let Some(new_conflict_id) = update_conflict_from_content( + let conflict = self.store.read_conflict(&repo_path, conflict_id)?; + if let Some(new_conflict) = update_conflict_from_content( self.store.as_ref(), &repo_path, - conflict_id, + &conflict, &content, ) .unwrap() { new_file_state.file_type = FileType::Conflict; *current_file_state = new_file_state; - tree_builder.set(repo_path, TreeValue::Conflict(new_conflict_id)); + if new_conflict != conflict { + let new_conflict_id = + self.store.write_conflict(&repo_path, &new_conflict)?; + tree_builder + .set(repo_path, TreeValue::Conflict(new_conflict_id)); + }; return Ok(()); } } diff --git a/lib/tests/test_conflicts.rs b/lib/tests/test_conflicts.rs index e21d5fca2..72380e529 100644 --- a/lib/tests/test_conflicts.rs +++ b/lib/tests/test_conflicts.rs @@ -644,36 +644,30 @@ fn test_update_conflict_from_content() { Some(file_value(&right_file_id)), ], ); - let conflict_id = store.write_conflict(&path, &conflict).unwrap(); // If the content is unchanged compared to the materialized value, we get the // old conflict id back. let mut materialized = vec![]; materialize_conflict(store, &path, &conflict, &mut materialized).unwrap(); - let result = update_conflict_from_content(store, &path, &conflict_id, &materialized).unwrap(); - assert_eq!(result, Some(conflict_id.clone())); + let result = update_conflict_from_content(store, &path, &conflict, &materialized).unwrap(); + assert_eq!(result, Some(conflict.clone())); // If the conflict is resolved, we get None back to indicate that. - let result = update_conflict_from_content( - store, - &path, - &conflict_id, - b"resolved 1\nline 2\nresolved 3\n", - ) - .unwrap(); + let result = + update_conflict_from_content(store, &path, &conflict, b"resolved 1\nline 2\nresolved 3\n") + .unwrap(); assert_eq!(result, None); // If the conflict is partially resolved, we get a new conflict back. let result = update_conflict_from_content( store, &path, - &conflict_id, + &conflict, b"resolved 1\nline 2\n<<<<<<<\n%%%%%%%\n-line 3\n+left 3\n+++++++\nright 3\n>>>>>>>\n", ) .unwrap(); - assert_ne!(result, None); - assert_ne!(result, Some(conflict_id)); - let new_conflict = store.read_conflict(&path, &result.unwrap()).unwrap(); + let new_conflict = result.unwrap(); + assert_ne!(new_conflict, conflict); // Calculate expected new FileIds let new_base_file_id = testutils::write_file(store, &path, "resolved 1\nline 2\nline 3\n"); let new_left_file_id = testutils::write_file(store, &path, "resolved 1\nline 2\nleft 3\n"); @@ -702,30 +696,27 @@ fn test_update_conflict_from_content_modify_delete() { vec![Some(file_value(&before_file_id))], vec![Some(file_value(&after_file_id)), None], ); - let conflict_id = store.write_conflict(&path, &conflict).unwrap(); // If the content is unchanged compared to the materialized value, we get the // old conflict id back. let mut materialized = vec![]; materialize_conflict(store, &path, &conflict, &mut materialized).unwrap(); - let result = update_conflict_from_content(store, &path, &conflict_id, &materialized).unwrap(); - assert_eq!(result, Some(conflict_id.clone())); + let result = update_conflict_from_content(store, &path, &conflict, &materialized).unwrap(); + assert_eq!(result, Some(conflict.clone())); // If the conflict is resolved, we get None back to indicate that. - let result = update_conflict_from_content(store, &path, &conflict_id, b"resolved\n").unwrap(); + let result = update_conflict_from_content(store, &path, &conflict, b"resolved\n").unwrap(); assert_eq!(result, None); // If the conflict is modified, we get a new conflict back. let result = update_conflict_from_content( store, &path, - &conflict_id, + &conflict, b"<<<<<<<\n%%%%%%%\n line 1\n-line 2 before\n+line 2 modified after\n line 3\n+++++++\n>>>>>>>\n", ) .unwrap(); - assert_ne!(result, None); - assert_ne!(result, Some(conflict_id)); - let new_conflict = store.read_conflict(&path, &result.unwrap()).unwrap(); + let new_conflict = result.unwrap(); // Calculate expected new FileIds let new_base_file_id = testutils::write_file(store, &path, "line 1\nline 2 before\nline 3\n"); let new_left_file_id = diff --git a/src/merge_tools.rs b/src/merge_tools.rs index 36da900e8..932511d56 100644 --- a/src/merge_tools.rs +++ b/src/merge_tools.rs @@ -246,12 +246,13 @@ pub fn run_mergetool( let mut new_tree_value: Option = None; if editor.merge_tool_edits_conflict_markers { - if let Some(new_conflict_id) = update_conflict_from_content( + if let Some(new_conflict) = update_conflict_from_content( tree.store(), repo_path, - &conflict_id, + &conflict, output_file_contents.as_slice(), )? { + let new_conflict_id = tree.store().write_conflict(repo_path, &new_conflict)?; new_tree_value = Some(TreeValue::Conflict(new_conflict_id)); } }