Submodule subscriptions: accept allowed but unsubscribed subscriptions

The current code produces a NPE when there are projects allowed
to be subscribed, but are not subscribed. This happens as the load()
function returns early without initializing `subscriptions`.

To fix that issue, we'll move the code from `load()` into the constructor
and make sure that `subscriptions` is always assigned a value.

Also add a regression test.

Change-Id: Ia66930a1f7d40f7feef990e1b82d6ceb886d55f9
Signed-off-by: Stefan Beller <sbeller@google.com>

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

@@ -16,6 +16,7 @@ package com.google.gerrit.acceptance.git;
 
 import static com.google.common.truth.Truth.assertThat;
 
+import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.GerritConfig;
 import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.reviewdb.client.Project;
@@ -27,7 +28,9 @@ import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevTree;
 import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.transport.PushResult;
 import org.eclipse.jgit.transport.RefSpec;
+import org.eclipse.jgit.transport.RemoteRefUpdate;
 import org.junit.Test;
 
 @NoHttpd
@@ -355,6 +358,31 @@ public class SubmoduleSubscriptionsIT extends AbstractSubmoduleSubscription {
         "subscribed-to-project", subHEAD);
   }
 
+  @Test
+  public void testAllowedButNotSubscribed() throws Exception {
+    TestRepository<?> superRepo = createProjectWithPush("super-project");
+    TestRepository<?> subRepo = createProjectWithPush("subscribed-to-project");
+    allowSubmoduleSubscription("subscribed-to-project", "refs/heads/master",
+        "super-project", "refs/heads/master");
+
+    pushChangeTo(subRepo, "master");
+    subRepo.branch("HEAD").commit().insertChangeId()
+        .message("some change")
+        .add("b.txt", "b contents for testing")
+        .create();
+    String refspec = "HEAD:refs/heads/master";
+    PushResult r = Iterables.getOnlyElement(subRepo.git().push()
+        .setRemote("origin")
+        .setRefSpecs(new RefSpec(refspec))
+        .call());
+    assertThat(r.getMessages()).doesNotContain("error");
+    assertThat(r.getRemoteUpdate("refs/heads/master").getStatus())
+    .isEqualTo(RemoteRefUpdate.Status.OK);
+
+    assertThat(hasSubmodule(superRepo, "master",
+        "subscribed-to-project")).isFalse();
+  }
+
   @Test
   public void testSubscriptionDeepRelative() throws Exception {
     TestRepository<?> superRepo = createProjectWithPush("super-project");

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

@@ -37,6 +37,7 @@ import org.slf4j.LoggerFactory;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Set;
 
 /**
@@ -53,11 +54,7 @@ public class GitModules {
 
   private static final String GIT_MODULES = ".gitmodules";
 
-  private final String canonicalWebUrl;
-  private final Branch.NameKey branch;
   private final String submissionId;
-  private final MergeOpRepoManager orm;
-
   Set<SubmoduleSubscription> subscriptions;
 
   @AssistedInject
@@ -65,14 +62,9 @@ public class GitModules {
       @CanonicalWebUrl @Nullable String canonicalWebUrl,
       @Assisted Branch.NameKey branch,
       @Assisted String submissionId,
-      @Assisted MergeOpRepoManager orm) {
-    this.orm = orm;
-    this.branch = branch;
+      @Assisted MergeOpRepoManager orm) throws IOException {
     this.submissionId = submissionId;
-    this.canonicalWebUrl = canonicalWebUrl;
-  }
 
-  void load() throws IOException {
     Project.NameKey project = branch.getParentKey();
     logDebug("Loading .gitmodules of {} for project {}", branch, project);
     try {
@@ -91,18 +83,18 @@ public class GitModules {
     TreeWalk tw = TreeWalk.forPath(or.repo, GIT_MODULES, commit.getTree());
     if (tw == null
         || (tw.getRawMode(0) & FileMode.TYPE_MASK) != FileMode.TYPE_FILE) {
+      subscriptions = Collections.emptySet();
       return;
     }
+    BlobBasedConfig bbc;
     try {
-      BlobBasedConfig bbc =
-          new BlobBasedConfig(null, or.repo, commit, GIT_MODULES);
-      subscriptions = new SubmoduleSectionParser(bbc, canonicalWebUrl,
-          branch).parseAllSections();
+      bbc = new BlobBasedConfig(null, or.repo, commit, GIT_MODULES);
     } catch (ConfigInvalidException e) {
-      throw new IOException(
-          "Could not read .gitmodule file of super project: " +
+      throw new IOException("Could not read .gitmodules of super project: " +
               branch.getParentKey(), e);
     }
+    subscriptions = new SubmoduleSectionParser(bbc, canonicalWebUrl,
+          branch).parseAllSections();
   }
 
   public Collection<SubmoduleSubscription> subscribedTo(Branch.NameKey src) {

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

@@ -150,7 +150,6 @@ public class SubmoduleOp {
           getDestinationBranches(branch, s, orm);
       for (Branch.NameKey targetBranch : branches) {
         GitModules m = gitmodulesFactory.create(targetBranch, updateId, orm);
-        m.load();
         for (SubmoduleSubscription ss : m.subscribedTo(branch)) {
           logDebug("Checking SubmoduleSubscription " + ss);
           if (projectCache.get(ss.getSubmodule().getParentKey()) != null) {