Bug

- merge was playing ping-pong with different undo.data results
  in a setup with three machines

src/TDB.cpp

@@ -1018,6 +1018,9 @@ void TDB::merge (const std::string& mergeFile)
   // list of modifications that we want to add to the local database
   std::list<Taskmod> mods;
 
+  // list of modifications that we want to add to the local history
+  std::list<Taskmod> mods_history;
+
   // list of modifications on the local database
   // has to be merged with mods to create the new undo.data
   std::list<Taskmod> lmods;
@@ -1224,6 +1227,9 @@ void TDB::merge (const std::string& mergeFile)
             {
               DEBUG_STR ("    cleaning up right side");
 
+              // add tmod_r to local history
+              mods_history.push_front (tmod_r);
+
               std::list<Taskmod>::iterator tmp_it = rmod_rit.base ();
               rmods.erase (--tmp_it);
               rmod_rit--;
@@ -1256,19 +1262,40 @@ void TDB::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 
+      	        // as if we switched the roles 'remote' and 'local'
 					
-                // TODO feature: make rejected changes on the remote branch restorable
+                // thus we have to find the oldest change on the local branch that is not on remote branch
-//                Taskmod reverse_tmod;
+                std::list<Taskmod>::iterator lmod_it;
-//
+                std::list<Taskmod>::iterator last = lmod_it;
-//                tmod_r.setBefore(lmod_rit->getAfter());
+                for (lmod_it = lmods.begin (); lmod_it != lmods.end (); ++lmod_it) {
-//                tmod_r.setTimestamp(lmod_rit->getTimestamp()+1);
+                  if ((*lmod_it).getUuid () == uuid) {
-//
+                    last = lmod_it;
-//                reverse_tmod.setAfter(tmod_r.getBefore());
+                  }
-//                reverse_tmod.setBefore(tmod_r.getAfter());
+                }
-//                reverse_tmod.setTimestamp(tmod_r.getTimestamp());
+
-//
+                if (tmod_l > tmod_r) { // local change is newer
-//                mods.push_back(tmod_r);
+                  last->setBefore(tmod_r.getAfter ());
-//                mods.push_back(reverse_tmod);
+
+                  // add tmod_r to local history
+                  lmods.push_back(tmod_r);
+                }
+                else { // both mods have equal timestamps
+                  // in this case the local branch wins as above, but the remote change with the
+                  // same timestamp will be discarded
+
+                  // find next (i.e. older) mod of this uuid on remote side
+                  std::list<Taskmod>::reverse_iterator rmod_rit2;
+                  for (rmod_rit2 = rmod_rit, ++rmod_rit2; rmod_rit2 != rmods.rend (); ++rmod_rit2) {
+                    Taskmod tmp_mod = *rmod_rit2;
+                    if (tmp_mod.getUuid () == uuid) {
+                      last->setBefore (tmp_mod.getAfter ());
+                      break;
+                    }
+                  }
+                }
+
+                // TODO feature: restore command? We would have to add a marker to the undo.file.
 
                 // delete tmod from right side
                 std::list<Taskmod>::iterator tmp_it = rmod_rit.base ();
@@ -1298,6 +1325,7 @@ void TDB::merge (const std::string& mergeFile)
 
     DEBUG_STR ("sorting taskmod list");
     mods.sort ();
+    mods_history.sort ();
   }
   else if (rit == r.end ())
   {
@@ -1501,13 +1529,18 @@ void TDB::merge (const std::string& mergeFile)
     // write completed file
     if (! File::write (completedFile, completed))
       throw std::string ("Could not write '") + completedFile + "'.";
+  }
 
+  if (!mods.empty() || !lmods.empty() || !mods_history.empty()) {
     // 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();
     mods.merge (lmods);
+    mods.merge (mods_history);
 
     // generate undo.data format
+    std::list<Taskmod>::iterator it;
     for (it = mods.begin (); it != mods.end (); it++)
       undo.push_back(it->toString ());
 
@@ -1519,6 +1552,7 @@ void TDB::merge (const std::string& mergeFile)
   // delete objects
   lmods.clear ();
   mods.clear ();
+  mods_history.clear ();
 }
 
 ////////////////////////////////////////////////////////////////////////////////

src/TDB2.cpp

@@ -1504,6 +1504,9 @@ void TDB::merge (const std::string& mergeFile)
   // list of modifications that we want to add to the local database
   std::list<Taskmod> mods;
 
+  // list of modifications that we want to add to the local history
+  std::list<Taskmod> mods_history;
+
   // list of modifications on the local database
   // has to be merged with mods to create the new undo.data
   std::list<Taskmod> lmods;
@@ -1710,6 +1713,9 @@ void TDB::merge (const std::string& mergeFile)
             {
               DEBUG_STR ("    cleaning up right side");
 
+              // add tmod_r to local history
+              mods_history.push_front (tmod_r);
+
               std::list<Taskmod>::iterator tmp_it = rmod_rit.base ();
               rmods.erase (--tmp_it);
               rmod_rit--;
@@ -1742,19 +1748,40 @@ void TDB::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 
+      	        // as if we switched the roles 'remote' and 'local'
 					
-                // TODO feature: make rejected changes on the remote branch restorable
+                // thus we have to find the oldest change on the local branch that is not on remote branch
-//                Taskmod reverse_tmod;
+                std::list<Taskmod>::iterator lmod_it;
-//
+                std::list<Taskmod>::iterator last = lmod_it;
-//                tmod_r.setBefore(lmod_rit->getAfter());
+                for (lmod_it = lmods.begin (); lmod_it != lmods.end (); ++lmod_it) {
-//                tmod_r.setTimestamp(lmod_rit->getTimestamp()+1);
+                  if ((*lmod_it).getUuid () == uuid) {
-//
+                    last = lmod_it;
-//                reverse_tmod.setAfter(tmod_r.getBefore());
+                  }
-//                reverse_tmod.setBefore(tmod_r.getAfter());
+                }
-//                reverse_tmod.setTimestamp(tmod_r.getTimestamp());
+
-//
+                if (tmod_l > tmod_r) { // local change is newer
-//                mods.push_back(tmod_r);
+                  last->setBefore(tmod_r.getAfter ());
-//                mods.push_back(reverse_tmod);
+
+                  // add tmod_r to local history
+                  lmods.push_back(tmod_r);
+                }
+                else { // both mods have equal timestamps
+                  // in this case the local branch wins as above, but the remote change with the
+                  // same timestamp will be discarded
+
+                  // find next (i.e. older) mod of this uuid on remote side
+                  std::list<Taskmod>::reverse_iterator rmod_rit2;
+                  for (rmod_rit2 = rmod_rit, ++rmod_rit2; rmod_rit2 != rmods.rend (); ++rmod_rit2) {
+                    Taskmod tmp_mod = *rmod_rit2;
+                    if (tmp_mod.getUuid () == uuid) {
+                      last->setBefore (tmp_mod.getAfter ());
+                      break;
+                    }
+                  }
+                }
+
+                // TODO feature: restore command? We would have to add a marker to the undo.file.
 
                 // delete tmod from right side
                 std::list<Taskmod>::iterator tmp_it = rmod_rit.base ();
@@ -1784,6 +1811,7 @@ void TDB::merge (const std::string& mergeFile)
 
     DEBUG_STR ("sorting taskmod list");
     mods.sort ();
+    mods_history.sort ();
   }
   else if (rit == r.end ())
   {
@@ -1987,13 +2015,18 @@ void TDB::merge (const std::string& mergeFile)
     // write completed file
     if (! File::write (completedFile, completed))
       throw std::string ("Could not write '") + completedFile + "'.";
+  }
 
+  if (!mods.empty() || !lmods.empty() || !mods_history.empty()) {
     // 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();
     mods.merge (lmods);
+    mods.merge (mods_history);
 
     // generate undo.data format
+    std::list<Taskmod>::iterator it;
     for (it = mods.begin (); it != mods.end (); it++)
       undo.push_back(it->toString ());
 
@@ -2005,6 +2038,7 @@ void TDB::merge (const std::string& mergeFile)
   // delete objects
   lmods.clear ();
   mods.clear ();
+  mods_history.clear ();
 }
 
 ////////////////////////////////////////////////////////////////////////////////

test/merge.duplicates.t

@@ -0,0 +1,223 @@
+#! /usr/bin/perl
+################################################################################
+## taskwarrior - a command line task list manager.
+##
+## Copyright 2006 - 2011, Paul Beckingham, Johannes Schlatow.
+## All rights reserved.
+##
+## This program is free software; you can redistribute it and/or modify it under
+## the terms of the GNU General Public License as published by the Free Software
+## Foundation; either version 2 of the License, or (at your option) any later
+## version.
+##
+## This program is distributed in the hope that it will be useful, but WITHOUT
+## ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+## FOR A PARTICULAR PURPOSE.  See the GNU General Public License for more
+## details.
+##
+## You should have received a copy of the GNU General Public License along with
+## this program; if not, write to the
+##
+##     Free Software Foundation, Inc.,
+##     51 Franklin Street, Fifth Floor,
+##     Boston, MA
+##     02110-1301
+##     USA
+##
+################################################################################
+
+use strict;
+use warnings;
+use Test::More tests => 45;
+use File::Copy;
+
+use constant false => 0;
+use constant true => 1;
+
+# Create data locations
+mkdir("data1", 0755);
+ok(-e 'data1', "Created directory data1");
+mkdir("data2", 0755);
+ok(-e 'data2', "Created directory data2");
+mkdir("data3", 0755);
+ok(-e 'data3', "Created directory data3");
+mkdir('backup', 0755);
+ok(-e 'backup', "Created directory backup");
+
+# Create the rc files.
+if (open my $fh, '>', '1.rc')
+{
+  print $fh "data.location=./data1\n",
+            "confirmation=no\n",
+				"merge.autopush=yes\n",
+				"merge.default.uri=./backup/\n",
+            "report.list.description=DESC\n",
+				"report.list.columns=id,project,active,priority,description,tags\n",
+				"report.list.labels=id,pro,a,pri,d,t\n",
+				"report.list.sort=id+\n",
+				"report.list.filter=status:pending\n";
+  close $fh;
+  ok (-r '1.rc', 'Created 1.rc');
+}
+
+# Create the rc files.
+if (open my $fh, '>', '2.rc')
+{
+  print $fh "data.location=./data2\n",
+            "confirmation=no\n",
+				"merge.autopush=yes\n",
+				"merge.default.uri=./backup/\n",
+            "report.list.description=DESC\n",
+				"report.list.columns=id,project,active,priority,description,tags\n",
+				"report.list.labels=id,pro,a,pri,d,t\n",
+				"report.list.sort=id+\n",
+				"report.list.filter=status:pending\n";
+  close $fh;
+  ok (-r '2.rc', 'Created 2.rc');
+}
+
+# Create the rc files.
+if (open my $fh, '>', '3.rc')
+{
+  print $fh "data.location=./data3\n",
+            "confirmation=no\n",
+				"merge.autopush=yes\n",
+				"merge.default.uri=./backup/\n",
+            "report.list.description=DESC\n",
+				"report.list.columns=id,project,active,priority,description,tags\n",
+				"report.list.labels=id,pro,a,pri,d,t\n",
+				"report.list.sort=id+\n",
+				"report.list.filter=status:pending\n";
+  close $fh;
+  ok (-r '3.rc', 'Created 3.rc');
+}
+
+#######################################
+# Create tasks on 1st resource
+qx{../src/task rc:1.rc add Task1};
+diag ("7 second delay");
+sleep(1);
+qx{../src/task rc:1.rc add Task2};
+sleep(1);
+qx{../src/task rc:1.rc add Task3};
+sleep(1);
+qx{../src/task rc:1.rc add Task4};
+
+# Merge with backup
+my $output = qx{../src/task rc:1.rc push ./backup/};
+
+#######################################
+# Modify on 2nd resource
+
+# first merge
+$output = qx{../src/task rc:2.rc merge};
+like ($output, qr/Merge complete/, "res2: pre-merge completed");
+
+# complete Task1
+qx{../src/task rc:2.rc 1 done};
+sleep(1);
+
+#######################################
+# Modify on 3rd resource
+
+# first merge
+$output = qx{../src/task rc:3.rc merge};
+like ($output, qr/Merge complete/, "res3: pre-merge completed");
+
+# complete Task1
+qx{../src/task rc:3.rc 1 done};
+sleep(1);
+
+# now merge 3rd resource
+$output = qx{../src/task rc:3.rc merge};
+like ($output, qr/Merge complete/, "res3: post-merge completed");
+unlike ($output, qr/Missing/, "no missing entry");
+
+# and merge 2nd resource
+$output = qx{../src/task rc:2.rc merge};
+like ($output, qr/Merge complete/, "res2: post-merge completed");
+unlike ($output, qr/Missing/, "no missing entry");
+
+# merge 3rd
+$output = qx{../src/task rc:3.rc merge};
+like ($output, qr/Merge complete/, "res3: post-merge completed");
+unlike ($output, qr/Missing/, "no missing entry");
+like ($output, qr/Retain/,  "retained changes");
+
+# pre-merge 1st
+$output = qx{../src/task rc:1.rc merge};
+like ($output, qr/Merge complete/, "res1: pre-merge completed");
+unlike ($output, qr/Missing/, "no missing entry");
+
+qx{../src/task rc:1.rc add Task5};
+sleep(1);
+qx(../src/task rc:1.rc 4 done);
+sleep(1);
+
+# merge
+$output = qx{../src/task rc:1.rc merge};
+like ($output, qr/Merge complete/, "res1: post-merge completed");
+unlike ($output, qr/Missing/, "no missing entry");
+
+# pre-merge 2nd res
+$output = qx{../src/task rc:2.rc merge};
+like ($output, qr/Merge complete/, "res2: pre-merge completed");
+unlike ($output, qr/Missing/, "no missing entry");
+
+# merge
+$output = qx{../src/task rc:1.rc merge};
+like ($output, qr/up-to-date/, "res1: up-to-date");
+unlike ($output, qr/Missing/, "no missing entry");
+
+# pre-merge 2nd res
+$output = qx{../src/task rc:2.rc merge};
+like ($output, qr/up-to-date/, "res2: up-to-date");
+unlike ($output, qr/Missing/, "no missing entry");
+
+# Cleanup.
+unlink 'data1/pending.data';
+ok (!-r 'data1/pending.data', 'Removed data1/pending.data');
+unlink 'data1/completed.data';
+ok (!-r 'data1/completed.data', 'Removed data1/completed.data');
+unlink 'data1/undo.data';
+ok (!-r 'data1/undo.data', 'Removed data1/undo.data');
+
+unlink 'data2/pending.data';
+ok (!-r 'data2/pending.data', 'Removed data2/pending.data');
+unlink 'data2/completed.data';
+ok (!-r 'data2/completed.data', 'Removed data2/completed.data');
+unlink 'data2/undo.data';
+ok (!-r 'data2/undo.data', 'Removed data2/undo.data');
+
+unlink 'data3/pending.data';
+ok (!-r 'data3/pending.data', 'Removed data3/pending.data');
+unlink 'data3/completed.data';
+ok (!-r 'data3/completed.data', 'Removed data3/completed.data');
+unlink 'data3/undo.data';
+ok (!-r 'data3/undo.data', 'Removed data3/undo.data');
+
+unlink 'backup/pending.data';
+ok (!-r 'backup/pending.data', 'Removed backup/pending.data');
+unlink 'backup/completed.data';
+ok (!-r 'backup/completed.data', 'Removed backup/completed.data');
+unlink 'backup/undo.data';
+ok (!-r 'backup/undo.data', 'Removed backup/undo.data');
+
+unlink '1.rc';
+ok (!-r '1.rc', 'Removed 1.rc');
+unlink '2.rc';
+ok (!-r '2.rc', 'Removed 2.rc');
+unlink '3.rc';
+ok (!-r '3.rc', 'Removed 3.rc');
+
+rmdir("data1");
+ok (!-e "data1", "Removed dir data1");
+rmdir("data2");
+ok (!-e "data2", "Removed dir data2");
+rmdir("data3");
+ok (!-e "data3", "Removed dir data3");
+rmdir("backup");
+ok (!-e "backup", "Removed dir backup");
+
+exit 0;
+