From 68c1ca3f69e137f208ec11d36aadf0293a5335aa Mon Sep 17 00:00:00 2001 From: Johannes Schlatow Date: Mon, 10 Sep 2012 22:53:44 +0200 Subject: [PATCH] Bug #1104 Fixed the bug where the sort order of transactions with equal timestamps was not stable, i.e. due to the lack of a total order, different merges produced different sortings and hence messed up transactions which have already been merged. --- src/TDB2.cpp | 6 +++--- src/Taskmod.cpp | 56 +++++++++++++++++++++++++++++++++++++----------- src/Taskmod.h | 9 ++++++++ test/bug.1104.t | 57 ++++++++++++++++++++++++++++++++++++++++++------- 4 files changed, 104 insertions(+), 24 deletions(-) diff --git a/src/TDB2.cpp b/src/TDB2.cpp index 3ed6d1a81..3e4daa689 100644 --- a/src/TDB2.cpp +++ b/src/TDB2.cpp @@ -1024,8 +1024,8 @@ void TDB2::merge (const std::string& mergeFile) mods.splice (mods.begin (), rmods); DEBUG_STR ("sorting taskmod list"); - mods.sort (); - mods_history.sort (); + mods.sort (compareTaskmod); + mods_history.sort (compareTaskmod); } else if (rit == r.end ()) { @@ -1232,7 +1232,7 @@ void TDB2::merge (const std::string& mergeFile) // at this point undo contains the lines up to the branch-off point // now we merge mods (new modifications from mergefile) // with lmods (part of old undo.data) - lmods.sort(); + lmods.sort(compareTaskmod); mods.merge (lmods); mods.merge (mods_history); diff --git a/src/Taskmod.cpp b/src/Taskmod.cpp index 8ae89809e..96b9eef67 100644 --- a/src/Taskmod.cpp +++ b/src/Taskmod.cpp @@ -33,22 +33,38 @@ #include #include +unsigned long Taskmod::curSequenceNumber = 0; + +bool compareTaskmod (Taskmod first, Taskmod second) +{ + if (first._timestamp == second._timestamp) + { + return first._sequenceNumber < second._sequenceNumber; + } + else + { + return first._timestamp < second._timestamp; + } +} + //////////////////////////////////////////////////////////////////////////////// Taskmod::Taskmod () { _timestamp = 0; _bAfterSet = false; _bBeforeSet = false; + _sequenceNumber = curSequenceNumber++; } //////////////////////////////////////////////////////////////////////////////// Taskmod::Taskmod (const Taskmod& other) { - this->_before = other._before; - this->_after = other._after; - this->_timestamp = other._timestamp; - this->_bAfterSet = other._bAfterSet; - this->_bBeforeSet = other._bBeforeSet; + this->_before = other._before; + this->_after = other._after; + this->_timestamp = other._timestamp; + this->_bAfterSet = other._bAfterSet; + this->_bBeforeSet = other._bBeforeSet; + this->_sequenceNumber = other._sequenceNumber; } //////////////////////////////////////////////////////////////////////////////// @@ -87,11 +103,12 @@ Taskmod& Taskmod::operator= (const Taskmod& other) { if (this != &other) { - this->_before = other._before; - this->_after = other._after; - this->_timestamp = other._timestamp; - this->_bAfterSet = other._bAfterSet; - this->_bBeforeSet = other._bBeforeSet; + this->_before = other._before; + this->_after = other._after; + this->_timestamp = other._timestamp; + this->_bAfterSet = other._bAfterSet; + this->_bBeforeSet = other._bBeforeSet; + this->_sequenceNumber = other._sequenceNumber; } return *this; @@ -100,9 +117,10 @@ Taskmod& Taskmod::operator= (const Taskmod& other) //////////////////////////////////////////////////////////////////////////////// void Taskmod::reset (long timestamp) { - this->_bAfterSet = false; - this->_bBeforeSet = false; - this->_timestamp = timestamp; + this->_bAfterSet = false; + this->_bBeforeSet = false; + this->_timestamp = timestamp; + this->_sequenceNumber = curSequenceNumber++; } //////////////////////////////////////////////////////////////////////////////// @@ -177,6 +195,12 @@ void Taskmod::setTimestamp (long timestamp) this->_timestamp = timestamp; } +//////////////////////////////////////////////////////////////////////////////// +void Taskmod::incSequenceNumber () +{ + this->_sequenceNumber++; +} + //////////////////////////////////////////////////////////////////////////////// Task& Taskmod::getAfter () { @@ -195,6 +219,12 @@ long Taskmod::getTimestamp () const return _timestamp; } +//////////////////////////////////////////////////////////////////////////////// +unsigned long Taskmod::getSequenceNumber () const +{ + return _sequenceNumber; +} + //////////////////////////////////////////////////////////////////////////////// std::string Taskmod::getTimeStr () const { diff --git a/src/Taskmod.h b/src/Taskmod.h index 219a89f73..8e3a04513 100644 --- a/src/Taskmod.h +++ b/src/Taskmod.h @@ -33,6 +33,8 @@ #include class Taskmod { + friend bool compareTaskmod (Taskmod first, Taskmod second); + public: Taskmod (); Taskmod (const Taskmod& other); @@ -59,11 +61,13 @@ public: void setAfter (const Task& after); void setBefore (const Task& before); void setTimestamp (long timestamp); + void incSequenceNumber (); // getter Task& getAfter (); Task& getBefore (); long getTimestamp () const; + unsigned long getSequenceNumber () const; std::string getTimeStr () const; protected: @@ -72,7 +76,12 @@ protected: long _timestamp; bool _bAfterSet; bool _bBeforeSet; + unsigned long _sequenceNumber; + + static unsigned long curSequenceNumber; }; +bool compareTaskmod (Taskmod first, Taskmod second); + #endif diff --git a/test/bug.1104.t b/test/bug.1104.t index 7d580d465..955d637d8 100755 --- a/test/bug.1104.t +++ b/test/bug.1104.t @@ -30,14 +30,16 @@ use strict; use warnings; use File::Copy; use File::Path; -use Test::More tests => 18; +use Test::More tests => 28; mkdir("1", 0755); mkdir("2", 0755); +mkdir("3", 0755); mkdir("dropbox", 0755); ok (-e "1", 'Created directory 1/'); ok (-e "2", 'Created directory 2/'); +ok (-e "3", 'Created directory 3/'); ok (-e "dropbox", 'Created directory dropbox/'); # Create the rc file. @@ -68,25 +70,47 @@ if (open my $fh, '>', '2.rc') ok (-r '2.rc', 'Created 2.rc'); } +# Create the rc file. +if (open my $fh, '>', '3.rc') +{ + print $fh "data.location=3/\n"; + print $fh "confirmation=no\n"; + print $fh "merge.autopush=yes\n"; + print $fh "merge.default.uri=./dropbox/\n"; + print $fh "push.default.uri=./dropbox/\n"; + print $fh "pull.default.uri=./dropbox/\n"; + + close $fh; + ok (-r '3.rc', 'Created 3.rc'); +} + # Once-only push from 1 --> dropbox my $output = qx{../src/task rc:1.rc add one 2>&1}; ok ($? == 0, 'Exit status check'); +$output = qx{../src/task rc:1.rc add two 2>&1}; +ok ($? == 0, 'Exit status check'); +$output = qx{../src/task rc:1.rc add three 2>&1}; +ok ($? == 0, 'Exit status check'); $output = qx{../src/task rc:1.rc push 2>&1}; ok ($? == 0, 'Exit status check'); -# Merges to 2 +# Merges to 2 and 3 $output = qx{../src/task rc:2.rc merge 2>&1}; ok ($? == 0, 'Exit status check'); +$output = qx{../src/task rc:3.rc merge 2>&1}; +ok ($? == 0, 'Exit status check'); # Make a different change in both locations -$output = qx{../src/task rc:1.rc add two 2>&1}; +$output = qx{../src/task rc:1.rc add four 2>&1}; ok ($? == 0, 'Exit status check'); -$output = qx{../src/task rc:1.rc two done 2>&1}; +$output = qx{../src/task rc:1.rc four done 2>&1}; ok ($? == 0, 'Exit status check'); -$output = qx{../src/task rc:2.rc 1 done 2>&1}; +$output = qx{../src/task rc:2.rc one done 2>&1}; +ok ($? == 0, 'Exit status check'); +$output = qx{../src/task rc:3.rc three delete 2>&1}; ok ($? == 0, 'Exit status check'); -# Merges everywhere +# Merges 1 and 2 $output = qx{../src/task rc:1.rc merge 2>&1}; ok ($? == 0, 'Exit status check'); $output = qx{../src/task rc:2.rc merge 2>&1}; @@ -102,8 +126,18 @@ ok ($? == 0, 'Exit status check'); $output = qx{../src/task rc:1.rc diag 2>&1}; unlike ($output, qr/Found duplicate/, "Found duplicate"); +# 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"); + # 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); +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' && ! -r '1/completed.data' && ! -r '1/undo.data' && @@ -116,13 +150,20 @@ ok (! -r '1/pending.data' && ! -r '2/backlog.data' && ! -r '2/synch.key' && ! -r '2.rc' && + ! -r '3/pending.data' && + ! -r '3/completed.data' && + ! -r '3/undo.data' && + ! -r '3/backlog.data' && + ! -r '3/synch.key' && + ! -r '3.rc' && ! -r 'dropbox/pending.data' && ! -r 'dropbox/completed.data' && ! -r 'dropbox/undo.data' , 'Cleanup'); -rmtree (['1', '2', 'dropbox'], 0, 1); +rmtree (['1', '2', '3', 'dropbox'], 0, 1); ok (! -e '1' && ! -e '2' && + ! -e '3' && ! -e 'dropbox', 'Removed directories'); exit 0;