gerrit approve: Allow approving multiple commits at once

By accepting multiple patch sets on the command line we now support
approving more than one patch set per invocation, which may be of
use in an automated builder.

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

2 files changed, 117 insertions, 64 deletions

diff --git a/src/main/java/com/google/gerrit/server/ssh/BaseCommand.java b/src/main/java/com/google/gerrit/server/ssh/BaseCommand.java
index fe6ecedf2d..ab6254466e 100644
--- a/src/main/java/com/google/gerrit/server/ssh/BaseCommand.java
+++ b/src/main/java/com/google/gerrit/server/ssh/BaseCommand.java
@@ -164,6 +164,10 @@ public abstract class BaseCommand implements Command {
     final CmdLineParser clp = newCmdLineParser();
     try {
       clp.parseArgument(list.toArray(new String[list.size()]));
+    } catch (IllegalArgumentException err) {
+      if (!help) {
+        throw new UnloggedFailure(1, "fatal: " + err.getMessage());
+      }
     } catch (CmdLineException err) {
       if (!help) {
         throw new UnloggedFailure(1, "fatal: " + err.getMessage());
diff --git a/src/main/java/com/google/gerrit/server/ssh/commands/ApproveCommand.java b/src/main/java/com/google/gerrit/server/ssh/commands/ApproveCommand.java
index bb14390f59..b258e39068 100644
--- a/src/main/java/com/google/gerrit/server/ssh/commands/ApproveCommand.java
+++ b/src/main/java/com/google/gerrit/server/ssh/commands/ApproveCommand.java
@@ -33,6 +33,7 @@ import com.google.gerrit.server.mail.CommentSender.Factory;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
 import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.project.ProjectControl;
 import com.google.gerrit.server.ssh.BaseCommand;
 import com.google.gerrit.server.workflow.FunctionState;
@@ -43,7 +44,10 @@ import com.google.inject.Inject;
 
 import org.kohsuke.args4j.Argument;
 import org.kohsuke.args4j.Option;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashSet;
@@ -51,6 +55,9 @@ import java.util.List;
 import java.util.Set;
 
 public class ApproveCommand extends BaseCommand {
+  private static final Logger log =
+      LoggerFactory.getLogger(ApproveCommand.class);
+
   @Override
   protected final CmdLineParser newCmdLineParser() {
     final CmdLineParser parser = super.newCmdLineParser();
@@ -60,8 +67,18 @@ public class ApproveCommand extends BaseCommand {
     return parser;
   }
 
-  @Argument(index = 0, required = true, metaVar = "{COMMIT | CHANGE,PATCHSET}", usage = "patch to approve")
-  private String patchIdentity;
+  private final Set<PatchSet.Id> patchSetIds = new HashSet<PatchSet.Id>();
+
+  @Argument(index = 0, required = true, multiValued = true, metaVar = "{COMMIT | CHANGE,PATCHSET}", usage = "patch to approve")
+  void addPatchSetId(final String token) {
+    try {
+      patchSetIds.addAll(parsePatchSetId(token));
+    } catch (UnloggedFailure e) {
+      throw new IllegalArgumentException(e.getMessage(), e);
+    } catch (OrmException e) {
+      throw new IllegalArgumentException("database error", e);
+    }
+  }
 
   @Option(name = "--project", aliases = "-p", usage = "project containing the patch set")
   private ProjectControl projectControl;
@@ -96,78 +113,96 @@ public class ApproveCommand extends BaseCommand {
   public final void start() {
     startThread(new CommandRunnable() {
       @Override
-      public void run() throws Exception {
+      public void run() throws Failure {
         initOptionList();
         parseCommandLine();
 
-        final PatchSet.Id patchSetId = parsePatchSetId();
-        final Change.Id changeId = patchSetId.getParentKey();
-        final ChangeControl changeControl =
-            changeControlFactory.validateFor(changeId);
-        final Change change = changeControl.getChange();
-
-        if (change.getStatus().isClosed()) {
-          throw error("change " + changeId + " is closed");
+        boolean ok = true;
+        for (final PatchSet.Id patchSetId : patchSetIds) {
+          try {
+            approveOne(patchSetId);
+          } catch (UnloggedFailure e) {
+            ok = false;
+            writeError("error: " + e.getMessage() + "\n");
+          } catch (Exception e) {
+            ok = false;
+            writeError("fatal: internal server error while approving "
+                + patchSetId + "\n");
+            log.error("internal error while approving " + patchSetId);
+          }
         }
-        if (!inProject(change)) {
-          throw error("change " + changeId + " not in project "
-              + projectControl.getProject().getName());
+        if (!ok) {
+          throw new UnloggedFailure(1, "one or more approvals failed;"
+              + " review output above");
         }
+      }
+    });
+  }
 
-        final Transaction txn = db.beginTransaction();
-        final StringBuffer msgBuf = new StringBuffer();
-        msgBuf.append("Patch Set ");
-        msgBuf.append(patchSetId.get());
-        msgBuf.append(": ");
-
-        for (ApproveOption co : optionList) {
-          final ApprovalCategory.Id category = co.getCategoryId();
-          PatchSetApproval.Key psaKey =
-              new PatchSetApproval.Key(patchSetId, currentUser.getAccountId(),
-                  category);
-          PatchSetApproval psa = db.patchSetApprovals().get(psaKey);
-
-          Short score = co.value();
-
-          if (score != null) {
-            addApproval(psaKey, score, change, co, txn);
-          } else {
-            if (psa == null) {
-              score = 0;
-              addApproval(psaKey, score, change, co, txn);
-            } else {
-              score = psa.getValue();
-            }
-          }
+  private void approveOne(final PatchSet.Id patchSetId)
+      throws NoSuchChangeException, UnloggedFailure, OrmException,
+      PatchSetInfoNotAvailableException, EmailException {
+    final Change.Id changeId = patchSetId.getParentKey();
+    final ChangeControl changeControl =
+        changeControlFactory.validateFor(changeId);
+    final Change change = changeControl.getChange();
+    if (change.getStatus().isClosed()) {
+      throw error("change " + changeId + " is closed");
+    }
 
-          String message =
-              db.approvalCategoryValues().get(
-                  new ApprovalCategoryValue.Id(category, score)).getName();
-          msgBuf.append(" " + message + ";");
-        }
+    final Transaction txn = db.beginTransaction();
+    final StringBuffer msgBuf = new StringBuffer();
+    msgBuf.append("Patch Set ");
+    msgBuf.append(patchSetId.get());
+    msgBuf.append(": ");
 
-        msgBuf.deleteCharAt(msgBuf.length() - 1);
-        msgBuf.append("\n\n");
+    for (ApproveOption co : optionList) {
+      final ApprovalCategory.Id category = co.getCategoryId();
+      PatchSetApproval.Key psaKey =
+          new PatchSetApproval.Key(patchSetId, currentUser.getAccountId(),
+              category);
+      PatchSetApproval psa = db.patchSetApprovals().get(psaKey);
 
-        if (changeComment != null) {
-          msgBuf.append(changeComment);
-        }
+      Short score = co.value();
 
-        String uuid = ChangeUtil.messageUUID(db);
-        ChangeMessage cm =
-            new ChangeMessage(new ChangeMessage.Key(changeId, uuid),
-                currentUser.getAccountId());
-        cm.setMessage(msgBuf.toString());
-        db.changeMessages().insert(Collections.singleton(cm), txn);
-        ChangeUtil.updated(change);
-        db.changes().update(Collections.singleton(change), txn);
-        txn.commit();
-        sendMail(change, change.currentPatchSetId(), cm);
+      if (score != null) {
+        addApproval(psaKey, score, change, co, txn);
+      } else {
+        if (psa == null) {
+          score = 0;
+          addApproval(psaKey, score, change, co, txn);
+        } else {
+          score = psa.getValue();
+        }
       }
-    });
+
+      String message =
+          db.approvalCategoryValues().get(
+              new ApprovalCategoryValue.Id(category, score)).getName();
+      msgBuf.append(" " + message + ";");
+    }
+
+    msgBuf.deleteCharAt(msgBuf.length() - 1);
+    msgBuf.append("\n\n");
+
+    if (changeComment != null) {
+      msgBuf.append(changeComment);
+    }
+
+    String uuid = ChangeUtil.messageUUID(db);
+    ChangeMessage cm =
+        new ChangeMessage(new ChangeMessage.Key(changeId, uuid), currentUser
+            .getAccountId());
+    cm.setMessage(msgBuf.toString());
+    db.changeMessages().insert(Collections.singleton(cm), txn);
+    ChangeUtil.updated(change);
+    db.changes().update(Collections.singleton(change), txn);
+    txn.commit();
+    sendMail(change, change.currentPatchSetId(), cm);
   }
 
-  private PatchSet.Id parsePatchSetId() throws UnloggedFailure, OrmException {
+  private Set<PatchSet.Id> parsePatchSetId(final String patchIdentity)
+      throws UnloggedFailure, OrmException {
     // By commit?
     //
     if (patchIdentity.matches("^([0-9a-fA-F]{4," + RevId.LEN + "})$")) {
@@ -189,7 +224,7 @@ public class ApproveCommand extends BaseCommand {
 
       switch (matches.size()) {
         case 1:
-          return matches.iterator().next();
+          return matches;
         case 0:
           throw error("\"" + patchIdentity + "\" no such patch set");
         default:
@@ -199,7 +234,7 @@ public class ApproveCommand extends BaseCommand {
 
     // By older style change,patchset?
     //
-    if (patchIdentity.matches("^[1-9][0-9]*,[1-9][0-9]$")) {
+    if (patchIdentity.matches("^[1-9][0-9]*,[1-9][0-9]*$")) {
       final PatchSet.Id patchSetId;
       try {
         patchSetId = PatchSet.Id.parse(patchIdentity);
@@ -209,7 +244,14 @@ public class ApproveCommand extends BaseCommand {
       if (db.patchSets().get(patchSetId) == null) {
         throw error("\"" + patchIdentity + "\" no such patch set");
       }
-      return patchSetId;
+      if (projectControl != null) {
+        final Change change = db.changes().get(patchSetId.getParentKey());
+        if (!inProject(change)) {
+          throw error("change " + change.getId() + " not in project "
+              + projectControl.getProject().getName());
+        }
+      }
+      return Collections.singleton(patchSetId);
     }
 
     throw error("\"" + patchIdentity + "\" is not a valid patch set");
@@ -285,6 +327,13 @@ public class ApproveCommand extends BaseCommand {
     }
   }
 
+  private void writeError(final String msg) {
+    try {
+      err.write(msg.getBytes(ENC));
+    } catch (IOException e) {
+    }
+  }
+
   private static UnloggedFailure error(final String msg) {
     return new UnloggedFailure(1, msg);
   }