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

@@ -646,8 +646,9 @@ void readTaskmods (std::vector <std::string> &input,
                    std::vector <std::string>::iterator &start,
                    std::list<Taskmod> &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)
+  if (lline.compare (rline) == 0) {
-  {
+    std::vector<std::string>::const_iterator tmp_lit = lit;
-    // iterate in local file to find rline
+    std::vector<std::string>::const_iterator tmp_rit = rit;
-    for ( ; lit != l.end (); ++lit)
+    tmp_lit++;
-    {
+    tmp_rit++;
-      lline = *lit;
 
-      // push the line to the new undo.data
+    int lookahead = 1;
-      undo_lines.push_back (lline + "\n");
+    if (tmp_lit->substr (0, 3) == "old") {
-
+      lookahead = 2;
-      // found first matching lines?
+    }
-      if (lline.compare (rline) == 0)
+
-        break;
+    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<std::string> 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<Taskmod>::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 (lmods, compareTaskmod);
-    mods.merge (mods_history);
+    mods.merge (mods_history, compareTaskmod);
 
     // generate undo.data format
     std::list<Taskmod>::iterator it;

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
 {

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;
 };

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'         &&

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;