diff --git a/src/TDB2.cpp b/src/TDB2.cpp index 7a4cb7c1b..b5f146452 100644 --- a/src/TDB2.cpp +++ b/src/TDB2.cpp @@ -42,6 +42,7 @@ #include #include #include "tc/Server.h" +#include "tc/util.h" bool TDB2::debug_mode = false; static void dependency_scan (std::vector &); @@ -207,10 +208,58 @@ void TDB2::get_changes (std::vector & changes) //////////////////////////////////////////////////////////////////////////////// void TDB2::revert () { - replica.undo (NULL); + auto undo_ops = replica.get_undo_ops(); + if (undo_ops.len == 0) { + std::cout << "No operations to undo."; + return; + } + if (confirm_revert(undo_ops)) { + // Has the side-effect of freeing undo_ops. + replica.commit_undo_ops(undo_ops, NULL); + } else { + replica.free_replica_ops(undo_ops); + } replica.rebuild_working_set (false); } +//////////////////////////////////////////////////////////////////////////////// +bool TDB2::confirm_revert (struct tc::ffi::TCReplicaOpList undo_ops) +{ + // TODO Use show_diff rather than this basic listing of operations, though + // this might be a worthy undo.style itself. + std::cout << "The following " << undo_ops.len << " operations would be reverted:\n"; + for (size_t i = 0; i < undo_ops.len; i++) { + std::cout << "- "; + tc::ffi::TCReplicaOp op = undo_ops.items[i]; + switch(op.operation_type) { + case tc::ffi::TCReplicaOpType::Create: + std::cout << "Create " << replica.get_op_uuid(op); + break; + case tc::ffi::TCReplicaOpType::Delete: + std::cout << "Delete " << replica.get_op_old_task_description(op); + break; + case tc::ffi::TCReplicaOpType::Update: + std::cout << "Update " << replica.get_op_uuid(op) << "\n"; + std::cout << " " << replica.get_op_property(op) << ": " << option_string(replica.get_op_old_value(op)) << " -> " << option_string(replica.get_op_value(op)); + break; + case tc::ffi::TCReplicaOpType::UndoPoint: + throw std::string ("Can't undo UndoPoint."); + break; + default: + throw std::string ("Can't undo non-operation."); + break; + } + std::cout << "\n"; + } + return ! Context::getContext ().config.getBoolean ("confirmation") || + confirm ("The undo command is not reversible. Are you sure you want to revert to the previous state?"); +} + +//////////////////////////////////////////////////////////////////////////////// +std::string TDB2::option_string(std::string input) { + return input == "" ? "" : input; +} + //////////////////////////////////////////////////////////////////////////////// void TDB2::show_diff ( const std::string& current, diff --git a/src/TDB2.h b/src/TDB2.h index 30cfffff1..bce69f960 100644 --- a/src/TDB2.h +++ b/src/TDB2.h @@ -78,13 +78,15 @@ public: void dump (); void sync (tc::Server server, bool avoid_snapshots); + bool confirm_revert(struct tc::ffi::TCReplicaOpList); private: tc::Replica replica; std::optional _working_set; const tc::WorkingSet &working_set (); - void show_diff (const std::string&, const std::string&, const std::string&); + static std::string option_string (std::string input); + static void show_diff (const std::string&, const std::string&, const std::string&); }; #endif diff --git a/src/tc/Replica.cpp b/src/tc/Replica.cpp index b4a0b3b4d..2cc0765bd 100644 --- a/src/tc/Replica.cpp +++ b/src/tc/Replica.cpp @@ -31,6 +31,7 @@ #include "tc/Server.h" #include "tc/WorkingSet.h" #include "tc/util.h" +#include using namespace tc::ffi; @@ -154,14 +155,68 @@ void tc::Replica::sync (Server server, bool avoid_snapshots) } //////////////////////////////////////////////////////////////////////////////// -void tc::Replica::undo (int32_t *undone_out) +TCReplicaOpList tc::Replica::get_undo_ops () { - auto res = tc_replica_undo (&*inner, undone_out); + return tc_replica_get_undo_ops(&*inner); +} + +//////////////////////////////////////////////////////////////////////////////// +void tc::Replica::commit_undo_ops (TCReplicaOpList tc_undo_ops, int32_t *undone_out) +{ + auto res = tc_replica_commit_undo_ops (&*inner, tc_undo_ops, undone_out); if (res != TC_RESULT_OK) { throw replica_error (); } } +//////////////////////////////////////////////////////////////////////////////// +void tc::Replica::free_replica_ops (TCReplicaOpList tc_undo_ops) +{ + tc_replica_op_list_free(&tc_undo_ops); +} + +//////////////////////////////////////////////////////////////////////////////// +std::string tc::Replica::get_op_uuid(TCReplicaOp &tc_replica_op) const +{ + TCString uuid = tc_replica_op_get_uuid(&tc_replica_op); + return tc2string(uuid); +} + +//////////////////////////////////////////////////////////////////////////////// +std::string tc::Replica::get_op_property(TCReplicaOp &tc_replica_op) const +{ + TCString property = tc_replica_op_get_property(&tc_replica_op); + return tc2string(property); +} + +//////////////////////////////////////////////////////////////////////////////// +std::string tc::Replica::get_op_value(TCReplicaOp &tc_replica_op) const +{ + TCString value = tc_replica_op_get_value(&tc_replica_op); + return tc2string(value); +} + +//////////////////////////////////////////////////////////////////////////////// +std::string tc::Replica::get_op_old_value(TCReplicaOp &tc_replica_op) const +{ + TCString old_value = tc_replica_op_get_old_value(&tc_replica_op); + return tc2string(old_value); +} + +//////////////////////////////////////////////////////////////////////////////// +std::string tc::Replica::get_op_timestamp(TCReplicaOp &tc_replica_op) const +{ + TCString timestamp = tc_replica_op_get_timestamp(&tc_replica_op); + return tc2string(timestamp); +} + +//////////////////////////////////////////////////////////////////////////////// +std::string tc::Replica::get_op_old_task_description(TCReplicaOp &tc_replica_op) const +{ + TCString description = tc_replica_op_get_old_task_description(&tc_replica_op); + return tc2string(description); +} + //////////////////////////////////////////////////////////////////////////////// int64_t tc::Replica::num_local_operations () { diff --git a/src/tc/Replica.h b/src/tc/Replica.h index 041f2bd62..6a7fcc371 100644 --- a/src/tc/Replica.h +++ b/src/tc/Replica.h @@ -93,7 +93,15 @@ namespace tc { tc::Task import_task_with_uuid (const std::string &uuid); // TODO: struct TCTask *tc_replica_import_task_with_uuid(struct TCReplica *rep, struct TCUuid tcuuid); void sync(Server server, bool avoid_snapshots); - void undo (int32_t *undone_out); + tc::ffi::TCReplicaOpList get_undo_ops (); + void commit_undo_ops (tc::ffi::TCReplicaOpList tc_undo_ops, int32_t *undone_out); + void free_replica_ops (tc::ffi::TCReplicaOpList tc_undo_ops); + std::string get_op_uuid(tc::ffi::TCReplicaOp &tc_replica_op) const; + std::string get_op_property(tc::ffi::TCReplicaOp &tc_replica_op) const; + std::string get_op_value(tc::ffi::TCReplicaOp &tc_replica_op) const; + std::string get_op_old_value(tc::ffi::TCReplicaOp &tc_replica_op) const; + std::string get_op_timestamp(tc::ffi::TCReplicaOp &tc_replica_op) const; + std::string get_op_old_task_description(tc::ffi::TCReplicaOp &tc_replica_op) const; int64_t num_local_operations (); int64_t num_undo_points (); // TODO: TCResult tc_replica_add_undo_point(struct TCReplica *rep, bool force); diff --git a/src/tc/rust/Cargo.lock b/src/tc/rust/Cargo.lock index be7cda8a7..1dd85d4f5 100644 --- a/src/tc/rust/Cargo.lock +++ b/src/tc/rust/Cargo.lock @@ -1758,9 +1758,9 @@ checksum = "daf8dba3b7eb870caf1ddeed7bc9d2a049f3cfdfae7cb521b087cc33ae4c49da" [[package]] name = "uuid" -version = "1.6.1" +version = "1.7.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5e395fcf16a7a3d8127ec99782007af141946b4795001f876d54fb0d55978560" +checksum = "f00cc9702ca12d3c81455259621e676d0f7251cec66a21e98fe2e9a37db93b2a" dependencies = [ "getrandom", "serde", diff --git a/taskchampion/integration-tests/src/bindings_tests/replica.c b/taskchampion/integration-tests/src/bindings_tests/replica.c index 7ea2a02a5..58c0e5f29 100644 --- a/taskchampion/integration-tests/src/bindings_tests/replica.c +++ b/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( diff --git a/taskchampion/lib/src/kv.rs b/taskchampion/lib/src/kv.rs index 19962d276..3831c7016 100644 --- a/taskchampion/lib/src/kv.rs +++ b/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] diff --git a/taskchampion/lib/src/lib.rs b/taskchampion/lib/src/lib.rs index 6e2faf1bc..ad1fc31f7 100644 --- a/taskchampion/lib/src/lib.rs +++ b/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; diff --git a/taskchampion/lib/src/replica.rs b/taskchampion/lib/src/replica.rs index 390d45179..e6956f431 100644 --- a/taskchampion/lib/src/replica.rs +++ b/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, +} + +impl From 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 = 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); + } +} diff --git a/taskchampion/lib/src/string.rs b/taskchampion/lib/src/string.rs index 44b4a382f..01036c999 100644 --- a/taskchampion/lib/src/string.rs +++ b/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, diff --git a/taskchampion/lib/src/task.rs b/taskchampion/lib/src/task.rs index ed18b9eba..a5d2e80de 100644 --- a/taskchampion/lib/src/task.rs +++ b/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; diff --git a/taskchampion/lib/src/uuid.rs b/taskchampion/lib/src/uuid.rs index 61d1ed99d..c979074ce 100644 --- a/taskchampion/lib/src/uuid.rs +++ b/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 { diff --git a/taskchampion/lib/taskchampion.h b/taskchampion/lib/taskchampion.h index 8e6de7969..2f3237ab2 100644 --- a/taskchampion/lib/taskchampion.h +++ b/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. diff --git a/taskchampion/taskchampion/src/replica.rs b/taskchampion/taskchampion/src/replica.rs index 4c5f00e8e..bad6cceba 100644 --- a/taskchampion/taskchampion/src/replica.rs +++ b/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 { - self.taskdb.undo() + pub fn get_undo_ops(&mut self) -> Result> { + 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) -> Result { + self.taskdb.commit_undo_ops(undo_ops) } /// Rebuild this replica's working set, based on whether tasks are pending or not. If diff --git a/taskchampion/taskchampion/src/storage/mod.rs b/taskchampion/taskchampion/src/storage/mod.rs index b5052e6fd..5674e1baf 100644 --- a/taskchampion/taskchampion/src/storage/mod.rs +++ b/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; diff --git a/taskchampion/taskchampion/src/taskdb/mod.rs b/taskchampion/taskchampion/src/taskdb/mod.rs index 9fa358485..6418f9279 100644 --- a/taskchampion/taskchampion/src/taskdb/mod.rs +++ b/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 { + pub fn get_undo_ops(&mut self) -> Result> { 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) -> Result { + 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 diff --git a/taskchampion/taskchampion/src/taskdb/undo.rs b/taskchampion/taskchampion/src/taskdb/undo.rs index 9ef52693b..c193d96bb 100644 --- a/taskchampion/taskchampion/src/taskdb/undo.rs +++ b/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 { - 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> { + let mut local_ops = txn.operations().unwrap(); + let mut undo_ops: Vec = 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) -> Result { + 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 { } } - 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(()) } diff --git a/test/undo.t b/test/undo.t index 6183a5269..55fedb789 100755 --- a/test/undo.t +++ b/test/undo.t @@ -74,7 +74,7 @@ class TestUndoStyle(TestCase): self.t("add one project:foo priority:H") self.t("1 modify +tag project:bar priority:") - @unittest.expectedFailure # undo diffs are not supported + @unittest.expectedFailure # undo diffs are not supported def test_undo_side_style(self): """Test that 'rc.undo.style:side' generates the right output""" self.t.config("undo.style", "side") @@ -82,7 +82,7 @@ class TestUndoStyle(TestCase): self.assertNotRegex(out, "-tags:\s*\n\+tags:\s+tag") self.assertRegex(out, "tags\s+tag\s*") - @unittest.expectedFailure # undo diffs are not supported + @unittest.expectedFailure # undo diffs are not supported def test_undo_diff_style(self): """Test that 'rc.undo.style:diff' generates the right output""" self.t.config("undo.style", "diff") @@ -90,6 +90,18 @@ class TestUndoStyle(TestCase): self.assertRegex(out, "-tags:\s*\n\+tags:\s+tag") self.assertNotRegex(out, "tags\s+tag\s*") + def test_undo_diff_operations(self): + code, out, err = self.t("undo", input="n\n") + + # If the clock ticks a second between `add` and `modify` there is a + # fifth operation setting the `modified` property. + self.assertRegex(out, "The following [4|5] operations would be reverted:") + + self.assertIn("tag_tag: -> x", out) + self.assertIn("tags: -> tag", out) + self.assertIn("project: foo -> bar", out) + self.assertIn("priority: H -> ", out) + class TestBug634(TestCase): def setUp(self):