review: Batch submit all changes at once

By batch submitting all of the changes to the MergeQueue we ensure
that the merge queue can sort them by topological order and doesn't
create unnecessary merge commits.  This can be relevant if the user
accidentally uses --reverse flag when submitting changes in bulk or
when there is clock skew on the branch:

  ssh r gerrit review -s $(git rev-list --reverse origin/master..)

Unfortunately we can't do a full batch submit, as there isn't that
level of transaction support avaliable in the database API.  But we
can avoid starting the merger on the branch until after all of the
affected changes have been marked SUBMITTED.

The better way to ensure submits happen correctly is to perform a
topological sort of the changes using the same sorter as MergeOp, and
then submit them from the root down.  This ensures that an early merge
attempt started by a different thread will be unable to merge the
change, because its parent isn't yet marked SUBMITTED.  Unfortunately
reusing that code here is non-trivial, so I'm punting on it for now.

Change-Id: I0242a360fcd8ba8f8c7fa5dcee021867f91352f8
Signed-off-by: Shawn O. Pearce <sop@google.com>

1 files changed, 39 insertions, 1 deletions

diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
index a0b9d252d4..cd87498fad 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
@@ -18,6 +18,8 @@ import com.google.gerrit.common.data.ApprovalType;
 import com.google.gerrit.common.data.ApprovalTypes;
 import com.google.gerrit.reviewdb.ApprovalCategory;
 import com.google.gerrit.reviewdb.ApprovalCategoryValue;
+import com.google.gerrit.reviewdb.Branch;
+import com.google.gerrit.reviewdb.Branch.NameKey;
 import com.google.gerrit.reviewdb.Change;
 import com.google.gerrit.reviewdb.PatchSet;
 import com.google.gerrit.reviewdb.PatchSetApproval;
@@ -26,6 +28,7 @@ import com.google.gerrit.reviewdb.ReviewDb;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.git.MergeOp;
+import com.google.gerrit.server.git.MergeOp.Factory;
 import com.google.gerrit.server.git.MergeQueue;
 import com.google.gerrit.server.patch.PublishComments;
 import com.google.gerrit.server.project.CanSubmitResult;
@@ -51,6 +54,7 @@ import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.concurrent.TimeUnit;
 
 public class ReviewCommand extends BaseCommand {
   private static final Logger log =
@@ -113,6 +117,8 @@ public class ReviewCommand extends BaseCommand {
   @Inject
   private PublishComments.Factory publishCommentsFactory;
 
+  private Set<PatchSet.Id> toSubmit = new HashSet<PatchSet.Id>();
+
   @Override
   public final void start(final Environment env) {
     startThread(new CommandRunnable() {
@@ -135,10 +141,42 @@ public class ReviewCommand extends BaseCommand {
             log.error("internal error while approving " + patchSetId, e);
           }
         }
+
         if (!ok) {
           throw new UnloggedFailure(1, "one or more approvals failed;"
               + " review output above");
         }
+
+        if (!toSubmit.isEmpty()) {
+          final Set<Branch.NameKey> toMerge = new HashSet<Branch.NameKey>();
+          try {
+            for (PatchSet.Id patchSetId : toSubmit) {
+              ChangeUtil.submit(opFactory, patchSetId, currentUser, db,
+                  new MergeQueue() {
+                    @Override
+                    public void merge(MergeOp.Factory mof, Branch.NameKey branch) {
+                      toMerge.add(branch);
+                    }
+
+                    @Override
+                    public void schedule(Branch.NameKey branch) {
+                      toMerge.add(branch);
+                    }
+
+                    @Override
+                    public void recheckAfter(Branch.NameKey branch, long delay,
+                        TimeUnit delayUnit) {
+                      toMerge.add(branch);
+                    }
+                  });
+            }
+            for (Branch.NameKey branch : toMerge) {
+              merger.merge(opFactory, branch);
+            }
+          } catch (OrmException updateError) {
+            throw new Failure(1, "one or more submits failed", updateError);
+          }
+        }
       }
     });
   }
@@ -170,7 +208,7 @@ public class ReviewCommand extends BaseCommand {
           changeControl.canSubmit(patchSetId, db, approvalTypes,
               functionStateFactory);
       if (result == CanSubmitResult.OK) {
-        ChangeUtil.submit(opFactory, patchSetId, currentUser, db, merger);
+        toSubmit.add(patchSetId);
       } else {
         throw error(result.getMessage());
       }