Merge "Support adding groups as reviewer by SSH command"

3 files changed, 36 insertions, 22 deletions

diff --git a/Documentation/cmd-modify-reviewers.txt b/Documentation/cmd-modify-reviewers.txt
index 0e77909a..68856276 100644
--- a/Documentation/cmd-modify-reviewers.txt
+++ b/Documentation/cmd-modify-reviewers.txt
@@ -10,8 +10,8 @@ SYNOPSIS
 [verse]
 'ssh' -p <port> <host> 'gerrit modify-reviewers'
   [--project <PROJECT>]
-  [--add EMAIL ...]
-  [--remove EMAIL ...]
+  [--add REVIEWER ...]
+  [--remove REVIEWER ...]
   [--]
   {COMMIT | CHANGE-ID}...
 
@@ -35,13 +35,16 @@ OPTIONS
 
 --add::
 -a::
-	Add this reviewer to the change. Multiple reviewers can be
-	added at once by using this option multiple times.
+	A user that should be added as reviewer to the change or a group
+	for which all members should be added as reviewers to the change.
+	Multiple users and groups can be added at once as reviewers by
+	using this option multiple times.
 
 --remove::
 -r::
-	Remove this reviewer to the change. Multiple reviewers can be
-	removed at once by using this option multiple times.
+	Remove this user from the reviewer list of the change. Multiple
+	users can be removed at once from the reviewer list by using this
+	option multiple times.
 
 --help::
 -h::
@@ -74,6 +77,13 @@ Add reviewer elvis to old-style change id 1935 specifying that the change is in
 	  1935
 =====
 
+Add all project owners as reviewers to change Iac6b2ac2.
+=====
+	$ ssh -p 29418 review.example.com gerrit modify-reviewers \
+	  -a "'Project Owners'" \
+	  Iac6b2ac2
+=====
+
 GERRIT
 ------
 Part of link:index.html[Gerrit Code Review]
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 26cc9b19..da00c6e0 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -36,6 +36,10 @@ If set to 0, the user will never be asked to confirm adding a group
 as reviewer.
 +
 Default is 10.
++
+This setting only applies for adding reviewers in the Gerrit WebUI,
+but is ignored when adding reviewers with the
+link:cmd-modify-reviewers.html[modify-reviewers] command.
 
 [[addreviewer.maxAllowed]]addreviewer.maxAllowed::
 +
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 29401224..f106e48f 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
@@ -38,7 +38,9 @@ import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.text.MessageFormat;
+import java.util.ArrayList;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Set;
 
 public class ModifyReviewersCommand extends BaseCommand {
@@ -48,12 +50,10 @@ public class ModifyReviewersCommand extends BaseCommand {
   @Option(name = "--project", aliases = "-p", usage = "project containing the change")
   private ProjectControl projectControl;
 
-  @Option(name = "--add", aliases = {"-a"}, metaVar = "EMAIL", usage = "reviewer to add")
-  void optionAdd(Account.Id who) {
-    toAdd.add(who);
-  }
+  @Option(name = "--add", aliases = {"-a"}, metaVar = "REVIEWER", usage = "user or group that should be added as reviewer")
+  private List<String> toAdd = new ArrayList<String>();
 
-  @Option(name = "--remove", aliases = {"-r"}, metaVar = "EMAIL", usage = "reviewer to remove")
+  @Option(name = "--remove", aliases = {"-r"}, metaVar = "REVIEWER", usage = "user that should be removed from the reviewer list")
   void optionRemove(Account.Id who) {
     toRemove.add(who);
   }
@@ -81,7 +81,6 @@ public class ModifyReviewersCommand extends BaseCommand {
   @Inject
   private ChangeControl.Factory changeControlFactory;
 
-  private Set<Account.Id> toAdd = new HashSet<Account.Id>();
   private Set<Account.Id> toRemove = new HashSet<Account.Id>();
   private Set<Change.Id> changes = new HashSet<Change.Id>();
 
@@ -139,13 +138,13 @@ public class ModifyReviewersCommand extends BaseCommand {
     // Add reviewers
     //
     result =
-        addReviewerFactory.create(changeId, stringSet(toAdd), false).call();
+        addReviewerFactory.create(changeId, toAdd, true).call();
     ok &= result.getErrors().isEmpty();
     for (ReviewerResult.Error resultError : result.getErrors()) {
       String message;
       switch (resultError.getType()) {
         case REVIEWER_NOT_FOUND:
-          message = "account {0} not found";
+          message = "account or group {0} not found";
           break;
         case ACCOUNT_INACTIVE:
           message = "account {0} inactive";
@@ -153,6 +152,15 @@ public class ModifyReviewersCommand extends BaseCommand {
         case CHANGE_NOT_VISIBLE:
           message = "change {1} not visible to {0}";
           break;
+        case GROUP_EMPTY:
+          message = "group {0} is empty";
+          break;
+        case GROUP_HAS_TOO_MANY_MEMBERS:
+          message = "group {0} has too many members";
+          break;
+        case GROUP_NOT_ALLOWED:
+          message = "group {0} is not allowed as reviewer";
+          break;
         default:
           message = "could not add {0}: {2}";
       }
@@ -163,14 +171,6 @@ public class ModifyReviewersCommand extends BaseCommand {
     return ok;
   }
 
-  private static Set<String> stringSet(Set<Account.Id> ids) {
-    Set<String> res = new HashSet<String>();
-    for (Account.Id id : ids) {
-      res.add(Integer.toString(id.get()));
-    }
-    return res;
-  }
-
   private Set<Change.Id> parseChangeId(String idstr)
       throws UnloggedFailure, OrmException {
     Set<Change.Id> matched = new HashSet<Change.Id>(4);