[geary/wip/713530-background-sync: 2/6] Ensure the account synchroniser actually does some vector expansion.

[Michael Gratton <mjog src gnome org>]

commit b96888e557d997f51feec88555b41a680214e9f8
Author: Michael James Gratton <mike vee net>
Date:   Fri Dec 1 12:07:47 2017 +1100

    Ensure the account synchroniser actually does some vector expansion.
    
    * src/engine/imap-engine/imap-engine-account-synchronizer.vala
      (AccountSynchronizer::sync_folder_async): For the last-ditch sync,
      ensure we list newest-to-oldest and for int.MAX count to make sure the
      folder is fully expanded.
    
    * src/engine/imap-engine/replay-ops/imap-engine-abstract-list-email.vala
      (AbstractListEmail): Update doc comments,
      (AbstractListEmail::expand_vector_async): Document, re-work to simplify
      the implementation and make it a bit more obvious how and why the lower
      and higher bounds are calculated, and make sure they conform to the API
      docs and use by existing call sites.
    
    * src/engine/imap/api/imap-folder.vala (Account::uid_to_position_async):
      Make caller's life easier by throwing an error if the results from the
      server are obviously bogus and never return null.

 .../imap-engine-account-synchronizer.vala          |   17 ++-
 .../imap-engine-abstract-list-email.vala           |  139 ++++++++++---------
 src/engine/imap/api/imap-folder.vala               |   38 ++++--
 3 files changed, 113 insertions(+), 81 deletions(-)
---
diff --git a/src/engine/imap-engine/imap-engine-account-synchronizer.vala 
b/src/engine/imap-engine/imap-engine-account-synchronizer.vala
index 085f0ef..522313d 100644
--- a/src/engine/imap-engine/imap-engine-account-synchronizer.vala
+++ b/src/engine/imap-engine/imap-engine-account-synchronizer.vala
@@ -400,8 +400,21 @@ private class Geary.ImapEngine.AccountSynchronizer : Geary.BaseObject {
                         remote_count
                     );
 
-                    yield folder.list_email_by_id_async(null, 1, Geary.Email.Field.NONE,
-                        Geary.Folder.ListFlags.OLDEST_TO_NEWEST, cancellable);
+                    // Per the contract for list_email_by_id_async, we
+                    // need to specify int.MAX count and ensure that
+                    // ListFlags.OLDEST_TO_NEWEST is *not* specified
+                    // to get all messages listed.
+                    //
+                    // XXX This is expensive, but should only usually
+                    // happen once per folder - at the end of a full
+                    // sync.
+                    yield folder.list_email_by_id_async(
+                        null,
+                        int.MAX,
+                        Geary.Email.Field.NONE,
+                        Geary.Folder.ListFlags.NONE,
+                        cancellable
+                    );
                 } else {
                     // don't go past proscribed epoch
                     if (current_epoch.compare(epoch) < 0)
diff --git a/src/engine/imap-engine/replay-ops/imap-engine-abstract-list-email.vala 
b/src/engine/imap-engine/replay-ops/imap-engine-abstract-list-email.vala
index 22056d2..4ce74be 100644
--- a/src/engine/imap-engine/replay-ops/imap-engine-abstract-list-email.vala
+++ b/src/engine/imap-engine/replay-ops/imap-engine-abstract-list-email.vala
@@ -4,6 +4,9 @@
  * (version 2.1 or later).  See the COPYING file in this distribution.
  */
 
+/**
+ * A base class for building replay operations that list messages.
+ */
 private abstract class Geary.ImapEngine.AbstractListEmail : Geary.ImapEngine.SendReplayOperation {
     private static int total_fetches_avoided = 0;
     
@@ -187,7 +190,10 @@ private abstract class Geary.ImapEngine.AbstractListEmail : Geary.ImapEngine.Sen
         
         return ReplayOperation.Status.COMPLETED;
     }
-    
+
+    /**
+     * Determines if the owning folder's vector is fully expanded.
+     */
     protected async Trillian is_fully_expanded_async() throws Error {
         int remote_count;
         owner.get_remote_counts(out remote_count, null);
@@ -204,16 +210,29 @@ private abstract class Geary.ImapEngine.AbstractListEmail : Geary.ImapEngine.Sen
         
         return Trillian.from_boolean(local_count_with_marked >= remote_count);
     }
-    
-    // Adds everything in the expansion to the unfulfilled set with ImapDB's field requirements ...
-    // UIDs are returned if anything else needs to be added to them
+
+    /**
+     * Expands the owning folder's vector.
+     *
+     * Lists on the remote messages needed to fulfill ImapDB's
+     * requirements from `initial_uid` (inclusive) forward to the
+     * start of the vector if the OLDEST_TO_NEWEST flag is set, else
+     * from `initial_uid` (inclusive) back at most by `count` number
+     * of messages. If `initial_uid` is null, the start or end of the
+     * remote is used, respectively.
+     *
+     * The returned UIDs are those added to the vector, which can then
+     * be examined and added to the messages to be fulfilled if
+     * needed.
+     */
     protected async Gee.Set<Imap.UID>? expand_vector_async(Imap.UID? initial_uid, int count) throws Error {
+        debug("%s: expanding vector...", owner.to_string());
         // watch out for situations where the entire folder is represented locally (i.e. no
         // expansion necessary)
         int remote_count = owner.get_remote_counts(null, null);
-        if (remote_count < 0)
+        if (remote_count <= 0)
             return null;
-        
+
         // include marked for removed in the count in case this is being called while a removal
         // is in process, in which case don't want to expand vector this moment because the
         // vector is in flux
@@ -223,74 +242,62 @@ private abstract class Geary.ImapEngine.AbstractListEmail : Geary.ImapEngine.Sen
         // watch out for attempts to expand vector when it's expanded as far as it will go
         if (local_count >= remote_count)
             return null;
-        
-        // determine low and high position for expansion ... default in most code paths for high
-        // is the SequenceNumber just below the lowest known message, unless no local messages
-        // are present
-        Imap.SequenceNumber? low_pos = null;
-        Imap.SequenceNumber? high_pos = null;
-        if (local_count > 0)
-            high_pos = new Imap.SequenceNumber(Numeric.int_floor(remote_count - local_count, 1));
-        
+
+        // Determine low and high position for expansion. The vector
+        // start position is based on the assumption that the vector
+        // end is the same as the remote end.
+        int64 vector_start = (remote_count - local_count + 1);
+        int64 low_pos = -1;
+        int64 high_pos = -1;
+        int64 initial_pos = -1;
+
+        if (initial_uid != null) {
+            Gee.Map<Imap.UID, Imap.SequenceNumber>? map =
+            yield owner.remote_folder.uid_to_position_async(
+                new Imap.MessageSet.uid(initial_uid), cancellable
+            );
+            Imap.SequenceNumber? pos = map.get(initial_uid);
+            if (pos != null) {
+                initial_pos = pos.value;
+            }
+        }
+
+        // Determine low and high position for expansion
         if (flags.is_oldest_to_newest()) {
-            if (initial_uid == null) {
-                // if oldest to newest and initial-id is null, then start at the bottom
-                low_pos = new Imap.SequenceNumber(Imap.SequenceNumber.MIN);
-            } else {
-                Gee.Map<Imap.UID, Imap.SequenceNumber>? map = yield 
owner.remote_folder.uid_to_position_async(
-                    new Imap.MessageSet.uid(initial_uid), cancellable);
-                if (map == null || map.size == 0 || !map.has_key(initial_uid)) {
-                    debug("%s: Unable to expand vector for initial_uid=%s: unable to convert to position",
-                        to_string(), initial_uid.to_string());
-                    
-                    return null;
-                }
-                
-                low_pos = map.get(initial_uid);
+            low_pos = Imap.SequenceNumber.MIN;
+            if (initial_pos > Imap.SequenceNumber.MIN) {
+                low_pos = initial_pos;
             }
+            high_pos = vector_start - 1;
         } else {
-            // newest to oldest
-            //
-            // if initial_id is null or no local earliest UID, then vector expansion is simple:
-            // merely count backwards from the top of the locally available vector
-            if (initial_uid == null || local_count == 0) {
-                low_pos = new Imap.SequenceNumber(Numeric.int_floor((remote_count - local_count) - count, 
1));
-                
-                // don't set high_pos, leave null to use symbolic "highest" in MessageSet
-                high_pos = null;
+            // Newest to oldest.
+            if (initial_pos <= Imap.SequenceNumber.MIN) {
+                high_pos = remote_count;
+                low_pos = Numeric.int64_floor(
+                    high_pos - count + 1, Imap.SequenceNumber.MIN
+                );
             } else {
-                // not so simple; need to determine the *remote* position of the earliest local
-                // UID and count backward from that; if no UIDs present, then it's as if no initial_id
-                // is specified.
-                //
-                // low position: count backwards; note that it's possible this will overshoot and
-                // pull in more email than technically required, but without a round-trip to the
-                // server to determine the position number of a particular UID, this makes sense
-                assert(high_pos != null);
-                low_pos = new Imap.SequenceNumber(
-                    Numeric.int64_floor((high_pos.value - count) + 1, 1));
+                high_pos = Numeric.int64_floor(
+                    initial_pos, vector_start - 1
+                );
+                low_pos = Numeric.int64_floor(
+                    initial_pos - (count - 1), Imap.SequenceNumber.MIN
+                );
             }
         }
-        
-        // low_pos must be defined by this point
-        assert(low_pos != null);
-        
-        if (high_pos != null && low_pos.value > high_pos.value) {
-            debug("%s: Aborting vector expansion, low_pos=%s > high_pos=%s", owner.to_string(),
-                low_pos.to_string(), high_pos.to_string());
-            
+
+        if (low_pos > high_pos) {
+            debug("%s: Aborting vector expansion, low_pos=%lld > high_pos=%lld",
+                  owner.to_string(), low_pos, high_pos);
             return null;
         }
-        
-        Imap.MessageSet msg_set;
-        int64 actual_count = -1;
-        if (high_pos != null) {
-            msg_set = new Imap.MessageSet.range_by_first_last(low_pos, high_pos);
-            actual_count = (high_pos.value - low_pos.value) + 1;
-        } else {
-            msg_set = new Imap.MessageSet.range_to_highest(low_pos);
-        }
-        
+
+        Imap.MessageSet msg_set = new Imap.MessageSet.range_by_first_last(
+            new Imap.SequenceNumber(low_pos),
+            new Imap.SequenceNumber(high_pos)
+        );
+        int64 actual_count = (high_pos - low_pos) + 1;
+
         debug("%s: Performing vector expansion using %s for initial_uid=%s count=%d actual_count=%s 
local_count=%d remote_count=%d oldest_to_newest=%s",
             owner.to_string(), msg_set.to_string(),
             (initial_uid != null) ? initial_uid.to_string() : "(null)", count, actual_count.to_string(),
diff --git a/src/engine/imap/api/imap-folder.vala b/src/engine/imap/api/imap-folder.vala
index 368e594..c0bd773 100644
--- a/src/engine/imap/api/imap-folder.vala
+++ b/src/engine/imap/api/imap-folder.vala
@@ -590,27 +590,39 @@ private class Geary.Imap.Folder : BaseObject {
         
         return (email_list.size > 0) ? email_list : null;
     }
-    
-    public async Gee.Map<UID, SequenceNumber>? uid_to_position_async(MessageSet msg_set,
-        Cancellable? cancellable) throws Error {
+
+    /**
+     * Returns the sequence numbers for a set of UIDs.
+     *
+     * The `msg_set` parameter must be a set containing UIDs. An error
+     * is thrown if the sequence numbers cannot be determined.
+     */
+    public async Gee.Map<UID, SequenceNumber> uid_to_position_async(MessageSet msg_set,
+                                                                    Cancellable? cancellable)
+        throws Error {
         check_open();
         
-        // MessageSet better be UID addressing
-        assert(msg_set.is_uid);
+        if (!msg_set.is_uid) {
+            throw new ImapError.NOT_SUPPORTED("Message set must contain UIDs");
+        }
         
         Gee.List<Command> cmds = new Gee.ArrayList<Command>();
         cmds.add(new FetchCommand.data_type(msg_set, FetchDataSpecifier.UID));
         
         Gee.HashMap<SequenceNumber, FetchedData>? fetched;
         yield exec_commands_async(cmds, out fetched, null, cancellable);
-        
-        if (fetched == null || fetched.size == 0)
-            return null;
-        
-        Gee.Map<UID, SequenceNumber> map = new Gee.HashMap<UID, SequenceNumber>();
-        foreach (SequenceNumber seq_num in fetched.keys)
-            map.set((UID) fetched.get(seq_num).data_map.get(FetchDataSpecifier.UID), seq_num);
-        
+
+        if (fetched == null || fetched.is_empty) {
+            throw new ImapError.INVALID("Server returned no sequence numbers");
+        }
+
+        Gee.Map<UID,SequenceNumber> map = new Gee.HashMap<UID,SequenceNumber>();
+        foreach (SequenceNumber seq_num in fetched.keys) {
+            map.set(
+                (UID) fetched.get(seq_num).data_map.get(FetchDataSpecifier.UID),
+                seq_num
+            );
+        }
         return map;
     }