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

@@ -1024,8 +1024,8 @@ void TDB2::merge (const std::string& mergeFile)
     mods.splice (mods.begin (), rmods);
 
     DEBUG_STR ("sorting taskmod list");
-    mods.sort ();
+    mods.sort (compareTaskmod);
-    mods_history.sort ();
+    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);
 

src/Taskmod.cpp

@@ -33,22 +33,38 @@
 #include <assert.h>
 #include <Taskmod.h>
 
+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->_before         = other._before;
-  this->_after      = other._after;
+  this->_after          = other._after;
-  this->_timestamp  = other._timestamp;
+  this->_timestamp      = other._timestamp;
-  this->_bAfterSet  = other._bAfterSet;
+  this->_bAfterSet      = other._bAfterSet;
-  this->_bBeforeSet = other._bBeforeSet;
+  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->_before         = other._before;
-    this->_after      = other._after;
+    this->_after          = other._after;
-    this->_timestamp  = other._timestamp;
+    this->_timestamp      = other._timestamp;
-    this->_bAfterSet  = other._bAfterSet;
+    this->_bAfterSet      = other._bAfterSet;
-    this->_bBeforeSet = other._bBeforeSet;
+    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->_bAfterSet      = false;
-  this->_bBeforeSet = false;
+  this->_bBeforeSet     = false;
-  this->_timestamp  = timestamp;
+  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
 {

src/Taskmod.h

@@ -33,6 +33,8 @@
 #include <Task.h>
 
 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
 

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;