show undo diff (#3213)

Exposes undo operations via the C API, and uses those to show a (new, differently formatted) diff before committing the undo.

taskchampion/integration-tests/src/bindings_tests/replica.c

@@ -25,7 +25,8 @@ static void test_replica_undo_empty(void) {
     TCReplica *rep = tc_replica_new_in_memory();
     TEST_ASSERT_NULL(tc_replica_error(rep).ptr);
     int undone;
-    int rv = tc_replica_undo(rep, &undone);
+    TCReplicaOpList undo_ops = tc_replica_get_undo_ops(rep);
+    int rv  = tc_replica_commit_undo_ops(rep, undo_ops, &undone);
     TEST_ASSERT_EQUAL(TC_RESULT_OK, rv);
     TEST_ASSERT_EQUAL(0, undone);
     TEST_ASSERT_NULL(tc_replica_error(rep).ptr);
@@ -114,11 +115,12 @@ static void test_replica_working_set(void) {
     tc_replica_free(rep);
 }
 
-// When tc_replica_undo is passed NULL for undone_out, it still succeeds
+// When tc_replica_commit_undo_ops is passed NULL for undone_out, it still succeeds
 static void test_replica_undo_empty_null_undone_out(void) {
     TCReplica *rep = tc_replica_new_in_memory();
     TEST_ASSERT_NULL(tc_replica_error(rep).ptr);
-    int rv = tc_replica_undo(rep, NULL);
+    TCReplicaOpList undo_ops = tc_replica_get_undo_ops(rep);
+    int rv  = tc_replica_commit_undo_ops(rep, undo_ops, NULL);
     TEST_ASSERT_EQUAL(TC_RESULT_OK, rv);
     TEST_ASSERT_NULL(tc_replica_error(rep).ptr);
     tc_replica_free(rep);
@@ -156,7 +158,6 @@ static void test_replica_task_creation(void) {
     tc_replica_free(rep);
 }
 
-// When tc_replica_undo is passed NULL for undone_out, it still succeeds
 static void test_replica_sync_local(void) {
     TCReplica *rep = tc_replica_new_in_memory();
     TEST_ASSERT_NULL(tc_replica_error(rep).ptr);
@@ -182,7 +183,6 @@ static void test_replica_sync_local(void) {
     tc_string_free(&err);
 }
 
-// When tc_replica_undo is passed NULL for undone_out, it still succeeds
 static void test_replica_remote_server(void) {
     TCString err;
     TCServer *server = tc_server_new_sync(

taskchampion/lib/src/kv.rs

@@ -17,6 +17,7 @@ use crate::types::*;
 /// } TCKV;
 /// ```
 #[repr(C)]
+#[derive(Debug)]
 pub struct TCKV {
     pub key: TCString,
     pub value: TCString,
@@ -68,11 +69,12 @@ impl PassByValue for TCKV {
 /// } TCKVList;
 /// ```
 #[repr(C)]
+#[derive(Debug)]
 pub struct TCKVList {
-    len: libc::size_t,
+    pub len: libc::size_t,
     /// total size of items (internal use only)
-    _capacity: libc::size_t,
-    items: *mut TCKV,
+    pub _capacity: libc::size_t,
+    pub items: *mut TCKV,
 }
 
 impl CList for TCKVList {
@@ -100,6 +102,14 @@ impl CList for TCKVList {
     }
 }
 
+impl Default for TCKVList {
+    fn default() -> Self {
+        // SAFETY:
+        //  - caller will free this list
+        unsafe { TCKVList::return_val(Vec::new()) }
+    }
+}
+
 // NOTE: callers never have a TCKV that is not in a list, so there is no tc_kv_free.
 
 #[ffizz_header::item]

taskchampion/lib/src/lib.rs

@@ -65,7 +65,7 @@ ffizz_header::snippet! {
 /// ## Pass by Pointer
 ///
 /// Several types such as TCReplica and TCString are "opaque" types and always handled as pointers
-/// in C. The bytes these pointers address are private to the Rust implemetation and must not be
+/// in C. The bytes these pointers address are private to the Rust implementation and must not be
 /// accessed from C.
 ///
 /// Pass-by-pointer values have exactly one owner, and that owner is responsible for freeing the
@@ -153,7 +153,7 @@ pub use workingset::*;
 
 pub(crate) mod types {
     pub(crate) use crate::annotation::{TCAnnotation, TCAnnotationList};
-    pub(crate) use crate::kv::{TCKVList, TCKV};
+    pub(crate) use crate::kv::TCKVList;
     pub(crate) use crate::replica::TCReplica;
     pub(crate) use crate::result::TCResult;
     pub(crate) use crate::server::TCServer;

taskchampion/lib/src/replica.rs

@@ -2,6 +2,7 @@ use crate::traits::*;
 use crate::types::*;
 use crate::util::err_to_ruststring;
 use std::ptr::NonNull;
+use taskchampion::storage::ReplicaOp;
 use taskchampion::{Replica, StorageConfig};
 
 #[ffizz_header::item]
@@ -130,6 +131,36 @@ where
     }
 }
 
+#[ffizz_header::item]
+#[ffizz(order = 900)]
+/// ***** TCReplicaOpType *****
+///
+/// ```c
+/// enum TCReplicaOpType
+/// #ifdef __cplusplus
+///   : uint32_t
+/// #endif // __cplusplus
+/// {
+///     Create = 0,
+///     Delete = 1,
+///     Update = 2,
+///     UndoPoint = 3,
+/// };
+/// #ifndef __cplusplus
+/// typedef uint32_t TCReplicaOpType;
+/// #endif // __cplusplus
+/// ```
+#[derive(Debug, Default)]
+#[repr(u32)]
+pub enum TCReplicaOpType {
+    Create = 0,
+    Delete = 1,
+    Update = 2,
+    UndoPoint = 3,
+    #[default]
+    Error = 4,
+}
+
 #[ffizz_header::item]
 #[ffizz(order = 901)]
 /// Create a new TCReplica with an in-memory database.  The contents of the database will be
@@ -186,6 +217,102 @@ pub unsafe extern "C" fn tc_replica_new_on_disk(
     )
 }
 
+#[ffizz_header::item]
+#[ffizz(order = 901)]
+/// ***** TCReplicaOp *****
+///
+/// ```c
+/// struct TCReplicaOp {
+///     TCReplicaOpType operation_type;
+///     void* inner;
+/// };
+///
+/// typedef struct TCReplicaOp TCReplicaOp;
+/// ```
+#[derive(Debug)]
+#[repr(C)]
+pub struct TCReplicaOp {
+    operation_type: TCReplicaOpType,
+    inner: Box<ReplicaOp>,
+}
+
+impl From<ReplicaOp> for TCReplicaOp {
+    fn from(replica_op: ReplicaOp) -> TCReplicaOp {
+        match replica_op {
+            ReplicaOp::Create { .. } => TCReplicaOp {
+                operation_type: TCReplicaOpType::Create,
+                inner: Box::new(replica_op),
+            },
+            ReplicaOp::Delete { .. } => TCReplicaOp {
+                operation_type: TCReplicaOpType::Delete,
+                inner: Box::new(replica_op),
+            },
+            ReplicaOp::Update { .. } => TCReplicaOp {
+                operation_type: TCReplicaOpType::Update,
+                inner: Box::new(replica_op),
+            },
+            ReplicaOp::UndoPoint => TCReplicaOp {
+                operation_type: TCReplicaOpType::UndoPoint,
+                inner: Box::new(replica_op),
+            },
+        }
+    }
+}
+
+#[ffizz_header::item]
+#[ffizz(order = 901)]
+/// ***** TCReplicaOpList *****
+///
+/// ```c
+/// struct TCReplicaOpList {
+///     struct TCReplicaOp *items;
+///     size_t len;
+///     size_t capacity;
+/// };
+///
+/// typedef struct TCReplicaOpList TCReplicaOpList;
+/// ```
+#[repr(C)]
+#[derive(Debug)]
+pub struct TCReplicaOpList {
+    items: *mut TCReplicaOp,
+    len: usize,
+    capacity: usize,
+}
+
+impl Default for TCReplicaOpList {
+    fn default() -> Self {
+        // SAFETY:
+        //  - caller will free this value
+        unsafe { TCReplicaOpList::return_val(Vec::new()) }
+    }
+}
+
+impl CList for TCReplicaOpList {
+    type Element = TCReplicaOp;
+
+    unsafe fn from_raw_parts(items: *mut Self::Element, len: usize, cap: usize) -> Self {
+        TCReplicaOpList {
+            len,
+            capacity: cap,
+            items,
+        }
+    }
+
+    fn slice(&mut self) -> &mut [Self::Element] {
+        // SAFETY:
+        //  - because we have &mut self, we have read/write access to items[0..len]
+        //  - all items are properly initialized Element's
+        //  - return value lifetime is equal to &mmut self's, so access is exclusive
+        //  - items and len came from Vec, so total size is < isize::MAX
+        unsafe { std::slice::from_raw_parts_mut(self.items, self.len) }
+    }
+
+    fn into_raw_parts(self) -> (*mut Self::Element, usize, usize) {
+        (self.items, self.len, self.capacity)
+    }
+}
+
 #[ffizz_header::item]
 #[ffizz(order = 902)]
 /// Get a list of all tasks in the replica.
@@ -408,20 +535,58 @@ pub unsafe extern "C" fn tc_replica_sync(
 
 #[ffizz_header::item]
 #[ffizz(order = 902)]
-/// Undo local operations until the most recent UndoPoint.
+/// Return undo local operations until the most recent UndoPoint.
+///
+/// ```c
+/// EXTERN_C TCReplicaOpList tc_replica_get_undo_ops(struct TCReplica *rep);
+/// ```
+#[no_mangle]
+pub unsafe extern "C" fn tc_replica_get_undo_ops(rep: *mut TCReplica) -> TCReplicaOpList {
+    wrap(
+        rep,
+        |rep| {
+            // SAFETY:
+            //  - caller will free this value, either with tc_replica_commit_undo_ops or
+            //  tc_replica_op_list_free.
+            Ok(unsafe {
+                TCReplicaOpList::return_val(
+                    rep.get_undo_ops()?
+                        .into_iter()
+                        .map(TCReplicaOp::from)
+                        .collect(),
+                )
+            })
+        },
+        TCReplicaOpList::default(),
+    )
+}
+
+#[ffizz_header::item]
+#[ffizz(order = 902)]
+/// Undo local operations in storage.
 ///
 /// If undone_out is not NULL, then on success it is set to 1 if operations were undone, or 0 if
 /// there are no operations that can be done.
 ///
 /// ```c
-/// EXTERN_C TCResult tc_replica_undo(struct TCReplica *rep, int32_t *undone_out);
+/// EXTERN_C TCResult tc_replica_commit_undo_ops(struct TCReplica *rep, TCReplicaOpList tc_undo_ops, int32_t *undone_out);
 /// ```
 #[no_mangle]
-pub unsafe extern "C" fn tc_replica_undo(rep: *mut TCReplica, undone_out: *mut i32) -> TCResult {
+pub unsafe extern "C" fn tc_replica_commit_undo_ops(
+    rep: *mut TCReplica,
+    tc_undo_ops: TCReplicaOpList,
+    undone_out: *mut i32,
+) -> TCResult {
     wrap(
         rep,
         |rep| {
-            let undone = i32::from(rep.undo()?);
+            // SAFETY:
+            // - `tc_undo_ops` is a valid value, as it was acquired from `tc_replica_get_undo_ops`.
+            let undo_ops: Vec<ReplicaOp> = unsafe { TCReplicaOpList::val_from_arg(tc_undo_ops) }
+                .into_iter()
+                .map(|op| *op.inner)
+                .collect();
+            let undone = i32::from(rep.commit_undo_ops(undo_ops)?);
             if !undone_out.is_null() {
                 // SAFETY:
                 //  - undone_out is not NULL (just checked)
@@ -565,3 +730,175 @@ pub unsafe extern "C" fn tc_replica_free(rep: *mut TCReplica) {
     }
     drop(replica);
 }
+
+#[ffizz_header::item]
+#[ffizz(order = 903)]
+/// Free a vector of ReplicaOp.  The vector may not be used after this function returns and must not be freed
+/// more than once.
+///
+/// ```c
+/// EXTERN_C void tc_replica_op_list_free(struct TCReplicaOpList *oplist);
+/// ```
+#[no_mangle]
+pub unsafe extern "C" fn tc_replica_op_list_free(oplist: *mut TCReplicaOpList) {
+    debug_assert!(!oplist.is_null());
+    // SAFETY:
+    // - arg is not NULL (just checked)
+    // - `*oplist` is valid (guaranteed by caller not double-freeing this value)
+    unsafe {
+        TCReplicaOpList::take_val_from_arg(
+            oplist,
+            // SAFETY:
+            //  - value is empty, so the caller need not free it.
+            TCReplicaOpList::return_val(Vec::new()),
+        )
+    };
+}
+
+#[ffizz_header::item]
+#[ffizz(order = 903)]
+/// Return uuid field of ReplicaOp.
+///
+/// ```c
+/// EXTERN_C struct TCString tc_replica_op_get_uuid(struct TCReplicaOp *op);
+/// ```
+#[no_mangle]
+pub unsafe extern "C" fn tc_replica_op_get_uuid(op: *const TCReplicaOp) -> TCString {
+    // SAFETY:
+    //   - inner is not null
+    //   - inner is a living object
+    let rop: &ReplicaOp = unsafe { (*op).inner.as_ref() };
+
+    if let ReplicaOp::Create { uuid }
+    | ReplicaOp::Delete { uuid, .. }
+    | ReplicaOp::Update { uuid, .. } = rop
+    {
+        let uuid_rstr: RustString = uuid.to_string().into();
+        // SAFETY:
+        //  - caller promises to free this string
+        unsafe { TCString::return_val(uuid_rstr) }
+    } else {
+        panic!("Operation has no uuid: {:#?}", rop);
+    }
+}
+
+#[ffizz_header::item]
+#[ffizz(order = 903)]
+/// Return property field of ReplicaOp.
+///
+/// ```c
+/// EXTERN_C struct TCString tc_replica_op_get_property(struct TCReplicaOp *op);
+/// ```
+#[no_mangle]
+pub unsafe extern "C" fn tc_replica_op_get_property(op: *const TCReplicaOp) -> TCString {
+    // SAFETY:
+    //   - inner is not null
+    //   - inner is a living object
+    let rop: &ReplicaOp = unsafe { (*op).inner.as_ref() };
+
+    if let ReplicaOp::Update { property, .. } = rop {
+        // SAFETY:
+        //  - caller promises to free this string
+        unsafe { TCString::return_val(property.clone().into()) }
+    } else {
+        panic!("Operation has no property: {:#?}", rop);
+    }
+}
+
+#[ffizz_header::item]
+#[ffizz(order = 903)]
+/// Return value field of ReplicaOp.
+///
+/// ```c
+/// EXTERN_C struct TCString tc_replica_op_get_value(struct TCReplicaOp *op);
+/// ```
+#[no_mangle]
+pub unsafe extern "C" fn tc_replica_op_get_value(op: *const TCReplicaOp) -> TCString {
+    // SAFETY:
+    //   - inner is not null
+    //   - inner is a living object
+    let rop: &ReplicaOp = unsafe { (*op).inner.as_ref() };
+
+    if let ReplicaOp::Update { value, .. } = rop {
+        let value_rstr: RustString = value.clone().unwrap_or(String::new()).into();
+        // SAFETY:
+        //  - caller promises to free this string
+        unsafe { TCString::return_val(value_rstr) }
+    } else {
+        panic!("Operation has no value: {:#?}", rop);
+    }
+}
+
+#[ffizz_header::item]
+#[ffizz(order = 903)]
+/// Return old value field of ReplicaOp.
+///
+/// ```c
+/// EXTERN_C struct TCString tc_replica_op_get_old_value(struct TCReplicaOp *op);
+/// ```
+#[no_mangle]
+pub unsafe extern "C" fn tc_replica_op_get_old_value(op: *const TCReplicaOp) -> TCString {
+    // SAFETY:
+    //   - inner is not null
+    //   - inner is a living object
+    let rop: &ReplicaOp = unsafe { (*op).inner.as_ref() };
+
+    if let ReplicaOp::Update { old_value, .. } = rop {
+        let old_value_rstr: RustString = old_value.clone().unwrap_or(String::new()).into();
+        // SAFETY:
+        //  - caller promises to free this string
+        unsafe { TCString::return_val(old_value_rstr) }
+    } else {
+        panic!("Operation has no old value: {:#?}", rop);
+    }
+}
+
+#[ffizz_header::item]
+#[ffizz(order = 903)]
+/// Return timestamp field of ReplicaOp.
+///
+/// ```c
+/// EXTERN_C struct TCString tc_replica_op_get_timestamp(struct TCReplicaOp *op);
+/// ```
+#[no_mangle]
+pub unsafe extern "C" fn tc_replica_op_get_timestamp(op: *const TCReplicaOp) -> TCString {
+    // SAFETY:
+    //   - inner is not null
+    //   - inner is a living object
+    let rop: &ReplicaOp = unsafe { (*op).inner.as_ref() };
+
+    if let ReplicaOp::Update { timestamp, .. } = rop {
+        let timestamp_rstr: RustString = timestamp.to_string().into();
+        // SAFETY:
+        //  - caller promises to free this string
+        unsafe { TCString::return_val(timestamp_rstr) }
+    } else {
+        panic!("Operation has no timestamp: {:#?}", rop);
+    }
+}
+
+#[ffizz_header::item]
+#[ffizz(order = 903)]
+/// Return description field of old task field of ReplicaOp.
+///
+/// ```c
+/// EXTERN_C struct TCString tc_replica_op_get_old_task_description(struct TCReplicaOp *op);
+/// ```
+#[no_mangle]
+pub unsafe extern "C" fn tc_replica_op_get_old_task_description(
+    op: *const TCReplicaOp,
+) -> TCString {
+    // SAFETY:
+    //   - inner is not null
+    //   - inner is a living object
+    let rop: &ReplicaOp = unsafe { (*op).inner.as_ref() };
+
+    if let ReplicaOp::Delete { old_task, .. } = rop {
+        let description_rstr: RustString = old_task["description"].clone().into();
+        // SAFETY:
+        //  - caller promises to free this string
+        unsafe { TCString::return_val(description_rstr) }
+    } else {
+        panic!("Operation has no timestamp: {:#?}", rop);
+    }
+}

taskchampion/lib/src/string.rs

@@ -55,6 +55,7 @@ use std::path::PathBuf;
 /// } TCString;
 /// ```
 #[repr(C)]
+#[derive(Debug)]
 pub struct TCString {
     // defined based on the type
     ptr: *mut libc::c_void,

taskchampion/lib/src/task.rs

@@ -1,6 +1,7 @@
 use crate::traits::*;
 use crate::types::*;
 use crate::util::err_to_ruststring;
+use crate::TCKV;
 use std::convert::TryFrom;
 use std::ops::Deref;
 use std::ptr::NonNull;

taskchampion/lib/src/uuid.rs

@@ -16,6 +16,7 @@ use taskchampion::Uuid;
 /// } TCUuid;
 /// ```
 #[repr(C)]
+#[derive(Debug, Default)]
 pub struct TCUuid([u8; 16]);
 
 impl PassByValue for TCUuid {

taskchampion/lib/taskchampion.h

@@ -33,7 +33,7 @@
 // ## Pass by Pointer
 //
 // Several types such as TCReplica and TCString are "opaque" types and always handled as pointers
-// in C. The bytes these pointers address are private to the Rust implemetation and must not be
+// in C. The bytes these pointers address are private to the Rust implementation and must not be
 // accessed from C.
 //
 // Pass-by-pointer values have exactly one owner, and that owner is responsible for freeing the
@@ -480,6 +480,38 @@ EXTERN_C void tc_server_free(struct TCServer *server);
 // TCReplicas are not threadsafe.
 typedef struct TCReplica TCReplica;
 
+// ***** TCReplicaOpType *****
+enum TCReplicaOpType
+#ifdef __cplusplus
+  : uint32_t
+#endif // __cplusplus
+{
+    Create = 0,
+    Delete = 1,
+    Update = 2,
+    UndoPoint = 3,
+};
+#ifndef __cplusplus
+typedef uint32_t TCReplicaOpType;
+#endif // __cplusplus
+
+// ***** TCReplicaOp *****
+struct TCReplicaOp {
+    TCReplicaOpType operation_type;
+    void* inner;
+};
+
+typedef struct TCReplicaOp TCReplicaOp;
+
+// ***** TCReplicaOpList *****
+struct TCReplicaOpList {
+    struct TCReplicaOp *items;
+    size_t len;
+    size_t capacity;
+};
+
+typedef struct TCReplicaOpList TCReplicaOpList;
+
 // Create a new TCReplica with an in-memory database.  The contents of the database will be
 // lost when it is freed with tc_replica_free.
 EXTERN_C struct TCReplica *tc_replica_new_in_memory(void);
@@ -509,6 +541,12 @@ EXTERN_C struct TCUuidList tc_replica_all_task_uuids(struct TCReplica *rep);
 // Returns a TCTaskList with a NULL items field on error.
 EXTERN_C struct TCTaskList tc_replica_all_tasks(struct TCReplica *rep);
 
+// Undo local operations in storage.
+//
+// If undone_out is not NULL, then on success it is set to 1 if operations were undone, or 0 if
+// there are no operations that can be done.
+EXTERN_C TCResult tc_replica_commit_undo_ops(struct TCReplica *rep, TCReplicaOpList tc_undo_ops, int32_t *undone_out);
+
 // Get the latest error for a replica, or a string with NULL ptr if no error exists.  Subsequent
 // calls to this function will return NULL.  The rep pointer must not be NULL.  The caller must
 // free the returned string.
@@ -520,6 +558,9 @@ EXTERN_C struct TCString tc_replica_error(struct TCReplica *rep);
 // to distinguish the two conditions.
 EXTERN_C struct TCTask *tc_replica_get_task(struct TCReplica *rep, struct TCUuid tcuuid);
 
+// Return undo local operations until the most recent UndoPoint.
+EXTERN_C TCReplicaOpList tc_replica_get_undo_ops(struct TCReplica *rep);
+
 // Create a new task.  The task must not already exist.
 //
 // Returns the task, or NULL on error.
@@ -549,12 +590,6 @@ EXTERN_C TCResult tc_replica_rebuild_working_set(struct TCReplica *rep, bool ren
 // The `server` argument remains owned by the caller, and must be freed explicitly.
 EXTERN_C TCResult tc_replica_sync(struct TCReplica *rep, struct TCServer *server, bool avoid_snapshots);
 
-// Undo local operations until the most recent UndoPoint.
-//
-// If undone_out is not NULL, then on success it is set to 1 if operations were undone, or 0 if
-// there are no operations that can be done.
-EXTERN_C TCResult tc_replica_undo(struct TCReplica *rep, int32_t *undone_out);
-
 // Get the current working set for this replica.  The resulting value must be freed
 // with tc_working_set_free.
 //
@@ -565,6 +600,28 @@ EXTERN_C struct TCWorkingSet *tc_replica_working_set(struct TCReplica *rep);
 // more than once.
 EXTERN_C void tc_replica_free(struct TCReplica *rep);
 
+// Return description field of old task field of ReplicaOp.
+EXTERN_C struct TCString tc_replica_op_get_old_task_description(struct TCReplicaOp *op);
+
+// Return old value field of ReplicaOp.
+EXTERN_C struct TCString tc_replica_op_get_old_value(struct TCReplicaOp *op);
+
+// Return property field of ReplicaOp.
+EXTERN_C struct TCString tc_replica_op_get_property(struct TCReplicaOp *op);
+
+// Return timestamp field of ReplicaOp.
+EXTERN_C struct TCString tc_replica_op_get_timestamp(struct TCReplicaOp *op);
+
+// Return uuid field of ReplicaOp.
+EXTERN_C struct TCString tc_replica_op_get_uuid(struct TCReplicaOp *op);
+
+// Return value field of ReplicaOp.
+EXTERN_C struct TCString tc_replica_op_get_value(struct TCReplicaOp *op);
+
+// Free a vector of ReplicaOp.  The vector may not be used after this function returns and must not be freed
+// more than once.
+EXTERN_C void tc_replica_op_list_free(struct TCReplicaOpList *oplist);
+
 // ***** TCTask *****
 //
 // A task, as publicly exposed by this library.

taskchampion/taskchampion/src/replica.rs

@@ -1,7 +1,7 @@
 use crate::depmap::DependencyMap;
 use crate::errors::Result;
 use crate::server::{Server, SyncOp};
-use crate::storage::{Storage, TaskMap};
+use crate::storage::{ReplicaOp, Storage, TaskMap};
 use crate::task::{Status, Task};
 use crate::taskdb::TaskDb;
 use crate::workingset::WorkingSet;
@@ -236,10 +236,15 @@ impl Replica {
         Ok(())
     }
 
-    /// Undo local operations until the most recent UndoPoint, returning false if there are no
+    /// Return undo local operations until the most recent UndoPoint, returning an empty Vec if there are no
     /// local operations to undo.
-    pub fn undo(&mut self) -> Result<bool> {
-        self.taskdb.undo()
+    pub fn get_undo_ops(&mut self) -> Result<Vec<ReplicaOp>> {
+        self.taskdb.get_undo_ops()
+    }
+
+    /// Undo local operations in storage, returning a boolean indicating success.
+    pub fn commit_undo_ops(&mut self, undo_ops: Vec<ReplicaOp>) -> Result<bool> {
+        self.taskdb.commit_undo_ops(undo_ops)
     }
 
     /// Rebuild this replica's working set, based on whether tasks are pending or not.  If

taskchampion/taskchampion/src/storage/mod.rs

@@ -1,3 +1,4 @@
+use crate::errors::Result;
 /**
 This module defines the backend storage used by [`Replica`](crate::Replica).
 It defines a [trait](crate::storage::Storage) for storage implementations, and provides a default on-disk implementation as well as an in-memory implementation for testing.
@@ -5,7 +6,6 @@ It defines a [trait](crate::storage::Storage) for storage implementations, and p
 Typical uses of this crate do not interact directly with this module; [`StorageConfig`](crate::StorageConfig) is sufficient.
 However, users who wish to implement their own storage backends can implement the traits defined here and pass the result to [`Replica`](crate::Replica).
 */
-use crate::errors::Result;
 use std::collections::HashMap;
 use uuid::Uuid;
 

taskchampion/taskchampion/src/taskdb/mod.rs

@@ -6,7 +6,7 @@ use uuid::Uuid;
 mod apply;
 mod snapshot;
 mod sync;
-mod undo;
+pub mod undo;
 mod working_set;
 
 /// A TaskDb is the backend for a replica.  It manages the storage, operations, synchronization,
@@ -114,11 +114,17 @@ impl TaskDb {
         sync::sync(server, txn.as_mut(), avoid_snapshots)
     }
 
-    /// Undo local operations until the most recent UndoPoint, returning false if there are no
+    /// Return undo local operations until the most recent UndoPoint, returning an empty Vec if there are no
     /// local operations to undo.
-    pub fn undo(&mut self) -> Result<bool> {
+    pub fn get_undo_ops(&mut self) -> Result<Vec<ReplicaOp>> {
         let mut txn = self.storage.txn()?;
-        undo::undo(txn.as_mut())
+        undo::get_undo_ops(txn.as_mut())
+    }
+
+    /// Undo local operations in storage, returning a boolean indicating success.
+    pub fn commit_undo_ops(&mut self, undo_ops: Vec<ReplicaOp>) -> Result<bool> {
+        let mut txn = self.storage.txn()?;
+        undo::commit_undo_ops(txn.as_mut(), undo_ops)
     }
 
     /// Get the number of un-synchronized operations in storage, excluding undo

taskchampion/taskchampion/src/taskdb/undo.rs

@@ -1,19 +1,53 @@
 use super::apply;
 use crate::errors::Result;
 use crate::storage::{ReplicaOp, StorageTxn};
-use log::{debug, trace};
+use log::{debug, info, trace};
 
-/// Undo local operations until an UndoPoint.
-pub(super) fn undo(txn: &mut dyn StorageTxn) -> Result<bool> {
-    let mut applied = false;
-    let mut popped = false;
-    let mut local_ops = txn.operations()?;
+/// Local operations until the most recent UndoPoint.
+pub fn get_undo_ops(txn: &mut dyn StorageTxn) -> Result<Vec<ReplicaOp>> {
+    let mut local_ops = txn.operations().unwrap();
+    let mut undo_ops: Vec<ReplicaOp> = Vec::new();
 
     while let Some(op) = local_ops.pop() {
-        popped = true;
         if op == ReplicaOp::UndoPoint {
             break;
         }
+        undo_ops.push(op);
+    }
+
+    Ok(undo_ops)
+}
+
+/// Commit operations to storage, returning a boolean indicating success.
+pub fn commit_undo_ops(txn: &mut dyn StorageTxn, mut undo_ops: Vec<ReplicaOp>) -> 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(ReplicaOp::UndoPoint);
+    }
+
+    // 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!(
+            "local_undo_ops={:#?}\nundo_ops={:#?}",
+            local_undo_ops, undo_ops
+        );
+        return Ok(applied);
+    }
+    undo_ops.reverse();
+    local_ops.truncate(new_len);
+
+    for op in undo_ops {
         debug!("Reversing operation {:?}", op);
         let rev_ops = op.reverse_ops();
         for op in rev_ops {
@@ -23,7 +57,7 @@ pub(super) fn undo(txn: &mut dyn StorageTxn) -> Result<bool> {
         }
     }
 
-    if popped {
+    if undo_len != 0 {
         txn.set_operations(local_ops)?;
         txn.commit()?;
     }
@@ -87,31 +121,33 @@ mod tests {
             timestamp,
         })?;
 
-        assert_eq!(db.operations().len(), 9);
+        assert_eq!(db.operations().len(), 9, "{:#?}", db.operations());
 
-        {
-            let mut txn = db.storage.txn()?;
-            assert!(undo(txn.as_mut())?);
-        }
+        let undo_ops = get_undo_ops(db.storage.txn()?.as_mut())?;
+        assert_eq!(undo_ops.len(), 3, "{:#?}", undo_ops);
 
-        // undo took db back to the snapshot
-        assert_eq!(db.operations().len(), 5);
-        assert_eq!(db.sorted_tasks(), db_state);
+        assert!(commit_undo_ops(db.storage.txn()?.as_mut(), undo_ops)?);
 
-        {
-            let mut txn = db.storage.txn()?;
-            assert!(undo(txn.as_mut())?);
-        }
+        // Note that we've subtracted the length of undo_ops plus one for the UndoPoint.
+        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())?;
+        assert_eq!(undo_ops.len(), 5, "{:#?}", undo_ops);
+
+        assert!(commit_undo_ops(db.storage.txn()?.as_mut(), undo_ops)?);
 
         // empty db
-        assert_eq!(db.operations().len(), 0);
-        assert_eq!(db.sorted_tasks(), vec![]);
+        assert_eq!(db.operations().len(), 0, "{:#?}", db.operations());
+        assert_eq!(db.sorted_tasks(), vec![], "{:#?}", db.sorted_tasks());
 
-        {
-            let mut txn = db.storage.txn()?;
-            // nothing left to undo, so undo() returns false
-            assert!(!undo(txn.as_mut())?);
-        }
+        let undo_ops = get_undo_ops(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)?);
 
         Ok(())
     }