Enhance review ssh command to abandon/restore patchsets.

The review command could only do approvals, verifies, comments, and submits. It now can also do abandons and restores like the WUI. Also refactor the abandon and restore logic out of the AbandonChange and RestoreChange classes and into the ChangeUtil class since it is used by both the WUI and SSH now. Bug: issue 674 Change-Id: I7ac24a0ff47f3edaebb6fcad8ad3127f370e5672
2010-10-05 14:39:51 -06:00 · 2010-10-05 14:39:51 -06:00 · 30f823c1f8
parent 78ea8756d3
commit 30f823c1f8
5 changed files with 188 additions and 108 deletions
--- a/Documentation/cmd-review.txt
+++ b/Documentation/cmd-review.txt
@ -8,8 +8,8 @@ gerrit review - Verify, approve and/or submit one or more patch sets
 SYNOPSIS
 --------
 [verse]
-'ssh' -p <port> <host> 'gerrit approve' [\--project <PROJECT>] [\--message <MESSAGE>] [\--verified <N>] [\--code-review <N>] [\--submit] {COMMIT | CHANGEID,PATCHSET}...
-'ssh' -p <port> <host> 'gerrit review' [\--project <PROJECT>] [\--message <MESSAGE>] [\--verified <N>] [\--code-review <N>] [\--submit] {COMMIT | CHANGEID,PATCHSET}...
+'ssh' -p <port> <host> 'gerrit approve' [\--project <PROJECT>] [\--message <MESSAGE>] [\--verified <N>] [\--code-review <N>] [\--abandon] [\--restore] [\--submit]  {COMMIT | CHANGEID,PATCHSET}...
+'ssh' -p <port> <host> 'gerrit review' [\--project <PROJECT>] [\--message <MESSAGE>] [\--verified <N>] [\--code-review <N>] [\--abandon] [\--restore] [\--submit]  {COMMIT | CHANGEID,PATCHSET}...

 DESCRIPTION
 -----------
@ -54,9 +54,18 @@ OPTIONS
 	differs per site, check the output of \--help, or contact
 	your site administrator for further details.

+\--abandon::
+	Abandon the specified patch set(s).
+	(option is mutually exclusive with --submit and --restore)
+
+\--restore::
+	Restore the specified abandonned patch set(s).
+	(option is mutually exclusive with --abandon)
+
 \--submit::
 -s::
 	Submit the specified patch set(s) for merging.
+	(option is mutually exclusive with --abandon)

 ACCESS
 ------
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/AbandonChange.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/AbandonChange.java
@ -19,9 +19,7 @@ import com.google.gerrit.common.data.ChangeDetail;
 import com.google.gerrit.common.errors.NoSuchEntityException;
 import com.google.gerrit.httpd.rpc.Handler;
 import com.google.gerrit.reviewdb.Change;
-import com.google.gerrit.reviewdb.ChangeMessage;
 import com.google.gerrit.reviewdb.PatchSet;
-import com.google.gerrit.reviewdb.PatchSetApproval;
 import com.google.gerrit.reviewdb.ReviewDb;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.IdentifiedUser;
@ -30,14 +28,10 @@ import com.google.gerrit.server.mail.EmailException;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.NoSuchChangeException;
-import com.google.gwtorm.client.AtomicUpdate;
 import com.google.gwtorm.client.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;

-import java.util.Collections;
-import java.util.List;
-
 import javax.annotation.Nullable;

 class AbandonChange extends Handler<ChangeDetail> {
@ -78,60 +72,15 @@ class AbandonChange extends Handler<ChangeDetail> {
  @Override
  public ChangeDetail call() throws NoSuchChangeException, OrmException,
      EmailException, NoSuchEntityException, PatchSetInfoNotAvailableException {
+
    final Change.Id changeId = patchSetId.getParentKey();
    final ChangeControl control = changeControlFactory.validateFor(changeId);
    if (!control.canAbandon()) {
      throw new NoSuchChangeException(changeId);
    }
-    Change change = control.getChange();
-    final PatchSet patch = db.patchSets().get(patchSetId);
-    if (patch == null) {
-      throw new NoSuchChangeException(changeId);
-    }

-    final ChangeMessage cmsg =
-        new ChangeMessage(new ChangeMessage.Key(changeId, ChangeUtil
-            .messageUUID(db)), currentUser.getAccountId());
-    final StringBuilder msgBuf =
-        new StringBuilder("Patch Set " + patchSetId.get() + ": Abandoned");
-    if (message != null && message.length() > 0) {
-      msgBuf.append("\n\n");
-      msgBuf.append(message);
-    }
-    cmsg.setMessage(msgBuf.toString());
-
-    change = db.changes().atomicUpdate(changeId, new AtomicUpdate<Change>() {
-      @Override
-      public Change update(Change change) {
-        if (change.getStatus().isOpen()
-            && change.currentPatchSetId().equals(patchSetId)) {
-          change.setStatus(Change.Status.ABANDONED);
-          ChangeUtil.updated(change);
-          return change;
-        } else {
-          return null;
-        }
-      }
-    });
-
-    if (change != null) {
-      db.changeMessages().insert(Collections.singleton(cmsg));
-
-      final List<PatchSetApproval> approvals =
-          db.patchSetApprovals().byChange(changeId).toList();
-      for (PatchSetApproval a : approvals) {
-        a.cache(change);
-      }
-      db.patchSetApprovals().update(approvals);
-
-      // Email the reviewers
-      final AbandonedSender cm = abandonedSenderFactory.create(change);
-      cm.setFrom(currentUser.getAccountId());
-      cm.setChangeMessage(cmsg);
-      cm.send();
-    }
-
-    hooks.doChangeAbandonedHook(change, currentUser.getAccount(), message);
+    ChangeUtil.abandon(patchSetId, currentUser, message, db,
+       abandonedSenderFactory, hooks);

    return changeDetailFactory.create(changeId).call();
  }
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/RestoreChange.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/RestoreChange.java
@ -73,59 +73,15 @@ class RestoreChange extends Handler<ChangeDetail> {
  @Override
  public ChangeDetail call() throws NoSuchChangeException, OrmException,
      EmailException, NoSuchEntityException, PatchSetInfoNotAvailableException {
+
    final Change.Id changeId = patchSetId.getParentKey();
    final ChangeControl control = changeControlFactory.validateFor(changeId);
    if (!control.canRestore()) {
      throw new NoSuchChangeException(changeId);
    }
-    final PatchSet patch = db.patchSets().get(patchSetId);
-    if (patch == null) {
-      throw new NoSuchChangeException(changeId);
-    }

-    final ChangeMessage cmsg =
-        new ChangeMessage(new ChangeMessage.Key(changeId, ChangeUtil
-            .messageUUID(db)), currentUser.getAccountId());
-    final StringBuilder msgBuf =
-        new StringBuilder("Patch Set " + patchSetId.get() + ": Restored");
-    if (message != null && message.length() > 0) {
-      msgBuf.append("\n\n");
-      msgBuf.append(message);
-    }
-    cmsg.setMessage(msgBuf.toString());
-
-    Change change = db.changes().atomicUpdate(changeId, new AtomicUpdate<Change>() {
-      @Override
-      public Change update(Change change) {
-        if (change.getStatus() == Change.Status.ABANDONED
-            && change.currentPatchSetId().equals(patchSetId)) {
-          change.setStatus(Change.Status.NEW);
-          ChangeUtil.updated(change);
-          return change;
-        } else {
-          return null;
-        }
-      }
-    });
-
-    if (change != null) {
-      db.changeMessages().insert(Collections.singleton(cmsg));
-
-      final List<PatchSetApproval> approvals =
-          db.patchSetApprovals().byChange(changeId).toList();
-      for (PatchSetApproval a : approvals) {
-        a.cache(change);
-      }
-      db.patchSetApprovals().update(approvals);
-
-      // Email the reviewers
-      final AbandonedSender cm = abandonedSenderFactory.create(change);
-      cm.setFrom(currentUser.getAccountId());
-      cm.setChangeMessage(cmsg);
-      cm.send();
-    }
-
-    hooks.doChangeRestoreHook(change, currentUser.getAccount(), message);
+    ChangeUtil.restore(patchSetId, currentUser, message, db,
+       abandonedSenderFactory, hooks);

    return changeDetailFactory.create(changeId).call();
  }
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
@ -16,19 +16,28 @@ package com.google.gerrit.server;

 import static com.google.gerrit.reviewdb.ApprovalCategory.SUBMIT;

+import com.google.gerrit.common.ChangeHookRunner;
+import com.google.gerrit.common.data.ChangeDetail;
+import com.google.gerrit.reviewdb.Change;
+import com.google.gerrit.reviewdb.ChangeMessage;
 import com.google.gerrit.reviewdb.Change;
 import com.google.gerrit.reviewdb.PatchSet;
 import com.google.gerrit.reviewdb.PatchSetApproval;
 import com.google.gerrit.reviewdb.ReviewDb;
 import com.google.gerrit.reviewdb.TrackingId;
+import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.config.TrackingFooter;
 import com.google.gerrit.server.config.TrackingFooters;
 import com.google.gerrit.server.git.MergeOp;
 import com.google.gerrit.server.git.MergeQueue;
+import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.gerrit.server.mail.AbandonedSender;
+import com.google.gerrit.server.mail.EmailException;
 import com.google.gwtorm.client.AtomicUpdate;
 import com.google.gwtorm.client.OrmConcurrencyException;
 import com.google.gwtorm.client.OrmException;

+
 import org.eclipse.jgit.revwalk.FooterLine;
 import org.eclipse.jgit.util.Base64;
 import org.eclipse.jgit.util.NB;
@ -178,6 +187,119 @@ public class ChangeUtil {
    return new PatchSetApproval(akey, (short) 1);
  }

+  public static void abandon(final PatchSet.Id patchSetId,
+      final IdentifiedUser user, final String message, final ReviewDb db,
+      final AbandonedSender.Factory abandonedSenderFactory,
+      final ChangeHookRunner hooks) throws NoSuchChangeException,
+      EmailException, OrmException {
+    final Change.Id changeId = patchSetId.getParentKey();
+    final PatchSet patch = db.patchSets().get(patchSetId);
+    if (patch == null) {
+      throw new NoSuchChangeException(changeId);
+    }
+
+    final ChangeMessage cmsg =
+        new ChangeMessage(new ChangeMessage.Key(changeId, ChangeUtil
+            .messageUUID(db)), user.getAccountId());
+    final StringBuilder msgBuf =
+        new StringBuilder("Patch Set " + patchSetId.get() + ": Abandoned");
+    if (message != null && message.length() > 0) {
+      msgBuf.append("\n\n");
+      msgBuf.append(message);
+    }
+    cmsg.setMessage(msgBuf.toString());
+
+    final Change change = db.changes().atomicUpdate(changeId,
+        new AtomicUpdate<Change>() {
+      @Override
+      public Change update(Change change) {
+        if (change.getStatus().isOpen()
+            && change.currentPatchSetId().equals(patchSetId)) {
+          change.setStatus(Change.Status.ABANDONED);
+          ChangeUtil.updated(change);
+          return change;
+        } else {
+          return null;
+        }
+      }
+    });
+
+    if (change != null) {
+      db.changeMessages().insert(Collections.singleton(cmsg));
+
+      final List<PatchSetApproval> approvals =
+          db.patchSetApprovals().byChange(changeId).toList();
+      for (PatchSetApproval a : approvals) {
+        a.cache(change);
+      }
+      db.patchSetApprovals().update(approvals);
+
+      // Email the reviewers
+      final AbandonedSender cm = abandonedSenderFactory.create(change);
+      cm.setFrom(user.getAccountId());
+      cm.setChangeMessage(cmsg);
+      cm.send();
+    }
+
+    hooks.doChangeAbandonedHook(change, user.getAccount(), message);
+  }
+
+  public static void restore(final PatchSet.Id patchSetId,
+      final IdentifiedUser user, final String message, final ReviewDb db,
+      final AbandonedSender.Factory abandonedSenderFactory,
+      final ChangeHookRunner hooks) throws NoSuchChangeException,
+      EmailException, OrmException {
+    final Change.Id changeId = patchSetId.getParentKey();
+    final PatchSet patch = db.patchSets().get(patchSetId);
+    if (patch == null) {
+      throw new NoSuchChangeException(changeId);
+    }
+
+    final ChangeMessage cmsg =
+        new ChangeMessage(new ChangeMessage.Key(changeId, ChangeUtil
+            .messageUUID(db)), user.getAccountId());
+    final StringBuilder msgBuf =
+        new StringBuilder("Patch Set " + patchSetId.get() + ": Restored");
+    if (message != null && message.length() > 0) {
+      msgBuf.append("\n\n");
+      msgBuf.append(message);
+    }
+    cmsg.setMessage(msgBuf.toString());
+
+    Change change = db.changes().atomicUpdate(changeId, new AtomicUpdate<Change>() {
+      @Override
+      public Change update(Change change) {
+        if (change.getStatus() == Change.Status.ABANDONED
+            && change.currentPatchSetId().equals(patchSetId)) {
+          change.setStatus(Change.Status.NEW);
+          ChangeUtil.updated(change);
+          return change;
+        } else {
+          return null;
+        }
+      }
+    });
+
+    if (change != null) {
+      db.changeMessages().insert(Collections.singleton(cmsg));
+
+      final List<PatchSetApproval> approvals =
+          db.patchSetApprovals().byChange(changeId).toList();
+      for (PatchSetApproval a : approvals) {
+        a.cache(change);
+      }
+      db.patchSetApprovals().update(approvals);
+
+      // Email the reviewers
+      final AbandonedSender cm = abandonedSenderFactory.create(change);
+      cm.setFrom(user.getAccountId());
+      cm.setChangeMessage(cmsg);
+      cm.send();
+    }
+
+    hooks.doChangeRestoreHook(change, user.getAccount(), message);
+  }
+
  public static String sortKey(long lastUpdated, int id){
    // The encoding uses minutes since Wed Oct 1 00:00:00 2008 UTC.
    // We overrun approximately 4,085 years later, so ~6093.
--- 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
@ -14,6 +14,7 @@

 package com.google.gerrit.sshd.commands;

+import com.google.gerrit.common.ChangeHookRunner;
 import com.google.gerrit.common.data.ApprovalType;
 import com.google.gerrit.common.data.ApprovalTypes;
 import com.google.gerrit.reviewdb.ApprovalCategory;
@ -30,6 +31,8 @@ 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.mail.AbandonedSender;
+import com.google.gerrit.server.mail.EmailException;
 import com.google.gerrit.server.patch.PublishComments;
 import com.google.gerrit.server.project.CanSubmitResult;
 import com.google.gerrit.server.project.ChangeControl;
@ -88,6 +91,12 @@ public class ReviewCommand extends BaseCommand {
  @Option(name = "--message", aliases = "-m", usage = "cover message to publish on change", metaVar = "MESSAGE")
  private String changeComment;

+  @Option(name = "--abandon", usage = "abandon the patch set")
+  private boolean abandonChange;
+
+  @Option(name = "--restore", usage = "restore an abandoned the patch set")
+  private boolean restoreChange;
+
  @Option(name = "--submit", aliases = "-s", usage = "submit the patch set")
  private boolean submitChange;

@ -110,13 +119,19 @@ public class ReviewCommand extends BaseCommand {
  private ChangeControl.Factory changeControlFactory;

  @Inject
-  private FunctionState.Factory functionStateFactory;
+  private AbandonedSender.Factory abandonedSenderFactory;

-  private List<ApproveOption> optionList;
+  @Inject
+  private FunctionState.Factory functionStateFactory;

  @Inject
  private PublishComments.Factory publishCommentsFactory;

+  @Inject
+  private ChangeHookRunner hooks;
+
+  private List<ApproveOption> optionList;
+
  private Set<PatchSet.Id> toSubmit = new HashSet<PatchSet.Id>();

  @Override
@ -126,6 +141,14 @@ public class ReviewCommand extends BaseCommand {
      public void run() throws Failure {
        initOptionList();
        parseCommandLine();
+        if (abandonChange) {
+          if (restoreChange) {
+            throw error("abandon and restore actions are mutually exclusive");
+          }
+          if (submitChange) {
+            throw error("abandon and submit actions are mutually exclusive");
+          }
+        }

        boolean ok = true;
        for (final PatchSet.Id patchSetId : patchSetIds) {
@ -182,11 +205,11 @@ public class ReviewCommand extends BaseCommand {
  }

  private void approveOne(final PatchSet.Id patchSetId)
-      throws NoSuchChangeException, UnloggedFailure, OrmException {
+      throws NoSuchChangeException, UnloggedFailure, OrmException,
+             EmailException {

    final Change.Id changeId = patchSetId.getParentKey();
-    final ChangeControl changeControl =
-        changeControlFactory.validateFor(changeId);
+    ChangeControl changeControl = changeControlFactory.validateFor(changeId);

    if (changeComment == null) {
      changeComment = "";
@ -203,6 +226,27 @@ public class ReviewCommand extends BaseCommand {

    publishCommentsFactory.create(patchSetId, changeComment, aps).call();

+    if (abandonChange) {
+      if (changeControl.canAbandon()) {
+        ChangeUtil.abandon(patchSetId, currentUser, changeComment, db,
+          abandonedSenderFactory, hooks);
+      } else {
+        throw error("Not permitted to abandon change");
+      }
+    }
+
+    if (restoreChange) {
+      if (changeControl.canRestore()) {
+        ChangeUtil.restore(patchSetId, currentUser, changeComment, db,
+          abandonedSenderFactory, hooks);
+      } else {
+        throw error("Not permitted to restore change");
+      }
+      if (submitChange) {
+        changeControl = changeControlFactory.validateFor(changeId);
+      }
+    }
+
    if (submitChange) {
      CanSubmitResult result =
          changeControl.canSubmit(patchSetId, db, approvalTypes,