On Fri, 2011-09-30 at 12:10 +0200, Patrick Ohly wrote: > > > In the case of iCalendar 2.0 I wonder whether merge=lines is > needed. > > > For modify/modify conflicts perhaps, but not so much for add/add > > > conflicts. Thoughts? > > > > If you want to differentiate these cases, I'd say we'll need a > > MERGEREASON() script function so the script can act differently > > depending on why the merge is initiated. So far I see the following > > different reasons: slowsync non-perfect match, syncml update > conflict, > > DB update conflict, DB add conflict. > > I'm inclined to rather remove the merge=lines and thus keep properties > from the winning side in all cases. > > Except... what if a dumb phone truncates a property and then a slow > sync > is done? Will the default merge mode preserve the longer property or > truncate if the version on the phone happens to be considered > "winning"?
I'm still curious about that. Note that the previous age comparison patch had the -1/+1 logic reversed. It worked correctly in my tests because the COMPARESCRIPT() macro always returned 1 although it should have returned -1. Later it failed for the inverse test. Fix for that and updated patch attached. -- Best Regards, Patrick Ohly The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter.
>From b0252bdf02720193d1c638ec9ec32c1f4cedc6f3 Mon Sep 17 00:00:00 2001 From: Patrick Ohly <[email protected]> Date: Fri, 30 Sep 2011 18:11:41 +0200 Subject: [PATCH 1/5] COMPAREFIELDS(): fix -1 case The compare result was only 1 or 0, never -1, because of a teriary operator which mapped all non-null values to 1. Cut-and-paste error? The fix passes the underlying value through unchanged. Perhaps this will return values like -999 to the caller, in contradiction to the definition of the macro. However, it is unclear what should be returned in that case. --- src/sysync/multifielditemtype.cpp | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/sysync/multifielditemtype.cpp b/src/sysync/multifielditemtype.cpp index 6f1d810..794baad 100755 --- a/src/sysync/multifielditemtype.cpp +++ b/src/sysync/multifielditemtype.cpp @@ -215,7 +215,7 @@ public: if (!mfitP->fFirstItemP) aTermP->setAsInteger(SYSYNC_NOT_COMPARABLE); else { aTermP->setAsInteger( - mfitP->fFirstItemP->standardCompareWith(*(mfitP->fSecondItemP),mfitP->fEqMode,OBJDEBUGTEST(mfitP,DBG_SCRIPTS+DBG_DATA+DBG_MATCH)) ? 1 : 0 + mfitP->fFirstItemP->standardCompareWith(*(mfitP->fSecondItemP),mfitP->fEqMode,OBJDEBUGTEST(mfitP,DBG_SCRIPTS+DBG_DATA+DBG_MATCH)) ); } }; // func_CompareFields -- 1.7.2.5
>From dbb5e1829e572b198694366a535e8b92ec0c07aa Mon Sep 17 00:00:00 2001 From: Patrick Ohly <[email protected]> Date: Fri, 30 Sep 2011 11:13:56 +0200 Subject: [PATCH 3/5] DB_Conflict (409): do age comparison before merging, avoid unneeded changes Instead of always assuming that the incoming item is meant to win, look at the age of the two items first. Also tell the code which updates the DB item whether it was changed at all and skip the DB write if it is not needed. Note that statistics are still wrong: a server add is counted as "item added" in all cases, even if it is turned into an update or becomes a nop. --- src/sysync/customimplds.cpp | 65 ++++++++++++++++++++++++++++++++++-------- src/sysync/customimplds.h | 2 +- 2 files changed, 53 insertions(+), 14 deletions(-) diff --git a/src/sysync/customimplds.cpp b/src/sysync/customimplds.cpp index f84b901..373cf4f 100755 --- a/src/sysync/customimplds.cpp +++ b/src/sysync/customimplds.cpp @@ -2718,9 +2718,12 @@ localstatus TCustomImplDS::implProcessMap(cAppCharP aRemoteID, cAppCharP aLocalI -/// helper to merge database version of an item with the passed version of the same item -TMultiFieldItem *TCustomImplDS::mergeWithDatabaseVersion(TSyncItem *aSyncItemP) +/// helper to merge database version of an item with the passed version of the same item; +/// does age comparison by default, with "local side wins" as fallback +TMultiFieldItem *TCustomImplDS::mergeWithDatabaseVersion(TSyncItem *aSyncItemP, bool &aChangedDBVersion) { + aChangedDBVersion = false; + TStatusCommand dummy(fSessionP); TMultiFieldItem *dbVersionItemP = (TMultiFieldItem *)newItemForRemote(aSyncItemP->getTypeID()); if (!dbVersionItemP) return NULL; @@ -2733,13 +2736,39 @@ TMultiFieldItem *TCustomImplDS::mergeWithDatabaseVersion(TSyncItem *aSyncItemP) bool ok=logicRetrieveItemByID(*dbVersionItemP,dummy); if (ok && dummy.getStatusCode()!=404) { // item found in DB, merge with original item - bool changedNewVersion, changedDBVersion; - aSyncItemP->mergeWith(*dbVersionItemP, changedNewVersion, changedDBVersion, this); - PDEBUGPRINTFX(DBG_DATA,( - "Merged incoming item (winning,relevant,%smodified) with version from database (loosing,to-be-replaced,%smodified)", - changedNewVersion ? "" : "NOT ", - changedDBVersion ? "" : "NOT " - )); + // TODO (?): make this configurable + TConflictResolution crstrategy = cr_newer_wins; + + if (crstrategy==cr_newer_wins) { + sInt16 cmpRes = aSyncItemP->compareWith(*dbVersionItemP, + eqm_nocompare,this +#ifdef SYDEBUG + ,PDEBUGTEST(DBG_CONFLICT+DBG_DETAILS) // show age comparisons only if we want to see details +#endif + ); + if (cmpRes==-1) crstrategy=cr_server_wins; + else crstrategy=cr_client_wins; + PDEBUGPRINTFX(DBG_DATA,( + "Newer item determined: %s", + crstrategy==cr_client_wins ? + "Incoming item is newer and wins" : + "DB item is newer and wins")); + + bool changedNewVersion; + if (crstrategy==cr_client_wins) { + aSyncItemP->mergeWith(*dbVersionItemP, changedNewVersion, aChangedDBVersion, this); + } else { + dbVersionItemP->mergeWith(*aSyncItemP, aChangedDBVersion, changedNewVersion, this); + } + PDEBUGPRINTFX(DBG_DATA,( + "Merged incoming item (%s,relevant,%smodified) with version from database (%s,%s,%smodified)", + crstrategy==cr_client_wins ? "winning" : "loosing", + changedNewVersion ? "" : "NOT ", + crstrategy==cr_server_wins ? "winning" : "loosing", + aChangedDBVersion ? "to-be-replaced" : "to-be-left-unchanged", + aChangedDBVersion ? "" : "NOT " + )); + } } else { // no item found, we cannot force a conflict @@ -2834,11 +2863,16 @@ bool TCustomImplDS::implProcessItem( if (sta==DB_Conflict) { // DB has detected item conflicts with data already stored in the database and // request merging current data from the backend with new data before storing. - augmentedItemP = mergeWithDatabaseVersion(myitemP); + bool changedDBVersion; + augmentedItemP = mergeWithDatabaseVersion(myitemP, changedDBVersion); if (augmentedItemP==NULL) sta = DB_Error; // no item found, DB error else { - sta = apiUpdateItem(*augmentedItemP); // store augmented version back to DB + // store augmented version back to DB only if modified + if (changedDBVersion) + sta = apiUpdateItem(*augmentedItemP); + else + sta = LOCERR_OK; // in server case, further process like backend merge (but no need to fetch again, we just keep augmentedItemP) if (IS_SERVER && sta==LOCERR_OK) sta = DB_DataMerged; } @@ -2944,11 +2978,16 @@ bool TCustomImplDS::implProcessItem( if (sta==DB_Conflict) { // DB has detected item conflicts with data already stored in the database and // request merging current data from the backend with new data before storing. - augmentedItemP = mergeWithDatabaseVersion(myitemP); + bool changedDBVersion; + augmentedItemP = mergeWithDatabaseVersion(myitemP, changedDBVersion); if (augmentedItemP==NULL) sta = DB_Error; // no item found, DB error else { - sta = apiUpdateItem(*augmentedItemP); // store augmented version back to DB + // store augmented version back to DB only if modified + if (changedDBVersion) + sta = apiUpdateItem(*augmentedItemP); + else + sta = LOCERR_OK; delete augmentedItemP; // forget now } } diff --git a/src/sysync/customimplds.h b/src/sysync/customimplds.h index 0437866..939f1ad 100755 --- a/src/sysync/customimplds.h +++ b/src/sysync/customimplds.h @@ -773,7 +773,7 @@ protected: // as a item copy with only finalisation-required fields void queueForFinalisation(TMultiFieldItem *aItemP); /// helper to merge database version of an item with the passed version of the same item - TMultiFieldItem *mergeWithDatabaseVersion(TSyncItem *aSyncItemP); + TMultiFieldItem *mergeWithDatabaseVersion(TSyncItem *aSyncItemP, bool &aChangedDBVersion); public: // - get last to-remote sync time lineartime_t getPreviousToRemoteSyncCmpRef(void) { return fPreviousToRemoteSyncCmpRef; }; -- 1.7.2.5
_______________________________________________ os-libsynthesis mailing list [email protected] http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis
