From ece1a2ac34ce357b3e459f18a6b87f88a6727506 Mon Sep 17 00:00:00 2001 From: Deniz Turkoglu Date: Mon, 9 Jan 2012 11:28:52 +0100 Subject: [PATCH] Add better explanations to rejection messages Gerrit currently rejects pushes by ambiguous messages e.g. 'fast-forward rejected', 'can not delete references'. This is really confusing for people who have never used gerrit and even for the administrators that don't know about the access rights. This patch aims to resolve this issue by telling why we reject a certain 'Push' and how the user can get it fixed. Ideally the MESSAGE_FOOTER would be read from a properties file, but this still requires a total rebuild since the strings file we have resides in the war package. Change-Id: I270f680fd4a3d7b87aa3609926223c1759f66646 --- .../gerrit/server/git/ReceiveCommits.java | 79 ++++++++++++++++++- 1 file changed, 77 insertions(+), 2 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index 290b162861..83877d0a48 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -16,6 +16,8 @@ package com.google.gerrit.server.git; import static com.google.gerrit.server.git.MultiProgressMonitor.UNKNOWN; +import com.google.common.collect.LinkedListMultimap; +import com.google.common.collect.ListMultimap; import com.google.gerrit.common.ChangeHooks; import com.google.gerrit.common.PageLinks; import com.google.gerrit.common.data.ApprovalType; @@ -112,6 +114,34 @@ public class ReceiveCommits { private static final FooterKey TESTED_BY = new FooterKey("Tested-by"); private static final FooterKey CHANGE_ID = new FooterKey("Change-Id"); + private static final String COMMAND_REJECTION_MESSAGE_FOOTER = + "Please read the documentation and contact an administrator\n" + + "if you feel the configuration is incorrect"; + + private enum Error { + CONFIG_UPDATE("You are not allowed to perform this operation.\n" + + "Configuration changes can only be pushed by project owners\n" + + "who also have 'Push' rights on " + GitRepositoryManager.REF_CONFIG), + UPDATE("You are not allowed to perform this operation.\n" + + "To push into this reference you need 'Push' rights."), + DELETE("You need 'Push' rights with the 'Force Push'\n" + + "flag set to delete references."), + CODE_REVIEW("You need 'Push' rights to upload code review requests.\n" + + "Verify that you are pushing to the right branch."), + CREATE("You are not allowed to perform this operation.\n" + + "To create new references you need 'Create Reference' rights."); + + private final String value; + + Error(String value) { + this.value = value; + } + + public String get() { + return value; + } + } + interface Factory { ReceiveCommits create(ProjectControl projectControl, Repository repository); } @@ -215,6 +245,7 @@ public class ReceiveCommits { private final SubmoduleOp.Factory subOpFactory; private final List messages = new ArrayList(); + private ListMultimap errors = LinkedListMultimap.create(); private Task newProgress; private Task replaceProgress; private Task closeProgress; @@ -438,6 +469,14 @@ public class ReceiveCommits { doReplaces(); replaceProgress.end(); + if (!errors.isEmpty()) { + for (Error error : errors.keySet()) { + rp.sendMessage(buildError(error, errors.get(error))); + } + rp.sendMessage(String.format("User: %s", displayName(currentUser))); + rp.sendMessage(COMMAND_REJECTION_MESSAGE_FOOTER); + } + for (final ReceiveCommand c : commands) { if (c.getResult() == Result.OK) { switch (c.getType()) { @@ -502,6 +541,30 @@ public class ReceiveCommits { } } + private String buildError(Error error, List branches) { + StringBuilder sb = new StringBuilder(); + if (branches.size() == 1) { + sb.append("Branch ").append(branches.get(0)).append(":\n"); + sb.append(error.get()); + return sb.toString(); + } + sb.append("Branches"); + String delim = " "; + for (String branch : branches) { + sb.append(delim).append(branch); + delim = ", "; + } + return sb.append(":\n").append(error.get()).toString(); + } + + private static String displayName(IdentifiedUser user) { + String displayName = user.getUserName(); + if (displayName == null) { + displayName = user.getAccount().getPreferredEmail(); + } + return displayName; + } + private Account.Id toAccountId(final String nameOrEmail) throws OrmException, NoSuchAccountException { final Account a = accountResolver.findByNameOrEmail(nameOrEmail); @@ -630,6 +693,7 @@ public class ReceiveCommits { validateNewCommits(ctl, cmd); cmd.execute(rp); } else { + errors.put(Error.CREATE, ctl.getRefName()); reject(cmd, "can not create new references"); } } @@ -644,6 +708,11 @@ public class ReceiveCommits { validateNewCommits(ctl, cmd); cmd.execute(rp); } else { + if (GitRepositoryManager.REF_CONFIG.equals(ctl.getRefName())) { + errors.put(Error.CONFIG_UPDATE, GitRepositoryManager.REF_CONFIG); + } else { + errors.put(Error.UPDATE, ctl.getRefName()); + } reject(cmd, "can not update the reference as a fast forward"); } } @@ -672,7 +741,12 @@ public class ReceiveCommits { if (ctl.canDelete()) { cmd.execute(rp); } else { - reject(cmd, "can not delete references"); + if (GitRepositoryManager.REF_CONFIG.equals(ctl.getRefName())) { + reject(cmd, "cannot delete project configuration"); + } else { + errors.put(Error.DELETE, ctl.getRefName()); + reject(cmd, "can not delete references"); + } } } @@ -769,7 +843,8 @@ public class ReceiveCommits { destBranchName.substring(0, split)); destBranchCtl = projectControl.controlForRef(destBranch); if (!destBranchCtl.canUpload()) { - reject(cmd, "can not upload a change to this reference"); + errors.put(Error.CODE_REVIEW, cmd.getRefName()); + reject(cmd, "can not upload review"); return; }