<DKIM> [PATCH 1/2] decouple utime_from_header from rtime

[Nicolas Sebrecht]

On Thu, Apr 02, 2015 at 02:41:47PM +0200, Abdo Roig-Maranges wrote:
> We were using rtime for two different purposes:
> - to store remote internal date
> - to use in the utime_from_header option
> 
> Let's decouple the utime_from_header logic from rtime, now rtime means
> remote internal date.
> 
> Signed-off-by: Abdo Roig-Maranges <abdo.roig at gmail.com>
> ---
>  offlineimap/folder/Base.py    | 6 ------
>  offlineimap/folder/Maildir.py | 7 +++++--
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/offlineimap/folder/Base.py b/offlineimap/folder/Base.py
> index 16b5819..5d81af4 100644
> --- a/offlineimap/folder/Base.py
> +++ b/offlineimap/folder/Base.py
> @@ -692,9 +692,6 @@ class BaseFolder(object):
>              message = None
>              flags = self.getmessageflags(uid)
>              rtime = self.getmessagetime(uid)
> -            if dstfolder.utime_from_header:
> -                content = self.getmessage(uid)
> -                rtime = emailutil.get_message_date(content, 'Date')
>  
>              # If any of the destinations actually stores the message body,
>              # load it up.
> @@ -766,9 +763,6 @@ class BaseFolder(object):
>                  # dst has message with that UID already, only update status
>                  flags = self.getmessageflags(uid)
>                  rtime = self.getmessagetime(uid)
> -                if dstfolder.utime_from_header:
> -                    content = self.getmessage(uid)
> -                    rtime = emailutil.get_message_date(content, 'Date')
>                  statusfolder.savemessage(uid, None, flags, rtime)
>                  continue

So, rtime is not anymore hijacked by the utime_from_header thing.

But by doing so, we are also _forbidding_ the possibility to fix the
remote INTERNALDATE (with the IMAP APPEND command) from what we've found
in the header.

I can't think of a use case where this hurts and consider this to come
from a wrong implementation (the rtime hijacking by Cyril).

It's fine because it was not documented and I expect few impacts on the
users. We still have to keep this in mind.

>  
> diff --git a/offlineimap/folder/Maildir.py b/offlineimap/folder/Maildir.py
> index 79c34a7..f1b8e8e 100644
> --- a/offlineimap/folder/Maildir.py
> +++ b/offlineimap/folder/Maildir.py
> @@ -324,8 +324,11 @@ class MaildirFolder(BaseFolder):
>          tmpdir = os.path.join(self.getfullname(), 'tmp')
>          messagename = self.new_message_filename(uid, flags)
>          tmpname = self.save_to_tmp_file(messagename, content)
> -        if rtime != None:
> -            os.utime(os.path.join(self.getfullname(), tmpname), (rtime, rtime))
> +
> +        if self.utime_from_header:
> +            date = emailutil.get_message_date(content, 'Date')
> +            if date != None:
> +                os.utime(os.path.join(self.getfullname(), tmpname), (date, date))
>  
>          self.messagelist[uid] = self.msglist_item_initializer(uid)
>          self.messagelist[uid]['flags'] = flags

I'm not entirely conviced by this last hunk.

When rtime was introduced, it really was to fix the modification file
times. The filename was not affected. This allowed John to feed Dovecot
directly with the OfflineIMAP Maildir, behind Dovecot's back. Let's call
this configuration "direct feeding".

This was at the time IMAP/IMAP didn't exist, yet. Today, I think direct
feeding configurations are just wrong and users should migrate from
their direct feeding to IMAP/IMAP, instead. I expect there's a lot of
direct feeding configurations out there because this is how it was
implemented for a long time and probably what users were told to do.

Since recent 25513e90387f6, Janna and me decided to put rtime
(INTERNALDATE) in the filename rather than use the modification file
time. But this was while fixing maxage in mind, only. By doing so, we
ARE breaking all existing direct feeding configurations.

IOW, the quick sync mode is used with WRONG assumptions of what the
modification file time really means FOR YEARS. This was mainly not
hurting because INTERNALDATE are always expected older than the download
date times of a sync.

What you are doing with this last hunk is not only decouple
utime_from_header from rtime but also FIX the modification file times to
reflect only what it should be: last modification time of the file, as
expected for quick syncs.

Not that it's incorrect FMPOV, but:
- the commit message should talk about that, at least;
- we actually have to explain to the users WHY they should avoid direct
  feeding and migrate to IMAP/IMAP configurations.

-- 
Nicolas Sebrecht