[BREAKING] Update undo support to use Operations

This updates the existing "undo" support to use `Operations`, while
making some additional changes to look forward to more general undo
support (GothenburgBitFactory#427 being the next step on that journey).

Breaking changes:

 - `Replica::get_undo_ops` is now `Replica::get_undo_operations` and returns an
   `Operations` value. It now returns the operations in the order they were
   applied, and includes the undo point operation, if one exists.
 - `Replica::commit_undo_ops` is now `Replica::commit_reversed_operations` and
   takes an `Operations` value. It now expects an `Operations` value as
   provided by `get_undo_operations`.

taskchampion/src/operation.rs

@@ -71,11 +71,6 @@ impl Operations {
         self.0.push(op);
     }
 
-    /// Determine if this set of operations is empty.
-    pub fn is_empty(&self) -> bool {
-        self.0.is_empty()
-    }
-
     /// For tests, it's useful to set the timestamps of all updates to the same value.
     #[cfg(test)]
     pub fn set_all_timestamps(&mut self, set_to: DateTime<Utc>) {
@@ -87,6 +82,20 @@ impl Operations {
     }
 }
 
+impl From<Vec<Operation>> for Operations {
+    fn from(value: Vec<Operation>) -> Self {
+        Self(value)
+    }
+}
+
+impl std::ops::Deref for Operations {
+    type Target = [Operation];
+
+    fn deref(&self) -> &Self::Target {
+        self.0.deref()
+    }
+}
+
 impl IntoIterator for Operations {
     type Item = Operation;
     type IntoIter = std::vec::IntoIter<Operation>;

taskchampion/src/replica.rs

@@ -329,15 +329,31 @@ impl Replica {
         Ok(())
     }
 
-    /// Return undo local operations until the most recent UndoPoint, returning an empty Vec if there are no
-    /// local operations to undo.
-    pub fn get_undo_ops(&mut self) -> Result<Vec<Operation>> {
-        self.taskdb.get_undo_ops()
+    /// Return the operations back to and including the last undo point, or since the last sync if
+    /// no undo point is found.
+    ///
+    /// The operations are returned in the order they were applied. Use
+    /// [`commit_reversed_operations`] to "undo" them.
+    pub fn get_undo_operations(&mut self) -> Result<Operations> {
+        self.taskdb.get_undo_operations()
     }
 
-    /// Undo local operations in storage, returning a boolean indicating success.
-    pub fn commit_undo_ops(&mut self, undo_ops: Vec<Operation>) -> Result<bool> {
-        self.taskdb.commit_undo_ops(undo_ops)
+    /// Commit the reverse of the given operations, beginning with the last operation in the given
+    /// operations and proceeding to the first.
+    ///
+    /// This method only supports reversing operations if they precisely match local operations
+    /// that have not yet been synchronized, and will return `false` if this is not the case.
+    pub fn commit_reversed_operations(&mut self, operations: Operations) -> Result<bool> {
+        if !self.taskdb.commit_reversed_operations(operations)? {
+            return Ok(false);
+        }
+
+        // Both the dependency map and the working set are potentially now invalid.
+        self.depmap = None;
+        self.rebuild_working_set(false)
+            .context("Failed to rebuild working set after committing reversed operations")?;
+
+        Ok(true)
     }
 
     /// Rebuild this replica's working set, based on whether tasks are pending or not.  If

taskchampion/src/taskdb/mod.rs

@@ -133,17 +133,24 @@ impl TaskDb {
         sync::sync(server, txn.as_mut(), avoid_snapshots)
     }
 
-    /// Return undo local operations until the most recent UndoPoint, returning an empty Vec if there are no
-    /// local operations to undo.
-    pub(crate) fn get_undo_ops(&mut self) -> Result<Vec<Operation>> {
+    /// Return the operations back to and including the last undo point, or since the last sync if
+    /// no undo point is found.
+    ///
+    /// The operations are returned in the order they were applied. Use
+    /// [`commit_reversed_operations`] to "undo" them.
+    pub(crate) fn get_undo_operations(&mut self) -> Result<Operations> {
         let mut txn = self.storage.txn()?;
-        undo::get_undo_ops(txn.as_mut())
+        undo::get_undo_operations(txn.as_mut())
     }
 
-    /// Undo local operations in storage, returning a boolean indicating success.
-    pub(crate) fn commit_undo_ops(&mut self, undo_ops: Vec<Operation>) -> Result<bool> {
+    /// Commit the reverse of the given operations, beginning with the last operation in the given
+    /// operations and proceeding to the first.
+    ///
+    /// This method only supports reversing operations if they precisely match local operations
+    /// that have not yet been synchronized, and will return `false` if this is not the case.
+    pub(crate) fn commit_reversed_operations(&mut self, undo_ops: Operations) -> Result<bool> {
         let mut txn = self.storage.txn()?;
-        undo::commit_undo_ops(txn.as_mut(), undo_ops)
+        undo::commit_reversed_operations(txn.as_mut(), undo_ops)
     }
 
     /// Get the number of un-synchronized operations in storage, excluding undo

taskchampion/src/taskdb/undo.rs

@@ -1,24 +1,29 @@
 use super::apply;
 use crate::errors::Result;
-use crate::operation::Operation;
+use crate::operation::{Operation, Operations};
 use crate::server::SyncOp;
 use crate::storage::StorageTxn;
 use chrono::Utc;
 use log::{debug, info, trace};
 
-/// Local operations until the most recent UndoPoint.
-pub fn get_undo_ops(txn: &mut dyn StorageTxn) -> Result<Vec<Operation>> {
-    let mut local_ops = txn.operations().unwrap();
-    let mut undo_ops: Vec<Operation> = Vec::new();
-
-    while let Some(op) = local_ops.pop() {
-        if op == Operation::UndoPoint {
-            break;
-        }
-        undo_ops.push(op);
+/// Return the operations back to and including the last undo point, or since the last sync if no
+/// undo point is found.
+///
+/// The operations are returned in the order they were applied. Use [`commit_reversed_operations`]
+/// to "undo" them.
+pub fn get_undo_operations(txn: &mut dyn StorageTxn) -> Result<Operations> {
+    let local_ops = txn.operations().unwrap();
+    let last_undo_op_idx = local_ops
+        .iter()
+        .enumerate()
+        .rev()
+        .find(|(_, op)| op.is_undo_point())
+        .map(|(i, _)| i);
+    if let Some(last_undo_op_idx) = last_undo_op_idx {
+        Ok(local_ops[last_undo_op_idx..].to_vec().into())
+    } else {
+        Ok(local_ops.into())
     }
-
-    Ok(undo_ops)
 }
 
 /// Generate a sequence of SyncOp's to reverse the effects of this Operation.
@@ -56,24 +61,22 @@ fn reverse_ops(op: Operation) -> Vec<SyncOp> {
     }
 }
 
-/// Commit operations to storage, returning a boolean indicating success.
-pub fn commit_undo_ops(txn: &mut dyn StorageTxn, mut undo_ops: Vec<Operation>) -> Result<bool> {
+/// Commit the reverse of the given operations, beginning with the last operation in the given
+/// operations and proceeding to the first.
+///
+/// This method only supports reversing operations if they precisely match local operations that
+/// have not yet been synchronized, and will return `false` if this is not the case.
+pub fn commit_reversed_operations(txn: &mut dyn StorageTxn, undo_ops: Operations) -> Result<bool> {
     let mut applied = false;
     let mut local_ops = txn.operations().unwrap();
-
-    // Add UndoPoint to undo_ops unless this undo will empty the operations database, in which case
-    // there is no UndoPoint.
-    if local_ops.len() > undo_ops.len() {
-        undo_ops.push(Operation::UndoPoint);
-    }
+    let mut undo_ops = undo_ops.to_vec();
 
     // Drop undo_ops iff they're the latest operations.
     // TODO Support concurrent undo by adding the reverse of undo_ops rather than popping from operations.
     let old_len = local_ops.len();
     let undo_len = undo_ops.len();
     let new_len = old_len - undo_len;
     let local_undo_ops = &local_ops[new_len..old_len];
-    undo_ops.reverse();
     if local_undo_ops != undo_ops {
         info!("Undo failed: concurrent changes to the database occurred.");
         debug!(
@@ -172,31 +175,47 @@ mod tests {
 
         assert_eq!(db.operations().len(), 9, "{:#?}", db.operations());
 
-        let undo_ops = get_undo_ops(db.storage.txn()?.as_mut())?;
-        assert_eq!(undo_ops.len(), 3, "{:#?}", undo_ops);
+        let undo_ops = get_undo_operations(db.storage.txn()?.as_mut())?;
+        assert_eq!(undo_ops.len(), 4, "{:#?}", undo_ops);
+        assert_eq!(&undo_ops[..], &db.operations()[5..]);
+
+        // Try committing the wrong set of ops.
+        assert!(!commit_reversed_operations(
+            db.storage.txn()?.as_mut(),
+            undo_ops[1..=2].to_vec().into(),
+        )?);
 
-        assert!(commit_undo_ops(db.storage.txn()?.as_mut(), undo_ops)?);
+        assert!(commit_reversed_operations(
+            db.storage.txn()?.as_mut(),
+            undo_ops
+        )?);
 
-        // Note that we've subtracted the length of undo_ops plus one for the UndoPoint.
+        // Note that we've subtracted the length of undo_ops.
         assert_eq!(db.operations().len(), 5, "{:#?}", db.operations());
         assert_eq!(db.sorted_tasks(), db_state, "{:#?}", db.sorted_tasks());
 
         // Note that the number of undo operations is equal to the number of operations in the
         // database here because there are no UndoPoints.
-        let undo_ops = get_undo_ops(db.storage.txn()?.as_mut())?;
+        let undo_ops = get_undo_operations(db.storage.txn()?.as_mut())?;
         assert_eq!(undo_ops.len(), 5, "{:#?}", undo_ops);
 
-        assert!(commit_undo_ops(db.storage.txn()?.as_mut(), undo_ops)?);
+        assert!(commit_reversed_operations(
+            db.storage.txn()?.as_mut(),
+            undo_ops
+        )?);
 
         // empty db
         assert_eq!(db.operations().len(), 0, "{:#?}", db.operations());
         assert_eq!(db.sorted_tasks(), vec![], "{:#?}", db.sorted_tasks());
 
-        let undo_ops = get_undo_ops(db.storage.txn()?.as_mut())?;
+        let undo_ops = get_undo_operations(db.storage.txn()?.as_mut())?;
         assert_eq!(undo_ops.len(), 0, "{:#?}", undo_ops);
 
         // nothing left to undo, so commit_undo_ops() returns false
-        assert!(!commit_undo_ops(db.storage.txn()?.as_mut(), undo_ops)?);
+        assert!(!commit_reversed_operations(
+            db.storage.txn()?.as_mut(),
+            undo_ops
+        )?);
 
         Ok(())
     }