octopus: cleaning up branch iterator implementation

The previous implementation tried to be an iterator over parents,
which ultimately was the wrong design choice and caused a lot of
headaches.  Instead, this implementation is a simple branch iterator,
that will update it's position to the next parent, if one exists.

Not really sure why this wasn't my first approach, but I think I was
thinking about this in terms of parents, not actually commits.  So I
didn't really see the forest for the trees.

apps/servers/octopus/supergit/src/bin/test.rs

@@ -25,21 +25,17 @@ fn main() {
     // Iterate over all branch iterators we get
     while let Some(biter) = rx.recv().ok() {
         use BranchCommit::*;
-        println!(
-            "{}: {}",
-            biter.current().id_str(),
-            biter.current().summary()
-        );
 
         biter.for_each(|bc| match bc {
             Commit(c) => println!("{}: {}", c.id_str(), c.summary()),
-            Merge(Some(c), b) => {
+            Merge(c, _b) => {
                 println!("{}: {}", c.id_str(), c.summary());
                 // tx.send(b.get_all()).unwrap();
             }
-            Merge(_, b) => {} //tx.send(b.get_all()).unwrap(),
             _ => todo!(),
         });
+
+        break;
     }
 
     // let rr = RawRepository::open(path.as_str()).unwrap();

apps/servers/octopus/supergit/src/branch.rs

@@ -31,6 +31,7 @@ impl Branch {
     }
 
     /// Get a branch handle starting at a certain commit
+    // TODO: do we want to check if this is actually a child?
     pub fn skip_to(&self, from: HashId) -> Self {
         match self.name {
             Some(ref name) => Self::new(&self.repo, name.clone(), from),
@@ -84,8 +85,7 @@ impl Branch {
 /// parent by setting
 pub struct BranchIter {
     repo: Arc<Repository>,
-    last: HashId,
+    curr: Option<HashId>,
-    cmd: IterCmd,
     limit: SegLimit,
 }
 
@@ -94,14 +94,13 @@ impl BranchIter {
     fn new(repo: Arc<Repository>, last: HashId, limit: SegLimit) -> Self {
         Self {
             repo,
-            last,
+            curr: Some(last),
-            cmd: IterCmd::Step,
             limit,
         }
     }
 
     pub fn current(&self) -> Commit {
-        Commit::new(&self.repo, self.last.clone()).unwrap()
+        Commit::new(&self.repo, self.curr.as_ref().unwrap().clone()).unwrap()
     }
 
     /// Get a commit object, if it exists
@@ -109,70 +108,35 @@ impl BranchIter {
         Commit::new(&self.repo, id.clone())
     }
 
-    /// Utility functiot to set last commit
+    /// For a current commit, get it's parents if they exists
-    fn set_last(&mut self, (bc, cmd): (BranchCommit, IterCmd)) -> BranchCommit {
+    fn parents(&self, curr: &Commit) -> (Option<Commit>, Option<Commit>) {
-        self.last = bc.id();
+        (curr.first_parent(), curr.parent(1))
-        self.cmd = cmd;
-        bc
     }
 
-    /// Get the parent, set the last, and return BranchCommit (maybe)
+    /// Take an optional commit and turn it into a branch commit
-    fn get_parent(&self, last: Option<Commit>, cmd: IterCmd) -> Option<(BranchCommit, IterCmd)> {
+    fn make_branch_commit(&self, curr: Commit) -> BranchCommit {
-        if let Some(id) = cmd.take() {
+        match curr.parent_count() {
-            let commit = Commit::new(&self.repo, id).unwrap();
+            0 | 1 => BranchCommit::Commit(curr),
-
-            return match commit.parent_count() {
-                // Special case: if the previous commit was a merge,
-                // but this was the first commit in the history, we
-                // need to return it here or else it will be forgotten
-                // about!
-                0 | 1 => Some((BranchCommit::Commit(commit), IterCmd::Step)),
-                2 => {
-                    let p1 = commit.first_parent().unwrap();
-                    let p2 = commit.parent(1).unwrap();
-
-                    Some((
-                        BranchCommit::Merge(
-                            // Here we return a commit via the merge
-                            // field, because otherwise it will be
-                            // dropped!  Because we just skipped
-                            // because of a merge (because merges are
-                            // normal commit parents).
-                            Some(commit.clone()),
-                            Branch::without_name(&self.repo, p2.id),
-                        ),
-                        IterCmd::Skip(p1.id),
-                    ))
-                }
-                _ => todo!(),
-            };
-        }
-
-        // This code is only entered when we are checking for the parents
-        last.and_then(|c| match c.parent_count() {
-            // No parent means we've reached the end of the branch
-            0 => None,
-            // One parent is a normal commit
-            1 => {
-                let parent = c.first_parent().unwrap();
-                Some((BranchCommit::Commit(parent), IterCmd::Step))
-            }
-            // Two parents is a normal merge commit
             2 => {
-                let p1 = c.first_parent().unwrap();
+                let p2 = self.parents(&curr).1.unwrap();
-                let p2 = c.parent(1).unwrap();
+                BranchCommit::Merge(curr, Branch::without_name(&self.repo, p2.id))
-
-                Some((
-                    // Set the Merge commit field to None because it's
-                    // used to communicate special states (like a
-                    // merge after another merge).
-                    BranchCommit::Merge(None, Branch::without_name(&self.repo, p2.id)),
-                    IterCmd::Skip(p1.id),
-                ))
             }
-            // More or negative parents means the universe is ending
+            _ => panic!("Octopus merges not yet implemented!"),
-            _ => panic!("Octopus merges are not implemented yet!"),
+        }
-        })
+    }
+
+    /// Get the current commit
+    ///
+    /// This function looks either at the "curr" field, or takes the
+    /// ID from `cmd`, if it is set to `IterCmd::Jump(...)`, which
+    /// indicates that the previous commit was a merge, and we need to escape
+    fn set_next(&mut self, current: Commit) -> Commit {
+        self.curr = match current.first_parent() {
+            Some(p1) => Some(p1.id),
+            None => None,
+        };
+
+        current
     }
 }
 
@@ -180,57 +144,27 @@ impl Iterator for BranchIter {
     type Item = BranchCommit;
 
     fn next(&mut self) -> Option<Self::Item> {
-        let cmd = mem::replace(&mut self.cmd, IterCmd::Step);
+        mem::replace(&mut self.curr, None)
-        let last = self.find_commit(&self.last);
+            .and_then(|id| self.find_commit(&id))
-
+            .map(|c| self.set_next(c))
-        match self.limit {
+            .and_then(|c| match self.limit {
-            // Get commits forever
+                SegLimit::None => Some(c),
-            SegLimit::None => self.get_parent(last, cmd).map(|bc| self.set_last(bc)),
+                SegLimit::Commit(ended, _) if ended => None,
-            // Get commits until hitting a certain ID
+                SegLimit::Commit(ref mut b, ref target) => {
-            SegLimit::Commit(ended, _) if ended => None,
+                    if &c.id == target {
-            SegLimit::Commit(_, ref c) => {
+                        *b = true;
-                let c = c.clone();
+                    }
-                self.get_parent(last, cmd)
+
-                    .map(|(bc, cmd)| {
+                    Some(c)
-                        // Set iterator to "done" if we have reached the commit
-                        if bc.id() == c {
-                            self.limit = SegLimit::Commit(true, c.clone());
-                            (bc, cmd)
-                        } else {
-                            (bc, cmd)
-                        }
-                    })
-                    // Set last in case there's more to iterate
-                    .map(|bc| self.set_last(bc))
-            }
-            // Get a certain number of commits
-            SegLimit::Length(ref mut curr, ref mut max) => {
-                if curr >= max {
-                    return None;
                 }
-
+                SegLimit::Length(ref mut curr, ref max) if *curr < *max => {
-                *curr += 1;
+                    *curr += 1;
-                self.get_parent(last, cmd).map(|bc| self.set_last(bc))
+                    Some(c)
-            }
+                }
-        }
+                SegLimit::Length(ref curr, ref mut max) if curr >= max => None,
-    }
+                SegLimit::Length(_, _) => unreachable!(), // oh rustc :)
-}
+            })
-
+            .map(|c| self.make_branch_commit(c))
-/// Specify how to trace actions on the iterator
-#[derive(Debug)]
-enum IterCmd {
-    /// Set the last commit to an ID
-    Step,
-    /// Specify a parent to step to next
-    Skip(HashId),
-}
-
-impl IterCmd {
-    fn take(self) -> Option<HashId> {
-        match self {
-            Self::Skip(id) => Some(id),
-            Self::Step => None,
-        }
     }
 }
 
@@ -253,7 +187,7 @@ pub enum BranchCommit {
     /// A single commit
     Commit(Commit),
     /// A merge commit from one other branch
-    Merge(Option<Commit>, Branch),
+    Merge(Commit, Branch),
     /// An octopus merge with multiple branches
     Octopus(Commit, Vec<Branch>),
 }