Merge changes I4d03b914,I2674ac68,Ie7321d80,I5959aa35

* changes:
  BatchUpdate.execute: Accept a list of listeners
  BatchUpdateListener: Offer BatchRefUpdate to the listener before exec
  BatchUpdate: Move firing ref change event to BatchUpdate proper
  MergeOp: Use submoduleOp.updateSuperprojects() instead of direct ops

java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java

@@ -16,9 +16,11 @@ package com.google.gerrit.server.notedb;
 
 import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.gerrit.server.logging.TraceContext.newTimer;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.MultimapBuilder;
 import com.google.gerrit.common.Nullable;
@@ -34,6 +36,7 @@ import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.logging.Metadata;
 import com.google.gerrit.server.logging.TraceContext;
+import com.google.gerrit.server.update.BatchUpdateListener;
 import com.google.gerrit.server.update.ChainedReceiveCommands;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -94,6 +97,7 @@ public class NoteDbUpdateManager implements AutoCloseable {
   private String refLogMessage;
   private PersonIdent refLogIdent;
   private PushCertificate pushCert;
+  private ImmutableList<BatchUpdateListener> batchUpdateListeners;
 
   @Inject
   NoteDbUpdateManager(
@@ -117,6 +121,7 @@ public class NoteDbUpdateManager implements AutoCloseable {
     robotCommentUpdates = MultimapBuilder.hashKeys().arrayListValues().build();
     rewriters = MultimapBuilder.hashKeys().arrayListValues().build();
     changesToDelete = new HashSet<>();
+    batchUpdateListeners = ImmutableList.of();
   }
 
   @Override
@@ -172,6 +177,13 @@ public class NoteDbUpdateManager implements AutoCloseable {
     return this;
   }
 
+  public NoteDbUpdateManager setBatchUpdateListeners(
+      ImmutableList<BatchUpdateListener> batchUpdateListeners) {
+    checkNotNull(batchUpdateListeners);
+    this.batchUpdateListeners = batchUpdateListeners;
+    return this;
+  }
+
   private void initChangeRepo() throws IOException {
     if (changeRepo == null) {
       changeRepo = OpenRepo.open(repoManager, projectName);
@@ -358,6 +370,9 @@ public class NoteDbUpdateManager implements AutoCloseable {
     bru.setAtomic(true);
     or.cmds.addTo(bru);
     bru.setAllowNonFastForwards(true);
+    for (BatchUpdateListener listener : batchUpdateListeners) {
+      bru = listener.beforeUpdateRefs(bru);
+    }
 
     if (!dryrun) {
       RefUpdateUtil.executeChecked(bru, or.rw);

java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java

@@ -48,7 +48,6 @@ import com.google.gerrit.server.restapi.change.CommentJson;
 import com.google.gerrit.server.restapi.change.CommentJson.HumanCommentFormatter;
 import com.google.gerrit.server.update.BatchUpdate;
 import com.google.gerrit.server.update.BatchUpdate.Factory;
-import com.google.gerrit.server.update.BatchUpdateListener;
 import com.google.gerrit.server.update.BatchUpdateOp;
 import com.google.gerrit.server.update.ChangeContext;
 import com.google.gerrit.server.update.UpdateException;
@@ -140,7 +139,7 @@ public class DeleteDraftComments
     // Currently there's no way to let some updates succeed even if others fail. Even if there were,
     // all updates from this operation only happen in All-Users and thus are fully atomic, so
     // allowing partial failure would have little value.
-    BatchUpdate.execute(updates.values(), BatchUpdateListener.NONE, false);
+    BatchUpdate.execute(updates.values(), ImmutableList.of(), false);
 
     return Response.ok(
         ops.stream().map(Op::getResult).filter(Objects::nonNull).collect(toImmutableList()));

java/com/google/gerrit/server/submit/MergeOp.java

@@ -24,6 +24,7 @@ import com.github.rholder.retry.Attempt;
 import com.github.rholder.retry.RetryListener;
 import com.google.auto.value.AutoValue;
 import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
@@ -97,6 +98,7 @@ import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.transport.ReceiveCommand;
 
 /**
  * Merges changes in submission order into a single branch.
@@ -228,6 +230,7 @@ public class MergeOp implements AutoCloseable {
   private final SubmitStrategyFactory submitStrategyFactory;
   private final SubscriptionGraph.Factory subscriptionGraphFactory;
   private final SubmoduleCommits.Factory submoduleCommitsFactory;
+  private final SubmoduleOp.Factory submoduleOpFactory;
   private final Provider<MergeOpRepoManager> ormProvider;
   private final NotifyResolver notifyResolver;
   private final RetryHelper retryHelper;
@@ -235,6 +238,8 @@ public class MergeOp implements AutoCloseable {
 
   // Changes that were updated by this MergeOp.
   private final Map<Change.Id, Change> updatedChanges;
+  // Refs that were updated by this MergeOp.
+  private final Map<BranchNameKey, ReceiveCommand> updatedRefs;
 
   private Timestamp ts;
   private SubmissionId submissionId;
@@ -259,6 +264,7 @@ public class MergeOp implements AutoCloseable {
       SubmitStrategyFactory submitStrategyFactory,
       SubmoduleCommits.Factory submoduleCommitsFactory,
       SubscriptionGraph.Factory subscriptionGraphFactory,
+      SubmoduleOp.Factory submoduleOpFactory,
       Provider<MergeOpRepoManager> ormProvider,
       NotifyResolver notifyResolver,
       TopicMetrics topicMetrics,
@@ -273,12 +279,14 @@ public class MergeOp implements AutoCloseable {
     this.submitStrategyFactory = submitStrategyFactory;
     this.submoduleCommitsFactory = submoduleCommitsFactory;
     this.subscriptionGraphFactory = subscriptionGraphFactory;
+    this.submoduleOpFactory = submoduleOpFactory;
     this.ormProvider = ormProvider;
     this.notifyResolver = notifyResolver;
     this.retryHelper = retryHelper;
     this.topicMetrics = topicMetrics;
     this.changeDataFactory = changeDataFactory;
     this.updatedChanges = new HashMap<>();
+    this.updatedRefs = new HashMap<>();
   }
 
   @Override
@@ -520,6 +528,9 @@ public class MergeOp implements AutoCloseable {
             .defaultTimeoutMultiplier(cs.projects().size())
             .call();
 
+        SubmoduleOp submoduleOp = submoduleOpFactory.create(updatedRefs, orm);
+        submoduleOp.updateSuperProjects(dryrun);
+
         if (projects > 1) {
           topicMetrics.topicSubmissionsCompleted.increment();
         }
@@ -619,13 +630,14 @@ public class MergeOp implements AutoCloseable {
       try {
         BatchUpdate.execute(
             batchUpdates,
-            new SubmitStrategyListener(submitInput, strategies, commitStatus),
+            ImmutableList.of(new SubmitStrategyListener(submitInput, strategies, commitStatus)),
             dryrun);
       } finally {
         // If the BatchUpdate fails it can be that merging some of the changes was actually
-        // successful. This is why we must to collect the updated changes also when an exception was
-        // thrown.
+        // successful. This is why we must to collect the updated changes and refs also when an
+        // exception was thrown.
         strategies.forEach(s -> updatedChanges.putAll(s.getUpdatedChanges()));
+        batchUpdates.forEach(bu -> updatedRefs.putAll(bu.getSuccessfullyUpdatedBranches(dryrun)));
 
         // Do not leave executed BatchUpdates in the OpenRepos
         if (!dryrun) {
@@ -682,8 +694,6 @@ public class MergeOp implements AutoCloseable {
     Set<CodeReviewCommit> allCommits =
         toSubmit.values().stream().map(BranchBatch::commits).flatMap(Set::stream).collect(toSet());
 
-    GitlinkOp.Factory gitlinkOpFactory = new GitlinkOp.Factory(submoduleCommits, subscriptionGraph);
-
     for (BranchNameKey branch : allBranches) {
       OpenRepo or = orm.getRepo(branch.project());
       if (toSubmit.containsKey(branch)) {
@@ -713,16 +723,9 @@ public class MergeOp implements AutoCloseable {
                 dryrun);
         strategies.add(strategy);
         strategy.addOps(or.getUpdate(), commitsToSubmit);
-        if (submitting.submitType().equals(SubmitType.FAST_FORWARD_ONLY)
-            && subscriptionGraph.hasSubscription(branch)) {
-          or.getUpdate().addRepoOnlyOp(gitlinkOpFactory.create(branch));
-        }
-      } else {
-        // no open change for this branch
-        // add submodule triggered op into BatchUpdate
-        or.getUpdate().addRepoOnlyOp(gitlinkOpFactory.create(branch));
       }
     }
+
     return strategies;
   }
 

java/com/google/gerrit/server/submit/SubmoduleOp.java

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.server.submit;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.entities.BranchNameKey;
 import com.google.gerrit.entities.Project;
@@ -23,7 +24,6 @@ import com.google.gerrit.server.git.CodeReviewCommit;
 import com.google.gerrit.server.project.NoSuchProjectException;
 import com.google.gerrit.server.submit.MergeOpRepoManager.OpenRepo;
 import com.google.gerrit.server.update.BatchUpdate;
-import com.google.gerrit.server.update.BatchUpdateListener;
 import com.google.gerrit.server.update.UpdateException;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
@@ -103,7 +103,7 @@ public class SubmoduleOp {
           }
         }
       }
-      BatchUpdate.execute(orm.batchUpdates(superProjects), BatchUpdateListener.NONE, dryrun);
+      BatchUpdate.execute(orm.batchUpdates(superProjects), ImmutableList.of(), dryrun);
     } catch (UpdateException | IOException | NoSuchProjectException e) {
       throw new StorageException("Cannot update gitlinks", e);
     }

java/com/google/gerrit/server/update/BatchUpdate.java

@@ -124,9 +124,9 @@ public class BatchUpdate implements AutoCloseable {
   }
 
   public static void execute(
-      Collection<BatchUpdate> updates, BatchUpdateListener listener, boolean dryrun)
+      Collection<BatchUpdate> updates, ImmutableList<BatchUpdateListener> listeners, boolean dryrun)
       throws UpdateException, RestApiException {
-    requireNonNull(listener);
+    requireNonNull(listeners);
     if (updates.isEmpty()) {
       return;
     }
@@ -140,16 +140,16 @@ public class BatchUpdate implements AutoCloseable {
         for (BatchUpdate u : updates) {
           u.executeUpdateRepo();
         }
-        listener.afterUpdateRepos();
+        notifyAfterUpdateRepo(listeners);
         for (BatchUpdate u : updates) {
-          changesHandles.add(u.executeChangeOps(dryrun));
+          changesHandles.add(u.executeChangeOps(listeners, dryrun));
         }
         for (ChangesHandle h : changesHandles) {
           h.execute();
           indexFutures.addAll(h.startIndexFutures());
         }
-        listener.afterUpdateRefs();
-        listener.afterUpdateChanges();
+        notifyAfterUpdateRefs(listeners);
+        notifyAfterUpdateChanges(listeners);
       } finally {
         for (ChangesHandle h : changesHandles) {
           h.close();
@@ -161,10 +161,7 @@ public class BatchUpdate implements AutoCloseable {
       // Fire ref update events only after all mutations are finished, since callers may assume a
       // patch set ref being created means the change was created, or a branch advancing meaning
       // some changes were closed.
-      updates.stream()
-          .filter(u -> u.batchRefUpdate != null)
-          .forEach(
-              u -> u.gitRefUpdated.fire(u.project, u.batchRefUpdate, u.getAccount().orElse(null)));
+      updates.forEach(BatchUpdate::fireRefChangeEvent);
 
       if (!dryrun) {
         for (BatchUpdate u : updates) {
@@ -176,6 +173,27 @@ public class BatchUpdate implements AutoCloseable {
     }
   }
 
+  private static void notifyAfterUpdateRepo(ImmutableList<BatchUpdateListener> listeners)
+      throws Exception {
+    for (BatchUpdateListener listener : listeners) {
+      listener.afterUpdateRepos();
+    }
+  }
+
+  private static void notifyAfterUpdateRefs(ImmutableList<BatchUpdateListener> listeners)
+      throws Exception {
+    for (BatchUpdateListener listener : listeners) {
+      listener.afterUpdateRefs();
+    }
+  }
+
+  private static void notifyAfterUpdateChanges(ImmutableList<BatchUpdateListener> listeners)
+      throws Exception {
+    for (BatchUpdateListener listener : listeners) {
+      listener.afterUpdateChanges();
+    }
+  }
+
   private static void checkDifferentProject(Collection<BatchUpdate> updates) {
     Multiset<Project.NameKey> projectCounts =
         updates.stream().map(u -> u.project).collect(toImmutableMultiset());
@@ -386,11 +404,11 @@ public class BatchUpdate implements AutoCloseable {
   }
 
   public void execute(BatchUpdateListener listener) throws UpdateException, RestApiException {
-    execute(ImmutableList.of(this), listener, false);
+    execute(ImmutableList.of(this), ImmutableList.of(listener), false);
   }
 
   public void execute() throws UpdateException, RestApiException {
-    execute(BatchUpdateListener.NONE);
+    execute(ImmutableList.of(this), ImmutableList.of(), false);
   }
 
   public BatchUpdate setRepository(Repository repo, RevWalk revWalk, ObjectInserter inserter) {
@@ -530,6 +548,12 @@ public class BatchUpdate implements AutoCloseable {
     }
   }
 
+  private void fireRefChangeEvent() {
+    if (batchRefUpdate != null) {
+      gitRefUpdated.fire(project, batchRefUpdate, getAccount().orElse(null));
+    }
+  }
+
   private class ChangesHandle implements AutoCloseable {
     private final NoteDbUpdateManager manager;
     private final boolean dryrun;
@@ -580,7 +604,8 @@ public class BatchUpdate implements AutoCloseable {
     }
   }
 
-  private ChangesHandle executeChangeOps(boolean dryrun) throws Exception {
+  private ChangesHandle executeChangeOps(
+      ImmutableList<BatchUpdateListener> batchUpdateListeners, boolean dryrun) throws Exception {
     logDebug("Executing change ops");
     initRepository();
     Repository repo = repoView.getRepository();
@@ -593,6 +618,7 @@ public class BatchUpdate implements AutoCloseable {
         new ChangesHandle(
             updateManagerFactory
                 .create(project)
+                .setBatchUpdateListeners(batchUpdateListeners)
                 .setChangeRepo(
                     repo, repoView.getRevWalk(), repoView.getInserter(), repoView.getCommands()),
             dryrun);

java/com/google/gerrit/server/update/BatchUpdateListener.java

@@ -14,6 +14,8 @@
 
 package com.google.gerrit.server.update;
 
+import org.eclipse.jgit.lib.BatchRefUpdate;
+
 /**
  * Interface for listening during batch update execution.
  *
@@ -21,11 +23,25 @@ package com.google.gerrit.server.update;
  * after that phase has been completed for <em>all</em> updates.
  */
 public interface BatchUpdateListener {
-  public static final BatchUpdateListener NONE = new BatchUpdateListener() {};
+  BatchUpdateListener NONE = new BatchUpdateListener() {};
 
   /** Called after updating all repositories and flushing objects but before updating any refs. */
   default void afterUpdateRepos() throws Exception {}
 
+  /**
+   * Optional setup of the {@link BatchRefUpdate} that is going to be executed.
+   *
+   * <p>Called after {@link #afterUpdateRepos()}, before {@link #afterUpdateRefs()} and {@link
+   * #afterUpdateChanges()}
+   *
+   * @param bru a batch ref update, ready but not executed yet
+   * @return a new {@link BatchRefUpdate}. Implementations can decide to modify and return the
+   *     incoming instance, but callers must not rely on that.
+   */
+  default BatchRefUpdate beforeUpdateRefs(BatchRefUpdate bru) {
+    return bru;
+  }
+
   /** Called after updating all refs. */
   default void afterUpdateRefs() throws Exception {}