Fix project ordering bug in submodule subscription

The initial idea for sorting project is relying on the sorted branches.
However, sorted branches does not always guarantee a valid order for
projects. For example:

Scenario: super.master -> sub.master(C1) and super.dev -> sub.dev(C2)
When change C1 and C2 are submitted together, there are multiple
possible sorted branches orders, here are two example.

1. sub.master - super.master - sub.dev - super.dev
2. sub.master - sub.dev - super.master - super.dev

Only #2 can derive a valid project order and #1 will give us a project
circular subscription exception.

Rewrite the project sorting algorithm:
1. find all *superbranches* (branches that have subscriptions) in a
superproject
2. find all submodule projects for these superbranches
Recursively doing 1,2 until there is no superbranches, and add that
project to the sorted project list.

Tests are also added for the example.

Change-Id: Ia8e794f630ce303320c242fb9922d831f502c186
(cherry picked from commit 893c1b6db0)

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java

@@ -273,7 +273,7 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
   }
 
   @Test
-  public void testDifferentPaths() throws Exception {
+  public void testSameProjectSameBranchDifferentPaths() throws Exception {
     TestRepository<?> superRepo = createProjectWithPush("super-project");
     TestRepository<?> sub = createProjectWithPush("sub");
 
@@ -305,6 +305,50 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
         .isEqualTo(superPreviousId);
   }
 
+  @Test
+  public void testSameProjectDifferentBranchDifferentPaths() throws Exception {
+    TestRepository<?> superRepo = createProjectWithPush("super-project");
+    TestRepository<?> sub = createProjectWithPush("sub");
+
+    allowMatchingSubmoduleSubscription("sub", "refs/heads/master",
+        "super-project", "refs/heads/master");
+    allowMatchingSubmoduleSubscription("sub", "refs/heads/dev",
+        "super-project", "refs/heads/master");
+
+    ObjectId devHead = pushChangeTo(sub, "dev");
+    Config config = new Config();
+    prepareSubmoduleConfigEntry(config, "sub", "sub-master", "master");
+    prepareSubmoduleConfigEntry(config, "sub", "sub-dev", "dev");
+    pushSubmoduleConfig(superRepo, "master", config);
+
+    ObjectId subMasterId =
+        pushChangeTo(sub, "refs/for/master", "some message", "b.txt",
+            "content b", "same-topic");
+
+    sub.reset(devHead);
+    ObjectId subDevId =
+        pushChangeTo(sub, "refs/for/dev", "some message in dev", "b.txt",
+            "content b", "same-topic");
+
+    approve(getChangeId(sub, subMasterId).get());
+    approve(getChangeId(sub, subDevId).get());
+
+    ObjectId superPreviousId = pushChangeTo(superRepo, "master");
+
+    gApi.changes().id(getChangeId(sub, subMasterId).get()).current().submit();
+
+    expectToHaveSubmoduleState(superRepo, "master", "sub-master", sub, "master");
+    expectToHaveSubmoduleState(superRepo, "master", "sub-dev", sub, "dev");
+
+    superRepo.git().fetch().setRemote("origin").call()
+        .getAdvertisedRef("refs/heads/master").getObjectId();
+
+    assertWithMessage("submodule subscription update "
+        + "should have made one commit")
+        .that(superRepo.getRepository().resolve("origin/master^"))
+        .isEqualTo(superPreviousId);
+  }
+
   @Test
   public void testNonSubmoduleInSameTopic() throws Exception {
     TestRepository<?> superRepo = createProjectWithPush("super-project");
@@ -535,4 +579,42 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
         getRemoteHead(name("project-b"), "refs/heads/dev").getShortMessage())
         .contains("some message in b dev.txt");
   }
+
+  @Test
+  public void testTwoProjectsMultipleBranchesWholeTopic() throws Exception {
+    TestRepository<?> repoA = createProjectWithPush("project-a");
+    TestRepository<?> repoB = createProjectWithPush("project-b");
+    // bootstrap the dev branch
+    ObjectId a0 = pushChangeTo(repoA, "dev");
+
+    // bootstrap the dev branch
+    ObjectId b0 = pushChangeTo(repoB, "dev");
+
+    allowMatchingSubmoduleSubscription("project-b",
+        "refs/heads/master", "project-a", "refs/heads/master");
+    allowMatchingSubmoduleSubscription("project-b", "refs/heads/dev",
+        "project-a", "refs/heads/dev");
+
+    createSubmoduleSubscription(repoA, "master", "project-b", "master");
+    createSubmoduleSubscription(repoA, "dev", "project-b", "dev");
+
+
+    // create a change for master branch in repo b
+    ObjectId bHead =
+        pushChangeTo(repoB, "refs/for/master", "master.txt", "content master B",
+            "some message in b master.txt", "same-topic");
+
+    // create a change for dev branch in repo b
+    repoB.reset(b0);
+    ObjectId bDevHead =
+        pushChangeTo(repoB, "refs/for/dev", "dev.txt", "content dev B",
+            "some message in b dev.txt", "same-topic");
+
+    approve(getChangeId(repoB, bHead).get());
+    approve(getChangeId(repoB, bDevHead).get());
+    gApi.changes().id(getChangeId(repoB, bHead).get()).current().submit();
+
+    expectToHaveSubmoduleState(repoA, "master", "project-b", repoB, "master");
+    expectToHaveSubmoduleState(repoA, "dev", "project-b", repoB, "dev");
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java

@@ -103,13 +103,21 @@ public class SubmoduleOp {
   private final ProjectState.Factory projectStateFactory;
   private final VerboseSuperprojectUpdate verboseSuperProject;
   private final boolean enableSuperProjectSubscriptions;
-  private final Multimap<Branch.NameKey, SubmoduleSubscription> targets;
-  private final Set<Branch.NameKey> updatedBranches;
-  private final Set<Branch.NameKey> affectedBranches;
   private final MergeOpRepoManager orm;
-  private final Map<Branch.NameKey, CodeReviewCommit> branchTips;
   private final Map<Branch.NameKey, GitModules> branchGitModules;
+
+  // always update-to-current branch tips during submit process
+  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
+  private final Set<Branch.NameKey> affectedBranches;
+  // sorted version of affectedBranches
   private final ImmutableSet<Branch.NameKey> sortedBranches;
+  // map of superproject branch and its submodule subscriptions
+  private final Multimap<Branch.NameKey, SubmoduleSubscription> targets;
+  // map of superproject and its branches which has submodule subscriptions
+  private final SetMultimap<Project.NameKey, Branch.NameKey> branchesByProject;
 
   @AssistedInject
   public SubmoduleOp(
@@ -135,6 +143,7 @@ public class SubmoduleOp {
     this.affectedBranches = new HashSet<>();
     this.branchTips = new HashMap<>();
     this.branchGitModules = new HashMap<>();
+    this.branchesByProject = HashMultimap.create();
     this.sortedBranches = calculateSubscriptionMap();
   }
 
@@ -186,10 +195,11 @@ public class SubmoduleOp {
       Collection<SubmoduleSubscription> subscriptions =
           superProjectSubscriptionsForSubmoduleBranch(current);
       for (SubmoduleSubscription sub : subscriptions) {
-        Branch.NameKey superProject = sub.getSuperProject();
+        Branch.NameKey superBranch = sub.getSuperProject();
-        searchForSuperprojects(superProject, currentVisited, allVisited);
+        searchForSuperprojects(superBranch, currentVisited, allVisited);
-        targets.put(superProject, sub);
+        targets.put(superBranch, sub);
-        affectedBranches.add(superProject);
+        branchesByProject.put(superBranch.getParentKey(), superBranch);
+        affectedBranches.add(superBranch);
         affectedBranches.add(sub.getSubmodule());
       }
     } catch (IOException e) {
@@ -326,16 +336,15 @@ public class SubmoduleOp {
       return;
     }
 
-    SetMultimap<Project.NameKey, Branch.NameKey> dst = branchesByProject();
     LinkedHashSet<Project.NameKey> superProjects = new LinkedHashSet<>();
     try {
       for (Project.NameKey project : projects) {
         // only need superprojects
-        if (dst.containsKey(project)) {
+        if (branchesByProject.containsKey(project)) {
           superProjects.add(project);
           // get a new BatchUpdate for the super project
           OpenRepo or = orm.openRepo(project);
-          for (Branch.NameKey branch : dst.get(project)) {
+          for (Branch.NameKey branch : branchesByProject.get(project)) {
             addOp(or.getUpdate(), branch);
           }
         }
@@ -545,32 +554,11 @@ public class SubmoduleOp {
     return dc;
   }
 
-  public SetMultimap<Project.NameKey, Branch.NameKey> branchesByProject() {
-    SetMultimap<Project.NameKey, Branch.NameKey> ret = HashMultimap.create();
-    for (Branch.NameKey branch : targets.keySet()) {
-      ret.put(branch.getParentKey(), branch);
-    }
-
-    return ret;
-  }
-
   public ImmutableSet<Project.NameKey> getProjectsInOrder()
       throws SubmoduleException {
     LinkedHashSet<Project.NameKey> projects = new LinkedHashSet<>();
-    if (sortedBranches != null) {
+    for (Project.NameKey project : branchesByProject.keySet()) {
-      Project.NameKey prev = null;
+      addAllSubmoduleProjects(project, new LinkedHashSet<Project.NameKey>(), projects);
-      for (Branch.NameKey branch : sortedBranches) {
-        Project.NameKey project = branch.getParentKey();
-        if (!project.equals(prev)) {
-          if (projects.contains(project)) {
-            throw new SubmoduleException(
-                "Project level circular subscriptions detected:  " +
-                    printCircularPath(projects, project));
-          }
-          projects.add(project);
-        }
-        prev = project;
-      }
     }
 
     for (Branch.NameKey branch : updatedBranches) {
@@ -579,6 +567,37 @@ public class SubmoduleOp {
     return ImmutableSet.copyOf(projects);
   }
 
+  private void addAllSubmoduleProjects(Project.NameKey project,
+      LinkedHashSet<Project.NameKey> current,
+      LinkedHashSet<Project.NameKey> projects)
+      throws SubmoduleException {
+    if (current.contains(project)) {
+      throw new SubmoduleException(
+          "Project level circular subscriptions detected:  " +
+              printCircularPath(current, project));
+    }
+
+    if (projects.contains(project)) {
+      return;
+    }
+
+    current.add(project);
+    Set<Project.NameKey> subprojects = new HashSet<>();
+    for (Branch.NameKey branch : branchesByProject.get(project)) {
+      Collection<SubmoduleSubscription> subscriptions = targets.get(branch);
+      for (SubmoduleSubscription s : subscriptions) {
+        subprojects.add(s.getSubmodule().getParentKey());
+      }
+    }
+
+    for (Project.NameKey p : subprojects) {
+      addAllSubmoduleProjects(p, current, projects);
+    }
+
+    current.remove(project);
+    projects.add(project);
+  }
+
   public ImmutableSet<Branch.NameKey> getBranchesInOrder() {
     LinkedHashSet<Branch.NameKey> branches = new LinkedHashSet<>();
     if (sortedBranches != null) {