Allow adding groups as reviewer

On the ChangeScreen it is now possible to add a group as reviewer for
a change. When a group is added as reviewer the group is resolved and
all its members are added as reviewers to the change.

To avoid that users accidentily add groups as reviewers that have a
large amount of members, Gerrit administrators can configure a
maximum number of reviewers that can be added at once by adding a
group as reviewer. In addition Gerrit administrators can configure
that users should confirm the adding of the reviewers if the number
of reviewers that should be added is over a certain limit.

It is also possible to add the system group 'Project Owners' as
reviewer. In this case all users which own the project are added as
reviewers.

If a user and a group have the same name, only the user is added as
reviewer.

Signed-off-by: Edwin Kempin <edwin.kempin@sap.com>
Bug: issue 881
Change-Id: I78a359f89ce045a81306e0a447384c37cda07d00

25 files changed, 698 insertions, 105 deletions

diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index e3d696e1..58f238e5 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -24,6 +24,29 @@ Sample `etc/gerrit.config`:
   diskbuffer = 10 m
 ----
 
+[[addreviewer]]Section addreviewer
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+[[addreviewer.maxWithoutConfirmation]]addreviewer.maxWithoutConfirmation::
++
+The maximum number of reviewers a user can add at once by adding a
+group as reviewer without being asked to confirm the operation.
++
+If set to 0, the user will never be asked to confirm adding a group
+as reviewer.
++
+Default is 10.
+
+[[addreviewer.maxAllowed]]addreviewer.maxAllowed::
++
+The maximum number of reviewers a user can add at once by adding a
+group as reviewer.
++
+If set to 0, there is no limit for the number of reviewers that can
+be added at once by adding a group as reviewer.
++
+Default is 20.
+
 [[auth]]Section auth
 ~~~~~~~~~~~~~~~~~~~~
 
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchDetailService.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchDetailService.java
index 5aec0c76..0899b6fc 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchDetailService.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/PatchDetailService.java
@@ -50,7 +50,7 @@ public interface PatchDetailService extends RemoteJsonService {
       AsyncCallback<VoidResult> callback);
 
   @SignInRequired
-  void addReviewers(Change.Id id, List<String> reviewers,
+  void addReviewers(Change.Id id, List<String> reviewers, boolean confirmed,
       AsyncCallback<ReviewerResult> callback);
 
   @SignInRequired
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewerInfo.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewerInfo.java
new file mode 100644
index 00000000..063f13be
--- /dev/null
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewerInfo.java
@@ -0,0 +1,58 @@
+// Copyright (C) 2011 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.common.data;
+
+/**
+ * Suggested reviewer for a change. Can be a user ({@link AccountInfo}) or a
+ * group ({@link GroupReference}).
+ */
+public class ReviewerInfo implements Comparable<ReviewerInfo> {
+  private AccountInfo accountInfo;
+  private GroupReference groupReference;
+
+  protected ReviewerInfo() {
+  }
+
+  public ReviewerInfo(final AccountInfo accountInfo) {
+    this.accountInfo = accountInfo;
+  }
+
+  public ReviewerInfo(final GroupReference groupReference) {
+    this.groupReference = groupReference;
+  }
+
+  public AccountInfo getAccountInfo() {
+    return accountInfo;
+  }
+
+  public GroupReference getGroup() {
+    return groupReference;
+  }
+
+  @Override
+  public int compareTo(final ReviewerInfo o) {
+    return getSortValue().compareTo(o.getSortValue());
+  }
+
+  private String getSortValue() {
+    if (accountInfo != null) {
+      if (accountInfo.getPreferredEmail() != null) {
+        return accountInfo.getPreferredEmail();
+      }
+      return accountInfo.getFullName().toString();
+    }
+    return groupReference.getName();
+  }
+}
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewerResult.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewerResult.java
index a535c173..d85095ad 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewerResult.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/ReviewerResult.java
@@ -24,6 +24,8 @@ import java.util.List;
 public class ReviewerResult {
   protected List<Error> errors;
   protected ChangeDetail change;
+  protected int memberCount;
+  protected boolean askForConfirmation;
 
   public ReviewerResult() {
     errors = new ArrayList<Error>();
@@ -45,10 +47,26 @@ public class ReviewerResult {
     change = d;
   }
 
+  public int getMemberCount() {
+    return memberCount;
+  }
+
+  public void setMemberCount(final int memberCount) {
+    this.memberCount = memberCount;
+  }
+
+  public boolean askForConfirmation() {
+    return askForConfirmation;
+  }
+
+  public void setAskForConfirmation(final boolean askForConfirmation) {
+    this.askForConfirmation = askForConfirmation;
+  }
+
   public static class Error {
     public static enum Type {
-      /** Name supplied does not match to a registered account. */
-      ACCOUNT_NOT_FOUND,
+      /** Name supplied does not match to a registered account or account group. */
+      REVIEWER_NOT_FOUND,
 
       /** The account is inactive. */
       ACCOUNT_INACTIVE,
@@ -56,6 +74,15 @@ public class ReviewerResult {
       /** The account is not permitted to see the change. */
       CHANGE_NOT_VISIBLE,
 
+      /** The groups has no members. */
+      GROUP_EMPTY,
+
+      /** The groups has too many members. */
+      GROUP_HAS_TOO_MANY_MEMBERS,
+
+      /** The group is not allowed to be added as reviewer. */
+      GROUP_NOT_ALLOWED,
+
       /** Could not remove this reviewer from the change due to ORMException. */
       COULD_NOT_REMOVE,
 
diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/SuggestService.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/SuggestService.java
index 976004fb..12c8b60c 100644
--- a/gerrit-common/src/main/java/com/google/gerrit/common/data/SuggestService.java
+++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/SuggestService.java
@@ -14,6 +14,7 @@
 
 package com.google.gerrit.common.data;
 
+import com.google.gerrit.reviewdb.AccountGroup;
 import com.google.gerrit.reviewdb.Project;
 import com.google.gwt.user.client.rpc.AsyncCallback;
 import com.google.gwtjsonrpc.client.RemoteJsonService;
@@ -32,4 +33,15 @@ public interface SuggestService extends RemoteJsonService {
 
   void suggestAccountGroup(String query, int limit,
       AsyncCallback<List<GroupReference>> callback);
+
+  /**
+   * Suggests reviewers. A reviewer can be a user or a group. Inactive users,
+   * the system groups {@link AccountGroup#ANONYMOUS_USERS} and
+   * {@link AccountGroup#REGISTERED_USERS} and groups that have more than the
+   * configured <code>addReviewer.maxAllowed</code> members are not suggested as
+   * reviewers.
+   * @param project the project for which reviewers should be suggested
+   */
+  void suggestReviewer(Project.NameKey project, String query, int limit,
+      AsyncCallback<List<ReviewerInfo>> callback);
 }
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.java
index 712dee61..4b3ea97a 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.java
@@ -46,6 +46,7 @@ public interface AdminConstants extends Constants {
   String emailOnlyAuthors();
   String descriptionNotifications();
   String buttonSaveGroupOptions();
+  String suggestedGroupLabel();
 
   String headingOwner();
   String headingParentProjectName();
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.properties
index 3b907ecf..86b0e405 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.properties
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminConstants.properties
@@ -23,6 +23,7 @@ requireChangeID = Require <a href="http://gerrit.googlecode.com/svn/documentatio
 headingGroupOptions = Group Options
 isVisibleToAll = Make group visible to all registered users.
 buttonSaveGroupOptions = Save Group Options
+suggestedGroupLabel = group
 
 emailOnlyAuthors = Authors
 descriptionNotifications = Send email notifications about comments and actions by users in this group only to:
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java
index 2e16addb..24908f49 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ApprovalTable.java
@@ -14,13 +14,15 @@
 
 package com.google.gerrit.client.changes;
 
+import com.google.gerrit.client.ConfirmationCallback;
+import com.google.gerrit.client.ConfirmationDialog;
 import com.google.gerrit.client.ErrorDialog;
 import com.google.gerrit.client.FormatUtil;
 import com.google.gerrit.client.Gerrit;
 import com.google.gerrit.client.patches.PatchUtil;
 import com.google.gerrit.client.rpc.GerritCallback;
+import com.google.gerrit.client.ui.ReviewerSuggestOracle;
 import com.google.gerrit.client.ui.AccountDashboardLink;
-import com.google.gerrit.client.ui.AccountSuggestOracle;
 import com.google.gerrit.client.ui.AddMemberBox;
 import com.google.gerrit.common.data.AccountInfoCache;
 import com.google.gerrit.common.data.ApprovalDetail;
@@ -39,11 +41,12 @@ import com.google.gwt.user.client.Element;
 import com.google.gwt.user.client.ui.Composite;
 import com.google.gwt.user.client.ui.FlowPanel;
 import com.google.gwt.user.client.ui.Grid;
+import com.google.gwt.user.client.ui.HTML;
+import com.google.gwt.user.client.ui.HTMLTable.CellFormatter;
 import com.google.gwt.user.client.ui.Image;
 import com.google.gwt.user.client.ui.Panel;
 import com.google.gwt.user.client.ui.PushButton;
 import com.google.gwt.user.client.ui.Widget;
-import com.google.gwt.user.client.ui.HTMLTable.CellFormatter;
 import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder;
 
 import java.util.ArrayList;
@@ -59,6 +62,7 @@ public class ApprovalTable extends Composite {
   private final Grid table;
   private final Widget missing;
   private final Panel addReviewer;
+  private final ReviewerSuggestOracle reviewerSuggestOracle;
   private final AddMemberBox addMemberBox;
   private Change.Id changeId;
   private AccountInfoCache accountCache = AccountInfoCache.empty();
@@ -77,10 +81,10 @@ public class ApprovalTable extends Composite {
 
     addReviewer = new FlowPanel();
     addReviewer.setStyleName(Gerrit.RESOURCES.css().addReviewer());
+    reviewerSuggestOracle = new ReviewerSuggestOracle();
     addMemberBox =
-      new AddMemberBox(Util.C.approvalTableAddReviewer(),
-          Util.C.approvalTableAddReviewerHint(),
-          new AccountSuggestOracle());
+        new AddMemberBox(Util.C.approvalTableAddReviewer(),
+            Util.C.approvalTableAddReviewerHint(), reviewerSuggestOracle);
     addMemberBox.addClickHandler(new ClickHandler() {
       @Override
       public void onClick(final ClickEvent event) {
@@ -131,6 +135,8 @@ public class ApprovalTable extends Composite {
   }
 
   void display(ChangeDetail detail) {
+    reviewerSuggestOracle.setProject(detail.getChange().getProject());
+
     List<String> columns = new ArrayList<String>();
     List<ApprovalDetail> rows = detail.getApprovals();
 
@@ -242,27 +248,38 @@ public class ApprovalTable extends Composite {
   }
 
   private void doAddReviewer() {
-    final String nameEmail = addMemberBox.getText();
-    if (nameEmail.length() == 0) {
+    final String reviewer = addMemberBox.getText();
+    if (reviewer.length() == 0) {
       return;
     }
 
     addMemberBox.setEnabled(false);
     final List<String> reviewers = new ArrayList<String>();
-    reviewers.add(nameEmail);
+    reviewers.add(reviewer);
+
+    addReviewers(reviewers, false);
+  }
 
-    PatchUtil.DETAIL_SVC.addReviewers(changeId, reviewers,
+  private void addReviewers(final List<String> reviewers,
+      final boolean confirmed) {
+    PatchUtil.DETAIL_SVC.addReviewers(changeId, reviewers, confirmed,
         new GerritCallback<ReviewerResult>() {
           public void onSuccess(final ReviewerResult result) {
             addMemberBox.setEnabled(true);
             addMemberBox.setText("");
 
+            final ChangeDetail changeDetail = result.getChange();
+            if (changeDetail != null) {
+              setAccountInfoCache(changeDetail.getAccounts());
+              display(changeDetail);
+            }
+
             if (!result.getErrors().isEmpty()) {
               final SafeHtmlBuilder r = new SafeHtmlBuilder();
               for (final ReviewerResult.Error e : result.getErrors()) {
                 switch (e.getType()) {
-                  case ACCOUNT_NOT_FOUND:
-                    r.append(Util.M.accountNotFound(e.getName()));
+                  case REVIEWER_NOT_FOUND:
+                    r.append(Util.M.reviewerNotFound(e.getName()));
                     break;
 
                   case ACCOUNT_INACTIVE:
@@ -273,6 +290,23 @@ public class ApprovalTable extends Composite {
                     r.append(Util.M.changeNotVisibleTo(e.getName()));
                     break;
 
+                  case GROUP_EMPTY:
+                    r.append(Util.M.groupIsEmpty(e.getName()));
+                    break;
+
+                  case GROUP_HAS_TOO_MANY_MEMBERS:
+                    if (result.askForConfirmation() && !confirmed) {
+                      askForConfirmation(e.getName(), result.getMemberCount());
+                      return;
+                    } else {
+                      r.append(Util.M.groupHasTooManyMembers(e.getName()));
+                    }
+                    break;
+
+                  case GROUP_NOT_ALLOWED:
+                    r.append(Util.M.groupIsNotAllowed(e.getName()));
+                    break;
+
                   default:
                     r.append(e.getName());
                     r.append(" - ");
@@ -283,14 +317,26 @@ public class ApprovalTable extends Composite {
               }
               new ErrorDialog(r).center();
             }
-
-            final ChangeDetail r = result.getChange();
-            if (r != null) {
-              setAccountInfoCache(r.getAccounts());
-              display(r);
-            }
           }
 
+          private void askForConfirmation(final String groupName,
+              final int memberCount) {
+            final StringBuilder message = new StringBuilder();
+            message.append("<b>");
+            message.append(Util.M.groupManyMembersConfirmation(groupName,
+                memberCount));
+            message.append("</b>");
+            final ConfirmationDialog confirmationDialog =
+                new ConfirmationDialog(Util.C
+                    .approvalTableAddManyReviewersConfirmationDialogTitle(),
+                    new HTML(message.toString()), new ConfirmationCallback() {
+                      @Override
+                      public void onOk() {
+                        addReviewers(reviewers, true);
+                      }
+                    });
+            confirmationDialog.center();
+          }
 
           @Override
           public void onFailure(final Throwable caught) {
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java
index d9bd6a6f..f80c0a31 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.java
@@ -81,6 +81,7 @@ public interface ChangeConstants extends Constants {
   String approvalTableRemoveNotPermitted();
   String approvalTableCouldNotRemove();
   String approvalTableAddReviewerHint();
+  String approvalTableAddManyReviewersConfirmationDialogTitle();
 
   String changeInfoBlockOwner();
   String changeInfoBlockProject();
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties
index ee452588..9223d2e9 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeConstants.properties
@@ -57,7 +57,8 @@ approvalTableReviewer = Reviewer
 approvalTableAddReviewer = Add Reviewer
 approvalTableRemoveNotPermitted = Not allowed to remove reviewer
 approvalTableCouldNotRemove = Could not remove reviewer
-approvalTableAddReviewerHint = Username or Email
+approvalTableAddReviewerHint = Name or Email or Group
+approvalTableAddManyReviewersConfirmationDialogTitle = Adding many reviewers
 
 changeInfoBlockOwner = Owner
 changeInfoBlockProject = Project
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.java
index 728d5673..41dc6c1a 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.java
@@ -51,9 +51,13 @@ public interface ChangeMessages extends Messages {
   String changeQueryWindowTitle(String query);
   String changeQueryPageTitle(String query);
 
-  String accountNotFound(String who);
+  String reviewerNotFound(String who);
   String accountInactive(String who);
   String changeNotVisibleTo(String who);
+  String groupIsEmpty(String group);
+  String groupIsNotAllowed(String group);
+  String groupHasTooManyMembers(String group);
+  String groupManyMembersConfirmation(String group, int memberCount);
 
   String anonymousDownload(String protocol);
 }
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.properties
index 3b908f31..d41b6e6a 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.properties
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeMessages.properties
@@ -32,8 +32,12 @@ lineHeader = Line {0}:
 changeQueryWindowTitle = {0}
 changeQueryPageTitle = Search for {0}
 
-accountNotFound = {0} is not a registered user.
+reviewerNotFound = {0} is neither a registered user nor a group.
 accountInactive = {0} is not an active user.
 changeNotVisibleTo = {0} cannot access the change.
+groupIsEmpty = {0} has no members.
+groupIsNotAllowed = {0} cannot be added as reviewer.
+groupHasTooManyMembers = {0} has too many members.
+groupManyMembersConfirmation = {0} has {1} members. Do you want to add them all as reviewers?
 
 anonymousDownload = Anonymous {0}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ReviewerSuggestOracle.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ReviewerSuggestOracle.java
new file mode 100644
index 00000000..75a3b834
--- /dev/null
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ReviewerSuggestOracle.java
@@ -0,0 +1,83 @@
+// Copyright (C) 2011 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.client.ui;
+
+import com.google.gerrit.client.FormatUtil;
+import com.google.gerrit.client.RpcStatus;
+import com.google.gerrit.client.admin.Util;
+import com.google.gerrit.client.rpc.GerritCallback;
+import com.google.gerrit.common.data.AccountInfo;
+import com.google.gerrit.common.data.ReviewerInfo;
+import com.google.gerrit.reviewdb.Project;
+import com.google.gwt.user.client.ui.SuggestOracle;
+import com.google.gwtexpui.safehtml.client.HighlightSuggestOracle;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/** Suggestion Oracle for reviewers. */
+public class ReviewerSuggestOracle extends HighlightSuggestOracle {
+
+  private Project.NameKey project;
+
+  @Override
+  protected void onRequestSuggestions(final Request req, final Callback callback) {
+    RpcStatus.hide(new Runnable() {
+      public void run() {
+        SuggestUtil.SVC.suggestReviewer(project, req.getQuery(),
+            req.getLimit(), new GerritCallback<List<ReviewerInfo>>() {
+              public void onSuccess(final List<ReviewerInfo> result) {
+                final List<ReviewerSuggestion> r =
+                    new ArrayList<ReviewerSuggestion>(result
+                        .size());
+                for (final ReviewerInfo reviewer : result) {
+                  r.add(new ReviewerSuggestion(reviewer));
+                }
+                callback.onSuggestionsReady(req, new Response(r));
+              }
+            });
+      }
+    });
+  }
+
+  public void setProject(final Project.NameKey project) {
+    this.project = project;
+  }
+
+  private static class ReviewerSuggestion implements SuggestOracle.Suggestion {
+    private final ReviewerInfo reviewerInfo;
+
+    ReviewerSuggestion(final ReviewerInfo reviewerInfo) {
+      this.reviewerInfo = reviewerInfo;
+    }
+
+    public String getDisplayString() {
+      final AccountInfo accountInfo = reviewerInfo.getAccountInfo();
+      if (accountInfo != null) {
+        return FormatUtil.nameEmail(accountInfo);
+      }
+      return reviewerInfo.getGroup().getName() + " ("
+          + Util.C.suggestedGroupLabel() + ")";
+    }
+
+    public String getReplacementString() {
+      final AccountInfo accountInfo = reviewerInfo.getAccountInfo();
+      if (accountInfo != null) {
+        return FormatUtil.nameEmail(accountInfo);
+      }
+      return reviewerInfo.getGroup().getName();
+    }
+  }
+}
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/SuggestServiceImpl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/SuggestServiceImpl.java
index bc05ec6a..273bd9d6 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/SuggestServiceImpl.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/SuggestServiceImpl.java
@@ -16,6 +16,7 @@ package com.google.gerrit.httpd.rpc;
 
 import com.google.gerrit.common.data.AccountInfo;
 import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.common.data.ReviewerInfo;
 import com.google.gerrit.common.data.SuggestService;
 import com.google.gerrit.common.errors.NoSuchGroupException;
 import com.google.gerrit.reviewdb.Account;
@@ -29,7 +30,10 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.GroupCache;
 import com.google.gerrit.server.account.GroupControl;
+import com.google.gerrit.server.account.GroupMembersFactory;
+import com.google.gerrit.server.account.GroupMembersFactory.Factory;
 import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.patch.AddReviewer;
 import com.google.gerrit.server.project.NoSuchProjectException;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.ProjectControl;
@@ -56,29 +60,34 @@ class SuggestServiceImpl extends BaseServiceImplementation implements
   private final ProjectCache projectCache;
   private final AccountCache accountCache;
   private final GroupControl.Factory groupControlFactory;
+  private final Factory groupMembersFactory;
   private final IdentifiedUser.GenericFactory userFactory;
   private final Provider<CurrentUser> currentUser;
   private final SuggestAccountsEnum suggestAccounts;
+  private final Config cfg;
   private final GroupCache groupCache;
 
+
   @Inject
   SuggestServiceImpl(final Provider<ReviewDb> schema,
       final ProjectControl.Factory projectControlFactory,
       final ProjectCache projectCache, final AccountCache accountCache,
       final GroupControl.Factory groupControlFactory,
+      final GroupMembersFactory.Factory groupMembersFactory,
       final IdentifiedUser.GenericFactory userFactory,
       final Provider<CurrentUser> currentUser,
-      @GerritServerConfig final Config cfg,
-      final GroupCache groupCache) {
+      @GerritServerConfig final Config cfg, final GroupCache groupCache) {
     super(schema, currentUser);
     this.projectControlFactory = projectControlFactory;
     this.projectCache = projectCache;
     this.accountCache = accountCache;
     this.groupControlFactory = groupControlFactory;
+    this.groupMembersFactory = groupMembersFactory;
     this.userFactory = userFactory;
     this.currentUser = currentUser;
     this.suggestAccounts =
         cfg.getEnum("suggest", null, "accounts", SuggestAccountsEnum.ALL);
+    this.cfg = cfg;
     this.groupCache = groupCache;
   }
 
@@ -106,43 +115,48 @@ class SuggestServiceImpl extends BaseServiceImplementation implements
 
   public void suggestAccount(final String query, final Boolean active,
       final int limit, final AsyncCallback<List<AccountInfo>> callback) {
+    run(callback, new Action<List<AccountInfo>>() {
+      public List<AccountInfo> run(final ReviewDb db) throws OrmException {
+        return suggestAccount(db, query, active, limit);
+      }
+    });
+  }
+
+  private List<AccountInfo> suggestAccount(final ReviewDb db,
+      final String query, final Boolean active, final int limit)
+      throws OrmException {
     if (suggestAccounts == SuggestAccountsEnum.OFF) {
-      callback.onSuccess(Collections.<AccountInfo> emptyList());
-      return;
+      return Collections.<AccountInfo> emptyList();
     }
 
-    run(callback, new Action<List<AccountInfo>>() {
-      public List<AccountInfo> run(final ReviewDb db) throws OrmException {
-        final String a = query;
-        final String b = a + MAX_SUFFIX;
-        final int max = 10;
-        final int n = limit <= 0 ? max : Math.min(limit, max);
-
-        final LinkedHashMap<Account.Id, AccountInfo> r =
-            new LinkedHashMap<Account.Id, AccountInfo>();
-        for (final Account p : db.accounts().suggestByFullName(a, b, n)) {
-          addSuggestion(r, p, new AccountInfo(p), active);
-        }
-        if (r.size() < n) {
-          for (final Account p : db.accounts().suggestByPreferredEmail(a, b,
-              n - r.size())) {
-            addSuggestion(r, p, new AccountInfo(p), active);
-          }
-        }
-        if (r.size() < n) {
-          for (final AccountExternalId e : db.accountExternalIds()
-              .suggestByEmailAddress(a, b, n - r.size())) {
-            if (!r.containsKey(e.getAccountId())) {
-              final Account p = accountCache.get(e.getAccountId()).getAccount();
-              final AccountInfo info = new AccountInfo(p);
-              info.setPreferredEmail(e.getEmailAddress());
-              addSuggestion(r, p, info, active);
-            }
-          }
+    final String a = query;
+    final String b = a + MAX_SUFFIX;
+    final int max = 10;
+    final int n = limit <= 0 ? max : Math.min(limit, max);
+
+    final LinkedHashMap<Account.Id, AccountInfo> r =
+        new LinkedHashMap<Account.Id, AccountInfo>();
+    for (final Account p : db.accounts().suggestByFullName(a, b, n)) {
+      addSuggestion(r, p, new AccountInfo(p), active);
+    }
+    if (r.size() < n) {
+      for (final Account p : db.accounts().suggestByPreferredEmail(a, b,
+          n - r.size())) {
+        addSuggestion(r, p, new AccountInfo(p), active);
+      }
+    }
+    if (r.size() < n) {
+      for (final AccountExternalId e : db.accountExternalIds()
+          .suggestByEmailAddress(a, b, n - r.size())) {
+        if (!r.containsKey(e.getAccountId())) {
+          final Account p = accountCache.get(e.getAccountId()).getAccount();
+          final AccountInfo info = new AccountInfo(p);
+          info.setPreferredEmail(e.getEmailAddress());
+          addSuggestion(r, p, info, active);
         }
-        return new ArrayList<AccountInfo>(r.values());
       }
-    });
+    }
+    return new ArrayList<AccountInfo>(r.values());
   }
 
   private void addSuggestion(Map<Account.Id, AccountInfo> map, Account account,
@@ -201,26 +215,90 @@ class SuggestServiceImpl extends BaseServiceImplementation implements
       final AsyncCallback<List<GroupReference>> callback) {
     run(callback, new Action<List<GroupReference>>() {
       public List<GroupReference> run(final ReviewDb db) throws OrmException {
-        final String a = query;
-        final String b = a + MAX_SUFFIX;
-        final int max = 10;
-        final int n = limit <= 0 ? max : Math.min(limit, max);
-        List<GroupReference> r = new ArrayList<GroupReference>(n);
-        for (AccountGroupName group : db.accountGroupNames()
-              .suggestByName(a, b, n)) {
-          try {
-            if (groupControlFactory.controlFor(group.getId()).isVisible()) {
-              AccountGroup g = groupCache.get(group.getId());
-              if (g != null && g.getGroupUUID() != null) {
-                r.add(GroupReference.forGroup(g));
-              }
-            }
-          } catch (NoSuchGroupException e) {
-            continue;
+        return suggestAccountGroup(db, query, limit);
+      }
+    });
+  }
+
+  private List<GroupReference> suggestAccountGroup(final ReviewDb db,
+      final String query, final int limit) throws OrmException {
+    final String a = query;
+    final String b = a + MAX_SUFFIX;
+    final int max = 10;
+    final int n = limit <= 0 ? max : Math.min(limit, max);
+    List<GroupReference> r = new ArrayList<GroupReference>(n);
+    for (AccountGroupName group : db.accountGroupNames().suggestByName(a, b, n)) {
+      try {
+        if (groupControlFactory.controlFor(group.getId()).isVisible()) {
+          AccountGroup g = groupCache.get(group.getId());
+          if (g != null && g.getGroupUUID() != null) {
+            r.add(GroupReference.forGroup(g));
           }
         }
-        return r;
+      } catch (NoSuchGroupException e) {
+        continue;
+      }
+    }
+    return r;
+  }
+
+  @Override
+  public void suggestReviewer(final Project.NameKey project,
+      final String query, final int limit,
+      final AsyncCallback<List<ReviewerInfo>> callback) {
+    run(callback, new Action<List<ReviewerInfo>>() {
+      public List<ReviewerInfo> run(final ReviewDb db) throws OrmException {
+        final List<AccountInfo> suggestedAccounts =
+            suggestAccount(db, query, Boolean.TRUE, limit);
+        final List<ReviewerInfo> reviewer =
+            new ArrayList<ReviewerInfo>(suggestedAccounts.size());
+        for (final AccountInfo a : suggestedAccounts) {
+          reviewer.add(new ReviewerInfo(a));
+        }
+        final List<GroupReference> suggestedAccountGroups =
+            suggestAccountGroup(db, query, limit);
+        for (final GroupReference g : suggestedAccountGroups) {
+          if (suggestGroupAsReviewer(project, g)) {
+            reviewer.add(new ReviewerInfo(g));
+          }
+        }
+
+        Collections.sort(reviewer);
+        if (reviewer.size() <= limit) {
+          return reviewer;
+        } else {
+          return reviewer.subList(0, limit);
+        }
       }
     });
   }
-}
+
+  private boolean suggestGroupAsReviewer(final Project.NameKey project,
+      final GroupReference group) throws OrmException {
+    if (!AddReviewer.isLegalReviewerGroup(group.getUUID())) {
+      return false;
+    }
+
+    try {
+      final Set<Account> members =
+          groupMembersFactory.create(project, group.getUUID()).call();
+
+      if (members.isEmpty()) {
+        return false;
+      }
+
+      final int maxAllowed =
+          cfg.getInt("addreviewer", "maxAllowed",
+              AddReviewer.DEFAULT_MAX_REVIEWERS);
+      if (maxAllowed > 0 && members.size() > maxAllowed) {
+        return false;
+      }
+    } catch (NoSuchGroupException e) {
+      return false;
+    } catch (NoSuchProjectException e) {
+      return false;
+    }
+
+    return true;
+  }
+}
\ No newline at end of file
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/AccountModule.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/AccountModule.java
index 13dd9b43..91c8aa92 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/AccountModule.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/AccountModule.java
@@ -32,7 +32,7 @@ public class AccountModule extends RpcServletModule {
         factory(CreateGroup.Factory.class);
         factory(DeleteExternalIds.Factory.class);
         factory(ExternalIdDetailFactory.Factory.class);
-        factory(GroupDetailFactory.Factory.class);
+        factory(GroupDetailHandler.Factory.class);
         factory(MyGroupsFactory.Factory.class);
         factory(RenameGroup.Factory.class);
       }
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupAdminServiceImpl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupAdminServiceImpl.java
index f89ef095..157c8962 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupAdminServiceImpl.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupAdminServiceImpl.java
@@ -63,7 +63,7 @@ class GroupAdminServiceImpl extends BaseServiceImplementation implements
 
   private final CreateGroup.Factory createGroupFactory;
   private final RenameGroup.Factory renameGroupFactory;
-  private final GroupDetailFactory.Factory groupDetailFactory;
+  private final GroupDetailHandler.Factory groupDetailFactory;
 
   @Inject
   GroupAdminServiceImpl(final Provider<ReviewDb> schema,
@@ -75,7 +75,7 @@ class GroupAdminServiceImpl extends BaseServiceImplementation implements
       final GroupControl.Factory groupControlFactory,
       final CreateGroup.Factory createGroupFactory,
       final RenameGroup.Factory renameGroupFactory,
-      final GroupDetailFactory.Factory groupDetailFactory) {
+      final GroupDetailHandler.Factory groupDetailFactory) {
     super(schema, currentUser);
     this.identifiedUser = currentUser;
     this.accountCache = accountCache;
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupDetailHandler.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupDetailHandler.java
new file mode 100644
index 00000000..513c90f7
--- /dev/null
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupDetailHandler.java
@@ -0,0 +1,46 @@
+// Copyright (C) 2011 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.httpd.rpc.account;
+
+import com.google.gerrit.common.data.GroupDetail;
+import com.google.gerrit.common.errors.NoSuchGroupException;
+import com.google.gerrit.httpd.rpc.Handler;
+import com.google.gerrit.reviewdb.AccountGroup;
+import com.google.gerrit.server.account.GroupDetailFactory;
+import com.google.gwtorm.client.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
+
+public class GroupDetailHandler extends Handler<GroupDetail> {
+  public interface Factory {
+    GroupDetailHandler create(AccountGroup.Id groupId);
+  }
+
+  private final GroupDetailFactory.Factory groupDetailFactory;
+
+  private final AccountGroup.Id groupId;
+
+  @Inject
+  GroupDetailHandler(final GroupDetailFactory.Factory groupDetailFactory,
+      @Assisted final AccountGroup.Id groupId) {
+    this.groupDetailFactory = groupDetailFactory;
+    this.groupId = groupId;
+  }
+
+  @Override
+  public GroupDetail call() throws OrmException, NoSuchGroupException {
+    return groupDetailFactory.create(groupId).call();
+  }
+}
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/RenameGroup.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/RenameGroup.java
index c8bf519b..d7cd195b 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/RenameGroup.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/RenameGroup.java
@@ -24,6 +24,7 @@ import com.google.gerrit.reviewdb.ReviewDb;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.GroupCache;
 import com.google.gerrit.server.account.GroupControl;
+import com.google.gerrit.server.account.GroupDetailFactory;
 import com.google.gerrit.server.git.RenameGroupOp;
 import com.google.gwtorm.client.OrmDuplicateKeyException;
 import com.google.gwtorm.client.OrmException;
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/AddReviewerHandler.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/AddReviewerHandler.java
index 4765eac5..953657b2 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/AddReviewerHandler.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/AddReviewerHandler.java
@@ -26,7 +26,8 @@ import java.util.Collection;
 
 class AddReviewerHandler extends Handler<ReviewerResult> {
   interface Factory {
-    AddReviewerHandler create(Change.Id changeId, Collection<String> nameOrEmails);
+    AddReviewerHandler create(Change.Id changeId, Collection<String> reviewers,
+        boolean confirmed);
   }
 
   private final AddReviewer.Factory addReviewerFactory;
@@ -34,23 +35,26 @@ class AddReviewerHandler extends Handler<ReviewerResult> {
 
   private final Change.Id changeId;
   private final Collection<String> reviewers;
+  private final boolean confirmed;
 
   @Inject
   AddReviewerHandler(final AddReviewer.Factory addReviewerFactory,
       final ChangeDetailFactory.Factory changeDetailFactory,
       @Assisted final Change.Id changeId,
-      @Assisted final Collection<String> nameOrEmails) {
+      @Assisted final Collection<String> reviewers,
+      @Assisted final boolean confirmed) {
 
     this.addReviewerFactory = addReviewerFactory;
     this.changeDetailFactory = changeDetailFactory;
 
     this.changeId = changeId;
-    this.reviewers = nameOrEmails;
+    this.reviewers = reviewers;
+    this.confirmed = confirmed;
   }
 
   @Override
   public ReviewerResult call() throws Exception {
-    ReviewerResult result = addReviewerFactory.create(changeId, reviewers).call();
+    ReviewerResult result = addReviewerFactory.create(changeId, reviewers, confirmed).call();
     result.setChange(changeDetailFactory.create(changeId).call());
     return result;
   }
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java
index 59755e35..0f3b4f8e 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java
@@ -155,8 +155,8 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
   }
 
   public void addReviewers(final Change.Id id, final List<String> reviewers,
-      final AsyncCallback<ReviewerResult> callback) {
-    addReviewerHandlerFactory.create(id, reviewers).to(callback);
+      final boolean confirmed, final AsyncCallback<ReviewerResult> callback) {
+    addReviewerHandlerFactory.create(id, reviewers, confirmed).to(callback);
   }
 
   public void removeReviewer(final Change.Id id, final Account.Id reviewerId,
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupDetailFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupDetailFactory.java
index da6ab65e..d4c85760 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupDetailFactory.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupDetailFactory.java
@@ -12,20 +12,15 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package com.google.gerrit.httpd.rpc.account;
+package com.google.gerrit.server.account;
 
 import com.google.gerrit.common.data.GroupDetail;
 import com.google.gerrit.common.errors.NoSuchGroupException;
-import com.google.gerrit.httpd.rpc.Handler;
 import com.google.gerrit.reviewdb.Account;
 import com.google.gerrit.reviewdb.AccountGroup;
 import com.google.gerrit.reviewdb.AccountGroupInclude;
 import com.google.gerrit.reviewdb.AccountGroupMember;
 import com.google.gerrit.reviewdb.ReviewDb;
-import com.google.gerrit.server.account.AccountInfoCacheFactory;
-import com.google.gerrit.server.account.GroupCache;
-import com.google.gerrit.server.account.GroupControl;
-import com.google.gerrit.server.account.GroupInfoCacheFactory;
 import com.google.gwtorm.client.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
@@ -34,9 +29,10 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
+import java.util.concurrent.Callable;
 
-class GroupDetailFactory extends Handler<GroupDetail> {
-  interface Factory {
+public class GroupDetailFactory implements Callable<GroupDetail> {
+  public interface Factory {
     GroupDetailFactory create(AccountGroup.Id groupId);
   }
 
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupMembersFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupMembersFactory.java
new file mode 100644
index 00000000..aae3a909
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupMembersFactory.java
@@ -0,0 +1,122 @@
+// Copyright (C) 2011 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.account;
+
+import com.google.gerrit.common.data.GroupDetail;
+import com.google.gerrit.common.errors.NoSuchGroupException;
+import com.google.gerrit.reviewdb.Account;
+import com.google.gerrit.reviewdb.AccountGroup;
+import com.google.gerrit.reviewdb.AccountGroupInclude;
+import com.google.gerrit.reviewdb.AccountGroupMember;
+import com.google.gerrit.reviewdb.Project;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.project.NoSuchProjectException;
+import com.google.gerrit.server.project.ProjectControl;
+import com.google.gwtorm.client.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
+
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.concurrent.Callable;
+
+public class GroupMembersFactory implements Callable<Set<Account>> {
+  public interface Factory {
+    GroupMembersFactory create(Project.NameKey project,
+        AccountGroup.UUID groupUUID);
+  }
+
+  private GroupCache groupCache;
+  private final GroupDetailFactory.Factory groupDetailFactory;
+  private final AccountCache accountCache;
+  private final ProjectControl.GenericFactory projectControl;
+  private final IdentifiedUser currentUser;
+
+  private final Project.NameKey project;
+  private final AccountGroup.UUID groupUUID;
+
+
+  @Inject
+  GroupMembersFactory(final GroupCache groupCache,
+      final GroupDetailFactory.Factory groupDetailFactory,
+      final AccountCache accountCache,
+      final ProjectControl.GenericFactory projectControl,
+      final IdentifiedUser currentUser,
+      @Assisted final Project.NameKey project,
+      @Assisted final AccountGroup.UUID groupUUID) {
+    this.groupCache = groupCache;
+    this.groupDetailFactory = groupDetailFactory;
+    this.accountCache = accountCache;
+    this.projectControl = projectControl;
+    this.currentUser = currentUser;
+
+    this.project = project;
+    this.groupUUID = groupUUID;
+  }
+
+  @Override
+  public Set<Account> call() throws NoSuchGroupException,
+      NoSuchProjectException, OrmException {
+    if (AccountGroup.PROJECT_OWNERS.equals(groupUUID)) {
+      return getProjectOwners();
+    }
+
+    return getAllGroupMembers(groupCache.get(groupUUID),
+        new HashSet<AccountGroup.Id>());
+  }
+
+  private Set<Account> getProjectOwners() throws NoSuchProjectException,
+      NoSuchGroupException, OrmException {
+    if (project == null) {
+      return Collections.emptySet();
+    }
+
+    final Set<AccountGroup.UUID> ownerGroups =
+        projectControl.controlFor(project, currentUser).getProjectState()
+            .getOwners();
+
+    final HashSet<Account> projectOwners = new HashSet<Account>();
+    for (final AccountGroup.UUID ownerGroup : ownerGroups) {
+      projectOwners.addAll(getAllGroupMembers(groupCache.get(ownerGroup),
+          new HashSet<AccountGroup.Id>()));
+    }
+    return projectOwners;
+  }
+
+  private Set<Account> getAllGroupMembers(final AccountGroup group,
+      final Set<AccountGroup.Id> seen) throws NoSuchGroupException,
+      OrmException {
+    seen.add(group.getId());
+    final GroupDetail groupDetail =
+        groupDetailFactory.create(group.getId()).call();
+
+    final Set<Account> members = new HashSet<Account>();
+    if (groupDetail.members != null) {
+      for (final AccountGroupMember member : groupDetail.members) {
+        members.add(accountCache.get(member.getAccountId()).getAccount());
+      }
+    }
+    if (groupDetail.includes != null) {
+      for (AccountGroupInclude groupInclude : groupDetail.includes) {
+        if (!seen.contains(groupInclude.getIncludeId())) {
+          members.addAll(getAllGroupMembers(
+              groupCache.get(groupInclude.getIncludeId()), seen));
+        }
+      }
+    }
+    return members;
+  }
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java
index 72dcd31d..3ba9aab4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java
@@ -22,6 +22,8 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.RequestCleanup;
 import com.google.gerrit.server.account.AccountResolver;
 import com.google.gerrit.server.account.GroupControl;
+import com.google.gerrit.server.account.GroupDetailFactory;
+import com.google.gerrit.server.account.GroupMembersFactory;
 import com.google.gerrit.server.account.PerformCreateGroup;
 import com.google.gerrit.server.git.CreateCodeReviewNotes;
 import com.google.gerrit.server.git.MergeOp;
@@ -87,5 +89,7 @@ public class GerritRequestModule extends FactoryModule {
     factory(MergeFailSender.Factory.class);
     factory(RegisterNewEmailSender.Factory.class);
     factory(PerformCreateGroup.Factory.class);
+    factory(GroupDetailFactory.Factory.class);
+    factory(GroupMembersFactory.Factory.class);
   }
 }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/AddReviewer.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/AddReviewer.java
index 4cb31987..be0ddacf 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/AddReviewer.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/AddReviewer.java
@@ -19,6 +19,7 @@ import com.google.gerrit.common.data.ApprovalType;
 import com.google.gerrit.common.data.ApprovalTypes;
 import com.google.gerrit.common.data.ReviewerResult;
 import com.google.gerrit.reviewdb.Account;
+import com.google.gerrit.reviewdb.AccountGroup;
 import com.google.gerrit.reviewdb.ApprovalCategory;
 import com.google.gerrit.reviewdb.Change;
 import com.google.gerrit.reviewdb.PatchSet;
@@ -26,12 +27,17 @@ import com.google.gerrit.reviewdb.PatchSetApproval;
 import com.google.gerrit.reviewdb.ReviewDb;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountResolver;
+import com.google.gerrit.server.account.GroupCache;
+import com.google.gerrit.server.account.GroupMembersFactory;
+import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.mail.AddReviewerSender;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gwtorm.client.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
 
+import org.eclipse.jgit.lib.Config;
+
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashSet;
@@ -40,42 +46,56 @@ import java.util.Set;
 import java.util.concurrent.Callable;
 
 public class AddReviewer implements Callable<ReviewerResult> {
+  public final static int DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK = 10;
+  public final static int DEFAULT_MAX_REVIEWERS = 20;
+
   public interface Factory {
-    AddReviewer create(Change.Id changeId, Collection<String> nameOrEmails);
+    AddReviewer create(Change.Id changeId,
+        Collection<String> userNameOrEmailOrGroupNames, boolean confirmed);
   }
 
   private final AddReviewerSender.Factory addReviewerSenderFactory;
   private final AccountResolver accountResolver;
+  private final GroupCache groupCache;
+  private final GroupMembersFactory.Factory groupMembersFactory;
   private final ChangeControl.Factory changeControlFactory;
   private final ReviewDb db;
   private final IdentifiedUser currentUser;
   private final IdentifiedUser.GenericFactory identifiedUserFactory;
   private final ApprovalCategory.Id addReviewerCategoryId;
+  private final Config cfg;
 
   private final Change.Id changeId;
   private final Collection<String> reviewers;
+  private final boolean confirmed;
 
   @Inject
   AddReviewer(final AddReviewerSender.Factory addReviewerSenderFactory,
-      final AccountResolver accountResolver,
+      final AccountResolver accountResolver, final GroupCache groupCache,
+      final GroupMembersFactory.Factory groupMembersFactory,
       final ChangeControl.Factory changeControlFactory, final ReviewDb db,
       final IdentifiedUser.GenericFactory identifiedUserFactory,
       final IdentifiedUser currentUser, final ApprovalTypes approvalTypes,
-      @Assisted final Change.Id changeId,
-      @Assisted final Collection<String> nameOrEmails) {
+      final @GerritServerConfig Config cfg, @Assisted final Change.Id changeId,
+      @Assisted final Collection<String> reviewers,
+      @Assisted final boolean confirmed) {
     this.addReviewerSenderFactory = addReviewerSenderFactory;
     this.accountResolver = accountResolver;
+    this.groupCache = groupCache;
+    this.groupMembersFactory = groupMembersFactory;
     this.db = db;
     this.changeControlFactory = changeControlFactory;
     this.identifiedUserFactory = identifiedUserFactory;
     this.currentUser = currentUser;
+    this.cfg = cfg;
 
     final List<ApprovalType> allTypes = approvalTypes.getApprovalTypes();
     addReviewerCategoryId =
         allTypes.get(allTypes.size() - 1).getCategory().getId();
 
     this.changeId = changeId;
-    this.reviewers = nameOrEmails;
+    this.reviewers = reviewers;
+    this.confirmed = confirmed;
   }
 
   @Override
@@ -84,18 +104,73 @@ public class AddReviewer implements Callable<ReviewerResult> {
     final ChangeControl control = changeControlFactory.validateFor(changeId);
 
     final ReviewerResult result = new ReviewerResult();
-    for (final String nameOrEmail : reviewers) {
-      final Account account = accountResolver.find(nameOrEmail);
+    for (final String reviewer : reviewers) {
+      final Account account = accountResolver.find(reviewer);
       if (account == null) {
-        result.addError(new ReviewerResult.Error(
-            ReviewerResult.Error.Type.ACCOUNT_NOT_FOUND, nameOrEmail));
+        AccountGroup group = groupCache.get(new AccountGroup.NameKey(reviewer));
+
+        if (group == null) {
+          result.addError(new ReviewerResult.Error(
+              ReviewerResult.Error.Type.REVIEWER_NOT_FOUND, reviewer));
+          continue;
+        }
+
+        if (!isLegalReviewerGroup(group.getGroupUUID())) {
+          result.addError(new ReviewerResult.Error(
+              ReviewerResult.Error.Type.GROUP_NOT_ALLOWED, reviewer));
+          continue;
+        }
+
+        final Set<Account> members =
+            groupMembersFactory.create(control.getProject().getNameKey(),
+                group.getGroupUUID()).call();
+        if (members == null || members.size() == 0) {
+          result.addError(new ReviewerResult.Error(
+              ReviewerResult.Error.Type.GROUP_EMPTY, reviewer));
+          continue;
+        }
+
+        // if maxAllowed is set to 0, it is allowed to add any number of
+        // reviewers
+        final int maxAllowed =
+            cfg.getInt("addreviewer", "maxAllowed", DEFAULT_MAX_REVIEWERS);
+        if (maxAllowed > 0 && members.size() > maxAllowed) {
+          result.setMemberCount(members.size());
+          result.setAskForConfirmation(false);
+          result.addError(new ReviewerResult.Error(
+              ReviewerResult.Error.Type.GROUP_HAS_TOO_MANY_MEMBERS, reviewer));
+          continue;
+        }
+
+        // if maxWithoutCheck is set to 0, we never ask for confirmation
+        final int maxWithoutConfirmation =
+            cfg.getInt("addreviewer", "maxWithoutConfirmation",
+                DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK);
+        if (!confirmed && maxWithoutConfirmation > 0
+            && members.size() > maxWithoutConfirmation) {
+          result.setMemberCount(members.size());
+          result.setAskForConfirmation(true);
+          result.addError(new ReviewerResult.Error(
+              ReviewerResult.Error.Type.GROUP_HAS_TOO_MANY_MEMBERS, reviewer));
+          continue;
+        }
+
+        for (final Account member : members) {
+          if (member.isActive()) {
+            final IdentifiedUser user =
+                identifiedUserFactory.create(member.getId());
+            if (control.forUser(user).isVisible()) {
+              reviewerIds.add(member.getId());
+            }
+          }
+        }
         continue;
       }
 
       if (!account.isActive()) {
         result.addError(new ReviewerResult.Error(
             ReviewerResult.Error.Type.ACCOUNT_INACTIVE,
-            formatUser(account, nameOrEmail)));
+            formatUser(account, reviewer)));
         continue;
       }
 
@@ -103,7 +178,7 @@ public class AddReviewer implements Callable<ReviewerResult> {
       if (!control.forUser(user).isVisible()) {
         result.addError(new ReviewerResult.Error(
             ReviewerResult.Error.Type.CHANGE_NOT_VISIBLE,
-            formatUser(account, nameOrEmail)));
+            formatUser(account, reviewer)));
         continue;
       }
 
@@ -165,4 +240,9 @@ public class AddReviewer implements Callable<ReviewerResult> {
     return new PatchSetApproval(new PatchSetApproval.Key(patchSetId,
         reviewerId, addReviewerCategoryId), (short) 0);
   }
+
+  public static boolean isLegalReviewerGroup(final AccountGroup.UUID groupUUID) {
+    return !(AccountGroup.ANONYMOUS_USERS.equals(groupUUID)
+             || AccountGroup.REGISTERED_USERS.equals(groupUUID));
+  }
 }
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ModifyReviewersCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ModifyReviewersCommand.java
index b351b163..29401224 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ModifyReviewersCommand.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ModifyReviewersCommand.java
@@ -138,12 +138,13 @@ public class ModifyReviewersCommand extends BaseCommand {
 
     // Add reviewers
     //
-    result = addReviewerFactory.create(changeId, stringSet(toAdd)).call();
+    result =
+        addReviewerFactory.create(changeId, stringSet(toAdd), false).call();
     ok &= result.getErrors().isEmpty();
     for (ReviewerResult.Error resultError : result.getErrors()) {
       String message;
       switch (resultError.getType()) {
-        case ACCOUNT_NOT_FOUND:
+        case REVIEWER_NOT_FOUND:
           message = "account {0} not found";
           break;
         case ACCOUNT_INACTIVE: