[Tickets #12597] Re: AS: Detect deletes on a non-CONDSTORE server

[noreply at bugs.horde.org]

DO NOT REPLY TO THIS MESSAGE. THIS EMAIL ADDRESS IS NOT MONITORED.

Ticket URL: http://bugs.horde.org/ticket/12597
------------------------------------------------------------------------------
  Ticket             | 12597
  Updated By         | Michael Rubinsky <mrubinsk at horde.org>
  Summary            | AS: Detect deletes on a non-CONDSTORE server
  Queue              | Synchronization
  Version            | Git master
  Type               | Enhancement
-State              | Assigned
+State              | Feedback
  Priority           | 1. Low
  Milestone          |
  Patch              | 1
  Owners             | Michael Rubinsky
------------------------------------------------------------------------------


Michael Rubinsky <mrubinsk at horde.org> (2013-08-22 15:38) wrote:

>> Committed, with a few tweaks and changes.
>>
>> Thanks!
>
> ehrm, those tweaks contain little bugs :o)
> -> Not all setChanges() calls were updated during "cleanup",
> it will still access $status['messages'] for CONDSTORE servers.

*sigh* These were still in my tree, but missed them when git adding.

> Also we would need another if() for CONDSTORE/non-CONDSTORE
> for the inital sync.

Why? There is already a conditional there around line 364.

> The code is more complex than it needs to be: We can specify  
> Horde_Imap_Client::STATUS_HIGHESTMODSEQ
> for non-CONDSTORE servers and the Imap_Client will return 0 for the  
> modseq in that case.
> This would get rid of the duplicated $status_flags and the  
> $imap->queryCapability('CONDSTORE') call.
>
> Also querying the amount of messages is very cheap for a CONDSTORE server,
> no need to over-optimize this and create more complex code paths.
> -> Let's store the number of messages for all cases.

I've been of the mindset from the beginning with the EAS email stuff  
that every single bit of optimization we can do in the Horde <-> IMAP  
communication should be done due to the high frequency that EAS  
clients can PING the servers. I understand that it might be relatively  
cheap, but the extra logic involved is also relatively minor.

> For the ping() code, I moved the modseq code path first
> so it will be the default for modern IMAP servers.
> Though I doubt this makes any noticeable performance difference ;)

I'm less concerned with the order the checks are done than I am with  
asking the IMAP server for information we don't *really* need.


> Please consider the attached patch. Works fine on a non-CONDSTORE server.

For new, empty, folders this would cause the initial sync code to hit  
on every sync even on CONDSTORE servers - after we already know we  
don't have changes. I had code similar to this in place (using only  
modseq == 0 as the final check) once apon a time, but needed to change  
it due to this issue (and others, IIRC though I'm drawing a blank on  
what they were).