Merge branch 'stable-2.13'

* stable-2.13: (22 commits)
  Allow submit of merge of non change branch
  Avoid unnecessary group visibility checks
  Fix broken submit tests
  Add @Sandboxed annotation for classes and methods
  Add tests for submit whole topic on multiple projects/branches
  Fix notes NPE for has:draft search predicate.
  Update git submodules
  Update download-commands plugin
  ProjectWatchIT: Test that watches of different users don't interfere
  Test that watches for other projects are not applied
  ProjectWatchIT#watchFile(): Use watchedRepo for both changes
  CommitBox: Remove unused parentWebLink elements
  Strip servlet API from GWT jar to avoid classpath collision
  Use recursive name lookup for PluginConfig
  BatchUpdate#newChangeContext: Add sanity check on unwrapped change
  Set version to 2.12.6
  AbstractSubmit: Add more assertions in submitWholeTopic
  Fix formatting of Apache Derby database documentation
  CherryPick: Update mergeTip for every cherry-pick
  AbstractSubmit: Add more assertions in submitWholeTopic
  Notice merged commits even if they appear on a different branch
  MergeTip: Expose initial tip

Change-Id: I39a30eec32b6cb0ef02622b8a328f5be345ebf8a

Documentation/database-setup.txt

@@ -25,12 +25,13 @@ If Derby is selected, Gerrit will automatically set up the embedded Derby
 database as backend so no set up or configuration is necessary.
 
 Currently only support for embedded mode is added. There are two other
-deployment options for Apache Derby that can be added later [1]:
+deployment options for Apache Derby that can be added later:
-+
+
-* Derby Network Server (standalone mode)
+* link:http://db.apache.org/derby/papers/DerbyTut/ns_intro.html#Network+Server+Options[
-* Embedded Server (hybrid mode)
+Derby Network Server (standalone mode)]
-+
+
-[1] http://db.apache.org/derby/papers/DerbyTut/ns_intro.html#ns
+* link:http://db.apache.org/derby/papers/DerbyTut/ns_intro.html#Embedded+Server[
+Embedded Server (hybrid mode)]
 
 [[createdb_postgres]]
 === PostgreSQL

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -30,6 +30,7 @@ import com.google.common.collect.Iterators;
 import com.google.common.collect.Sets;
 import com.google.common.primitives.Chars;
 import com.google.gerrit.acceptance.AcceptanceTestRequestScope.Context;
+import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.AccessSection;
 import com.google.gerrit.common.data.ContributorAgreement;
 import com.google.gerrit.common.data.GroupReference;
@@ -1138,4 +1139,13 @@ public abstract class AbstractDaemonTest {
     assertThat(contentEntry.editB).isNull();
     assertThat(contentEntry.skip).isNull();
   }
+
+  protected TestRepository<?> createProjectWithPush(String name,
+      @Nullable Project.NameKey parent,
+      SubmitType submitType) throws Exception {
+    Project.NameKey project = createProject(name, parent, true, submitType);
+    grant(Permission.PUSH, project, "refs/heads/*");
+    grant(Permission.SUBMIT, project, "refs/for/refs/heads/*");
+    return cloneProject(project);
+  }
 }

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

@@ -26,6 +26,7 @@ import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS
 import static java.util.concurrent.TimeUnit.SECONDS;
 import static org.junit.Assert.fail;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -34,6 +35,7 @@ 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.SubmitInput;
+import com.google.gerrit.extensions.api.projects.BranchInput;
 import com.google.gerrit.extensions.api.projects.ProjectInput;
 import com.google.gerrit.extensions.client.ChangeStatus;
 import com.google.gerrit.extensions.client.InheritableBoolean;
@@ -81,6 +83,7 @@ import java.io.ByteArrayOutputStream;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.stream.Collectors;
 
 @NoHttpd
 public abstract class AbstractSubmit extends AbstractDaemonTest {
@@ -346,27 +349,127 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
     submit(result.getChangeId());
   }
 
+  @Test
+  public void submitWholeTopicMultipleProjects() throws Exception {
+    assume().that(isSubmitWholeTopicEnabled()).isTrue();
+    String topic = "test-topic";
+
+    // Create test projects
+    TestRepository<?> repoA = createProjectWithPush(
+        "project-a", null, getSubmitType());
+    TestRepository<?> repoB = createProjectWithPush(
+        "project-b", null, getSubmitType());
+
+    // Create changes on project-a
+    PushOneCommit.Result change1 =
+        createChange(repoA, "master", "Change 1", "a.txt", "content", topic);
+    PushOneCommit.Result change2 =
+        createChange(repoA, "master", "Change 2", "b.txt", "content", topic);
+
+    // Create changes on project-b
+    PushOneCommit.Result change3 =
+        createChange(repoB, "master", "Change 3", "a.txt", "content", topic);
+    PushOneCommit.Result change4 =
+        createChange(repoB, "master", "Change 4", "b.txt", "content", topic);
+
+    approve(change1.getChangeId());
+    approve(change2.getChangeId());
+    approve(change3.getChangeId());
+    approve(change4.getChangeId());
+    submit(change4.getChangeId());
+
+    String expectedTopic = name(topic);
+    change1.assertChange(Change.Status.MERGED, expectedTopic, admin);
+    change2.assertChange(Change.Status.MERGED, expectedTopic, admin);
+    change3.assertChange(Change.Status.MERGED, expectedTopic, admin);
+    change4.assertChange(Change.Status.MERGED, expectedTopic, admin);
+  }
+
+  @Test
+  public void submitWholeTopicMultipleBranchesOnSameProject() throws Exception {
+    assume().that(isSubmitWholeTopicEnabled()).isTrue();
+    String topic = "test-topic";
+
+    // Create test project
+    String projectName = "project-a";
+    TestRepository<?> repoA = createProjectWithPush(
+        projectName, null, getSubmitType());
+
+    RevCommit initialHead =
+        getRemoteHead(new Project.NameKey(name(projectName)), "master");
+
+    // Create the dev branch on the test project
+    BranchInput in = new BranchInput();
+    in.revision = initialHead.name();
+    gApi.projects().name(name(projectName)).branch("dev").create(in);
+
+    // Create changes on master
+    PushOneCommit.Result change1 =
+        createChange(repoA, "master", "Change 1", "a.txt", "content", topic);
+    PushOneCommit.Result change2 =
+        createChange(repoA, "master", "Change 2", "b.txt", "content", topic);
+
+    // Create  changes on dev
+    repoA.reset(initialHead);
+    PushOneCommit.Result change3 =
+        createChange(repoA, "dev", "Change 3", "a.txt", "content", topic);
+    PushOneCommit.Result change4 =
+        createChange(repoA, "dev", "Change 4", "b.txt", "content", topic);
+
+    approve(change1.getChangeId());
+    approve(change2.getChangeId());
+    approve(change3.getChangeId());
+    approve(change4.getChangeId());
+    submit(change4.getChangeId());
+
+    String expectedTopic = name(topic);
+    change1.assertChange(Change.Status.MERGED, expectedTopic, admin);
+    change2.assertChange(Change.Status.MERGED, expectedTopic, admin);
+    change3.assertChange(Change.Status.MERGED, expectedTopic, admin);
+    change4.assertChange(Change.Status.MERGED, expectedTopic, admin);
+  }
+
   @Test
   public void submitWholeTopic() throws Exception {
     assume().that(isSubmitWholeTopicEnabled()).isTrue();
+    String topic = "test-topic";
     PushOneCommit.Result change1 =
-        createChange("Change 1", "a.txt", "content", "test-topic");
+        createChange("Change 1", "a.txt", "content", topic);
     PushOneCommit.Result change2 =
-        createChange("Change 2", "b.txt", "content", "test-topic");
+        createChange("Change 2", "b.txt", "content", topic);
     PushOneCommit.Result change3 =
-        createChange("Change 3", "c.txt", "content", "test-topic");
+        createChange("Change 3", "c.txt", "content", topic);
     approve(change1.getChangeId());
     approve(change2.getChangeId());
     approve(change3.getChangeId());
     submit(change3.getChangeId());
-    change1.assertChange(Change.Status.MERGED, name("test-topic"), admin);
+    String expectedTopic = name(topic);
-    change2.assertChange(Change.Status.MERGED, name("test-topic"), admin);
+    change1.assertChange(Change.Status.MERGED, expectedTopic, admin);
-    change3.assertChange(Change.Status.MERGED, name("test-topic"), admin);
+    change2.assertChange(Change.Status.MERGED, expectedTopic, admin);
+    change3.assertChange(Change.Status.MERGED, expectedTopic, admin);
+
     // Check for the exact change to have the correct submitter.
     assertSubmitter(change3);
     // Also check submitters for changes submitted via the topic relationship.
     assertSubmitter(change1);
     assertSubmitter(change2);
+
+    // Check that the repo has the expected commits
+    List<RevCommit> log = getRemoteLog();
+    List<String> commitsInRepo = log.stream()
+        .map(c -> c.getShortMessage())
+        .collect(Collectors.toList());
+    int expectedCommitCount = getSubmitType() == SubmitType.MERGE_ALWAYS
+        ? 5 // initial commit + 3 commits + merge commit
+        : 4; // initial commit + 3 commits
+    assertThat(log).hasSize(expectedCommitCount);
+
+    assertThat(commitsInRepo).containsAllOf(
+        "Initial empty repository", "Change 1", "Change 2", "Change 3");
+    if (getSubmitType() == SubmitType.MERGE_ALWAYS) {
+      assertThat(commitsInRepo).contains(
+          "Merge changes from topic '" + expectedTopic + "'");
+    }
   }
 
   @Test
@@ -407,6 +510,66 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
         "A change to be submitted with " + num + " is not visible");
   }
 
+  @Test
+  public void submitChangeWhenParentOfOtherBranchTip() throws Exception {
+    // Chain of two commits
+    // Push both to topic-branch
+    // Push the first commit for review and submit
+    //
+    // C2 -- tip of topic branch
+    //  |
+    // C1 -- pushed for review
+    //  |
+    // C0 -- Master
+    //
+    ProjectConfig config = projectCache.checkedGet(project).getConfig();
+    config.getProject().setCreateNewChangeForAllNotInTarget(
+        InheritableBoolean.TRUE);
+    saveProjectConfig(project, config);
+
+    PushOneCommit push1 = pushFactory.create(db, admin.getIdent(), testRepo,
+        PushOneCommit.SUBJECT, "a.txt", "content");
+    PushOneCommit.Result c1 = push1.to("refs/heads/topic");
+    c1.assertOkStatus();
+    PushOneCommit push2 = pushFactory.create(db, admin.getIdent(), testRepo,
+        PushOneCommit.SUBJECT, "b.txt", "anotherContent");
+    PushOneCommit.Result c2 = push2.to("refs/heads/topic");
+    c2.assertOkStatus();
+
+    PushOneCommit.Result change1 = push1.to("refs/for/master");
+    change1.assertOkStatus();
+
+    approve(change1.getChangeId());
+    submit(change1.getChangeId());
+  }
+
+  @Test
+  public void submitMergeOfNonChangeBranchTip() throws Exception {
+    // Merge a branch with commits that have not been submitted as
+    // changes.
+    //
+    // M  -- mergeCommit (pushed for review and submitted)
+    // | \
+    // |  S -- stable (pushed directly to refs/heads/stable)
+    // | /
+    // I   -- master
+    //
+    RevCommit master = getRemoteHead(project, "master");
+    PushOneCommit stableTip = pushFactory.create(db, admin.getIdent(), testRepo,
+        "Tip of branch stable", "stable.txt", "");
+    PushOneCommit.Result stable = stableTip.to("refs/heads/stable");
+    PushOneCommit mergeCommit = pushFactory.create(db, admin.getIdent(),
+        testRepo, "The merge commit", "merge.txt", "");
+    mergeCommit.setParents(ImmutableList.of(master, stable.getCommit()));
+    PushOneCommit.Result mergeReview = mergeCommit.to("refs/for/master");
+    approve(mergeReview.getChangeId());
+    submit(mergeReview.getChangeId());
+
+    List<RevCommit> log = getRemoteLog();
+    assertThat(log).contains(stable.getCommit());
+    assertThat(log).contains(mergeReview.getCommit());
+  }
+
   private void assertSubmitter(PushOneCommit.Result change) throws Exception {
     ChangeInfo info = get(change.getChangeId(), ListChangesOption.MESSAGES);
     assertThat(info.messages).isNotNull();

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/ProjectWatchIT.java

@@ -19,6 +19,7 @@ import static com.google.common.truth.Truth.assertThat;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestAccount;
 import com.google.gerrit.extensions.client.ProjectWatchInfo;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType;
@@ -111,12 +112,18 @@ public class ProjectWatchIT extends AbstractDaemonTest {
 
   @Test
   public void watchFile() throws Exception {
-    // watch file in project
     String watchedProject = createProject("watchedProject").get();
+    String otherWatchedProject = createProject("otherWatchedProject").get();
     setApiUser(user);
+
+    // watch file in project as user
     watch(watchedProject, "file:a.txt");
 
-    // push a change to watched file -> should trigger email notification
+    // watch other project as user
+    watch(otherWatchedProject, null);
+
+    // push a change to watched file -> should trigger email notification for
+    // user
     setApiUser(admin);
     TestRepository<InMemoryRepository> watchedRepo =
         cloneProject(new Project.NameKey(watchedProject), admin);
@@ -125,19 +132,33 @@ public class ProjectWatchIT extends AbstractDaemonTest {
         .to("refs/for/master");
     r.assertOkStatus();
 
-    // push a change to non-watched file -> should not trigger email
+    // assert email notification for user
-    // notification
-    r = pushFactory.create(db, admin.getIdent(), testRepo,
-        "DONT_TRIGGER", "b.txt", "b1").to("refs/for/master");
-    r.assertOkStatus();
-
-    // assert email notification
     List<Message> messages = sender.getMessages();
     assertThat(messages).hasSize(1);
     Message m = messages.get(0);
     assertThat(m.rcpt()).containsExactly(user.emailAddress);
     assertThat(m.body()).contains("Change subject: TRIGGER\n");
     assertThat(m.body()).contains("Gerrit-PatchSet: 1\n");
+    sender.clear();
+
+    // watch project as user2
+    TestAccount user2 = accounts.create("user2", "user2@test.com", "User2");
+    setApiUser(user2);
+    watch(watchedProject, null);
+
+    // push a change to non-watched file -> should not trigger email
+    // notification for user, only for user2
+    r = pushFactory.create(db, admin.getIdent(), watchedRepo,
+        "TRIGGER_USER2", "b.txt", "b1").to("refs/for/master");
+    r.assertOkStatus();
+
+    // assert email notification
+    messages = sender.getMessages();
+    assertThat(messages).hasSize(1);
+    m = messages.get(0);
+    assertThat(m.rcpt()).containsExactly(user2.emailAddress);
+    assertThat(m.body()).contains("Change subject: TRIGGER_USER2\n");
+    assertThat(m.body()).contains("Gerrit-PatchSet: 1\n");
   }
 
   private void watch(String project, String filter)

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

@@ -56,7 +56,6 @@ class CommitBox extends Composite {
     String collapsed();
     String expanded();
     String clippy();
-    String parentWebLink();
   }
 
   @UiField Style style;
@@ -67,7 +66,6 @@ class CommitBox extends Composite {
   @UiField FlowPanel webLinkPanel;
   @UiField TableRowElement firstParent;
   @UiField FlowPanel parentCommits;
-  @UiField FlowPanel parentWebLinks;
   @UiField InlineHyperlink authorNameEmail;
   @UiField Element authorDate;
   @UiField InlineHyperlink committerNameEmail;

gerrit-gwtui/src/main/java/com/google/gerrit/client/change/CommitBox.ui.xml

@@ -88,13 +88,6 @@ limitations under the License.
       margin-left:2px;
     }
 
-    .parentWebLink a:first-child {
-      margin-left:16px;
-    }
-    .parentWebLink>a {
-      margin-left:2px;
-    }
-
     .commit {
       margin-right: 3px;
       float: left;
@@ -185,9 +178,6 @@ limitations under the License.
         <td>
           <g:FlowPanel ui:field='parentCommits'/>
         </td>
-        <td>
-          <g:FlowPanel ui:field='parentWebLinks' styleName='{style.parentWebLink}'/>
-        </td>
       </tr>
       <tr>
         <th><ui:msg>Change-Id</ui:msg></th>

gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java

@@ -1029,10 +1029,14 @@ public class BatchUpdate implements AutoCloseable {
     }
 
     private ChangeContext newChangeContext(ReviewDb db, Repository repo,
-        RevWalk rw, Change.Id id) throws Exception {
+        RevWalk rw, Change.Id id) throws OrmException, NoSuchChangeException {
       Change c = newChanges.get(id);
       if (c == null) {
         c = ChangeNotes.readOneReviewDbChange(db, id);
+        if (c == null) {
+          logDebug("Failed to get change {} from unwrapped db", id);
+          throw new NoSuchChangeException(id);
+        }
       }
       // Pass in preloaded change to controlFor, to avoid:
       //  - reading from a db that does not belong to this update

gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java

@@ -679,7 +679,10 @@ public class MergeUtil {
       rw.sort(RevSort.REVERSE, true);
       rw.markStart(mergeTip);
       for (RevCommit c : alreadyAccepted) {
-        rw.markUninteresting(c);
+        // If branch was not created by this submit.
+        if (c != mergeTip) {
+          rw.markUninteresting(c);
+        }
       }
 
       CodeReviewCommit c;

gerrit-server/src/main/java/com/google/gerrit/server/git/RebaseSorter.java

@@ -32,13 +32,15 @@ import java.util.Set;
 public class RebaseSorter {
   private final CodeReviewRevWalk rw;
   private final RevFlag canMergeFlag;
-  private final Set<RevCommit> accepted;
+  private final RevCommit initialTip;
+  private final Set<RevCommit> alreadyAccepted;
 
-  public RebaseSorter(CodeReviewRevWalk rw, Set<RevCommit> alreadyAccepted,
+  public RebaseSorter(CodeReviewRevWalk rw, RevCommit initialTip,
-      RevFlag canMergeFlag) {
+      Set<RevCommit> alreadyAccepted, RevFlag canMergeFlag) {
     this.rw = rw;
     this.canMergeFlag = canMergeFlag;
-    this.accepted = alreadyAccepted;
+    this.initialTip = initialTip;
+    this.alreadyAccepted = alreadyAccepted;
   }
 
   public List<CodeReviewCommit> sort(Collection<CodeReviewCommit> incoming)
@@ -50,17 +52,18 @@ public class RebaseSorter {
 
       rw.resetRetain(canMergeFlag);
       rw.markStart(n);
-      for (RevCommit c : accepted) {
+      if (initialTip != null) {
-        // n also tip of directly pushed branch => n remains 'interesting' here
+        rw.markUninteresting(initialTip);
-        if (!c.equals(n)) {
-          rw.markUninteresting(c);
-        }
       }
 
       CodeReviewCommit c;
       final List<CodeReviewCommit> contents = new ArrayList<>();
       while ((c = rw.next()) != null) {
         if (!c.has(canMergeFlag) || !incoming.contains(c)) {
+          if (isAlreadyMerged(c)) {
+            rw.markUninteresting(c);
+            break;
+          }
           // We cannot merge n as it would bring something we
           // aren't permitted to merge at this time. Drop n.
           //
@@ -86,6 +89,21 @@ public class RebaseSorter {
     return sorted;
   }
 
+  private boolean isAlreadyMerged(CodeReviewCommit commit) throws IOException {
+    try (CodeReviewRevWalk mirw =
+        CodeReviewCommit.newRevWalk(rw.getObjectReader())) {
+      mirw.reset();
+      mirw.markStart(commit);
+      for (RevCommit accepted : alreadyAccepted) {
+        if (mirw.isMergedInto(mirw.parseCommit(accepted),
+            mirw.parseCommit(commit))) {
+          return true;
+        }
+      }
+    }
+    return false;
+  }
+
   private static <T> T removeOne(final Collection<T> c) {
     final Iterator<T> i = c.iterator();
     final T r = i.next();

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java

@@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkState;
 import static com.google.gerrit.server.git.strategy.CommitMergeStatus.SKIPPED_IDENTICAL_TREE;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.extensions.restapi.MergeConflictException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.RestApiException;
@@ -39,6 +40,7 @@ import com.google.gwtorm.server.OrmException;
 
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.transport.ReceiveCommand;
 
 import java.io.IOException;
@@ -60,7 +62,7 @@ public class RebaseSubmitStrategy extends SubmitStrategy {
   @Override
   public List<SubmitStrategyOp> buildOps(
       Collection<CodeReviewCommit> toMerge) throws IntegrationException {
-    List<CodeReviewCommit> sorted = sort(toMerge);
+    List<CodeReviewCommit> sorted = sort(toMerge, args.mergeTip.getCurrentTip());
     List<SubmitStrategyOp> ops = new ArrayList<>(sorted.size());
     boolean first = true;
 
@@ -256,8 +258,10 @@ public class RebaseSubmitStrategy extends SubmitStrategy {
             args.inserter, args.destBranch, mergeTip.getCurrentTip(), toMerge);
         mergeTip.moveTipTo(amendGitlink(newTip), toMerge);
       }
+      RevCommit initialTip = mergeTip.getInitialTip();
       args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag,
-          mergeTip.getCurrentTip(), args.alreadyAccepted);
+          mergeTip.getCurrentTip(), initialTip == null ?
+              ImmutableSet.<RevCommit>of() : ImmutableSet.of(initialTip));
       acceptMergeTip(mergeTip);
     }
   }
@@ -266,11 +270,11 @@ public class RebaseSubmitStrategy extends SubmitStrategy {
     args.alreadyAccepted.add(mergeTip.getCurrentTip());
   }
 
-  private List<CodeReviewCommit> sort(Collection<CodeReviewCommit> toSort)
+  private List<CodeReviewCommit> sort(Collection<CodeReviewCommit> toSort,
-      throws IntegrationException {
+      RevCommit initialTip) throws IntegrationException {
     try {
-      return new RebaseSorter(
+      return new RebaseSorter(args.rw, initialTip, args.alreadyAccepted,
-          args.rw, args.alreadyAccepted, args.canMergeFlag).sort(toSort);
+          args.canMergeFlag).sort(toSort);
     } catch (IOException e) {
       throw new IntegrationException("Commit sorting failed", e);
     }

gerrit-server/src/main/java/com/google/gerrit/server/group/ListGroups.java

@@ -326,12 +326,6 @@ public class ListGroups implements RestReadView<TopLevelResource> {
           continue;
         }
       }
-      if (!isAdmin) {
-        GroupControl c = groupControlFactory.controlFor(group);
-        if (!c.isVisible()) {
-          continue;
-        }
-      }
       if (visibleToAll && !group.isVisibleToAll()) {
         continue;
       }
@@ -339,6 +333,12 @@ public class ListGroups implements RestReadView<TopLevelResource> {
           && !groupsToInspect.contains(group.getGroupUUID())) {
         continue;
       }
+      if (!isAdmin) {
+        GroupControl c = groupControlFactory.controlFor(group);
+        if (!c.isVisible()) {
+          continue;
+        }
+      }
       filteredGroups.add(group);
     }
     Collections.sort(filteredGroups, new GroupComparator());

gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java

@@ -1129,7 +1129,7 @@ public class ChangeData {
         return Collections.emptySet();
       }
       draftsByUser = new HashSet<>();
-      for (Comment sc : commentsUtil.draftByChange(db, notes)) {
+      for (Comment sc : commentsUtil.draftByChange(db, notes())) {
         draftsByUser.add(sc.author.getId());
       }
     }