From 7a6c5346361e40012de563ca5913b5303a55e4ec Mon Sep 17 00:00:00 2001 From: Paul Beckingham Date: Sun, 6 Oct 2013 11:02:25 -0400 Subject: [PATCH 1/3] Code Cleanup - Restructured 'undo' code to make backlog rollback more readily implementable. --- src/TDB2.cpp | 308 ++++++++++++++++++++++++++++++++++----------------- src/TDB2.h | 5 + 2 files changed, 210 insertions(+), 103 deletions(-) diff --git a/src/TDB2.cpp b/src/TDB2.cpp index 34bbb3e50..a1df16eb4 100644 --- a/src/TDB2.cpp +++ b/src/TDB2.cpp @@ -1274,18 +1274,101 @@ void TDB2::merge (const std::string& mergeFile) //////////////////////////////////////////////////////////////////////////////// void TDB2::revert () { + // Extract the details of the last txn, and roll it back. std::vector u = undo.get_lines (); + std::string uuid; + std::string when; + std::string current; + std::string prior; + revert_undo (u, uuid, when, current, prior); + + // Display diff and confirm. + show_diff (current, prior, when); + if (! context.config.getBoolean ("confirmation") || + confirm (STRING_TDB2_UNDO_CONFIRM)) + { + // There are six kinds of change possible. Determine which one, and act + // accordingly. + // + // Revert: task add + // [1] 0 --> p + // - erase from pending + // - if in backlog, erase, else add deletion + // + // Revert: task modify + // [2] p --> p' + // - write prior over current in pending + // - add prior to backlog + // + // Revert: task done/delete + // [3] p --> c + // - add prior to pending + // - erase from completed + // - add prior to backlog + // + // Revert: task modify + // [4] c --> p + // - add prior to completed + // - erase from pending + // - add prior to backlog + // + // Revert: task modify + // [5] c --> c' + // - write prior over current in completed + // - add prior to backlog + // + // Revert: task log + // [6] 0 --> c + // - erase from completed + // - if in backlog, erase, else add deletion + + // Modify other data files accordingly. + std::vector p = pending.get_lines (); + revert_pending (p, uuid, current, prior); + + std::vector c = completed.get_lines (); + revert_completed (p, c, uuid, current, prior); + + std::vector b = backlog.get_lines (); + revert_backlog (b, uuid, current, prior); + + // Commit. If processing makes it this far with no exceptions, then we're + // done. + File::write (undo._file._data, u); + File::write (pending._file._data, p); + File::write (completed._file._data, c); + File::write (backlog._file._data, b); + +/* + // Perhaps user hand-edited the data files? + // Perhaps the task was in completed.data, which was still in file format 3? + std::cout << format (STRING_TDB2_MISSING_TASK, uuid.substr (6, 36)) + << "\n" + << STRING_TDB2_UNDO_IMPOSSIBLE + << "\n"; +*/ + } + else + std::cout << STRING_CMD_CONFIG_NO_CHANGE << "\n"; +} + +//////////////////////////////////////////////////////////////////////////////// +void TDB2::revert_undo ( + std::vector & u, + std::string& uuid, + std::string& when, + std::string& current, + std::string& prior) +{ if (u.size () < 3) throw std::string (STRING_TDB2_NO_UNDO); // pop last tx u.pop_back (); // separator. - std::string current = u.back ().substr (4); + current = u.back ().substr (4); u.pop_back (); - std::string prior; - std::string when; if (u.back ().substr (0, 5) == "time ") { when = u.back ().substr (5); @@ -1300,6 +1383,125 @@ void TDB2::revert () u.pop_back (); } + // Extract identifying uuid. + std::string::size_type uuidAtt = current.find ("uuid:\""); + if (uuidAtt != std::string::npos) + uuid = current.substr (uuidAtt, 43); // 43 = uuid:"..." + else + throw std::string (STRING_TDB2_MISSING_UUID); +} + +//////////////////////////////////////////////////////////////////////////////// +void TDB2::revert_pending ( + std::vector & p, + const std::string& uuid, + const std::string& current, + const std::string& prior) +{ + // is 'current' in pending? + std::vector ::iterator task; + for (task = p.begin (); task != p.end (); ++task) + { + if (task->find (uuid) != std::string::npos) + { + context.debug ("TDB::revert - task found in pending.data"); + + // Either revert if there was a prior state, or remove the task. + if (prior != "") + { + *task = prior; + std::cout << STRING_TDB2_REVERTED << "\n"; + } + else + { + p.erase (task); + std::cout << STRING_TDB2_REMOVED << "\n"; + } + + break; + } + } +} + +//////////////////////////////////////////////////////////////////////////////// +void TDB2::revert_completed ( + std::vector & p, + std::vector & c, + const std::string& uuid, + const std::string& current, + const std::string& prior) +{ + // is 'current' in completed? + std::vector ::iterator task; + for (task = c.begin (); task != c.end (); ++task) + { + if (task->find (uuid) != std::string::npos) + { + context.debug ("TDB::revert - task found in completed.data"); + + // Either revert if there was a prior state, or remove the task. + if (prior != "") + { + *task = prior; + if (task->find ("status:\"pending\"") != std::string::npos || + task->find ("status:\"waiting\"") != std::string::npos || + task->find ("status:\"recurring\"") != std::string::npos) + { + c.erase (task); + p.push_back (prior); + std::cout << STRING_TDB2_REVERTED << "\n"; + context.debug ("TDB::revert - task belongs in pending.data"); + } + else + { + std::cout << STRING_TDB2_REVERTED << "\n"; + context.debug ("TDB::revert - task belongs in completed.data"); + } + } + else + { + c.erase (task); + + std::cout << STRING_TDB2_REVERTED << "\n"; + context.debug ("TDB::revert - task removed"); + } + + std::cout << STRING_TDB2_UNDO_COMPLETE << "\n"; + break; + } + } +} + +//////////////////////////////////////////////////////////////////////////////// +void TDB2::revert_backlog ( + std::vector & b, + const std::string& uuid, + const std::string& current, + const std::string& prior) +{ + std::vector ::iterator task; + for (task = b.begin (); task != b.end (); ++task) + { + if (task->find (uuid) != std::string::npos) + { +/* + context.debug ("TDB::revert_backlog - task found in backlog.data"); + +// // Write prior to backlog.data +// Task json_task (prior); +// backlog.add_line (json_task.composeJSON () + "\n"); + +*/ + } + } +} + +//////////////////////////////////////////////////////////////////////////////// +void TDB2::show_diff ( + const std::string& current, + const std::string& prior, + const std::string& when) +{ Date lastChange (strtol (when.c_str (), NULL, 10)); // Set the colors. @@ -1512,106 +1714,6 @@ void TDB2::revert () << "\n"; } - // Output displayed, now confirm. - if (context.config.getBoolean ("confirmation") && - !confirm (STRING_TDB2_UNDO_CONFIRM)) - { - std::cout << STRING_CMD_CONFIG_NO_CHANGE << "\n"; - return; - } - - // Extract identifying uuid. - std::string uuid; - std::string::size_type uuidAtt = current.find ("uuid:\""); - if (uuidAtt != std::string::npos) - uuid = current.substr (uuidAtt, 43); // 43 = uuid:"..." - else - throw std::string (STRING_TDB2_MISSING_UUID); - - // load pending.data - std::vector p = pending.get_lines (); - - // is 'current' in pending? - std::vector ::iterator task; - for (task = p.begin (); task != p.end (); ++task) - { - if (task->find (uuid) != std::string::npos) - { - context.debug ("TDB::undo - task found in pending.data"); - - // Either revert if there was a prior state, or remove the task. - if (prior != "") - { - *task = prior; - std::cout << STRING_TDB2_REVERTED << "\n"; - } - else - { - p.erase (task); - std::cout << STRING_TDB2_REMOVED << "\n"; - } - - // Rewrite files. - File::write (pending._file._data, p); - File::write (undo._file._data, u); - return; - } - } - - // load completed.data - std::vector c = completed.get_lines (); - - // is 'current' in completed? - for (task = c.begin (); task != c.end (); ++task) - { - if (task->find (uuid) != std::string::npos) - { - context.debug ("TDB::undo - task found in completed.data"); - - // Either revert if there was a prior state, or remove the task. - if (prior != "") - { - *task = prior; - if (task->find ("status:\"pending\"") != std::string::npos || - task->find ("status:\"waiting\"") != std::string::npos || - task->find ("status:\"recurring\"") != std::string::npos) - { - c.erase (task); - p.push_back (prior); - File::write (completed._file._data, c); - File::write (pending._file._data, p); - File::write (undo._file._data, u); - std::cout << STRING_TDB2_REVERTED << "\n"; - context.debug ("TDB::undo - task belongs in pending.data"); - } - else - { - File::write (completed._file._data, c); - File::write (undo._file._data, u); - std::cout << STRING_TDB2_REVERTED << "\n"; - context.debug ("TDB::undo - task belongs in completed.data"); - } - } - else - { - c.erase (task); - File::write (completed._file._data, c); - File::write (undo._file._data, u); - std::cout << STRING_TDB2_REVERTED << "\n"; - context.debug ("TDB::undo - task removed"); - } - - std::cout << STRING_TDB2_UNDO_COMPLETE << "\n"; - return; - } - } - - // Perhaps user hand-edited the data files? - // Perhaps the task was in completed.data, which was still in file format 3? - std::cout << format (STRING_TDB2_MISSING_TASK, uuid.substr (6, 36)) - << "\n" - << STRING_TDB2_UNDO_IMPOSSIBLE - << "\n"; } //////////////////////////////////////////////////////////////////////////////// diff --git a/src/TDB2.h b/src/TDB2.h index 199724828..292417fd0 100644 --- a/src/TDB2.h +++ b/src/TDB2.h @@ -127,6 +127,11 @@ public: private: bool verifyUniqueUUID (const std::string&); + void show_diff (const std::string&, const std::string&, const std::string&); + void revert_undo (std::vector &, std::string&, std::string&, std::string&, std::string&); + void revert_pending (std::vector &, const std::string&, const std::string&, const std::string&); + void revert_completed (std::vector &, std::vector &, const std::string&, const std::string&, const std::string&); + void revert_backlog (std::vector &, const std::string&, const std::string&, const std::string&); public: TF2 pending; From f367989afa934dc35b93ec7b48aa0ff92a5b17d5 Mon Sep 17 00:00:00 2001 From: Paul Beckingham Date: Sun, 6 Oct 2013 13:43:21 -0400 Subject: [PATCH 2/3] Bug #1270 - The 'undo' command is now properly removing backlog entries. --- ChangeLog | 1 + src/TDB2.cpp | 72 ++++++++++++++++++++++++++++++--------------------- src/en-US.h | 1 + src/es-ES.h | 1 + src/fr-FR.h | 1 + src/it-IT.h | 1 + test/delete.t | 2 +- 7 files changed, 48 insertions(+), 31 deletions(-) diff --git a/ChangeLog b/ChangeLog index a8ca9b8a7..677928acb 100644 --- a/ChangeLog +++ b/ChangeLog @@ -66,6 +66,7 @@ Bugs Wilk). + #1263 The 'waiting' report properly lists only pending tasks with a wait date (thanks to Fidel Mato). + + #1270 The 'undo' command is now properly removing backlog entries. + #1300 Encode/decode pairing is now properly balanced. + #1305 Commit hash now available in tarball builds (thanks to Ben Boeckel). + #1387 ZSH Auto-Completion dates are not current (thanks to Benjamin Weber). diff --git a/src/TDB2.cpp b/src/TDB2.cpp index a1df16eb4..91f5cbea8 100644 --- a/src/TDB2.cpp +++ b/src/TDB2.cpp @@ -766,7 +766,7 @@ void TDB2::merge (const std::string& mergeFile) tmp_lit++; tmp_rit++; } - + if (lookahead == -1) { // at this point we know that the first lines are the same undo_lines.push_back (lline + "\n"); @@ -783,7 +783,7 @@ void TDB2::merge (const std::string& mergeFile) bool found = false; for (std::vector::const_iterator tmp_lit = lit, tmp_rit = rit; (tmp_lit != l.end ()) && (tmp_rit != r.end ()); - ++tmp_lit, ++tmp_rit) + ++tmp_lit, ++tmp_rit) { lline = *tmp_lit; rline = *tmp_rit; @@ -875,7 +875,7 @@ void TDB2::merge (const std::string& mergeFile) // point in time. Normally this case will be solved by the merge logic, // BUT if the UUID is considered new the merge logic will be skipped. // - // This flaw resulted in a couple of duplication issues and bloated + // This flaw resulted in a couple of duplication issues and bloated // undo files (e.g. #1104). // // This is just a "hack" which discards all the modifications of the @@ -989,7 +989,7 @@ void TDB2::merge (const std::string& mergeFile) // inserting right mod into history of local database // so that it can be restored later - // AND more important: create a history that looks the same + // AND more important: create a history that looks the same // as if we switched the roles 'remote' and 'local' // thus we have to find the oldest change on the local branch that is not on remote branch @@ -1293,7 +1293,7 @@ void TDB2::revert () // Revert: task add // [1] 0 --> p // - erase from pending - // - if in backlog, erase, else add deletion + // - if in backlog, erase, else cannot undo // // Revert: task modify // [2] p --> p' @@ -1320,7 +1320,7 @@ void TDB2::revert () // Revert: task log // [6] 0 --> c // - erase from completed - // - if in backlog, erase, else add deletion + // - if in backlog, erase, else cannot undo // Modify other data files accordingly. std::vector p = pending.get_lines (); @@ -1338,15 +1338,6 @@ void TDB2::revert () File::write (pending._file._data, p); File::write (completed._file._data, c); File::write (backlog._file._data, b); - -/* - // Perhaps user hand-edited the data files? - // Perhaps the task was in completed.data, which was still in file format 3? - std::cout << format (STRING_TDB2_MISSING_TASK, uuid.substr (6, 36)) - << "\n" - << STRING_TDB2_UNDO_IMPOSSIBLE - << "\n"; -*/ } else std::cout << STRING_CMD_CONFIG_NO_CHANGE << "\n"; @@ -1386,7 +1377,7 @@ void TDB2::revert_undo ( // Extract identifying uuid. std::string::size_type uuidAtt = current.find ("uuid:\""); if (uuidAtt != std::string::npos) - uuid = current.substr (uuidAtt, 43); // 43 = uuid:"..." + uuid = current.substr (uuidAtt + 6, 36); // "uuid:"" --> else throw std::string (STRING_TDB2_MISSING_UUID); } @@ -1398,11 +1389,13 @@ void TDB2::revert_pending ( const std::string& current, const std::string& prior) { + std::string uuid_att = "uuid:\"" + uuid + "\""; + // is 'current' in pending? std::vector ::iterator task; for (task = p.begin (); task != p.end (); ++task) { - if (task->find (uuid) != std::string::npos) + if (task->find (uuid_att) != std::string::npos) { context.debug ("TDB::revert - task found in pending.data"); @@ -1431,13 +1424,15 @@ void TDB2::revert_completed ( const std::string& current, const std::string& prior) { + std::string uuid_att = "uuid:\"" + uuid + "\""; + // is 'current' in completed? std::vector ::iterator task; for (task = c.begin (); task != c.end (); ++task) { - if (task->find (uuid) != std::string::npos) + if (task->find (uuid_att) != std::string::npos) { - context.debug ("TDB::revert - task found in completed.data"); + context.debug ("TDB::revert_completed - task found in completed.data"); // Either revert if there was a prior state, or remove the task. if (prior != "") @@ -1450,12 +1445,12 @@ void TDB2::revert_completed ( c.erase (task); p.push_back (prior); std::cout << STRING_TDB2_REVERTED << "\n"; - context.debug ("TDB::revert - task belongs in pending.data"); + context.debug ("TDB::revert_completed - task belongs in pending.data"); } else { std::cout << STRING_TDB2_REVERTED << "\n"; - context.debug ("TDB::revert - task belongs in completed.data"); + context.debug ("TDB::revert_completed - task belongs in completed.data"); } } else @@ -1463,7 +1458,7 @@ void TDB2::revert_completed ( c.erase (task); std::cout << STRING_TDB2_REVERTED << "\n"; - context.debug ("TDB::revert - task removed"); + context.debug ("TDB::revert_completed - task removed"); } std::cout << STRING_TDB2_UNDO_COMPLETE << "\n"; @@ -1479,21 +1474,38 @@ void TDB2::revert_backlog ( const std::string& current, const std::string& prior) { - std::vector ::iterator task; - for (task = b.begin (); task != b.end (); ++task) + std::string uuid_att = "\"uuid\":\"" + uuid + "\""; + + bool found = false; + std::vector ::reverse_iterator task; + for (task = b.rbegin (); task != b.rend (); ++task) { - if (task->find (uuid) != std::string::npos) + if (task->find (uuid_att) != std::string::npos) { -/* context.debug ("TDB::revert_backlog - task found in backlog.data"); + found = true; -// // Write prior to backlog.data -// Task json_task (prior); -// backlog.add_line (json_task.composeJSON () + "\n"); + // If this is a new task (no prior), then just remove it from the backlog. + if (current != "" && prior == "") + { + // Yes, this is what is needed, when you want to erase using a reverse + // iterator. + b.erase ((++task).base ()); + } -*/ + // If this is a modification of some kind, add the prior to the backlog. + else + { + Task t (prior); + b.push_back (t.composeJSON ()); + } + + break; } } + + if (!found) + throw std::string (STRING_TDB2_UNDO_SYNCED); } //////////////////////////////////////////////////////////////////////////////// diff --git a/src/en-US.h b/src/en-US.h index a988c55dc..639cb2e61 100644 --- a/src/en-US.h +++ b/src/en-US.h @@ -833,6 +833,7 @@ #define STRING_TDB2_UNDO_COMPLETE "Undo complete." #define STRING_TDB2_MISSING_TASK "Task with UUID {1} not found in data." #define STRING_TDB2_UNDO_IMPOSSIBLE "No undo possible." +#define STRING_TDB2_UNDO_SYNCED "Cannot undo change because the task was already synced. Modify the task instead." // text // A comma-separated list of commands is appended. diff --git a/src/es-ES.h b/src/es-ES.h index 479c3c78e..623115503 100644 --- a/src/es-ES.h +++ b/src/es-ES.h @@ -848,6 +848,7 @@ #define STRING_TDB2_UNDO_COMPLETE "Deshacer completado." #define STRING_TDB2_MISSING_TASK "Tarea con UUID {1} no encontrada en los datos." #define STRING_TDB2_UNDO_IMPOSSIBLE "No es posible deshacer." +#define STRING_TDB2_UNDO_SYNCED "Cannot undo change because the task was already synced. Modify the task instead." // text // Se añade al final una lista de comandos separados por comas. diff --git a/src/fr-FR.h b/src/fr-FR.h index 3f7d663ab..c86b23196 100644 --- a/src/fr-FR.h +++ b/src/fr-FR.h @@ -833,6 +833,7 @@ #define STRING_TDB2_UNDO_COMPLETE "Undo complete." #define STRING_TDB2_MISSING_TASK "Task with UUID {1} not found in data." #define STRING_TDB2_UNDO_IMPOSSIBLE "No undo possible." +#define STRING_TDB2_UNDO_SYNCED "Cannot undo change because the task was already synced. Modify the task instead." // text // A comma-separated list of commands is appended. diff --git a/src/it-IT.h b/src/it-IT.h index 013f8f6e2..8328e05ce 100644 --- a/src/it-IT.h +++ b/src/it-IT.h @@ -834,6 +834,7 @@ #define STRING_TDB2_UNDO_COMPLETE "Undo completato." #define STRING_TDB2_MISSING_TASK "Task con UUID {1} non trovato nei dati." #define STRING_TDB2_UNDO_IMPOSSIBLE "Nessun undo possibile." +#define STRING_TDB2_UNDO_SYNCED "Cannot undo change because the task was already synced. Modify the task instead." // text // A comma-separated list of commands is appended. diff --git a/test/delete.t b/test/delete.t index 4d07087f6..8ad1ab2a0 100755 --- a/test/delete.t +++ b/test/delete.t @@ -53,7 +53,7 @@ $output = qx{../src/task rc:delete.rc 1 delete 2>&1; ../src/task rc:delete.rc in like ($output, qr/Status\s+Deleted\n/, 'Deleted'); ok (-r 'completed.data', 'completed.data created'); -$output = qx{echo 'y' | ../src/task rc:delete.rc undo 2>&1; ../src/task rc:delete.rc info 1 2>&1}; +$output = qx{../src/task rc:delete.rc undo 2>&1; ../src/task rc:delete.rc info 1 2>&1}; like ($output, qr/Status\s+Pending\n/, 'Pending'); ok (-r 'completed.data', 'completed.data created'); From 5f98e41801725645d8cca14d7892db387520f9d3 Mon Sep 17 00:00:00 2001 From: Paul Beckingham Date: Sun, 6 Oct 2013 13:55:42 -0400 Subject: [PATCH 3/3] Unit Tests - Added more undo tests, to cover both cases for backlog rollback. --- test/undo.t | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/test/undo.t b/test/undo.t index d17bf8f8f..52411dccc 100755 --- a/test/undo.t +++ b/test/undo.t @@ -28,7 +28,7 @@ use strict; use warnings; -use Test::More tests => 16; +use Test::More tests => 20; # Ensure environment has no influence. delete $ENV{'TASKDATA'}; @@ -65,6 +65,15 @@ $output = qx{../src/task rc:undo.rc undo 1 2>&1}; unlike ($output, qr/Unknown error/, 'No unknown error'); like ($output, qr/The undo command does not allow further task modification/, 'Correct error caught and reported'); +# Add a new task and undo it. +$output = qx{../src/task rc:undo.rc add two 2>&1; ../src/task rc:undo.rc info 1 2>&1}; +unlike ($output, qr/Unknown error/, 'No unknown error'); +like ($output, qr/Status\s+Pending\n/, 'Pending'); + +$output = qx{../src/task rc:undo.rc undo 2>&1; ../src/task rc:undo.rc info 1 2>&1}; +like ($output, qr/Task removed\.\n/, 'Task removed'); +like ($output, qr/No matches\.\n/, 'No matches'); + # Inspect backlog.data if (open my $fh, '<', 'backlog.data') {