Add error messages for abandon and restore when in bad state

The abandon and restore routines can fail if the change
is in the wrong state (abandoned already, or open still).
These failure lead to internal NPEs.  Instead, create a
new exception and give a reason for the failure to the user.

Change-Id: Id7861d75e535c439c12329f7e891797c5b1f6eca

1 files changed, 23 insertions, 19 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 0a6033bc..a8561a53 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
@@ -34,6 +34,7 @@ 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;
+import com.google.gerrit.server.project.InvalidChangeOperationException;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.project.ProjectControl;
 import com.google.gerrit.server.workflow.FunctionState;
@@ -202,9 +203,8 @@ public class ReviewCommand extends BaseCommand {
     });
   }
 
-  private void approveOne(final PatchSet.Id patchSetId)
-      throws NoSuchChangeException, UnloggedFailure, OrmException,
-             EmailException {
+  private void approveOne(final PatchSet.Id patchSetId) throws
+      NoSuchChangeException, UnloggedFailure, OrmException, EmailException {
 
     final Change.Id changeId = patchSetId.getParentKey();
     ChangeControl changeControl = changeControlFactory.validateFor(changeId);
@@ -224,25 +224,29 @@ 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");
+    try {
+      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 (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);
+        }
       }
+    } catch (InvalidChangeOperationException e) {
+      throw error(e.getMessage());
     }
 
     if (submitChange) {