From 01f18e99ff3d8ba84f62ece3a29695698eb6364c Mon Sep 17 00:00:00 2001
From: Lina Cambridge <lina@yakshaving.ninja>
Date: Sun, 10 Feb 2019 00:20:00 -0800
Subject: [PATCH] Add cycle detection. See #22.

---
 src/error.rs |  4 ++++
 src/tests.rs | 25 +++++++++++++++++++++++++
 src/tree.rs  | 43 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+)

diff --git a/src/error.rs b/src/error.rs
index 31dc6b7..a248a33 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -76,6 +76,9 @@ impl fmt::Display for Error {
                 write!(f, "Can't insert item {} into nonexistent parent {}",
                        child_guid, parent_guid)
             },
+            ErrorKind::Cycle(guid) => {
+                write!(f, "Item {} can't contain itself", guid)
+            },
             ErrorKind::MergeConflict => {
                 write!(f, "Local tree changed during merge")
             },
@@ -103,6 +106,7 @@ pub enum ErrorKind {
     InvalidParent(Guid, Guid),
     MissingParent(Guid, Guid),
     MissingItem(Guid),
+    Cycle(Guid),
     MergeConflict,
     UnmergedLocalItems,
     UnmergedRemoteItems,
diff --git a/src/tests.rs b/src/tests.rs
index 7b72d23..7599e57 100644
--- a/src/tests.rs
+++ b/src/tests.rs
@@ -2135,3 +2135,28 @@ fn moved_user_content_roots() {
 
     assert_eq!(merger.telemetry(), &expected_telem);
 }
+
+#[test]
+fn cycle() {
+    before_each();
+
+    // Try to create a cycle: move A into B, and B into the menu, but keep
+    // B's parent by children as A.
+    let mut b = nodes!({
+        ("menu________", Folder)
+    }).into_builder().unwrap();
+
+    b.item(Item::new("folderAAAAAA".into(), Kind::Folder))
+     .and_then(|p| p.by_parent_guid("folderBBBBBB".into()))
+     .expect("Should insert A");
+
+    b.item(Item::new("folderBBBBBB".into(), Kind::Folder))
+     .and_then(|p| p.by_parent_guid("menu________".into()))
+     .and_then(|b| b.parent_for(&"folderBBBBBB".into()).by_children(&"folderAAAAAA".into()))
+     .expect("Should insert B");
+
+    match b.into_tree().expect_err("Should not build tree with cycles").kind() {
+        ErrorKind::Cycle(guid) => assert_eq!(guid, &Guid::from("folderAAAAAA")),
+        err @ _ => assert!(false, "Wrong error kind for cycle: {:?}", err),
+    }
+}
diff --git a/src/tree.rs b/src/tree.rs
index 199dc47..6d76958 100644
--- a/src/tree.rs
+++ b/src/tree.rs
@@ -448,6 +448,9 @@ impl IntoTree for Builder {
         // a vector of resolved parents, and a lookup table for reparented
         // orphaned children.
         let (parents, mut reparented_orphans_by_parent) = self.resolve();
+        if let Some(index) = detect_cycles(&parents) {
+            return Err(ErrorKind::Cycle(self.entries[index].item.guid.clone()).into());
+        }
         for reparented_orphans in reparented_orphans_by_parent.values_mut() {
             // Use a deterministic order for reparented orphans.
             reparented_orphans.sort_unstable_by(|&index, &other_index| {
@@ -742,6 +745,46 @@ enum ResolvedParent {
     ByParentGuid(Index),
 }
 
+impl ResolvedParent {
+    fn index(&self) -> Option<Index> {
+        match self {
+            ResolvedParent::Root => None,
+            ResolvedParent::Unchanged(index)
+            | ResolvedParent::ByChildren(index)
+            | ResolvedParent::ByParentGuid(index) => Some(*index),
+        }
+    }
+}
+
+/// Detects cycles in resolved parents, using Floyd's tortoise and the hare
+/// algorithm. Returns the index of the entry where the cycle was detected,
+/// or `None` if there aren't any cycles.
+fn detect_cycles(parents: &[ResolvedParent]) -> Option<Index> {
+    let mut seen = vec![false; parents.len()];
+    for (entry_index, parent) in parents.iter().enumerate() {
+        if seen[entry_index] {
+            continue;
+        }
+        let mut parent_index = parent.index();
+        let mut grandparent_index = parent.index()
+            .and_then(|index| parents[index].index());
+        while let (Some(i), Some(j)) = (parent_index, grandparent_index) {
+            if i == j {
+                return Some(i);
+            }
+            if seen[i] || seen[j] {
+                break;
+            }
+            parent_index = parent_index.and_then(|index| parents[index].index());
+            grandparent_index = grandparent_index
+                .and_then(|index| parents[index].index())
+                .and_then(|index| parents[index].index());
+        }
+        seen[entry_index] = true;
+    }
+    None
+}
+
 #[derive(Debug)]
 enum Divergence {
     /// The node's structure is already correct, and doesn't need to be