More clearly document SubmoduleOp setup

* Convert field comments to Javadoc and flesh them out slightly.
* Make updatedBranches immutable, to contrast it with all the mutable
  fields.
* Rename "calculateSubscriptionMap" -> "Maps", and document its side effects.
* Add a TODO with an idea for future cleanup, which is explicitly out of
  scope for this series.

Change-Id: I85c740322f06f39ff66ad4b57e62eeb1a00ddce6

java/com/google/gerrit/server/submit/SubmoduleOp.java

@@ -141,17 +141,31 @@ public class SubmoduleOp {
   private final MergeOpRepoManager orm;
   private final Map<Branch.NameKey, GitModules> branchGitModules;
 
-  // always update-to-current branch tips during submit process
+  /** Branches updated as part of the enclosing submit or push batch. */
+  private final ImmutableSet<Branch.NameKey> updatedBranches;
+
+  /**
+   * Current branch tips, taking into account commits created during the submit process as well as
+   * submodule updates produced by this class.
+   */
   private final Map<Branch.NameKey, CodeReviewCommit> branchTips;
-  // branches for all the submitting changes
-  private final Set<Branch.NameKey> updatedBranches;
-  // branches which in either a submodule or a superproject
+
+  /**
+   * Branches in a superproject that contain submodule subscriptions, plus branches in submodules
+   * which are subscribed to by some superproject.
+   */
   private final Set<Branch.NameKey> affectedBranches;
-  // sorted version of affectedBranches
+
+  /** Copy of {@link #affectedBranches}, sorted by submodule traversal order. */
   private final ImmutableSet<Branch.NameKey> sortedBranches;
-  // map of superproject branch and its submodule subscriptions
+
+  /** Multimap of superproject branch to submodule subscriptions contained in that branch. */
   private final SetMultimap<Branch.NameKey, SubmoduleSubscription> targets;
-  // map of superproject and its branches which has submodule subscriptions
+
+  /**
+   * Multimap of superproject name to all branch names within that superproject which have submodule
+   * subscriptions.
+   */
   private final SetMultimap<Project.NameKey, Branch.NameKey> branchesByProject;
 
   private SubmoduleOp(
@@ -175,16 +189,44 @@ public class SubmoduleOp {
         cfg.getLong("submodule", "maxCombinedCommitMessageSize", 256 << 10);
     this.maxCommitMessages = cfg.getLong("submodule", "maxCommitMessages", 1000);
     this.orm = orm;
-    this.updatedBranches = updatedBranches;
+    this.updatedBranches = ImmutableSet.copyOf(updatedBranches);
     this.targets = MultimapBuilder.hashKeys().hashSetValues().build();
     this.affectedBranches = new HashSet<>();
     this.branchTips = new HashMap<>();
     this.branchGitModules = new HashMap<>();
     this.branchesByProject = MultimapBuilder.hashKeys().hashSetValues().build();
-    this.sortedBranches = calculateSubscriptionMap();
+    this.sortedBranches = calculateSubscriptionMaps();
   }
 
-  private ImmutableSet<Branch.NameKey> calculateSubscriptionMap() throws SubmoduleException {
+  /**
+   * Calculate the internal maps used by the operation.
+   *
+   * <p>In addition to the return value, the following fields are populated as a side effect:
+   *
+   * <ul>
+   *   <li>{@link #affectedBranches}
+   *   <li>{@link #targets}
+   *   <li>{@link #branchesByProject}
+   * </ul>
+   *
+   * @return the ordered set to be stored in {@link #sortedBranches}.
+   * @throws SubmoduleException if an error occurred walking projects.
+   */
+  // TODO(dborowitz): This setup process is hard to follow, in large part due to the accumulation of
+  // mutable maps, which makes this whole class difficult to understand.
+  //
+  // A cleaner architecture for this process might be:
+  //   1. Separate out the code to parse submodule subscriptions and build up an in-memory data
+  //      structure representing the subscription graph, using a separate class with a properly-
+  //      documented interface.
+  //   2. Walk the graph to produce a work plan. This would be a list of items indicating: create a
+  //      commit in project X reading branch tips for submodules S1..Sn and updating gitlinks in X.
+  //   3. Execute the work plan, i.e. convert the items into BatchUpdate.Ops and add them to the
+  //      relevant updates.
+  //
+  // In addition to improving readability, this approach has the advantage of making (1) and (2)
+  // testable using small tests.
+  private ImmutableSet<Branch.NameKey> calculateSubscriptionMaps() throws SubmoduleException {
     if (!enableSuperProjectSubscriptions) {
       logDebug("Updating superprojects disabled");
       return null;