From 0e28374131c794d743684e4f0378472bb3ac7230 Mon Sep 17 00:00:00 2001 From: Johannes Schlatow Date: Thu, 22 Nov 2012 00:11:53 +0100 Subject: [PATCH] Bug #1117 Fixed some other merge issues with the sorting order of equally timestamped entries in undo.data (see #1104). 1. The fact that both files can begin with equal timestamps but different modifications has not been taken into account. 2. Besides the fact that the relative order within the same data file must be preservered as introduced before, we also need a unique order for entries of different data files so that each machine comes to the same merge result. This has now been achieved by taking the UUIDs into account as soon as the timestamps are equal. --- src/TDB2.cpp | 52 ++++++++++++---------- src/Taskmod.cpp | 27 +++++++++++- src/Taskmod.h | 3 ++ test/bug.1104.t | 32 +++++++++++++- test/bug.1117.t | 113 ++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 201 insertions(+), 26 deletions(-) create mode 100755 test/bug.1117.t diff --git a/src/TDB2.cpp b/src/TDB2.cpp index d5b1c28c6..1d5cfdef1 100644 --- a/src/TDB2.cpp +++ b/src/TDB2.cpp @@ -646,8 +646,9 @@ void readTaskmods (std::vector &input, std::vector ::iterator &start, std::list &list) { + static int resourceID = 1; std::string line; - Taskmod tmod_tmp; + Taskmod tmod_tmp(resourceID++); DEBUG_STR ("reading taskmods from file: "); @@ -745,26 +746,31 @@ void TDB2::merge (const std::string& mergeFile) /////////////////////////////////////// // find the branch-off point: - // first lines are not equal => assuming mergeFile starts at a + // first mods are not equal => assuming mergeFile starts at a // later point in time - if (lline.compare (rline) != 0) - { - // iterate in local file to find rline - for ( ; lit != l.end (); ++lit) - { - lline = *lit; + if (lline.compare (rline) == 0) { + std::vector::const_iterator tmp_lit = lit; + std::vector::const_iterator tmp_rit = rit; + tmp_lit++; + tmp_rit++; - // push the line to the new undo.data - undo_lines.push_back (lline + "\n"); - - // found first matching lines? - if (lline.compare (rline) == 0) - break; + int lookahead = 1; + if (tmp_lit->substr (0, 3) == "old") { + lookahead = 2; + } + + while (lookahead--) { + if (tmp_lit->compare(*tmp_rit) != 0) { + break; + } + 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"); } - } - else - { - undo_lines.push_back (lline + "\n"); } // Add some color. @@ -802,7 +808,7 @@ void TDB2::merge (const std::string& mergeFile) if (!found) { - // set iterators to r.end() or l.end() if they are point to the last line + // set iterators to r.end() or l.end() if they point to the last line if (++rit != r.end()) --rit; @@ -823,14 +829,14 @@ void TDB2::merge (const std::string& mergeFile) std::set uuid_new, uuid_left; // 1. read taskmods out of the remaining lines - readTaskmods (l, lit, lmods); readTaskmods (r, rit, rmods); + readTaskmods (l, lit, lmods); // 2. move new uuids into mods DEBUG_STR_PART ("adding new uuids (left) to skip list..."); // modifications on the left side are already in the database - // we just need them to merge conflicts, so we add new the mods for + // we just need them to merge conflicts, so we add the mods for // new uuids to the skip-list 'uuid_left' std::list::iterator lmod_it; for (lmod_it = lmods.begin (); lmod_it != lmods.end (); lmod_it++) @@ -1236,8 +1242,8 @@ void TDB2::merge (const std::string& mergeFile) // now we merge mods (new modifications from mergefile) // with lmods (part of old undo.data) lmods.sort(compareTaskmod); - mods.merge (lmods); - mods.merge (mods_history); + mods.merge (lmods, compareTaskmod); + mods.merge (mods_history, compareTaskmod); // generate undo.data format std::list::iterator it; diff --git a/src/Taskmod.cpp b/src/Taskmod.cpp index 96b9eef67..78b1670a7 100644 --- a/src/Taskmod.cpp +++ b/src/Taskmod.cpp @@ -39,7 +39,12 @@ bool compareTaskmod (Taskmod first, Taskmod second) { if (first._timestamp == second._timestamp) { - return first._sequenceNumber < second._sequenceNumber; + // preserve relative order within the same resource + if (first._resource == second._resource) + return first._sequenceNumber < second._sequenceNumber; + // sort by UUID if mods where made on different resources + else + return first._resource < second._resource; } else { @@ -54,6 +59,16 @@ Taskmod::Taskmod () _bAfterSet = false; _bBeforeSet = false; _sequenceNumber = curSequenceNumber++; + _resource = -1; +} + +Taskmod::Taskmod (int resourceID) +{ + _timestamp = 0; + _bAfterSet = false; + _bBeforeSet = false; + _sequenceNumber = curSequenceNumber++; + _resource = resourceID; } //////////////////////////////////////////////////////////////////////////////// @@ -65,6 +80,7 @@ Taskmod::Taskmod (const Taskmod& other) this->_bAfterSet = other._bAfterSet; this->_bBeforeSet = other._bBeforeSet; this->_sequenceNumber = other._sequenceNumber; + this->_resource = other._resource; } //////////////////////////////////////////////////////////////////////////////// @@ -108,7 +124,8 @@ Taskmod& Taskmod::operator= (const Taskmod& other) this->_timestamp = other._timestamp; this->_bAfterSet = other._bAfterSet; this->_bBeforeSet = other._bBeforeSet; - this->_sequenceNumber = other._sequenceNumber; + this->_sequenceNumber = other._sequenceNumber; + this->_resource = other._resource; } return *this; @@ -225,6 +242,12 @@ unsigned long Taskmod::getSequenceNumber () const return _sequenceNumber; } +//////////////////////////////////////////////////////////////////////////////// +int Taskmod::getResource () const +{ + return _resource; +} + //////////////////////////////////////////////////////////////////////////////// std::string Taskmod::getTimeStr () const { diff --git a/src/Taskmod.h b/src/Taskmod.h index 8e3a04513..adce8b31a 100644 --- a/src/Taskmod.h +++ b/src/Taskmod.h @@ -37,6 +37,7 @@ class Taskmod { public: Taskmod (); + Taskmod (int resourceID); Taskmod (const Taskmod& other); ~Taskmod (); @@ -68,6 +69,7 @@ public: Task& getBefore (); long getTimestamp () const; unsigned long getSequenceNumber () const; + int getResource () const; std::string getTimeStr () const; protected: @@ -77,6 +79,7 @@ protected: bool _bAfterSet; bool _bBeforeSet; unsigned long _sequenceNumber; + int _resource; static unsigned long curSequenceNumber; }; diff --git a/test/bug.1104.t b/test/bug.1104.t index 955d637d8..0021b733f 100755 --- a/test/bug.1104.t +++ b/test/bug.1104.t @@ -30,7 +30,7 @@ use strict; use warnings; use File::Copy; use File::Path; -use Test::More tests => 28; +use Test::More tests => 33; mkdir("1", 0755); mkdir("2", 0755); @@ -136,6 +136,36 @@ $output = qx{../src/task rc:1.rc merge 2>&1}; ok ($? == 0, 'Exit status check'); unlike ($output, qr/Retaining/, "Must not retain changes"); +# Merges 1 +$output = qx{../src/task rc:2.rc merge 2>&1}; +ok ($? == 0, 'Exit status check'); +unlike ($output, qr/Retaining/, "Must not retain changes"); + +# now all three instances must be in sync +$output = qx{diff 1/undo.data dropbox/undo.data}; +ok ($? == 0, 'Resource 1 up-to-date check'); + +$output = qx{diff 2/undo.data dropbox/undo.data}; +ok ($? == 0, 'Resource 2 up-to-date check'); + +$output = qx{diff 3/undo.data dropbox/undo.data}; +ok ($? == 0, 'Resource 3 up-to-date check'); + +## Merges 3 +#$output = qx{../src/task rc:3.rc merge 2>&1}; +#ok ($? == 0, 'Exit status check'); +#unlike ($output, qr/Retaining/, "Must not retain changes"); +# +## Merges 1 +#$output = qx{../src/task rc:1.rc merge 2>&1}; +#ok ($? == 0, 'Exit status check'); +#unlike ($output, qr/Retaining/, "Must not retain changes"); +# +## Merges 1 +#$output = qx{../src/task rc:2.rc merge 2>&1}; +#ok ($? == 0, 'Exit status check'); +#unlike ($output, qr/Retaining/, "Must not retain changes"); + # Cleanup. unlink qw(1.rc 1/pending.data 1/completed.data 1/undo.data 1/backlog.data 1/synch.key 2/pending.data 2/completed.data 2/undo.data 2.rc 2/backlog.data 2/synch.key dropbox/completed.data dropbox/pending.data dropbox/undo.data 3/pending.data 3/undo.data 3/completed.data 3/backlog.data 3/synch.key 3.rc); ok (! -r '1/pending.data' && diff --git a/test/bug.1117.t b/test/bug.1117.t new file mode 100755 index 000000000..f1bc6eb3d --- /dev/null +++ b/test/bug.1117.t @@ -0,0 +1,113 @@ +#! /usr/bin/env perl +################################################################################ +## taskwarrior - a command line task list manager. +## +## Copyright 2006-2012, Paul Beckingham, Federico Hernandez. +## +## Permission is hereby granted, free of charge, to any person obtaining a copy +## of this software and associated documentation files (the "Software"), to deal +## in the Software without restriction, including without limitation the rights +## to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +## copies of the Software, and to permit persons to whom the Software is +## furnished to do so, subject to the following conditions: +## +## The above copyright notice and this permission notice shall be included +## in all copies or substantial portions of the Software. +## +## THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +## OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +## FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL +## THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +## LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +## OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +## SOFTWARE. +## +## http://www.opensource.org/licenses/mit-license.php +## +################################################################################ + +use strict; +use warnings; +use File::Copy; +use File::Path; +use Test::More tests => 13; + +mkdir("1", 0755); +mkdir("2", 0755); +mkdir("merge", 0755); + +ok (-e "1", 'Created directory 1/'); +ok (-e "2", 'Created directory 2/'); +ok (-e "merge", 'Created directory merge/'); + +# Create the rc file. +if (open my $fh, '>', '1.rc') +{ + print $fh "data.location=1/\n"; + print $fh "confirmation=no\n"; + print $fh "merge.autopush=yes\n"; + print $fh "merge.default.uri=./merge/\n"; + print $fh "push.default.uri=./merge/\n"; + + close $fh; + ok (-r '1.rc', 'Created 1.rc'); +} + +# Create the rc file. +if (open my $fh, '>', '2.rc') +{ + print $fh "data.location=2/\n"; + print $fh "confirmation=no\n"; + print $fh "merge.autopush=yes\n"; + print $fh "merge.default.uri=./merge/\n"; + print $fh "push.default.uri=./merge/\n"; + + close $fh; + ok (-r '2.rc', 'Created 2.rc'); +} + +# add and push on 1 +my $output = qx{../src/task rc:1.rc add foo1 2>&1}; +ok ($? == 0, 'Exit status check'); +$output = qx{../src/task rc:1.rc push}; +ok ($? == 0, 'Exit status check'); + +# add and merge on 2 +$output = qx{../src/task rc:2.rc add foo2 2>&1}; +ok ($? == 0, 'Exit status check'); + +$output = qx{../src/task rc:2.rc merge 2>&1}; +ok ($? == 0, 'Exit status check'); + +# merge 1 +$output = qx{../src/task rc:1.rc merge 2>&1}; +ok ($? == 0, 'Exit status check'); + +# undo.data files must not differ +$output = qx{diff 1/undo.data 2/undo.data}; +ok ($? == 0, 'undo.data diff result'); + +# Cleanup. +unlink qw(1.rc 1/pending.data 1/completed.data 1/undo.data 1/backlog.data 1/synch.key 2/pending.data 2/completed.data 2/undo.data 2.rc 2/backlog.data 2/synch.key merge/completed.data merge/pending.data merge/undo.data); +ok (! -r '1/pending.data' && + ! -r '1/completed.data' && + ! -r '1/undo.data' && + ! -r '1/backlog.data' && + ! -r '1/synch.key' && + ! -r '1.rc' && + ! -r '2/pending.data' && + ! -r '2/completed.data' && + ! -r '2/undo.data' && + ! -r '2/backlog.data' && + ! -r '2/synch.key' && + ! -r '2.rc' && + ! -r 'merge/pending.data' && + ! -r 'merge/completed.data' && + ! -r 'merge/undo.data' , 'Cleanup'); + +rmtree (['1', '2', 'merge'], 0, 1); +ok (! -e '1' && + ! -e '2' && + ! -e 'merge', 'Removed directories'); + +exit 0;