Merge branch 'stable-2.12'

* stable-2.12:
  Add tests for reviewing and submitting refs/meta/config changes
  Update 2.11.8 release notes
  Prevent NPE in the SshLog
  Clear the input box after cancelling add reviewer action
  ReviewCommand: Don't add message twice on abandon or restore
  Correct schema migration instructions for MySQL in 2.12.1 release notes
  Documentation: remove submitted status from user search and review
  RebaseChangeOp: adding not null check for PatchSet groups
  Set version to 2.11.8
  Release notes for Gerrit 2.11.8
  Fix keyboard shortcuts for non-US keyboards
  Update commons-collections to 3.2.2
  Update commons-collections to 3.2.2
  Update 2.12.1 release notes
  Fix various formatting glitches in the 2.12.1 release notes
  Set version to 2.12.1
  Update 2.12.1 release notes
  Move the logic out of SuggestReviewers and make super class
  Submit: Point at problematic other commits in tooltip

Change-Id: Icf745ad1c95e7410ae638cfa9e5f0094541de094

Documentation/user-review-ui.txt

@@ -186,10 +186,6 @@ change is merged into the destination branch.
 The `Submit` button is available if the change is submittable and
 the link:access-control.html#category_submit[Submit] access right is
 assigned.
-+
-It is also possible to submit changes that have merge conflicts. This
-allows to do the conflict resolution for a change series in a single
-merge commit and submit the changes in reverse order.
 
 ** [[abandon]]`Abandon`:
 +

Documentation/user-search.txt

@@ -299,7 +299,7 @@ is:closed::
 +
 True if the change is either merged or abandoned.
 
-is:submitted, is:merged, is:abandoned::
+is:merged, is:abandoned::
 +
 Same as <<status,status:'STATE'>>.
 
@@ -320,10 +320,6 @@ Same as 'is:reviewed', matches if any user has commented on the change
 more recently than the last update (comment or patch set) from the
 change owner.
 
-status:submitted::
-+
-Change has been submitted, but is waiting for a dependency.
-
 status:closed::
 +
 True if the change is either 'merged' or 'abandoned'.

ReleaseNotes/ReleaseNotes-2.11.8.txt

@@ -0,0 +1,43 @@
+Release notes for Gerrit 2.11.8
+===============================
+
+Gerrit 2.11.8 is now available:
+
+link:https://gerrit-releases.storage.googleapis.com/gerrit-2.11.8.war[
+https://gerrit-releases.storage.googleapis.com/gerrit-2.11.8.war]
+
+There are no schema changes from link:ReleaseNotes-2.11.7.html[2.11.7].
+
+Bug Fixes
+---------
+
+* Upgrade Apache commons-collections to version 3.2.2.
++
+Includes a fix for a link:https://issues.apache.org/jira/browse/COLLECTIONS-580[
+remote code execution exploit].
+
+* link:https://code.google.com/p/gerrit/issues/detail?id=1207[Issue 1207]:
+Fix keyboard shortcuts for non-US keyboards on side-by-side diff screen.
++
+The forward/backward navigation keys `[` and `]` only worked on keyboards where
+these characters could be typed without using any modifier key (like CTRL, ALT,
+etc.).
++
+Note that the problem still exists on the unified diff screen.
+
+* link:https://code.google.com/p/gerrit/issues/detail?id=3919[Issue 3919]:
+Explicitly set parent project to 'All-Projects' when a project is created
+without giving the parent.
+
+* Don't add message twice on abandon or restore via ssh review command.
++
+When abandoning or reviewing a change via the ssh `review` command, and
+providing a message with the `--message` option, the message was added to
+the change twice.
+
+* Clear the input box after cancelling add reviewer action.
++
+When the action was cancelled, the content of the input box was still
+there when opening it again.
+
+* Fix internal server error when aborting ssh command.

ReleaseNotes/ReleaseNotes-2.12.1.txt

@@ -11,31 +11,48 @@ link:ReleaseNotes-2.11.6.html[Gerrit 2.11.6] and
 link:ReleaseNotes-2.11.7.html[Gerrit 2.11.7]. These bug fixes are *not*
 listed in these release notes.
 
+Schema Upgrade
+--------------
+
 *WARNING:* This version includes a manual schema upgrade when upgrading
 from 2.12.
-+
+
-If you have already upgraded to 2.12, you need to issue this SQL statement
+When upgrading a site that is already running version 2.12, the `patch_sets`
-manually (e.g. using the `gerrit gsql` SSH command or the `gqsl` site
+table must be manually migrated using the `gerrit gsql` SSH command or the
-program):
+`gqsl` site program.
-+
+
+For the default H2 database, execute the command:
+
+----
   alter table patch_sets modify push_certficate clob;
-+
+----
-Or with this command if the site is configured to use PostgreSQL:
+
-+
+For MySQL, execute the command:
+
+----
+  alter table patch_sets modify push_certficate text;
+----
+
+For PostgreSQL, execute the command:
+
+----
   alter table patch_sets alter column push_certficate type text;
-+
+----
+
+For other database types, execute the appropriate equivalent command.
+
 Note that the misspelled `push_certficate` is the actual name of the
 column.
-+
+
-If you are upgrading from a version earlier than 2.12, this manual step is
+When upgrading from a version earlier than 2.12, this manual step is not
-not necessary and should be omitted.
+necessary and should be omitted.
 
 
 Bug Fixes
 ---------
 
 General
-^^^^^^^
+~~~~~~~
 
 * Fix column type for signed push certificates.
 +
@@ -69,7 +86,7 @@ in other tables, a null change was returned which would then lead to an
 'Internal Server Error' that was difficult to track down. Now an error is
 raised earlier which will help administrators to find the root cause.
 
-* https://code.google.com/p/gerrit/issues/detail?id=3743[Issue 3743]:
+* link:https://code.google.com/p/gerrit/issues/detail?id=3743[Issue 3743]:
 Use submitter identity as committer when using 'Rebase if Necessary' merge
 strategy.
 +
@@ -125,8 +142,29 @@ is reduced from 'no limit' to 1024.
 Explicitly set parent project to 'All-Projects' when a project is created
 without giving the parent.
 
+* link:https://code.google.com/p/gerrit/issues/detail?id=3948[Issue 3948]:
+Fix submit of project parent updates on `refs/meta/config`.
++
+When submitting a change on `refs/meta/config` to update a project's parent,
+the error 'The change must be submitted by a Gerrit administrator' was being
+displayed even when the submitter was an admin. The submit was successful
+when clicking 'Submit' a second time.
+
+* link:https://code.google.com/p/gerrit/issues/detail?id=3811[Issue 3811]:
+Fix submittability of merge commits that resolve merge conflicts.
++
+If a series of changes contained a change that conflicted with the destination
+branch, but the conflict was solved by a merge commit at the tip of the
+series, the series was not submittable.
+
+* link:https://code.google.com/p/gerrit/issues/detail?id=3883[Issue 3883]:
+Respect the `core.commentchar` setting from `.gitconfig` in `commit-msg` hook.
+
 UI
-^^
+~~
+
+* link:https://code.google.com/p/gerrit/issues/detail?id=3894[Issue 3894]:
+Fix display of 'Related changes' after change is rebased in web UI:
 
 * link:https://code.google.com/p/gerrit/issues/detail?id=3071[Issue 3071]:
 Fix display of submodule differences in side-by-side view.
@@ -153,22 +191,11 @@ etc..).
 +
 Note that the problem still exists on the unified diff screen.
 
+* Improve tooltip on 'Submit' button when 'Submit whole topic' is enabled
+and the topic can't be submitted due to some changes not being ready.
+
 Plugins
-^^^^^^^
+~~~~~~~
-
-* Allow plugins to get the caller in merge validation requests.
-+
-Plugins that implement the `MergeValidationListener` interface now get the
-caller (the user who initiated the merge) in the `onPreMerge` method.
-+
-Existing plugins that implement this interface must be adapted to the new
-method signature.
-
-* link:https://code.google.com/p/gerrit/issues/detail?id=3741[Issue 3741]:
-Fix handling of merge validation exceptions emitted by plugins.
-+
-If a plugin raised an exception, it was reported to the user as 'Change is
-new', rather than 'Missing dependency'.
 
 * link:https://code.google.com/p/gerrit/issues/detail?id=3821[Issue 3821]:
 Fix repeated reloading of plugins when running on OpenJDK 8.
@@ -178,11 +205,31 @@ are POSIX 2008 compatible. This leads to precision incompatibility when
 comparing the plugin's JAR file timestamp, resulting in the plugin being
 reloaded every minute.
 
+* link:https://code.google.com/p/gerrit/issues/detail?id=3741[Issue 3741]:
+Fix handling of merge validation exceptions emitted by plugins.
++
+If a plugin raised an exception, it was reported to the user as 'Change is
+new', rather than 'Missing dependency'.
+
+* Allow plugins to get the caller in merge validation requests.
++
+Plugins that implement the `MergeValidationListener` interface now get the
+caller (the user who initiated the merge) in the `onPreMerge` method.
++
+Existing plugins that implement this interface must be adapted to the new
+method signature.
+
+* link:https://code.google.com/p/gerrit/issues/detail?id=3892[Issue 3892]:
+Allow plugins to suggest reviewers based on either change or project
+resources.
+
 Documentation
-^^^^^^^^^^^^^
+~~~~~~~~~~~~~
 
 * Update documentation of `commentlink` to reflect changed search URL.
 
+* Add missing documentation of valid `database.type` values.
+
 Upgrades
 --------
 

ReleaseNotes/index.txt

@@ -15,6 +15,7 @@ Version 2.12.x
 [[2_11]]
 Version 2.11.x
 --------------
+* link:ReleaseNotes-2.11.8.html[2.11.8]
 * link:ReleaseNotes-2.11.7.html[2.11.7]
 * link:ReleaseNotes-2.11.6.html[2.11.6]
 * link:ReleaseNotes-2.11.5.html[2.11.5]

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java

@@ -103,7 +103,7 @@ public class ActionsIT extends AbstractDaemonTest {
       assertThat(info.label).isEqualTo("Submit whole topic");
       assertThat(info.method).isEqualTo("POST");
       assertThat(info.title).isEqualTo(
-          "See the \"Submitted Together\" tab for problems");
+          "See the \"Submitted Together\" tab for problems, specially see: 2");
     } else {
       noSubmitWholeTopicAssertions(actions, 1);
     }

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ConfigChangeIT.java

@@ -0,0 +1,154 @@
+// Copyright (C) 2016 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.acceptance.rest.change;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.junit.Assert.fail;
+
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestProjectInput;
+import com.google.gerrit.common.data.Permission;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.projects.ProjectInput;
+import com.google.gerrit.extensions.client.ChangeStatus;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.project.Util;
+
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.ObjectLoader;
+import org.eclipse.jgit.revwalk.RevObject;
+import org.eclipse.jgit.revwalk.RevTree;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.transport.RefSpec;
+import org.junit.Before;
+import org.junit.Test;
+
+public class ConfigChangeIT extends AbstractDaemonTest {
+  @Before
+  public void setUp() throws Exception {
+    ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+    Util.allow(cfg, Permission.OWNER, REGISTERED_USERS, "refs/*");
+    Util.allow(
+        cfg, Permission.PUSH, REGISTERED_USERS, "refs/for/refs/meta/config");
+    Util.allow(cfg, Permission.SUBMIT, REGISTERED_USERS, "refs/meta/config");
+    saveProjectConfig(project, cfg);
+
+    setApiUser(user);
+    fetchRefsMetaConfig();
+  }
+
+  @Test
+  @TestProjectInput(cloneAs = "user")
+  public void updateProjectConfig() throws Exception {
+    Config cfg = readProjectConfig();
+    assertThat(cfg.getString("project", null, "description")).isNull();
+    String desc = "new project description";
+    cfg.setString("project", null, "description", desc);
+
+    PushOneCommit.Result r = createConfigChange(cfg);
+    String id = r.getChangeId();
+
+    gApi.changes().id(id).current().review(ReviewInput.approve());
+    gApi.changes().id(id).current().submit();
+
+    assertThat(gApi.changes().id(id).info().status)
+        .isEqualTo(ChangeStatus.MERGED);
+    assertThat(gApi.projects().name(project.get()).get().description)
+        .isEqualTo(desc);
+    fetchRefsMetaConfig();
+    assertThat(readProjectConfig().getString("project", null, "description"))
+        .isEqualTo(desc);
+  }
+
+  @Test
+  @TestProjectInput(cloneAs = "user")
+  public void onlyAdminMayUpdateProjectParent() throws Exception {
+    setApiUser(admin);
+    ProjectInput parent = new ProjectInput();
+    parent.name = name("parent");
+    parent.permissionsOnly = true;
+    gApi.projects().create(parent);
+
+    setApiUser(user);
+    Config cfg = readProjectConfig();
+    assertThat(cfg.getString("access", null, "inheritFrom"))
+        .isAnyOf(null, allProjects.get());
+    cfg.setString("access", null, "inheritFrom", parent.name);
+
+    PushOneCommit.Result r = createConfigChange(cfg);
+    String id = r.getChangeId();
+
+    gApi.changes().id(id).current().review(ReviewInput.approve());
+    try {
+      gApi.changes().id(id).current().submit();
+      fail("expected submit to fail");
+    } catch (ResourceConflictException e) {
+      int n = gApi.changes().id(id).info()._number;
+      assertThat(e).hasMessage(
+          "Failed to submit 1 change due to the following problems:\n"
+          + "Change " + n + ": Change contains a project configuration that"
+          +" changes the parent project.\n"
+          + "The change must be submitted by a Gerrit administrator.");
+    }
+
+    assertThat(gApi.projects().name(project.get()).get().parent)
+        .isEqualTo(allProjects.get());
+    fetchRefsMetaConfig();
+    assertThat(readProjectConfig().getString("access", null, "inheritFrom"))
+        .isAnyOf(null, allProjects.get());
+
+    setApiUser(admin);
+    gApi.changes().id(id).current().submit();
+    assertThat(gApi.changes().id(id).info().status)
+        .isEqualTo(ChangeStatus.MERGED);
+    assertThat(gApi.projects().name(project.get()).get().parent)
+        .isEqualTo(parent.name);
+    fetchRefsMetaConfig();
+    assertThat(readProjectConfig().getString("access", null, "inheritFrom"))
+        .isEqualTo(parent.name);
+  }
+
+  private void fetchRefsMetaConfig() throws Exception {
+    git().fetch().setRefSpecs(new RefSpec("refs/meta/config:refs/meta/config"))
+        .call();
+    testRepo.reset("refs/meta/config");
+  }
+
+  private Config readProjectConfig() throws Exception {
+    RevWalk rw = testRepo.getRevWalk();
+    RevTree tree = rw.parseTree(testRepo.getRepository().resolve("HEAD"));
+    RevObject obj = rw.parseAny(testRepo.get(tree, "project.config"));
+    ObjectLoader loader = rw.getObjectReader().open(obj);
+    String text = new String(loader.getCachedBytes(), UTF_8);
+    Config cfg = new Config();
+    cfg.fromText(text);
+    return cfg;
+  }
+
+  private PushOneCommit.Result createConfigChange(Config cfg) throws Exception {
+    PushOneCommit.Result r = pushFactory.create(
+            db, user.getIdent(), testRepo,
+            "Update project config",
+            "project.config",
+            cfg.toText())
+        .to("refs/for/refs/meta/config");
+    r.assertOkStatus();
+    return r;
+  }
+}

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitResolvingMergeCommitIT.java

@@ -294,7 +294,7 @@ public class SubmitResolvingMergeCommitIT extends AbstractDaemonTest {
       OrmException {
     ChangeSet cs =
         mergeSuperSet.completeChangeSet(db, change.change(), user(admin));
-    assertThat(submit.isPatchSetMergeable(cs)).isEqualTo(expected);
+    assertThat(submit.unmergeableChanges(cs).isEmpty()).isEqualTo(expected);
   }
 
   private void assertMergeable(ChangeData change, boolean expected)

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/AbandonRestoreIT.java

@@ -0,0 +1,89 @@
+// Copyright (C) 2016 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.acceptance.ssh;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assert_;
+
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.acceptance.PushOneCommit.Result;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.ChangeMessageInfo;
+
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Locale;
+
+@NoHttpd
+public class AbandonRestoreIT extends AbstractDaemonTest {
+
+  @Test
+  public void withMessage() throws Exception {
+    Result result = createChange();
+    String commit = result.getCommit().name();
+    executeCmd(commit, "abandon", "'abandon it'");
+    executeCmd(commit, "restore", "'restore it'");
+    assertChangeMessages(result.getChangeId(), ImmutableList.of(
+        "Uploaded patch set 1.",
+        "Abandoned\n\nabandon it",
+        "Restored\n\nrestore it"));
+  }
+
+  @Test
+  public void withoutMessage() throws Exception {
+    Result result = createChange();
+    String commit = result.getCommit().name();
+    executeCmd(commit, "abandon", null);
+    executeCmd(commit, "restore", null);
+    assertChangeMessages(result.getChangeId(), ImmutableList.of(
+        "Uploaded patch set 1.",
+        "Abandoned",
+        "Restored"));
+  }
+
+  private void executeCmd(String commit, String op, String message)
+      throws Exception {
+    StringBuilder command = new StringBuilder("gerrit review ")
+        .append(commit)
+        .append(" --")
+        .append(op);
+    if (message != null) {
+      command.append(" --message ").append(message);
+    }
+    String response = sshSession.exec(command.toString());
+    assert_()
+      .withFailureMessage(sshSession.getError())
+      .that(sshSession.hasError())
+      .isFalse();
+    assertThat(response.toLowerCase(Locale.US)).doesNotContain("error");
+  }
+
+  private void assertChangeMessages(String changeId, List<String> expected)
+      throws Exception {
+    ChangeInfo c = get(changeId);
+    Iterable<ChangeMessageInfo> messages = c.messages;
+    assertThat(messages).isNotNull();
+    assertThat(messages).hasSize(expected.size());
+    List<String> actual = new ArrayList<>();
+    for (ChangeMessageInfo info : messages) {
+      actual.add(info.message);
+    }
+    assertThat(actual).containsExactlyElementsIn(expected);
+  }
+}

gerrit-gwtui/src/main/java/com/google/gerrit/client/change/Reviewers.java

@@ -142,6 +142,7 @@ public class Reviewers extends Composite {
     addReviewerIcon.setVisible(true);
     UIObject.setVisible(form, false);
     suggestBox.setFocus(false);
+    suggestBox.setText("");
   }
 
   private void addReviewer(final String reviewer, boolean confirmed) {

gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java

@@ -0,0 +1,286 @@
+// Copyright (C) 2016 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;
+
+import com.google.common.base.Function;
+import com.google.common.base.MoreObjects;
+import com.google.common.base.Strings;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Ordering;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.common.errors.NoSuchGroupException;
+import com.google.gerrit.extensions.common.AccountInfo;
+import com.google.gerrit.extensions.common.GroupBaseInfo;
+import com.google.gerrit.extensions.common.SuggestedReviewerInfo;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.Url;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountExternalId;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.account.AccountControl;
+import com.google.gerrit.server.account.AccountLoader;
+import com.google.gerrit.server.account.GroupBackend;
+import com.google.gerrit.server.account.GroupMembers;
+import com.google.gerrit.server.change.PostReviewers;
+import com.google.gerrit.server.change.ReviewerSuggestionCache;
+import com.google.gerrit.server.change.SuggestReviewers;
+import com.google.gerrit.server.project.NoSuchProjectException;
+import com.google.gerrit.server.project.ProjectControl;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+public class ReviewersUtil {
+  private static final String MAX_SUFFIX = "\u9fa5";
+  private static final Ordering<SuggestedReviewerInfo> ORDERING =
+      Ordering.natural().onResultOf(new Function<SuggestedReviewerInfo, String>() {
+        @Nullable
+        @Override
+        public String apply(@Nullable SuggestedReviewerInfo suggestedReviewerInfo) {
+          if (suggestedReviewerInfo == null) {
+            return null;
+          }
+          return suggestedReviewerInfo.account != null
+              ? MoreObjects.firstNonNull(suggestedReviewerInfo.account.email,
+              Strings.nullToEmpty(suggestedReviewerInfo.account.name))
+              : Strings.nullToEmpty(suggestedReviewerInfo.group.name);
+        }
+      });
+  private final AccountLoader accountLoader;
+  private final AccountCache accountCache;
+  private final ReviewerSuggestionCache reviewerSuggestionCache;
+  private final AccountControl accountControl;
+  private final Provider<ReviewDb> dbProvider;
+  private final GroupBackend groupBackend;
+  private final GroupMembers.Factory groupMembersFactory;
+  private final Provider<CurrentUser> currentUser;
+
+  @Inject
+  ReviewersUtil(AccountLoader.Factory accountLoaderFactory,
+      AccountCache accountCache,
+      ReviewerSuggestionCache reviewerSuggestionCache,
+      AccountControl.Factory accountControlFactory,
+      Provider<ReviewDb> dbProvider,
+      GroupBackend groupBackend,
+      GroupMembers.Factory groupMembersFactory,
+      Provider<CurrentUser> currentUser) {
+    this.accountLoader = accountLoaderFactory.create(true);
+    this.accountCache = accountCache;
+    this.reviewerSuggestionCache = reviewerSuggestionCache;
+    this.accountControl = accountControlFactory.get();
+    this.dbProvider = dbProvider;
+    this.groupBackend = groupBackend;
+    this.groupMembersFactory = groupMembersFactory;
+    this.currentUser = currentUser;
+  }
+
+  public interface VisibilityControl {
+    boolean isVisibleTo(Account.Id account) throws OrmException;
+  }
+
+  public List<SuggestedReviewerInfo> suggestReviewers(
+      SuggestReviewers suggestReviewers, ProjectControl projectControl,
+      VisibilityControl visibilityControl)
+      throws IOException, OrmException, BadRequestException {
+    String query = suggestReviewers.getQuery();
+    boolean suggestAccounts = suggestReviewers.getSuggestAccounts();
+    int suggestFrom = suggestReviewers.getSuggestFrom();
+    boolean useFullTextSearch = suggestReviewers.getUseFullTextSearch();
+    int limit = suggestReviewers.getLimit();
+
+    if (Strings.isNullOrEmpty(query)) {
+      throw new BadRequestException("missing query field");
+    }
+
+    if (!suggestAccounts || query.length() < suggestFrom) {
+      return Collections.emptyList();
+    }
+
+    List<AccountInfo> suggestedAccounts;
+    if (useFullTextSearch) {
+      suggestedAccounts = suggestAccountFullTextSearch(suggestReviewers, visibilityControl);
+    } else {
+      suggestedAccounts = suggestAccount(suggestReviewers, visibilityControl);
+    }
+
+    List<SuggestedReviewerInfo> reviewer = Lists.newArrayList();
+    for (AccountInfo a : suggestedAccounts) {
+      SuggestedReviewerInfo info = new SuggestedReviewerInfo();
+      info.account = a;
+      reviewer.add(info);
+    }
+
+    for (GroupReference g : suggestAccountGroup(suggestReviewers, projectControl)) {
+      if (suggestGroupAsReviewer(suggestReviewers, projectControl.getProject(),
+          g, visibilityControl)) {
+        GroupBaseInfo info = new GroupBaseInfo();
+        info.id = Url.encode(g.getUUID().get());
+        info.name = g.getName();
+        SuggestedReviewerInfo suggestedReviewerInfo = new SuggestedReviewerInfo();
+        suggestedReviewerInfo.group = info;
+        reviewer.add(suggestedReviewerInfo);
+      }
+    }
+
+    reviewer = ORDERING.immutableSortedCopy(reviewer);
+    if (reviewer.size() <= limit) {
+      return reviewer;
+    } else {
+      return reviewer.subList(0, limit);
+    }
+  }
+
+  private List<AccountInfo> suggestAccountFullTextSearch(
+      SuggestReviewers suggestReviewers, VisibilityControl visibilityControl)
+          throws IOException, OrmException {
+    List<AccountInfo> results = reviewerSuggestionCache.search(
+        suggestReviewers.getQuery(), suggestReviewers.getFullTextMaxMatches());
+
+    Iterator<AccountInfo> it = results.iterator();
+    while (it.hasNext()) {
+      Account.Id accountId = new Account.Id(it.next()._accountId);
+      if (!(visibilityControl.isVisibleTo(accountId)
+          && accountControl.canSee(accountId))) {
+        it.remove();
+      }
+    }
+
+    return results;
+  }
+
+  private List<AccountInfo> suggestAccount(SuggestReviewers suggestReviewers,
+      VisibilityControl visibilityControl)
+      throws OrmException {
+    String query = suggestReviewers.getQuery();
+    int limit = suggestReviewers.getLimit();
+
+    String a = query;
+    String b = a + MAX_SUFFIX;
+
+    Map<Account.Id, AccountInfo> r = new LinkedHashMap<>();
+    Map<Account.Id, String> queryEmail = new HashMap<>();
+
+    for (Account p : dbProvider.get().accounts()
+        .suggestByFullName(a, b, limit)) {
+      if (p.isActive()) {
+        addSuggestion(r, p.getId(), visibilityControl);
+      }
+    }
+
+    if (r.size() < limit) {
+      for (Account p : dbProvider.get().accounts()
+          .suggestByPreferredEmail(a, b, limit - r.size())) {
+        if (p.isActive()) {
+          addSuggestion(r, p.getId(), visibilityControl);
+        }
+      }
+    }
+
+    if (r.size() < limit) {
+      for (AccountExternalId e : dbProvider.get().accountExternalIds()
+          .suggestByEmailAddress(a, b, limit - r.size())) {
+        if (!r.containsKey(e.getAccountId())) {
+          Account p = accountCache.get(e.getAccountId()).getAccount();
+          if (p.isActive()) {
+            if (addSuggestion(r, p.getId(), visibilityControl)) {
+              queryEmail.put(e.getAccountId(), e.getEmailAddress());
+            }
+          }
+        }
+      }
+    }
+
+    accountLoader.fill();
+    for (Map.Entry<Account.Id, String> p : queryEmail.entrySet()) {
+      AccountInfo info = r.get(p.getKey());
+      if (info != null) {
+        info.email = p.getValue();
+      }
+    }
+    return new ArrayList<>(r.values());
+  }
+
+  private boolean addSuggestion(Map<Account.Id, AccountInfo> map,
+      Account.Id account, VisibilityControl visibilityControl)
+      throws OrmException {
+    if (!map.containsKey(account)
+        // Can the suggestion see the change?
+        && visibilityControl.isVisibleTo(account)
+        // Can the account see the current user?
+        && accountControl.canSee(account)) {
+      map.put(account, accountLoader.get(account));
+      return true;
+    }
+    return false;
+  }
+
+  private List<GroupReference> suggestAccountGroup(
+      SuggestReviewers suggestReviewers, ProjectControl ctl) {
+    return Lists.newArrayList(
+        Iterables.limit(groupBackend.suggest(suggestReviewers.getQuery(), ctl),
+            suggestReviewers.getLimit()));
+  }
+
+  private boolean suggestGroupAsReviewer(SuggestReviewers suggestReviewers,
+      Project project, GroupReference group,
+      VisibilityControl visibilityControl) throws OrmException, IOException {
+    int maxAllowed = suggestReviewers.getMaxAllowed();
+
+    if (!PostReviewers.isLegalReviewerGroup(group.getUUID())) {
+      return false;
+    }
+
+    try {
+      Set<Account> members = groupMembersFactory
+          .create(currentUser.get())
+          .listAccounts(group.getUUID(), project.getNameKey());
+
+      if (members.isEmpty()) {
+        return false;
+      }
+
+      if (maxAllowed > 0 && members.size() > maxAllowed) {
+        return false;
+      }
+
+      // require that at least one member in the group can see the change
+      for (Account account : members) {
+        if (visibilityControl.isVisibleTo(account.getId())) {
+          return true;
+        }
+      }
+    } catch (NoSuchGroupException e) {
+      return false;
+    } catch (NoSuchProjectException e) {
+      return false;
+    }
+
+    return false;
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java

@@ -52,7 +52,7 @@ import com.google.gerrit.server.change.Revert;
 import com.google.gerrit.server.change.Reviewers;
 import com.google.gerrit.server.change.Revisions;
 import com.google.gerrit.server.change.SubmittedTogether;
-import com.google.gerrit.server.change.SuggestReviewers;
+import com.google.gerrit.server.change.SuggestChangeReviewers;
 import com.google.gerrit.server.git.UpdateException;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.server.OrmException;
@@ -77,7 +77,7 @@ class ChangeApiImpl implements ChangeApi {
   private final Revisions revisions;
   private final ReviewerApiImpl.Factory reviewerApi;
   private final RevisionApiImpl.Factory revisionApi;
-  private final Provider<SuggestReviewers> suggestReviewers;
+  private final Provider<SuggestChangeReviewers> suggestReviewers;
   private final ChangeResource change;
   private final Abandon abandon;
   private final Revert revert;
@@ -104,7 +104,7 @@ class ChangeApiImpl implements ChangeApi {
       Revisions revisions,
       ReviewerApiImpl.Factory reviewerApi,
       RevisionApiImpl.Factory revisionApi,
-      Provider<SuggestReviewers> suggestReviewers,
+      Provider<SuggestChangeReviewers> suggestReviewers,
       Abandon abandon,
       Revert revert,
       Restore restore,
@@ -304,7 +304,7 @@ class ChangeApiImpl implements ChangeApi {
   private List<SuggestedReviewerInfo> suggestReviewers(SuggestedReviewersRequest r)
       throws RestApiException {
     try {
-      SuggestReviewers mySuggestReviewers = suggestReviewers.get();
+      SuggestChangeReviewers mySuggestReviewers = suggestReviewers.get();
       mySuggestReviewers.setQuery(r.getQuery());
       mySuggestReviewers.setLimit(r.getLimit());
       return mySuggestReviewers.apply(change);

gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java

@@ -26,6 +26,7 @@ import static com.google.gerrit.server.change.VoteResource.VOTE_KIND;
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.restapi.RestApiModule;
 import com.google.gerrit.server.account.AccountLoader;
+import com.google.gerrit.server.change.SuggestChangeReviewers;
 import com.google.gerrit.server.change.Reviewed.DeleteReviewed;
 import com.google.gerrit.server.change.Reviewed.PutReviewed;
 
@@ -72,7 +73,7 @@ public class Module extends RestApiModule {
     post(CHANGE_KIND, "index").to(Index.class);
 
     post(CHANGE_KIND, "reviewers").to(PostReviewers.class);
-    get(CHANGE_KIND, "suggest_reviewers").to(SuggestReviewers.class);
+    get(CHANGE_KIND, "suggest_reviewers").to(SuggestChangeReviewers.class);
     child(CHANGE_KIND, "reviewers").to(Reviewers.class);
     get(REVIEWER_KIND).to(GetReviewer.class);
     delete(REVIEWER_KIND).to(DeleteReviewer.class);

gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerSuggestionCache.java

@@ -102,7 +102,7 @@ public class ReviewerSuggestionCache {
             });
   }
 
-  List<AccountInfo> search(String query, int n) throws IOException {
+  public List<AccountInfo> search(String query, int n) throws IOException {
     IndexSearcher searcher = get();
     if (searcher == null) {
       return Collections.emptyList();

gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java

@@ -15,11 +15,14 @@
 package com.google.gerrit.server.change;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Function;
+import com.google.common.base.Joiner;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Predicate;
 import com.google.common.base.Strings;
 import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Multimap;
 import com.google.common.collect.Sets;
 import com.google.gerrit.common.data.ParameterizedString;
@@ -71,6 +74,7 @@ import org.slf4j.LoggerFactory;
 import java.io.IOException;
 import java.util.Collection;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -96,7 +100,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
   private static final String CLICK_FAILURE_TOOLTIP =
       "Clicking the button would fail";
   private static final String CHANGES_NOT_MERGEABLE =
-      "See the \"Submitted Together\" tab for problems";
+      "See the \"Submitted Together\" tab for problems, specially see: ";
 
   public static class Output {
     transient Change change;
@@ -263,11 +267,18 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
         MergeOp.checkSubmitRule(c);
       }
 
-      Boolean csIsMergeable = isPatchSetMergeable(cs);
+      Collection<ChangeData> unmergeable = unmergeableChanges(cs);
-      if (csIsMergeable == null) {
+      if (unmergeable == null) {
         return CLICK_FAILURE_TOOLTIP;
-      } else if (!csIsMergeable) {
+      } else if (!unmergeable.isEmpty()) {
-        return CHANGES_NOT_MERGEABLE;
+        return CHANGES_NOT_MERGEABLE + Joiner.on(", ").join(
+            Iterables.transform(unmergeable,
+                new Function<ChangeData, String>() {
+              @Override
+              public String apply(ChangeData cd) {
+                return String.valueOf(cd.getId().get());
+              }
+            }));
       }
     } catch (ResourceConflictException e) {
       return BLOCKED_SUBMIT_TOOLTIP;
@@ -407,11 +418,11 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
     return change != null ? change.getStatus().name().toLowerCase() : "deleted";
   }
 
-  public Boolean isPatchSetMergeable(ChangeSet cs)
+  public Collection<ChangeData> unmergeableChanges(ChangeSet cs)
       throws OrmException, IOException {
-    Map<ChangeData, Boolean> mergeabilityMap = new HashMap<>();
+    Set<ChangeData> mergeabilityMap = new HashSet<>();
     for (ChangeData change : cs.changes()) {
-      mergeabilityMap.put(change, false);
+      mergeabilityMap.add(change);
     }
 
     Multimap<Branch.NameKey, ChangeData> cbb = cs.changesByBranch();
@@ -442,17 +453,19 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
           // Skip whole check, cannot determine if mergeable
           return null;
         }
-        mergeabilityMap.put(change, mergeable);
+        if (mergeable) {
+          mergeabilityMap.remove(change);
+        }
 
         if (isLastInChain && isMergeCommit && mergeable) {
           for (ChangeData c : targetBranch) {
-            mergeabilityMap.put(c, true);
+            mergeabilityMap.remove(c);
           }
           break;
         }
       }
     }
-    return !mergeabilityMap.values().contains(Boolean.FALSE);
+    return mergeabilityMap;
   }
 
   private HashMap<Change.Id, RevCommit> findCommits(

gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestChangeReviewers.java

@@ -0,0 +1,76 @@
+// Copyright (C) 2016 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.change;
+
+import com.google.gerrit.extensions.common.SuggestedReviewerInfo;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.RestReadView;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.ReviewersUtil;
+import com.google.gerrit.server.IdentifiedUser.GenericFactory;
+import com.google.gerrit.server.ReviewersUtil.VisibilityControl;
+import com.google.gerrit.server.account.AccountVisibility;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+
+import org.eclipse.jgit.lib.Config;
+
+import java.io.IOException;
+import java.util.List;
+
+public class SuggestChangeReviewers extends SuggestReviewers
+    implements RestReadView<ChangeResource> {
+  @Inject
+  SuggestChangeReviewers(AccountVisibility av,
+      GenericFactory identifiedUserFactory,
+      Provider<ReviewDb> dbProvider,
+      @GerritServerConfig Config cfg,
+      ReviewersUtil reviewersUtil) {
+    super(av, identifiedUserFactory, dbProvider, cfg, reviewersUtil);
+  }
+
+  @Override
+  public List<SuggestedReviewerInfo> apply(ChangeResource rsrc)
+      throws BadRequestException, OrmException, IOException {
+    return reviewersUtil.suggestReviewers(this,
+        rsrc.getControl().getProjectControl(), getVisibility(rsrc));
+  }
+
+  private VisibilityControl getVisibility(final ChangeResource rsrc) {
+    if (rsrc.getControl().getRefControl().isVisibleByRegisteredUsers()) {
+      return new VisibilityControl() {
+        @Override
+        public boolean isVisibleTo(Account.Id account) throws OrmException {
+          return true;
+        }
+      };
+    } else {
+      return new VisibilityControl() {
+        @Override
+        public boolean isVisibleTo(Account.Id account) throws OrmException {
+          IdentifiedUser who =
+              identifiedUserFactory.create(dbProvider, account);
+          // we can't use changeControl directly as it won't suggest reviewers
+          // to drafts
+          return rsrc.getControl().forUser(who).isRefVisible();
+        }
+      };
+    }
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestReviewers.java

@@ -14,89 +14,33 @@
 
 package com.google.gerrit.server.change;
 
-import com.google.common.base.Function;
-import com.google.common.base.MoreObjects;
-import com.google.common.base.Strings;
-import com.google.common.collect.Iterables;
-import com.google.common.collect.Lists;
-import com.google.common.collect.Ordering;
-import com.google.gerrit.common.Nullable;
-import com.google.gerrit.common.data.GroupReference;
-import com.google.gerrit.common.errors.NoSuchGroupException;
-import com.google.gerrit.extensions.common.AccountInfo;
-import com.google.gerrit.extensions.common.GroupBaseInfo;
-import com.google.gerrit.extensions.common.SuggestedReviewerInfo;
-import com.google.gerrit.extensions.restapi.BadRequestException;
-import com.google.gerrit.extensions.restapi.RestReadView;
-import com.google.gerrit.extensions.restapi.Url;
-import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.AccountExternalId;
-import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.account.AccountCache;
+import com.google.gerrit.server.ReviewersUtil;
-import com.google.gerrit.server.account.AccountControl;
-import com.google.gerrit.server.account.AccountLoader;
 import com.google.gerrit.server.account.AccountVisibility;
-import com.google.gerrit.server.account.GroupBackend;
-import com.google.gerrit.server.account.GroupMembers;
 import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.project.NoSuchProjectException;
-import com.google.gerrit.server.project.ProjectControl;
-import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
 import org.eclipse.jgit.lib.Config;
 import org.kohsuke.args4j.Option;
 
-import java.io.IOException;
+public class SuggestReviewers {
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Iterator;
-import java.util.LinkedHashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-
-public class SuggestReviewers implements RestReadView<ChangeResource> {
-  private static final String MAX_SUFFIX = "\u9fa5";
   private static final int DEFAULT_MAX_SUGGESTED = 10;
   private static final int DEFAULT_MAX_MATCHES = 100;
-  private static final Ordering<SuggestedReviewerInfo> ORDERING =
-      Ordering.natural().onResultOf(new Function<SuggestedReviewerInfo, String>() {
-        @Nullable
-        @Override
-        public String apply(@Nullable SuggestedReviewerInfo suggestedReviewerInfo) {
-          if (suggestedReviewerInfo == null) {
-            return null;
-          }
-          return suggestedReviewerInfo.account != null
-              ? MoreObjects.firstNonNull(suggestedReviewerInfo.account.email,
-              Strings.nullToEmpty(suggestedReviewerInfo.account.name))
-              : Strings.nullToEmpty(suggestedReviewerInfo.group.name);
-        }
-      });
 
-  private final AccountLoader accountLoader;
+  protected final Provider<ReviewDb> dbProvider;
-  private final AccountControl accountControl;
+  protected final IdentifiedUser.GenericFactory identifiedUserFactory;
-  private final GroupMembers.Factory groupMembersFactory;
+  protected final ReviewersUtil reviewersUtil;
-  private final AccountCache accountCache;
+
-  private final Provider<ReviewDb> dbProvider;
-  private final Provider<CurrentUser> currentUser;
-  private final IdentifiedUser.GenericFactory identifiedUserFactory;
-  private final GroupBackend groupBackend;
   private final boolean suggestAccounts;
   private final int suggestFrom;
   private final int maxAllowed;
-  private int limit;
+  protected int limit;
-  private String query;
+  protected String query;
   private boolean useFullTextSearch;
   private final int fullTextMaxMatches;
-  private final int maxSuggestedReviewers;
+  protected final int maxSuggestedReviewers;
-  private final ReviewerSuggestionCache reviewerSuggestionCache;
 
   @Option(name = "--limit", aliases = {"-n"}, metaVar = "CNT",
       usage = "maximum number of reviewers to list")
@@ -112,27 +56,43 @@ public class SuggestReviewers implements RestReadView<ChangeResource> {
     this.query = q;
   }
 
+  public String getQuery() {
+    return query;
+  }
+
+  public boolean getSuggestAccounts() {
+    return suggestAccounts;
+  }
+
+  public int getSuggestFrom() {
+    return suggestFrom;
+  }
+
+  public boolean getUseFullTextSearch() {
+    return useFullTextSearch;
+  }
+
+  public int getFullTextMaxMatches() {
+    return fullTextMaxMatches;
+  }
+
+  public int getLimit() {
+    return limit;
+  }
+
+  public int getMaxAllowed() {
+    return maxAllowed;
+  }
+
   @Inject
-  SuggestReviewers(AccountVisibility av,
+  public SuggestReviewers(AccountVisibility av,
-      AccountLoader.Factory accountLoaderFactory,
-      AccountControl.Factory accountControlFactory,
-      AccountCache accountCache,
-      GroupMembers.Factory groupMembersFactory,
       IdentifiedUser.GenericFactory identifiedUserFactory,
-      Provider<CurrentUser> currentUser,
       Provider<ReviewDb> dbProvider,
       @GerritServerConfig Config cfg,
-      GroupBackend groupBackend,
+      ReviewersUtil reviewersUtil) {
-      ReviewerSuggestionCache reviewerSuggestionCache) {
-    this.accountLoader = accountLoaderFactory.create(true);
-    this.accountControl = accountControlFactory.get();
-    this.accountCache = accountCache;
-    this.groupMembersFactory = groupMembersFactory;
     this.dbProvider = dbProvider;
     this.identifiedUserFactory = identifiedUserFactory;
-    this.currentUser = currentUser;
+    this.reviewersUtil = reviewersUtil;
-    this.groupBackend = groupBackend;
-    this.reviewerSuggestionCache = reviewerSuggestionCache;
     this.maxSuggestedReviewers =
         cfg.getInt("suggest", "maxSuggestedReviewers", DEFAULT_MAX_SUGGESTED);
     this.limit = this.maxSuggestedReviewers;
@@ -152,196 +112,4 @@ public class SuggestReviewers implements RestReadView<ChangeResource> {
     this.maxAllowed = cfg.getInt("addreviewer", "maxAllowed",
         PostReviewers.DEFAULT_MAX_REVIEWERS);
   }
-
-  private interface VisibilityControl {
-    boolean isVisibleTo(Account.Id account) throws OrmException;
-  }
-
-  @Override
-  public List<SuggestedReviewerInfo> apply(ChangeResource rsrc)
-      throws BadRequestException, OrmException, IOException {
-    if (Strings.isNullOrEmpty(query)) {
-      throw new BadRequestException("missing query field");
-    }
-
-    if (!suggestAccounts || query.length() < suggestFrom) {
-      return Collections.emptyList();
-    }
-
-    VisibilityControl visibilityControl = getVisibility(rsrc);
-    List<AccountInfo> suggestedAccounts;
-    if (useFullTextSearch) {
-      suggestedAccounts = suggestAccountFullTextSearch(visibilityControl);
-    } else {
-      suggestedAccounts = suggestAccount(visibilityControl);
-    }
-
-    List<SuggestedReviewerInfo> reviewer = Lists.newArrayList();
-    for (AccountInfo a : suggestedAccounts) {
-      SuggestedReviewerInfo info = new SuggestedReviewerInfo();
-      info.account = a;
-      reviewer.add(info);
-    }
-
-    Project p = rsrc.getControl().getProject();
-    for (GroupReference g : suggestAccountGroup(
-        rsrc.getControl().getProjectControl())) {
-      if (suggestGroupAsReviewer(p, g, visibilityControl)) {
-        GroupBaseInfo info = new GroupBaseInfo();
-        info.id = Url.encode(g.getUUID().get());
-        info.name = g.getName();
-        SuggestedReviewerInfo suggestedReviewerInfo = new SuggestedReviewerInfo();
-        suggestedReviewerInfo.group = info;
-        reviewer.add(suggestedReviewerInfo);
-      }
-    }
-
-    reviewer = ORDERING.immutableSortedCopy(reviewer);
-    if (reviewer.size() <= limit) {
-      return reviewer;
-    } else {
-      return reviewer.subList(0, limit);
-    }
-  }
-
-  private VisibilityControl getVisibility(final ChangeResource rsrc) {
-    if (rsrc.getControl().getRefControl().isVisibleByRegisteredUsers()) {
-      return new VisibilityControl() {
-        @Override
-        public boolean isVisibleTo(Account.Id account) throws OrmException {
-          return true;
-        }
-      };
-    } else {
-      return new VisibilityControl() {
-        @Override
-        public boolean isVisibleTo(Account.Id account) throws OrmException {
-          IdentifiedUser who =
-              identifiedUserFactory.create(dbProvider, account);
-          // we can't use changeControl directly as it won't suggest reviewers
-          // to drafts
-          return rsrc.getControl().forUser(who).isRefVisible();
-        }
-      };
-    }
-  }
-
-  private List<GroupReference> suggestAccountGroup(ProjectControl ctl) {
-    return Lists.newArrayList(
-        Iterables.limit(groupBackend.suggest(query, ctl), limit));
-  }
-
-  private List<AccountInfo> suggestAccount(VisibilityControl visibilityControl)
-      throws OrmException {
-    String a = query;
-    String b = a + MAX_SUFFIX;
-
-    Map<Account.Id, AccountInfo> r = new LinkedHashMap<>();
-    Map<Account.Id, String> queryEmail = new HashMap<>();
-
-    for (Account p : dbProvider.get().accounts()
-        .suggestByFullName(a, b, limit)) {
-      if (p.isActive()) {
-        addSuggestion(r, p.getId(), visibilityControl);
-      }
-    }
-
-    if (r.size() < limit) {
-      for (Account p : dbProvider.get().accounts()
-          .suggestByPreferredEmail(a, b, limit - r.size())) {
-        if (p.isActive()) {
-          addSuggestion(r, p.getId(), visibilityControl);
-        }
-      }
-    }
-
-    if (r.size() < limit) {
-      for (AccountExternalId e : dbProvider.get().accountExternalIds()
-          .suggestByEmailAddress(a, b, limit - r.size())) {
-        if (!r.containsKey(e.getAccountId())) {
-          Account p = accountCache.get(e.getAccountId()).getAccount();
-          if (p.isActive()) {
-            if (addSuggestion(r, p.getId(), visibilityControl)) {
-              queryEmail.put(e.getAccountId(), e.getEmailAddress());
-            }
-          }
-        }
-      }
-    }
-
-    accountLoader.fill();
-    for (Map.Entry<Account.Id, String> p : queryEmail.entrySet()) {
-      AccountInfo info = r.get(p.getKey());
-      if (info != null) {
-        info.email = p.getValue();
-      }
-    }
-    return new ArrayList<>(r.values());
-  }
-
-  private List<AccountInfo> suggestAccountFullTextSearch(
-      VisibilityControl visibilityControl) throws IOException, OrmException {
-    List<AccountInfo> results = reviewerSuggestionCache.search(
-        query, fullTextMaxMatches);
-
-    Iterator<AccountInfo> it = results.iterator();
-    while (it.hasNext()) {
-      Account.Id accountId = new Account.Id(it.next()._accountId);
-      if (!(visibilityControl.isVisibleTo(accountId)
-          && accountControl.canSee(accountId))) {
-        it.remove();
-      }
-    }
-
-    return results;
-  }
-
-  private boolean addSuggestion(Map<Account.Id, AccountInfo> map,
-      Account.Id account, VisibilityControl visibilityControl)
-      throws OrmException {
-    if (!map.containsKey(account)
-        // Can the suggestion see the change?
-        && visibilityControl.isVisibleTo(account)
-        // Can the account see the current user?
-        && accountControl.canSee(account)) {
-      map.put(account, accountLoader.get(account));
-      return true;
-    }
-    return false;
-  }
-
-  private boolean suggestGroupAsReviewer(Project project,
-      GroupReference group, VisibilityControl visibilityControl)
-      throws OrmException, IOException {
-    if (!PostReviewers.isLegalReviewerGroup(group.getUUID())) {
-      return false;
-    }
-
-    try {
-      Set<Account> members = groupMembersFactory
-          .create(currentUser.get())
-          .listAccounts(group.getUUID(), project.getNameKey());
-
-      if (members.isEmpty()) {
-        return false;
-      }
-
-      if (maxAllowed > 0 && members.size() > maxAllowed) {
-        return false;
-      }
-
-      // require that at least one member in the group can see the change
-      for (Account account : members) {
-        if (visibilityControl.isVisibleTo(account.getId())) {
-          return true;
-        }
-      }
-    } catch (NoSuchGroupException e) {
-      return false;
-    } catch (NoSuchProjectException e) {
-      return false;
-    }
-
-    return false;
-  }
 }

gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java

@@ -157,6 +157,9 @@ class SshLog implements LifecycleListener {
   }
 
   private Multimap<String, ?> extractParameters(DispatchCommand dcmd) {
+    if (dcmd == null) {
+      return ArrayListMultimap.create(0, 0);
+    }
     String[] cmdArgs = dcmd.getArguments();
     String paramName = null;
     int argPos = 0;
@@ -274,6 +277,9 @@ class SshLog implements LifecycleListener {
   }
 
   private String extractWhat(DispatchCommand dcmd) {
+    if (dcmd == null) {
+      return "Command was already destroyed";
+    }
     StringBuilder commandName = new StringBuilder(dcmd.getCommandName());
     String[] args = dcmd.getArguments();
     for (int i = 1; i < args.length; i++) {

gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java

@@ -262,9 +262,6 @@ public class ReviewCommand extends SshCommand {
   }
 
   private void reviewPatchSet(final PatchSet patchSet) throws Exception {
-    if (changeComment == null) {
-      changeComment = "";
-    }
     if (notify == null) {
       notify = NotifyHandling.ALL;
     }
@@ -283,22 +280,20 @@ public class ReviewCommand extends SshCommand {
     }
     review.labels.putAll(customLabels);
 
-    // If review labels are being applied, the comment will be included
+    // We don't need to add the review comment when abandoning/restoring.
-    // on the review note. We don't need to add it again on the abandon
+    if (abandonChange || restoreChange) {
-    // or restore comment.
+      review.message = null;
-    if (!review.labels.isEmpty() && (abandonChange || restoreChange)) {
-      changeComment = null;
     }
 
     try {
       if (abandonChange) {
         AbandonInput input = new AbandonInput();
-        input.message = changeComment;
+        input.message = Strings.emptyToNull(changeComment);
         applyReview(patchSet, review);
         changeApi(patchSet).abandon(input);
       } else if (restoreChange) {
         RestoreInput input = new RestoreInput();
-        input.message = changeComment;
+        input.message = Strings.emptyToNull(changeComment);
         changeApi(patchSet).restore(input);
         applyReview(patchSet, review);
       } else {

lib/commons/BUCK

@@ -10,8 +10,8 @@ maven_jar(
 
 maven_jar(
   name = 'collections',
-  id = 'commons-collections:commons-collections:3.2.1',
+  id = 'commons-collections:commons-collections:3.2.2',
-  sha1 = '761ea405b9b37ced573d2df0d1e3a4e0f9edc668',
+  sha1 = '8ad72fe39fa8c91eaaf12aadb21e0c3661fe26d5',
   license = 'Apache2.0',
   exclude = ['META-INF/LICENSE.txt', 'META-INF/NOTICE.txt'],
   attach_source = False,