gerrit approve: Cleanup error reporting for missing objects
If a database object is not present, or the change is closed, try to use a consistent formatting for this error message. We catch the two major "not found" types up in the top level of the work thread, making it easier for the action code to throw directly out of the control objects. Change-Id: I6e731ad648738245b6b15840f40711ce677eb957 Signed-off-by: Shawn O. Pearce <sop@google.com>
This commit is contained in:
		@@ -17,6 +17,8 @@ package com.google.gerrit.server.ssh;
 | 
				
			|||||||
import com.google.gerrit.client.reviewdb.Account;
 | 
					import com.google.gerrit.client.reviewdb.Account;
 | 
				
			||||||
import com.google.gerrit.pgm.CmdLineParser;
 | 
					import com.google.gerrit.pgm.CmdLineParser;
 | 
				
			||||||
import com.google.gerrit.server.RequestCleanup;
 | 
					import com.google.gerrit.server.RequestCleanup;
 | 
				
			||||||
 | 
					import com.google.gerrit.server.project.NoSuchChangeException;
 | 
				
			||||||
 | 
					import com.google.gerrit.server.project.NoSuchProjectException;
 | 
				
			||||||
import com.google.gerrit.server.ssh.SshScopes.Context;
 | 
					import com.google.gerrit.server.ssh.SshScopes.Context;
 | 
				
			||||||
import com.google.inject.Inject;
 | 
					import com.google.inject.Inject;
 | 
				
			||||||
 | 
					
 | 
				
			||||||
@@ -243,7 +245,13 @@ public abstract class BaseCommand implements Command {
 | 
				
			|||||||
            active.add(cmd);
 | 
					            active.add(cmd);
 | 
				
			||||||
          }
 | 
					          }
 | 
				
			||||||
          SshScopes.current.set(context);
 | 
					          SshScopes.current.set(context);
 | 
				
			||||||
 | 
					          try {
 | 
				
			||||||
            thunk.run();
 | 
					            thunk.run();
 | 
				
			||||||
 | 
					          } catch (NoSuchProjectException e) {
 | 
				
			||||||
 | 
					            throw new UnloggedFailure(1, e.getMessage() + " no such project");
 | 
				
			||||||
 | 
					          } catch (NoSuchChangeException e) {
 | 
				
			||||||
 | 
					            throw new UnloggedFailure(1, e.getMessage() + " no such change");
 | 
				
			||||||
 | 
					          }
 | 
				
			||||||
          out.flush();
 | 
					          out.flush();
 | 
				
			||||||
          err.flush();
 | 
					          err.flush();
 | 
				
			||||||
        } catch (Throwable e) {
 | 
					        } catch (Throwable e) {
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -55,8 +55,6 @@ public class ApproveCommand extends BaseCommand {
 | 
				
			|||||||
    return parser;
 | 
					    return parser;
 | 
				
			||||||
  }
 | 
					  }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
  private static final int CMD_ERR = 3;
 | 
					 | 
				
			||||||
 | 
					 | 
				
			||||||
  @Argument(index = 0, required = true, metaVar = "CHANGE,PATCHSET", usage = "Patch set to approve")
 | 
					  @Argument(index = 0, required = true, metaVar = "CHANGE,PATCHSET", usage = "Patch set to approve")
 | 
				
			||||||
  private PatchSet.Id patchSetId;
 | 
					  private PatchSet.Id patchSetId;
 | 
				
			||||||
 | 
					
 | 
				
			||||||
@@ -99,7 +97,7 @@ public class ApproveCommand extends BaseCommand {
 | 
				
			|||||||
        final PatchSet ps = db.patchSets().get(patchSetId);
 | 
					        final PatchSet ps = db.patchSets().get(patchSetId);
 | 
				
			||||||
 | 
					
 | 
				
			||||||
        if (ps == null) {
 | 
					        if (ps == null) {
 | 
				
			||||||
          throw new UnloggedFailure(CMD_ERR, "Invalid patchset id");
 | 
					          throw error("" + patchSetId + " no such patch set");
 | 
				
			||||||
        }
 | 
					        }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
        final Change.Id cid = ps.getId().getParentKey();
 | 
					        final Change.Id cid = ps.getId().getParentKey();
 | 
				
			||||||
@@ -107,7 +105,7 @@ public class ApproveCommand extends BaseCommand {
 | 
				
			|||||||
        final Change c = control.getChange();
 | 
					        final Change c = control.getChange();
 | 
				
			||||||
 | 
					
 | 
				
			||||||
        if (c.getStatus().isClosed()) {
 | 
					        if (c.getStatus().isClosed()) {
 | 
				
			||||||
          throw new UnloggedFailure(CMD_ERR, "Change is closed.");
 | 
					          throw error("change " + cid + " is closed");
 | 
				
			||||||
        }
 | 
					        }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
        StringBuffer sb = new StringBuffer();
 | 
					        StringBuffer sb = new StringBuffer();
 | 
				
			||||||
@@ -193,8 +191,7 @@ public class ApproveCommand extends BaseCommand {
 | 
				
			|||||||
    psa.setValue(score);
 | 
					    psa.setValue(score);
 | 
				
			||||||
    fs.normalize(approvalTypes.getApprovalType(psa.getCategoryId()), psa);
 | 
					    fs.normalize(approvalTypes.getApprovalType(psa.getCategoryId()), psa);
 | 
				
			||||||
    if (score != psa.getValue()) {
 | 
					    if (score != psa.getValue()) {
 | 
				
			||||||
      throw new UnloggedFailure(CMD_ERR, co.name() + "=" + co.value()
 | 
					      throw error(co.name() + "=" + co.value() + " not permitted");
 | 
				
			||||||
          + " not permitted");
 | 
					 | 
				
			||||||
    }
 | 
					    }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
    psa.setGranted();
 | 
					    psa.setGranted();
 | 
				
			||||||
@@ -226,4 +223,8 @@ public class ApproveCommand extends BaseCommand {
 | 
				
			|||||||
          category.getName()));
 | 
					          category.getName()));
 | 
				
			||||||
    }
 | 
					    }
 | 
				
			||||||
  }
 | 
					  }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					  private static UnloggedFailure error(final String msg) {
 | 
				
			||||||
 | 
					    return new UnloggedFailure(1, msg);
 | 
				
			||||||
 | 
					  }
 | 
				
			||||||
}
 | 
					}
 | 
				
			||||||
 
 | 
				
			|||||||
		Reference in New Issue
	
	Block a user