Merge "Fix incorrect submodule subscriptions" into stable-2.9

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 =