diff options
author | Gabor Somossy <gabor.somossy@gmail.com> | 2014-10-19 15:04:12 +0100 |
---|---|---|
committer | David Pursehouse <david.pursehouse@sonymobile.com> | 2014-11-25 10:25:09 +0900 |
commit | 4894d578d17ed7a60ee1020a2a3a4e1326810968 (patch) | |
tree | 9abe46b1cf442fd948d24c4fb740748a053eb01e | |
parent | 2b2d62b1d1704b639287fc62fe87bedf2cab85bf (diff) |
Fix incorrect submodule subscriptions
The gitlinks update fails after deleting a branch in a super project
which has other branches subscribed to the same submodule branch.
How to reproduce:
1. subscribe two branches in a super project to the same submodule
branch
2. delete one of the branches in the super project
3. push a change to the subscribed submodule branch
Proposed fixes:
Delete submodule subscriptions when deleting a branch via UI or Git wire
protocol.
Delete incorrect subscriptions when the gitlinks update fails in
SubmoduleOp.
Bug: Issue 2989
Change-Id: I91bbdffa0770eb1754efc6cb7f1549e7dab5c3ce
4 files changed, 76 insertions, 34 deletions
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index ef1ca4567d..ef58c2b2d1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -65,6 +65,7 @@ import com.google.gerrit.reviewdb.client.PatchSetInfo; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.client.RevId; +import com.google.gerrit.reviewdb.client.SubmoduleSubscription; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ApprovalCopier; import com.google.gerrit.server.ApprovalsUtil; @@ -599,6 +600,17 @@ public class ReceiveCommits { break; case DELETE: + ResultSet<SubmoduleSubscription> submoduleSubscriptions = null; + Branch.NameKey projRef = new Branch.NameKey(project.getNameKey(), + c.getRefName()); + try { + submoduleSubscriptions = + db.submoduleSubscriptions().bySuperProject(projRef); + db.submoduleSubscriptions().delete(submoduleSubscriptions); + } catch (OrmException e) { + log.error("Cannot delete submodule subscription(s) of branch " + + projRef + ": " + submoduleSubscriptions, e); + } break; } 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 5deb8c34ef..e4e059bfc0 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 @@ -14,6 +14,7 @@ package com.google.gerrit.server.git; +import com.google.common.collect.Lists; import com.google.gerrit.common.ChangeHooks; import com.google.gerrit.common.Nullable; import com.google.gerrit.reviewdb.client.Account; @@ -233,25 +234,38 @@ public class SubmoduleOp { } // update subscribers of this module + List<SubmoduleSubscription> incorrectSubscriptions = Lists.newLinkedList(); for (final SubmoduleSubscription s : subscribers) { - if (!updatedSubscribers.add(s.getSuperProject())) { - log.error("Possible circular subscription involving " - + s.toString()); - } else { - - Map<Branch.NameKey, ObjectId> modules = - new HashMap<Branch.NameKey, ObjectId>(1); - modules.put(updatedBranch, mergedCommit); - - Map<Branch.NameKey, String> paths = - new HashMap<Branch.NameKey, String>(1); - paths.put(updatedBranch, s.getPath()); - - try { + try { + if (!updatedSubscribers.add(s.getSuperProject())) { + log.error("Possible circular subscription involving " + s); + } else { + + Map<Branch.NameKey, ObjectId> modules = + new HashMap<Branch.NameKey, ObjectId>(1); + modules.put(updatedBranch, mergedCommit); + + Map<Branch.NameKey, String> paths = + new HashMap<Branch.NameKey, String>(1); + paths.put(updatedBranch, s.getPath()); updateGitlinks(s.getSuperProject(), myRw, modules, paths, msgbuf); - } catch (SubmoduleException e) { - throw e; } + } catch (SubmoduleException e) { + log.warn("Cannot update gitlinks for " + s + " due to " + e.getMessage()); + incorrectSubscriptions.add(s); + } catch (Exception e) { + log.error("Cannot update gitlinks for " + s, e); + } + } + + if (!incorrectSubscriptions.isEmpty()) { + try { + schema.submoduleSubscriptions().delete(incorrectSubscriptions); + log.info("Deleted incorrect submodule subscription(s) " + + incorrectSubscriptions); + } catch (OrmException e) { + log.error("Cannot delete submodule subscription(s) " + + incorrectSubscriptions, e); } } } @@ -362,8 +376,8 @@ public class SubmoduleOp { // Recursive call: update subscribers of the subscriber updateSuperProjects(subscriber, recRw, commitId, msgbuf.toString()); } catch (IOException e) { - logAndThrowSubmoduleException("Cannot update gitlinks for " - + subscriber.get(), e); + throw new SubmoduleException("Cannot update gitlinks for " + + subscriber.get(), e); } finally { if (recRw != null) { recRw.release(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranch.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranch.java index 87e233f444..041fd4a7ce 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranch.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranch.java @@ -19,12 +19,14 @@ import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestModifyView; +import com.google.gerrit.reviewdb.client.SubmoduleSubscription; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.project.DeleteBranch.Input; import com.google.gwtorm.server.OrmException; +import com.google.gwtorm.server.ResultSet; import com.google.inject.Inject; import com.google.inject.Provider; @@ -90,6 +92,9 @@ public class DeleteBranch implements RestModifyView<BranchResource, Input>{ case FORCED: referenceUpdated.fire(rsrc.getNameKey(), u); hooks.doRefUpdatedHook(rsrc.getBranchKey(), u, identifiedUser.get().getAccount()); + ResultSet<SubmoduleSubscription> submoduleSubscriptions = + dbProvider.get().submoduleSubscriptions().bySuperProject(rsrc.getBranchKey()); + dbProvider.get().submoduleSubscriptions().delete(submoduleSubscriptions); break; case REJECTED_CURRENT_BRANCH: diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java index 7dc6bf9fbf..6de5c61306 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java @@ -14,13 +14,10 @@ package com.google.gerrit.server.git; +import static org.easymock.EasyMock.anyObject; import static org.easymock.EasyMock.capture; -import static org.easymock.EasyMock.createNiceMock; -import static org.easymock.EasyMock.createStrictMock; import static org.easymock.EasyMock.eq; import static org.easymock.EasyMock.expect; -import static org.easymock.EasyMock.replay; -import static org.easymock.EasyMock.verify; import static org.junit.Assert.assertEquals; import com.google.gerrit.common.ChangeHooks; @@ -41,6 +38,8 @@ import com.google.gwtorm.server.StandardKeyEncoder; import com.google.inject.Provider; import org.easymock.Capture; +import org.easymock.EasyMock; +import org.easymock.IMocksControl; import org.eclipse.jgit.api.Git; import org.eclipse.jgit.dircache.DirCacheBuilder; import org.eclipse.jgit.dircache.DirCacheEntry; @@ -75,6 +74,7 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { private static final String newLine = System.getProperty("line.separator"); + private IMocksControl mockMaker; private SchemaFactory<ReviewDb> schemaFactory; private SubmoduleSubscriptionAccess subscriptions; private ReviewDb schema; @@ -89,23 +89,22 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { public void setUp() throws Exception { super.setUp(); - schemaFactory = createStrictMock(SchemaFactory.class); - schema = createStrictMock(ReviewDb.class); - subscriptions = createStrictMock(SubmoduleSubscriptionAccess.class); - urlProvider = createStrictMock(Provider.class); - repoManager = createStrictMock(GitRepositoryManager.class); - gitRefUpdated = createStrictMock(GitReferenceUpdated.class); - changeHooks = createNiceMock(ChangeHooks.class); + mockMaker = EasyMock.createStrictControl(); + schemaFactory = mockMaker.createMock(SchemaFactory.class); + schema = mockMaker.createMock(ReviewDb.class); + subscriptions = mockMaker.createMock(SubmoduleSubscriptionAccess.class); + urlProvider = mockMaker.createMock(Provider.class); + repoManager = mockMaker.createMock(GitRepositoryManager.class); + gitRefUpdated = mockMaker.createMock(GitReferenceUpdated.class); + changeHooks = mockMaker.createMock(ChangeHooks.class); } private void doReplay() { - replay(schemaFactory, schema, subscriptions, urlProvider, repoManager, - gitRefUpdated); + mockMaker.replay(); } private void doVerify() { - verify(schemaFactory, schema, subscriptions, urlProvider, repoManager, - gitRefUpdated); + mockMaker.verify(); } /** @@ -651,13 +650,18 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { Capture<RefUpdate> ruCapture = new Capture<RefUpdate>(); gitRefUpdated.fire(eq(targetBranchNameKey.getParentKey()), capture(ruCapture)); + changeHooks.doRefUpdatedHook(eq(targetBranchNameKey), + anyObject(RefUpdate.class), EasyMock.<Account>isNull()); expect(schema.submoduleSubscriptions()).andReturn(subscriptions); final ResultSet<SubmoduleSubscription> emptySubscriptions = - new ListResultSet<SubmoduleSubscription>(new ArrayList<SubmoduleSubscription>()); + new ListResultSet<SubmoduleSubscription>(new ArrayList<SubmoduleSubscription>()); expect(subscriptions.bySubmodule(targetBranchNameKey)).andReturn( emptySubscriptions); + expect(schema.submoduleSubscriptions()).andReturn(subscriptions); + subscriptions.delete(EasyMock.<Iterable<SubmoduleSubscription>>anyObject()); + schema.close(); final PersonIdent myIdent = @@ -755,6 +759,8 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { Capture<RefUpdate> ruCapture = new Capture<RefUpdate>(); gitRefUpdated.fire(eq(targetBranchNameKey.getParentKey()), capture(ruCapture)); + changeHooks.doRefUpdatedHook(eq(targetBranchNameKey), + anyObject(RefUpdate.class), EasyMock.<Account>isNull()); expect(schema.submoduleSubscriptions()).andReturn(subscriptions); final ResultSet<SubmoduleSubscription> incorrectSubscriptions = @@ -764,6 +770,11 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase { expect(subscriptions.bySubmodule(targetBranchNameKey)).andReturn( incorrectSubscriptions); + expect(schema.submoduleSubscriptions()).andReturn(subscriptions); + subscriptions.delete(EasyMock.<Iterable<SubmoduleSubscription>>anyObject()); + expect(schema.submoduleSubscriptions()).andReturn(subscriptions); + subscriptions.delete(EasyMock.<Iterable<SubmoduleSubscription>>anyObject()); + schema.close(); final PersonIdent myIdent = |