From f16d2a237b95a2d8605fe594a69125f37ce4c4df Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 31 Mar 2022 09:21:50 -0700 Subject: [PATCH] backend: pass in path when reading/writing conflicts as well We do it for all the other kinds of objects already. It's useful to have the path for backends that store objects by path (we don't have any such backends yet). I think the reason I didn't do it from the beginning was because we had separate `RepoPath` types for files and directories back then. --- lib/src/backend.rs | 6 ++---- lib/src/conflicts.rs | 4 ++-- lib/src/git_backend.rs | 4 ++-- lib/src/local_backend.rs | 4 ++-- lib/src/store.rs | 12 ++++++++---- lib/src/tree.rs | 24 +++++++++++++++-------- lib/src/working_copy.rs | 2 +- lib/tests/test_conflicts.rs | 4 ++-- lib/tests/test_merge_trees.rs | 35 ++++++++++++++++++++++++++-------- lib/tests/test_working_copy.rs | 2 +- src/commands.rs | 4 ++-- 11 files changed, 65 insertions(+), 36 deletions(-) diff --git a/lib/src/backend.rs b/lib/src/backend.rs index be88f1c94..773e6cd3c 100644 --- a/lib/src/backend.rs +++ b/lib/src/backend.rs @@ -393,9 +393,7 @@ pub trait Backend: Send + Sync + Debug { fn write_commit(&self, contents: &Commit) -> BackendResult; - // TODO: Pass in the paths here too even though they are unused, just like for - // files and trees? - fn read_conflict(&self, id: &ConflictId) -> BackendResult; + fn read_conflict(&self, path: &RepoPath, id: &ConflictId) -> BackendResult; - fn write_conflict(&self, contents: &Conflict) -> BackendResult; + fn write_conflict(&self, path: &RepoPath, contents: &Conflict) -> BackendResult; } diff --git a/lib/src/conflicts.rs b/lib/src/conflicts.rs index 6b0c33224..fdf21e6cd 100644 --- a/lib/src/conflicts.rs +++ b/lib/src/conflicts.rs @@ -321,7 +321,7 @@ pub fn update_conflict_from_content( conflict_id: &ConflictId, content: &[u8], ) -> BackendResult> { - let mut conflict = store.read_conflict(conflict_id)?; + let mut conflict = store.read_conflict(path, conflict_id)?; // First check if the new content is unchanged compared to the old content. If // it is, we don't need parse the content or write any new objects to the @@ -379,7 +379,7 @@ pub fn update_conflict_from_content( panic!("Found conflict markers in merge of non-files"); } } - let new_conflict_id = store.write_conflict(&conflict)?; + let new_conflict_id = store.write_conflict(path, &conflict)?; Ok(Some(new_conflict_id)) } else { Ok(None) diff --git a/lib/src/git_backend.rs b/lib/src/git_backend.rs index b6b8ea46f..52edfbf7d 100644 --- a/lib/src/git_backend.rs +++ b/lib/src/git_backend.rs @@ -434,7 +434,7 @@ impl Backend for GitBackend { Ok(id) } - fn read_conflict(&self, id: &ConflictId) -> BackendResult { + fn read_conflict(&self, _path: &RepoPath, id: &ConflictId) -> BackendResult { let mut file = self.read_file( &RepoPath::from_internal_string("unused"), &FileId::new(id.to_bytes()), @@ -448,7 +448,7 @@ impl Backend for GitBackend { }) } - fn write_conflict(&self, conflict: &Conflict) -> BackendResult { + fn write_conflict(&self, _path: &RepoPath, conflict: &Conflict) -> BackendResult { let json = serde_json::json!({ "removes": conflict_part_list_to_json(&conflict.removes), "adds": conflict_part_list_to_json(&conflict.adds), diff --git a/lib/src/local_backend.rs b/lib/src/local_backend.rs index 92a7f9eae..fcdb4c7d6 100644 --- a/lib/src/local_backend.rs +++ b/lib/src/local_backend.rs @@ -215,7 +215,7 @@ impl Backend for LocalBackend { Ok(id) } - fn read_conflict(&self, id: &ConflictId) -> BackendResult { + fn read_conflict(&self, _path: &RepoPath, id: &ConflictId) -> BackendResult { let path = self.conflict_path(id); let mut file = File::open(path).map_err(not_found_to_backend_error)?; @@ -223,7 +223,7 @@ impl Backend for LocalBackend { Ok(conflict_from_proto(&proto)) } - fn write_conflict(&self, conflict: &Conflict) -> BackendResult { + fn write_conflict(&self, _path: &RepoPath, conflict: &Conflict) -> BackendResult { let temp_file = NamedTempFile::new_in(&self.path)?; let proto = conflict_to_proto(conflict); diff --git a/lib/src/store.rs b/lib/src/store.rs index 7074a69fc..3e7a0f53a 100644 --- a/lib/src/store.rs +++ b/lib/src/store.rs @@ -192,12 +192,16 @@ impl Store { self.backend.write_symlink(path, contents) } - pub fn read_conflict(&self, id: &ConflictId) -> BackendResult { - self.backend.read_conflict(id) + pub fn read_conflict(&self, path: &RepoPath, id: &ConflictId) -> BackendResult { + self.backend.read_conflict(path, id) } - pub fn write_conflict(&self, contents: &Conflict) -> BackendResult { - self.backend.write_conflict(contents) + pub fn write_conflict( + &self, + path: &RepoPath, + contents: &Conflict, + ) -> BackendResult { + self.backend.write_conflict(path, contents) } pub fn tree_builder(self: &Arc, base_tree_id: TreeId) -> TreeBuilder { diff --git a/lib/src/tree.rs b/lib/src/tree.rs index e66a5320f..d4e5ea2fb 100644 --- a/lib/src/tree.rs +++ b/lib/src/tree.rs @@ -603,7 +603,8 @@ fn merge_tree_value( value: side2.clone(), }); } - let conflict = simplify_conflict(store, &conflict)?; + let filename = dir.join(basename); + let conflict = simplify_conflict(store, &filename, &conflict)?; if conflict.adds.is_empty() { // If there are no values to add, then the path doesn't exist return Ok(None); @@ -612,14 +613,13 @@ fn merge_tree_value( // 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)?; + let conflict_id = store.write_conflict(&filename, &conflict)?; Some(TreeValue::Conflict(conflict_id)) } } @@ -703,10 +703,14 @@ fn try_resolve_file_conflict( } } -fn conflict_part_to_conflict(store: &Store, part: &ConflictPart) -> Result { +fn conflict_part_to_conflict( + store: &Store, + path: &RepoPath, + part: &ConflictPart, +) -> Result { match &part.value { TreeValue::Conflict(id) => { - let conflict = store.read_conflict(id)?; + let conflict = store.read_conflict(path, id)?; Ok(conflict) } other => Ok(Conflict { @@ -718,7 +722,11 @@ fn conflict_part_to_conflict(store: &Store, part: &ConflictPart) -> Result Result { +fn simplify_conflict( + store: &Store, + path: &RepoPath, + conflict: &Conflict, +) -> Result { // Important cases to simplify: // // D @@ -756,7 +764,7 @@ fn simplify_conflict(store: &Store, conflict: &Conflict) -> Result { - let conflict = conflict_part_to_conflict(store, part)?; + let conflict = conflict_part_to_conflict(store, path, part)?; new_removes.extend_from_slice(&conflict.removes); new_adds.extend_from_slice(&conflict.adds); } @@ -768,7 +776,7 @@ fn simplify_conflict(store: &Store, conflict: &Conflict) -> Result { - let conflict = conflict_part_to_conflict(store, part)?; + let conflict = conflict_part_to_conflict(store, path, part)?; new_removes.extend_from_slice(&conflict.adds); new_adds.extend_from_slice(&conflict.removes); } diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index 58fc58276..bb856cf63 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -512,7 +512,7 @@ impl TreeState { fn write_conflict(&self, disk_path: &Path, path: &RepoPath, id: &ConflictId) -> FileState { create_parent_dirs(disk_path); - let conflict = self.store.read_conflict(id).unwrap(); + let conflict = self.store.read_conflict(path, id).unwrap(); // TODO: Check that we're not overwriting an un-ignored file here (which might // be created by a concurrent process). let mut file = OpenOptions::new() diff --git a/lib/tests/test_conflicts.rs b/lib/tests/test_conflicts.rs index 6b86cbe6f..b0268c9ff 100644 --- a/lib/tests/test_conflicts.rs +++ b/lib/tests/test_conflicts.rs @@ -452,7 +452,7 @@ fn test_update_conflict_from_content() { }, ], }; - let conflict_id = store.write_conflict(&conflict).unwrap(); + 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. @@ -475,7 +475,7 @@ fn test_update_conflict_from_content() { let result = update_conflict_from_content(store, &path, &conflict_id, b"resolved 1\nline 2\n<<<<<<<\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(&result.unwrap()).unwrap(); + let new_conflict = store.read_conflict(&path, &result.unwrap()).unwrap(); // 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"); diff --git a/lib/tests/test_merge_trees.rs b/lib/tests/test_merge_trees.rs index 02bb6f125..2f925ec8d 100644 --- a/lib/tests/test_merge_trees.rs +++ b/lib/tests/test_merge_trees.rs @@ -119,7 +119,9 @@ fn test_same_type(use_git: bool) { // Check the conflicting cases match merged_tree.value(&RepoPathComponent::from("_ab")).unwrap() { TreeValue::Conflict(id) => { - let conflict = store.read_conflict(id).unwrap(); + let conflict = store + .read_conflict(&RepoPath::from_internal_string("_ab"), id) + .unwrap(); assert_eq!( conflict.adds, vec![ @@ -143,7 +145,9 @@ fn test_same_type(use_git: bool) { }; match merged_tree.value(&RepoPathComponent::from("a_b")).unwrap() { TreeValue::Conflict(id) => { - let conflict = store.read_conflict(id).unwrap(); + let conflict = store + .read_conflict(&RepoPath::from_internal_string("a_b"), id) + .unwrap(); assert_eq!( conflict.removes, vec![ConflictPart { @@ -167,7 +171,9 @@ fn test_same_type(use_git: bool) { }; match merged_tree.value(&RepoPathComponent::from("ab_")).unwrap() { TreeValue::Conflict(id) => { - let conflict = store.read_conflict(id).unwrap(); + let conflict = store + .read_conflict(&RepoPath::from_internal_string("ab_"), id) + .unwrap(); assert_eq!( conflict.removes, vec![ConflictPart { @@ -191,7 +197,9 @@ fn test_same_type(use_git: bool) { }; match merged_tree.value(&RepoPathComponent::from("abc")).unwrap() { TreeValue::Conflict(id) => { - let conflict = store.read_conflict(id).unwrap(); + let conflict = store + .read_conflict(&RepoPath::from_internal_string("abc"), id) + .unwrap(); assert_eq!( conflict.removes, vec![ConflictPart { @@ -373,7 +381,12 @@ fn test_types(use_git: bool) { .unwrap() { TreeValue::Conflict(id) => { - let conflict = store.read_conflict(id).unwrap(); + let conflict = store + .read_conflict( + &RepoPath::from_internal_string("normal_executable_symlink"), + id, + ) + .unwrap(); assert_eq!( conflict.removes, vec![ConflictPart { @@ -408,7 +421,9 @@ fn test_types(use_git: bool) { .unwrap() { TreeValue::Conflict(id) => { - let conflict = store.read_conflict(id).unwrap(); + let conflict = store + .read_conflict(&RepoPath::from_internal_string("tree_normal_symlink"), id) + .unwrap(); assert_eq!( conflict.removes, vec![ConflictPart { @@ -496,7 +511,9 @@ fn test_simplify_conflict(use_git: bool) { .unwrap() { TreeValue::Conflict(id) => { - let conflict = store.read_conflict(id).unwrap(); + let conflict = store + .read_conflict(&RepoPath::from_internal_string("file"), id) + .unwrap(); assert_eq!( conflict.removes, vec![ConflictPart { @@ -532,7 +549,9 @@ fn test_simplify_conflict(use_git: bool) { .unwrap() { TreeValue::Conflict(id) => { - let conflict = store.read_conflict(id).unwrap(); + let conflict = store + .read_conflict(&RepoPath::from_internal_string("file"), id) + .unwrap(); assert_eq!( conflict.removes, vec![ConflictPart { diff --git a/lib/tests/test_working_copy.rs b/lib/tests/test_working_copy.rs index 4f1b2877f..62e7b3fbc 100644 --- a/lib/tests/test_working_copy.rs +++ b/lib/tests/test_working_copy.rs @@ -125,7 +125,7 @@ fn test_checkout_file_transitions(use_git: bool) { }, ], }; - let conflict_id = store.write_conflict(&conflict).unwrap(); + let conflict_id = store.write_conflict(path, &conflict).unwrap(); TreeValue::Conflict(conflict_id) } Kind::Symlink => { diff --git a/src/commands.rs b/src/commands.rs index b926fc467..7dbc964fa 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -2120,7 +2120,7 @@ fn diff_content( Ok(format!("Git submodule checked out at {}", id.hex()).into_bytes()) } TreeValue::Conflict(id) => { - let conflict = repo.store().read_conflict(id).unwrap(); + let conflict = repo.store().read_conflict(path, id).unwrap(); let mut content = vec![]; conflicts::materialize_conflict(repo.store(), path, &conflict, &mut content).unwrap(); Ok(content) @@ -2271,7 +2271,7 @@ fn git_diff_part( TreeValue::Conflict(id) => { mode = "100644".to_string(); hash = id.hex(); - let conflict = repo.store().read_conflict(id).unwrap(); + let conflict = repo.store().read_conflict(path, id).unwrap(); conflicts::materialize_conflict(repo.store(), path, &conflict, &mut content).unwrap(); } }