diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java index d62a27bcd5..43f19b9e80 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java +++ b/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"); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java index 857cbeae39..dcb49c2e69 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java @@ -85,16 +85,6 @@ public class ChangeSet { return changeData; } - public SetMultimap branchesByProject() - throws OrmException { - SetMultimap ret = - HashMultimap.create(); - for (ChangeData cd : changeData.values()) { - ret.put(cd.change().getProject(), cd.change().getDest()); - } - return ret; - } - public Multimap changesByBranch() throws OrmException { ListMultimap ret = diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index 71bc9f457d..a3125a5f34 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/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 toSubmit = new HashMap<>(); - logDebug("Perform the merges"); - Multimap br; Multimap cbb; try { - br = cs.branchesByProject(); cbb = cs.changesByBranch(); } catch (OrmException e) { throw new IntegrationException("Error reading changes to submit", e); } - Set projects = br.keySet(); Set branches = cbb.keySet(); - openRepos(projects); - for (Branch.NameKey branch : branches) { - OpenRepo or = orm.getRepo(branch.getParentKey()); - toSubmit.put(branch, validateChangeList(or, cbb.get(branch))); + OpenRepo or = openRepo(branch.getParentKey()); + 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 strategies = getSubmitStrategies(toSubmit, submoduleOp, dryrun); - Set allProjects = submoduleOp.getProjectsInOrder(); - // in case superproject subscription is disabled, allProjects would be null - if (allProjects == null) { - allProjects = projects; - } - this.allProjects = allProjects; + this.allProjects = submoduleOp.getProjectsInOrder(); 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 strategies = new ArrayList<>(); Set 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 projects) + private OpenRepo openRepo(Project.NameKey project) throws IntegrationException { - for (Project.NameKey project : projects) { - try { - orm.openRepo(project); - } catch (NoSuchProjectException noProject) { - logWarn("Project " + noProject.project() + " no longer exists, " - + "abandoning open changes"); - abandonAllOpenChangeForDeletedProject(noProject.project()); - } catch (IOException e) { - throw new IntegrationException("Error opening project " + project, e); - } + try { + return orm.openRepo(project); + } catch (NoSuchProjectException noProject) { + logWarn("Project " + noProject.project() + " no longer exists, " + + "abandoning open changes"); + abandonAllOpenChangeForDeletedProject(noProject.project()); + } catch (IOException e) { + throw new IntegrationException("Error opening project " + project, e); } + return null; } private void abandonAllOpenChangeForDeletedProject( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java index a1578ce2b5..1421556110 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java +++ b/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 targets; private final Set updatedBranches; + private final Set affectedBranches; private final MergeOpRepoManager orm; private final Map branchTips; private final Map 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 - // submodule in sortedBranches, need reverse the order of it + // Since the searchForSuperprojects will add all branches (related or + // 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 getProjectsInOrder() throws SubmoduleException { - if (sortedBranches == null) { - return null; - } - LinkedHashSet projects = new LinkedHashSet<>(); - Project.NameKey prev = null; - 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)); + if (sortedBranches != null) { + Project.NameKey prev = null; + 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); } - projects.add(project); + prev = project; } - prev = project; } + for (Branch.NameKey branch : updatedBranches) { + projects.add(branch.getParentKey()); + } return ImmutableSet.copyOf(projects); } public ImmutableSet getBranchesInOrder() { - return sortedBranches; + LinkedHashSet branches = new LinkedHashSet<>(); + if (sortedBranches != null) { + branches.addAll(sortedBranches); + } + branches.addAll(updatedBranches); + return ImmutableSet.copyOf(branches); } public boolean hasSubscription(Branch.NameKey branch) {