fix(merge): conflict on delete modify collisions (#215)

Author: presempathy-awb

src/git/versioned_store/backends.rs

@@ -234,13 +234,36 @@ impl<const N: usize> VersionedKvStore<N, GitNodeStorage<N>, GitMetadataBackend>
                     }
                 }
                 // Key deleted in source, still exists in dest
-                (Some(_base), None, Some(_dest)) => {
-                    // Source deleted it - apply deletion
-                    merge_results.push(crate::diff::MergeResult::Removed(key));
+                (Some(base), None, Some(dest)) => {
+                    if base == dest {
+                        // Destination unchanged from base - safe to apply source deletion
+                        merge_results.push(crate::diff::MergeResult::Removed(key));
+                    } else {
+                        // Destination modified while source deleted - conflict
+                        let conflict = crate::diff::MergeConflict {
+                            key: key.clone(),
+                            base_value: Some(base.clone()),
+                            source_value: None,
+                            destination_value: Some(dest.clone()),
+                        };
+                        merge_results.push(crate::diff::MergeResult::Conflict(conflict));
+                    }
                 }
                 // Key deleted in dest, still exists in source - keep deletion (no-op)
-                (Some(_base), Some(_source), None) => {
-                    continue;
+                (Some(base), Some(source), None) => {
+                    if base == source {
+                        // Source unchanged from base - keep destination deletion
+                        continue;
+                    } else {
+                        // Source modified while destination deleted - conflict
+                        let conflict = crate::diff::MergeConflict {
+                            key: key.clone(),
+                            base_value: Some(base.clone()),
+                            source_value: Some(source.clone()),
+                            destination_value: None,
+                        };
+                        merge_results.push(crate::diff::MergeResult::Conflict(conflict));
+                    }
                 }
                 // Key deleted in both - no-op
                 (Some(_base), None, None) => {

src/git/versioned_store/history.rs

@@ -506,13 +506,36 @@ where
                     }
                 }
                 // Key deleted in source, still exists in dest
-                (Some(_base), None, Some(_dest)) => {
-                    // Source deleted it - apply deletion
-                    merge_results.push(crate::diff::MergeResult::Removed(key.clone()));
+                (Some(base), None, Some(dest)) => {
+                    if base == dest {
+                        // Destination unchanged from base - safe to apply source deletion
+                        merge_results.push(crate::diff::MergeResult::Removed(key.clone()));
+                    } else {
+                        // Destination modified while source deleted - conflict
+                        let conflict = crate::diff::MergeConflict {
+                            key: key.clone(),
+                            base_value: Some(base.clone()),
+                            source_value: None,
+                            destination_value: Some(dest.clone()),
+                        };
+                        merge_results.push(crate::diff::MergeResult::Conflict(conflict));
+                    }
                 }
                 // Key deleted in dest, still exists in source - keep deletion (no-op)
-                (Some(_base), Some(_source), None) => {
-                    continue;
+                (Some(base), Some(source), None) => {
+                    if base == source {
+                        // Source unchanged from base - keep destination deletion
+                        continue;
+                    } else {
+                        // Source modified while destination deleted - conflict
+                        let conflict = crate::diff::MergeConflict {
+                            key: key.clone(),
+                            base_value: Some(base.clone()),
+                            source_value: Some(source.clone()),
+                            destination_value: None,
+                        };
+                        merge_results.push(crate::diff::MergeResult::Conflict(conflict));
+                    }
                 }
                 // Key deleted in both - no-op
                 (Some(_base), None, None) => {

tests/conflict_resolvers.rs

@@ -18,8 +18,19 @@ limitations under the License.
 
 mod common;
 
-use prollytree::diff::{IgnoreConflictsResolver, TakeDestinationResolver, TakeSourceResolver};
-use prollytree::git::versioned_store::GitVersionedKvStore;
+use prollytree::diff::{
+    ConflictResolver, IgnoreConflictsResolver, MergeConflict, MergeResult, TakeDestinationResolver,
+    TakeSourceResolver,
+};
+use prollytree::git::versioned_store::{GitVersionedKvStore, InMemoryVersionedKvStore};
+
+struct LeaveUnresolved;
+
+impl ConflictResolver for LeaveUnresolved {
+    fn resolve_conflict(&self, _conflict: &MergeConflict) -> Option<MergeResult> {
+        None
+    }
+}
 
 // ---------------------------------------------------------------------------
 // Helper: set up divergent branches with a conflict on a shared key
@@ -232,3 +243,36 @@ fn test_mixed_adds_deletes_conflicts() {
 
     std::mem::forget(_temp);
 }
+
+#[test]
+fn test_delete_modify_merge_surfaces_conflict_and_preserves_destination() {
+    let (_temp, dataset) = common::setup_repo_and_dataset();
+    let mut store = InMemoryVersionedKvStore::<32>::init(&dataset).expect("init");
+
+    store.insert(b"K".to_vec(), b"v0".to_vec()).unwrap();
+    store.commit("base").unwrap();
+    store.create_branch("feature").unwrap();
+    store.checkout_generic("main").unwrap();
+
+    store.insert(b"K".to_vec(), b"v1".to_vec()).unwrap();
+    store.commit("main modifies K").unwrap();
+
+    store.checkout_generic("feature").unwrap();
+    assert!(store.delete(b"K").unwrap());
+    store.commit("feature deletes K").unwrap();
+
+    store.checkout_generic("main").unwrap();
+    let result = store.merge_generic("feature", &LeaveUnresolved);
+
+    assert!(
+        result.is_err(),
+        "delete/modify merge must surface an unresolved conflict"
+    );
+    assert_eq!(
+        store.get(b"K"),
+        Some(b"v1".to_vec()),
+        "destination modification must not be silently deleted"
+    );
+
+    std::mem::forget(_temp);
+}