Merge "Fix a bug in submodule topological projects"

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

@@ -533,4 +533,56 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
         .isFalse();
     assertThat(hasSubmodule(subRepo, "dev", "super-project")).isFalse();
   }
+
+  @Test
+  public void testProjectNoSubscriptionWholeTopic() 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");
+
+    // create a change for master branch in repo a
+    ObjectId aHead =
+        pushChangeTo(repoA, "refs/for/master", "master.txt", "content master A",
+            "some message in a master.txt", "same-topic");
+
+    // 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 a
+    repoA.reset(a0);
+    ObjectId aDevHead =
+        pushChangeTo(repoA, "refs/for/dev", "dev.txt", "content dev A",
+            "some message in a dev.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(repoA, aHead).get());
+    approve(getChangeId(repoB, bHead).get());
+    approve(getChangeId(repoA, aDevHead).get());
+    approve(getChangeId(repoB, bDevHead).get());
+
+    gApi.changes().id(getChangeId(repoA, aDevHead).get()).current().submit();
+    assertThat(
+        getRemoteHead(name("project-a"), "refs/heads/master").getShortMessage())
+        .contains("some message in a master.txt");
+    assertThat(
+        getRemoteHead(name("project-a"), "refs/heads/dev").getShortMessage())
+        .contains("some message in a dev.txt");
+    assertThat(
+        getRemoteHead(name("project-b"), "refs/heads/master").getShortMessage())
+        .contains("some message in b master.txt");
+    assertThat(
+        getRemoteHead(name("project-b"), "refs/heads/dev").getShortMessage())
+        .contains("some message in b dev.txt");
+  }
 }

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

@@ -85,16 +85,6 @@ public class ChangeSet {
     return changeData;
   }
 
-  public SetMultimap<Project.NameKey, Branch.NameKey> branchesByProject()
-      throws OrmException {
-    SetMultimap<Project.NameKey, Branch.NameKey> ret =
-        HashMultimap.create();
-    for (ChangeData cd : changeData.values()) {
-      ret.put(cd.change().getProject(), cd.change().getDest());
-    }
-    return ret;
-  }
-
   public Multimap<Branch.NameKey, ChangeData> changesByBranch()
       throws OrmException {
     ListMultimap<Branch.NameKey, ChangeData> ret =

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

@@ -456,23 +456,19 @@ public class MergeOp implements AutoCloseable {
         "cannot integrate hidden changes into history");
     logDebug("Beginning merge attempt on {}", cs);
     Map<Branch.NameKey, BranchBatch> toSubmit = new HashMap<>();
-    logDebug("Perform the merges");
 
-    Multimap<Project.NameKey, Branch.NameKey> br;
     Multimap<Branch.NameKey, ChangeData> cbb;
     try {
-      br = cs.branchesByProject();
       cbb = cs.changesByBranch();
     } catch (OrmException e) {
       throw new IntegrationException("Error reading changes to submit", e);
     }
-    Set<Project.NameKey> projects = br.keySet();
     Set<Branch.NameKey> branches = cbb.keySet();
-    openRepos(projects);
-
     for (Branch.NameKey branch : branches) {
-      OpenRepo or = orm.getRepo(branch.getParentKey());
+      OpenRepo or = openRepo(branch.getParentKey());
-      toSubmit.put(branch, validateChangeList(or, cbb.get(branch)));
+      if (or != null) {
+        toSubmit.put(branch, validateChangeList(or, cbb.get(branch)));
+      }
     }
     // Done checks that don't involve running submit strategies.
     commits.maybeFailVerbose();
@@ -480,12 +476,7 @@ public class MergeOp implements AutoCloseable {
     try {
       List<SubmitStrategy> strategies = getSubmitStrategies(toSubmit,
           submoduleOp, dryrun);
-      Set<Project.NameKey> allProjects = submoduleOp.getProjectsInOrder();
+      this.allProjects = submoduleOp.getProjectsInOrder();
-      // in case superproject subscription is disabled, allProjects would be null
-      if (allProjects == null) {
-        allProjects = projects;
-      }
-      this.allProjects = allProjects;
       BatchUpdate.execute(orm.batchUpdates(allProjects),
           new SubmitStrategyListener(submitInput, strategies, commits),
           submissionId, dryrun);
@@ -521,11 +512,6 @@ public class MergeOp implements AutoCloseable {
       boolean dryrun) throws IntegrationException {
     List<SubmitStrategy> strategies = new ArrayList<>();
     Set<Branch.NameKey> allBranches = submoduleOp.getBranchesInOrder();
-    // in case superproject subscription is disabled, allBranches would be null
-    if (allBranches == null) {
-      allBranches = toSubmit.keySet();
-    }
-
     for (Branch.NameKey branch : allBranches) {
       OpenRepo or = orm.getRepo(branch.getParentKey());
       if (toSubmit.containsKey(branch)) {
@@ -747,19 +733,18 @@ public class MergeOp implements AutoCloseable {
     }
   }
 
-  private void openRepos(Collection<Project.NameKey> projects)
+  private OpenRepo openRepo(Project.NameKey project)
       throws IntegrationException {
-    for (Project.NameKey project : projects) {
+    try {
-      try {
+      return orm.openRepo(project);
-        orm.openRepo(project);
+    } catch (NoSuchProjectException noProject) {
-      } catch (NoSuchProjectException noProject) {
+      logWarn("Project " + noProject.project() + " no longer exists, "
-        logWarn("Project " + noProject.project() + " no longer exists, "
+          + "abandoning open changes");
-            + "abandoning open changes");
+      abandonAllOpenChangeForDeletedProject(noProject.project());
-        abandonAllOpenChangeForDeletedProject(noProject.project());
+    } catch (IOException e) {
-      } catch (IOException e) {
+      throw new IntegrationException("Error opening project " + project, e);
-        throw new IntegrationException("Error opening project " + project, e);
-      }
     }
+    return null;
   }
 
   private void abandonAllOpenChangeForDeletedProject(

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

@@ -105,6 +105,7 @@ public class SubmoduleOp {
   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;
@@ -131,6 +132,7 @@ public class SubmoduleOp {
     this.orm = orm;
     this.updatedBranches = updatedBranches;
     this.targets = HashMultimap.create();
+    this.affectedBranches = new HashSet<>();
     this.branchTips = new HashMap<>();
     this.branchGitModules = new HashMap<>();
     this.sortedBranches = calculateSubscriptionMap();
@@ -154,8 +156,11 @@ public class SubmoduleOp {
           allVisited);
     }
 
-    // Since the searchForSuperprojects will add the superprojects before one
+    // Since the searchForSuperprojects will add all branches (related or
-    // submodule in sortedBranches, need reverse the order of it
+    // unrelated) and ensure the superproject's branches get added first before
+    // a submodule branch. Need remove all unrelated branches and reverse
+    // the order.
+    allVisited.retainAll(affectedBranches);
     reverse(allVisited);
     return ImmutableSet.copyOf(allVisited);
   }
@@ -184,6 +189,8 @@ public class SubmoduleOp {
         Branch.NameKey superProject = sub.getSuperProject();
         searchForSuperprojects(superProject, currentVisited, allVisited);
         targets.put(superProject, sub);
+        affectedBranches.add(superProject);
+        affectedBranches.add(sub.getSubmodule());
       }
     } catch (IOException e) {
       throw new SubmoduleException("Cannot find superprojects for " + current,
@@ -549,30 +556,36 @@ public class SubmoduleOp {
 
   public ImmutableSet<Project.NameKey> getProjectsInOrder()
       throws SubmoduleException {
-    if (sortedBranches == null) {
-      return null;
-    }
-
     LinkedHashSet<Project.NameKey> projects = new LinkedHashSet<>();
-    Project.NameKey prev = null;
+    if (sortedBranches != null) {
-    for (Branch.NameKey branch : sortedBranches) {
+      Project.NameKey prev = null;
-      Project.NameKey project = branch.getParentKey();
+      for (Branch.NameKey branch : sortedBranches) {
-      if (!project.equals(prev)) {
+        Project.NameKey project = branch.getParentKey();
-        if (projects.contains(project)) {
+        if (!project.equals(prev)) {
-          throw new SubmoduleException(
+          if (projects.contains(project)) {
-              "Project level circular subscriptions detected:  " +
+            throw new SubmoduleException(
-                  printCircularPath(projects, project));
+                "Project level circular subscriptions detected:  " +
+                    printCircularPath(projects, project));
+          }
+          projects.add(project);
         }
-        projects.add(project);
+        prev = project;
       }
-      prev = project;
     }
 
+    for (Branch.NameKey branch : updatedBranches) {
+      projects.add(branch.getParentKey());
+    }
     return ImmutableSet.copyOf(projects);
   }
 
   public ImmutableSet<Branch.NameKey> getBranchesInOrder() {
-    return sortedBranches;
+    LinkedHashSet<Branch.NameKey> branches = new LinkedHashSet<>();
+    if (sortedBranches != null) {
+      branches.addAll(sortedBranches);
+    }
+    branches.addAll(updatedBranches);
+    return ImmutableSet.copyOf(branches);
   }
 
   public boolean hasSubscription(Branch.NameKey branch) {