<DKIM> maxage causes loss of local email

[Janna Martl]

On Mon, Mar 09, 2015 at 11:09:13AM +0100, Nicolas Sebrecht wrote:
>Please, work on the current 'next' branch, just updated. This will avoid
>merge conflicts.

Oops, sorry!

>> +
>> +    def time_is_within_maxage(self, t, maxage):
>> +        """ Checks if time t (expressed as seconds since the epoch) is later than
>> +        00:00 UTC on (today's date) - (maxage - 1 days). We do this because
>> +        SINCE in an IMAP search has maximum granularity of days. """
>>
> def s_time_is_within_d_range(self, t, d):
>     """Check if time t is later than (today - d days)."""
> ...
>I think the implementation get it wrong. If you just strip off the
>hrs/mins, you are only be reducing up to
>
>  (maxage - X hours) with X within [0; 24],
>
>not the (maxage - 24 hours) we want.

I'll address the rest of your comments later, but I'm not following
here. This isn't where the "request +/- 1 day's worth of
extra messages" is happening; this is just pre-existing code that got
moved... to recap, the strategy is:

(1) Get local messages of "acceptable age".
(2) Get remote messages. Because of timezone issues, when we make
SEARCH SINCE queries, we don't know exactly what we're getting, but we
can at least make sure we're getting *at least* the messages of
"acceptable age" (but possibly more).
(3) Reduce that list to messages of "acceptable age".

I don't think you need to ask anyone for messages of age
< maxage - 1.

It shouldn't matter exactly what "acceptable age" means, just that
it's consistent everywhere I use this concept. So in my patch, the
definition is "time_is_within_maxage(time, maxage)"; this isn't the
obvious definition of time > now - maxage*24*60*60, but is the
(pre-existing) somewhat complicated definition copied from the old
_iswithinmaxage() that I described in the comment above.

Now, this definition does seem like it could be simplified (especially
since I get the feeling it was written out of a mistaken intention to
capture exactly what the SEARCH SINCE command is doing); defining
"acceptable age" as time > now - maxage*24*60*60 makes more intuitive
sense, but this cutoff might change over the course of offlineimap's
run, if things take multiple seconds.


Another thing that I really want to clarify is the place in Base.py
where the deletion occurs:

def __syncmessagesto_delete(self, dstfolder, statusfolder):
	...
    deletelist = filter(lambda uid: uid>=0 \
                            and not self.uidexists(uid),
                        statusfolder.getmessageuidlist())
		...
        self.ui.deletingmessages(deletelist, [dstfolder])
		...
        for folder in [statusfolder, dstfolder]:
            folder.deletemessages(deletelist)

So deletelist = messages in statusfolder that aren't in self's
messagelist. Immediately afterwards, dstfolder.deletemessages() ends
up failing to delete messages that aren't in dstfolder's messagelist
(e.g. messages that are excluded because of maxage), but the "deleting
blah" message is printed before those messages are excluded.  It would
be clearer, both to the user and the reader of code, to make
deletelist = messages in statusfolder that aren't in self.messagelist
but *are* in dstfolder.messagelist. But then the statusfolder isn't
updated, which is bad (?) because now it contains messages that were
excluded because of maxage. So I was going to change it to

deletelist = filter(lambda uid: uid>=0 \
                        and not self.uidexists(uid),
                    statusfolder.getmessageuidlist())
	...
	# delete messages from statusfolder that are either deleted, or
	# not being tracked because of maxage
	statusfolder.deletemessages(deletelist)
	deletelist = filter(lambda uid: dstfolder.uidexists(uid))
		self.ui.deletingmessages(deletelist, [dstfolder])
		dstfolder.deletemessages(deletelist)

but I'm not really confident that this is actually an improvement.

Also, even though I think messages are no longer being deleted when
they shouldn't be, I can still cause messages to get copied when they
already exist in the destination... I'll try to look into that some
more.


-- J.M.