sync: Loop committing items without downloading updates (v2)

sync/engine/get_commit_ids_command.h

Index: sync/engine/get_commit_ids_command.h
diff --git a/sync/engine/get_commit_ids_command.h b/sync/engine/get_commit_ids_command.h
index 334fb6b9d406fd8f8d87c62c41e3c8299ec65e48..09a6f9d368bb5c7c5d2aa6b6ec0a000d09015224 100644
--- a/sync/engine/get_commit_ids_command.h
+++ b/sync/engine/get_commit_ids_command.h
@@ -20,11 +20,32 @@ using std::vector;
 
 namespace browser_sync {
 
+// A class that contains the code used to search the syncable::Directory for
+// locally modified items that are ready to be committed to the server.
+//
+// This class, like all SyncerCommands, has a lot more in common with a function
+// than a class.  It expects to be instantiated, have its ExecuteImpl() function

[tim (not reviewing), 2012/05/15 22:25:00]

The instantiate / execute / cleanup pattern is characteristic of SyncerCommand
and documented there with an example usage snippet. We shouldn't duplicate this
comment everywhere (or worse, only in some cases and not others). 

Feel free to augment the comment in SyncerCommand, but don't add / copy this
around here.  Also see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=File_C...,
the "duplicate comments diverge" part applies in spirit.

+// called, then be destroyed.
 class GetCommitIdsCommand : public SyncerCommand {
   friend class SyncerTest;
 
  public:
-  explicit GetCommitIdsCommand(int commit_batch_size);
+  // Because it must conform to the interface defined in SyncerCommand, this

[tim (not reviewing), 2012/05/15 22:25:00]

Constructors are permitted to have parameters. The defensive perspective of this
comment is pretty awkward / confusing. It doesn't provide functional benefit to
a reader over individual descriptions for batch_size and ordered_commit_set, and
just talks about implementation details that don't concern the caller. I think
it's best to remove it, or at least make it un-defensive / work-aroundy sounding
to reduce undue confusion. See also
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
- it's a stretch to say it applies here, but the "people reading the code know
what ctors are for" is partially relevant in my mind. 

+  // class's ExecuteImpl() function can't receive input parameters or output any
+  // return values other than a SyncerError.  To work around this limitation, we
+  // pass in parameters to the constructor and save them for use when
+  // ExecuteImpl() is eventually called.  There will be one ExecuteImpl() call
+  // per instantiation.
+  //
+  // The batch_size parameter is the maximum number of entries we are allowed
+  // to commit in a single batch.  This value can be modified by the server.
+  //
+  // The ordered_commit_set parameter is an output parameter that will contain a
+  // set of items that are ready to commit.  Its size shall not exceed the
+  // provided batch_size.  This contents of this "set" will be ordered; see the
+  // comments in this class' implementation for details.
+  GetCommitIdsCommand(const size_t commit_batch_size,
+                      sessions::OrderedCommitSet* ordered_commit_set);
   virtual ~GetCommitIdsCommand();
 
   // SyncerCommand implementation.
@@ -93,8 +114,7 @@ class GetCommitIdsCommand : public SyncerCommand {
                                sessions::OrderedCommitSet* result) const;
 
   // Appends all commit ready predecessors of |item|, followed by |item| itself,
-  // to |ordered_commit_set_|, iff item and all its predecessors not in
-  // conflict.
+  // to |commit_set|, iff item and all its predecessors not in conflict.
   // Return values:
   //   False: if there was an entry in conflict.
   //   True: if all entries were checked for commit readiness and added to
@@ -103,7 +123,7 @@ class GetCommitIdsCommand : public SyncerCommand {
                                const ModelSafeRoutingInfo& routes,
                                const std::set<int64>& ready_unsynced_set,
                                const syncable::Entry& item,
-                               sessions::OrderedCommitSet* result) const;
+                               sessions::OrderedCommitSet* commit_set) const;
 
   bool IsCommitBatchFull() const;
 
@@ -114,9 +134,11 @@ class GetCommitIdsCommand : public SyncerCommand {
   void AddDeletes(syncable::WriteTransaction* write_transaction,
                   const std::set<int64>& ready_unsynced_set);
 
-  scoped_ptr<sessions::OrderedCommitSet> ordered_commit_set_;
+  // Input parameter; see constructor comment.
+  const size_t requested_commit_batch_size_;
 
-  int requested_commit_batch_size_;
+  // Output parameter; see constructor comment.
+  sessions::OrderedCommitSet* commit_set_;
 
   DISALLOW_COPY_AND_ASSIGN(GetCommitIdsCommand);
 };