On Mon, 2014-07-14 at 18:15 +0200, Lukas Zeller wrote: > Hello Patrick, > > On 11 Jul 2014, at 15:27, Patrick Ohly <patrick.o...@intel.com> wrote: > > > I'm currently investigating why a comparison of two PHOTO fields of > > different length returns "field equal". PHOTO is defined as: > > > > <field name="PHOTO" type="string" compare="conflict" > > merge="fillempty"/> > > My first question would be why PHOTO was defined "string" here, not "blob"?
I changed that in this commit: commit 6d3b1cf64b09cf1603b813137d14d529a31dda8b Author: Patrick Ohly <patrick.o...@intel.com> Date: Fri Jul 5 09:45:15 2013 +0200 EDS: update PHOTO+GEO during slow sync, avoid rewriting PHOTO file If PHOTO and/or GEO were the only modified properties during a slow sync, the updated item was not written into local storage because they were marked as compare="never" = "not relevant". For PHOTO this was intentional in the sample config, with the rationale that local storages often don't store the data exactly as requested. When that happens, comparing the data would lead to unnecessary writes. But EDS and probably all other local SyncEvolution storages (KDE, file) store the photo exactly as requested, so not considering changes had the undesirable effect of not always writing new photo data. For GEO, ignoring it was accidental. A special merge script handles EDS file:// photo URIs. When the loosing item has the data in a file and the winning item has binary data, the data in the file may still be up-to-date, so before allowing MERGEFIELDS() to overwrite the file reference with binary data and thus forcing EDS to write a new file, check the content. If it matches, use the file reference in both items. Unfortunately I did not write down why I changed the type, instead of merely changing the compare setting. Does it matter at all? > IMHO The real bug is that TBlobField does not override compareWith. > TBlobField's compareWith could ignore aCaseInsensitive and use memcmp, > for the case of actually comparing two blobs, and fall back to > TStringField for comparison with other types. > > > We have std::string as value and therefore can store null bytes as part > > of the data, but the actual comparison falls back to C-string > > operations, which only work for null bytes at the end. > > There's to much C strings throughout the entire project, so for text, > C string semantics are assumed pretty much everywhere. > > TBlobField however makes use of the true binary byte string capability of > std::string exactly this way. That's also true for TStringField, except that it occasionally falls back to plain C operations, which is only correct for real strings. > > I think the strcmp() needs to be replaced with something that also looks > > at the rest of the string if no difference is found. Agreed? > > A fully std::string compatible comparison would be nicer here, and fix the > problem. Here's the patch that I ended up using: commit 9278e054e9a9a2aa8c73aed98cb42bf1f9bfd0fe Author: Patrick Ohly <patrick.o...@intel.com> Date: Mon Jul 14 05:00:54 2014 -0700 string fields: full compare String fields also get used for arbitrary binary data, like photos. In that case we need to compare the entire std::string, not just the part before any embedded null byte. This gets done using std::string::compare. Case-insensitive comparison still uses C strucmp() and thus should never be used for non-string data. diff --git a/src/sysync/itemfield.cpp b/src/sysync/itemfield.cpp index e6f7d71..64867e0 100644 --- a/src/sysync/itemfield.cpp +++ b/src/sysync/itemfield.cpp @@ -801,7 +801,7 @@ bool TStringField::merge(TItemField &aItemField, const char aSep) // Note: Both fields must be assigned. NO TEST IS DONE HERE! sInt16 TStringField::compareWith(TItemField &aItemField, bool aCaseInsensitive) { - sInt16 result; + int result; PULLFROMPROXY; if (aItemField.isBasedOn(fty_string)) { TStringField *sfP = static_cast<TStringField *>(&aItemField); @@ -812,7 +812,7 @@ sInt16 TStringField::compareWith(TItemField &aItemField, bool aCaseInsensitive) if (aCaseInsensitive) result=strucmp(fString.c_str(),sfP->fString.c_str()); else - result=strcmp(fString.c_str(),sfP->fString.c_str()); + result=fString.compare(sfP->fString); } else { // convert other field to string @@ -821,7 +821,7 @@ sInt16 TStringField::compareWith(TItemField &aItemField, bool aCaseInsensitive) if (aCaseInsensitive) result=strucmp(fString.c_str(),s.c_str()); else - result=strcmp(fString.c_str(),s.c_str()); + result=fString.compare(s); } return result >0 ? 1 : (result<0 ? -1 : 0); } // TStringField::compareWith -- 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. _______________________________________________ os-libsynthesis mailing list os-libsynthesis@synthesis.ch http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis