ReceiveCommits: Fix set comparison to avoid no-op group updates

Group equality should be using set equality, but we were accidentally
comparing a Set with a List, always returning false. This resulted in
a no-op group update every time a previously-seen change was walked
during ReceiveCommits pushing a new change.

This led to all sorts of flaky test failures under notedb, since the
last updated timestamp of an earlier change that was not updated might
be moved forward ahead of a later change. Tests expected the newest
change to have the most recent timestamp (Thanks to Edwin for tracking
down the cause of this flakiness!)

Fix this by being a little more explicit about types. Add a regression
test to GetRelatedIT asserting that the change is unmodified when a
child change is pushed. We use ETag as it does the right thing
depending on whether notedb is enabled, either using the rowVersion
column or the latest meta commit SHA-1 as appropriate.

Change-Id: I3c5ef4287cf5fbef733f0bb01bfe3857522c5d0a

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java

@@ -27,6 +27,7 @@ import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.common.CommitInfo;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.server.change.ChangesCollection;
 import com.google.gerrit.server.change.GetRelated.ChangeAndCommit;
 import com.google.gerrit.server.change.GetRelated.RelatedInfo;
 import com.google.gerrit.server.edit.ChangeEditModifier;
@@ -71,6 +72,9 @@ public class GetRelatedIT extends AbstractDaemonTest {
   @Inject
   private BatchUpdate.Factory updateFactory;
 
+  @Inject
+  private ChangesCollection changes;
+
   @Test
   public void getRelatedNoResult() throws Exception {
     PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo);
@@ -99,6 +103,38 @@ public class GetRelatedIT extends AbstractDaemonTest {
     }
   }
 
+  @Test
+  public void getRelatedLinearSeparatePushes() throws Exception {
+    // 1,1---2,1
+    RevCommit c1_1 = commitBuilder()
+        .add("a.txt", "1")
+        .message("subject: 1")
+        .create();
+    RevCommit c2_1 = commitBuilder()
+        .add("b.txt", "2")
+        .message("subject: 2")
+        .create();
+
+    testRepo.reset(c1_1);
+    pushHead(testRepo, "refs/for/master", false);
+    PatchSet.Id ps1_1 = getPatchSetId(c1_1);
+    String oldETag = changes.parse(ps1_1.getParentKey()).getETag();
+
+    testRepo.reset(c2_1);
+    pushHead(testRepo, "refs/for/master", false);
+    PatchSet.Id ps2_1 = getPatchSetId(c2_1);
+
+    // Push of change 2 should not affect groups (or anything else) of change 1.
+    assertThat(changes.parse(ps1_1.getParentKey()).getETag())
+        .isEqualTo(oldETag);
+
+    for (PatchSet.Id ps : ImmutableList.of(ps2_1, ps1_1)) {
+      assertRelated(ps,
+          changeAndCommit(ps2_1, c2_1, 1),
+          changeAndCommit(ps1_1, c1_1, 1));
+    }
+  }
+
   @Test
   public void getRelatedReorder() throws Exception {
     // 1,1---2,1

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

@@ -2448,7 +2448,7 @@ public class ReceiveCommits {
               if (groups == null) {
                 return false;
               }
-            } else if (Sets.newHashSet(oldGroups).equals(groups)) {
+            } else if (sameGroups(oldGroups, groups)) {
               return false;
             }
             psUtil.setGroups(ctx.getDb(), ctx.getUpdate(psId), ps, groups);
@@ -2459,6 +2459,10 @@ public class ReceiveCommits {
       }
     }
 
+    private boolean sameGroups(List<String> a, List<String> b) {
+      return Sets.newHashSet(a).equals(Sets.newHashSet(b));
+    }
+
     CheckedFuture<Void, RestApiException> updateGroups() {
       final Thread caller = Thread.currentThread();
       ListenableFuture<Void> future = changeUpdateExector.submit(