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 893c1b6db0fb10ff6bfd19b5b6bcb7f508c6d9fe)
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 e9e79e1..e414e63 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
@@ -273,7 +273,7 @@
}
@Test
- public void testDifferentPaths() throws Exception {
+ public void testSameProjectSameBranchDifferentPaths() throws Exception {
TestRepository<?> superRepo = createProjectWithPush("super-project");
TestRepository<?> sub = createProjectWithPush("sub");
@@ -306,6 +306,50 @@
}
@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");
TestRepository<?> sub = createProjectWithPush("sub");
@@ -535,4 +579,42 @@
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");
+ }
}
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 8c1b3d0..470ea84 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
@@ -103,13 +103,21 @@
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 @@
this.affectedBranches = new HashSet<>();
this.branchTips = new HashMap<>();
this.branchGitModules = new HashMap<>();
+ this.branchesByProject = HashMultimap.create();
this.sortedBranches = calculateSubscriptionMap();
}
@@ -186,10 +195,11 @@
Collection<SubmoduleSubscription> subscriptions =
superProjectSubscriptionsForSubmoduleBranch(current);
for (SubmoduleSubscription sub : subscriptions) {
- Branch.NameKey superProject = sub.getSuperProject();
- searchForSuperprojects(superProject, currentVisited, allVisited);
- targets.put(superProject, sub);
- affectedBranches.add(superProject);
+ Branch.NameKey superBranch = sub.getSuperProject();
+ searchForSuperprojects(superBranch, currentVisited, allVisited);
+ targets.put(superBranch, sub);
+ branchesByProject.put(superBranch.getParentKey(), superBranch);
+ affectedBranches.add(superBranch);
affectedBranches.add(sub.getSubmodule());
}
} catch (IOException e) {
@@ -326,16 +336,15 @@
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 @@
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) {
- 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);
- }
- prev = project;
- }
+ for (Project.NameKey project : branchesByProject.keySet()) {
+ addAllSubmoduleProjects(project, new LinkedHashSet<Project.NameKey>(), projects);
}
for (Branch.NameKey branch : updatedBranches) {
@@ -579,6 +567,37 @@
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) {
