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

Reply via email to